Hi Nicolas,
On Fri, 2021-08-20 at 20:27 +0200, Nicolas Frattaroli wrote: [...]
diff --git a/sound/soc/rockchip/rockchip_i2s_tdm.c b/sound/soc/rockchip/rockchip_i2s_tdm.c new file mode 100644 index 000000000000..c02b66f3c913 --- /dev/null +++ b/sound/soc/rockchip/rockchip_i2s_tdm.c @@ -0,0 +1,1737 @@
[...]
+static void rockchip_snd_xfer_reset_assert(struct rk_i2s_tdm_dev *i2s_tdm,
int tx_bank, int tx_offset,int rx_bank, int rx_offset)+{
- void __iomem *cru_reset;
- unsigned long flags;
- cru_reset = i2s_tdm->cru_base + i2s_tdm->soc_data->softrst_offset;
- if (tx_bank == rx_bank) {
writel(BIT(tx_offset) | BIT(rx_offset) |(BIT(tx_offset) << 16) | (BIT(rx_offset) << 16),cru_reset + (tx_bank * 4));- } else {
local_irq_save(flags);writel(BIT(tx_offset) | (BIT(tx_offset) << 16),cru_reset + (tx_bank * 4));writel(BIT(rx_offset) | (BIT(rx_offset) << 16),cru_reset + (rx_bank * 4));local_irq_restore(flags);- }
+}
+static void rockchip_snd_xfer_reset_deassert(struct rk_i2s_tdm_dev *i2s_tdm,
int tx_bank, int tx_offset,int rx_bank, int rx_offset)+{
- void __iomem *cru_reset;
- unsigned long flags;
- cru_reset = i2s_tdm->cru_base + i2s_tdm->soc_data->softrst_offset;
- if (tx_bank == rx_bank) {
writel((BIT(tx_offset) << 16) | (BIT(rx_offset) << 16),cru_reset + (tx_bank * 4));- } else {
local_irq_save(flags);writel((BIT(tx_offset) << 16),cru_reset + (tx_bank * 4));writel((BIT(rx_offset) << 16),cru_reset + (rx_bank * 4));local_irq_restore(flags);- }
+}
+/*
- Makes sure that both tx and rx are reset at the same time to sync lrck
- when clk_trcm > 0.
- */
+static void rockchip_snd_xfer_sync_reset(struct rk_i2s_tdm_dev *i2s_tdm) +{
- int tx_id, rx_id;
- int tx_bank, rx_bank, tx_offset, rx_offset;
- if (!i2s_tdm->cru_base || !i2s_tdm->soc_data)
return;- tx_id = i2s_tdm->tx_reset_id;
- rx_id = i2s_tdm->rx_reset_id;
- if (tx_id < 0 || rx_id < 0)
return;- tx_bank = tx_id / 16;
- tx_offset = tx_id % 16;
- rx_bank = rx_id / 16;
- rx_offset = rx_id % 16;
- dev_dbg(i2s_tdm->dev,
"tx_bank: %d, rx_bank: %d, tx_offset: %d, rx_offset: %d\n",tx_bank, rx_bank, tx_offset, rx_offset);- rockchip_snd_xfer_reset_assert(i2s_tdm, tx_bank, tx_offset,
rx_bank, rx_offset);- udelay(150);
- rockchip_snd_xfer_reset_deassert(i2s_tdm, tx_bank, tx_offset,
rx_bank, rx_offset);+}
I'm not too fond of reimplementing half a reset controller in here. The reset framework does not support synchronized (de)assertion of multiple reset controls, I wonder if this would be useful to add. Without having thought about this too hard, I could imagine this as an extension to the bulk API, or possibly a call to join multiple reset controls into a reset control array.
+static void rockchip_snd_reset(struct reset_control *rc) +{
- if (IS_ERR(rc))
return;
This shouldn't be called with an error code, see the comment about optional reset controls below.
- reset_control_assert(rc);
- udelay(1);
What is the reason for the different delays in rockchip_snd_xfer_sync_reset() and rockchip_snd_reset()?
- reset_control_deassert(rc);
+}
[...]
+static int rockchip_i2s_tdm_probe(struct platform_device *pdev) +{
- struct device_node *node = pdev->dev.of_node;
- struct device_node *cru_node;
- const struct of_device_id *of_id;
- struct rk_i2s_tdm_dev *i2s_tdm;
- struct resource *res;
- void __iomem *regs;
- int ret;
- int val;
- i2s_tdm = devm_kzalloc(&pdev->dev, sizeof(*i2s_tdm), GFP_KERNEL);
- if (!i2s_tdm)
return -ENOMEM;- i2s_tdm->dev = &pdev->dev;
- of_id = of_match_device(rockchip_i2s_tdm_match, &pdev->dev);
- if (!of_id || !of_id->data)
return -EINVAL;- spin_lock_init(&i2s_tdm->lock);
- i2s_tdm->soc_data = (struct rk_i2s_soc_data *)of_id->data;
- i2s_tdm->frame_width = 64;
- if (!of_property_read_u32(node, "rockchip,frame-width", &val)) {
if (val >= 32 && (val % 2 == 0) && val <= 512) {i2s_tdm->frame_width = val;} else {dev_err(i2s_tdm->dev, "unsupported frame width: '%d'\n",val);return -EINVAL;}- }
- i2s_tdm->clk_trcm = TRCM_TXRX;
- if (of_property_read_bool(node, "rockchip,trcm-sync-tx-only"))
i2s_tdm->clk_trcm = TRCM_TX;- if (of_property_read_bool(node, "rockchip,trcm-sync-rx-only")) {
if (i2s_tdm->clk_trcm) {dev_err(i2s_tdm->dev, "invalid trcm-sync configuration\n");return -EINVAL;}i2s_tdm->clk_trcm = TRCM_RX;- }
- if (i2s_tdm->clk_trcm != TRCM_TXRX)
i2s_tdm_dai.symmetric_rate = 1;- i2s_tdm->tdm_fsync_half_frame =
of_property_read_bool(node, "rockchip,tdm-fsync-half-frame");- i2s_tdm->grf = syscon_regmap_lookup_by_phandle(node, "rockchip,grf");
- if (IS_ERR(i2s_tdm->grf))
return dev_err_probe(i2s_tdm->dev, PTR_ERR(i2s_tdm->grf),"Error in rockchip,grf\n");- if (i2s_tdm->clk_trcm != TRCM_TXRX) {
cru_node = of_parse_phandle(node, "rockchip,cru", 0);i2s_tdm->cru_base = of_iomap(cru_node, 0);
This is a bit ugly if there is another driver sitting on the rockchip,cru compatible node. Which reset controller driver is backing the reset controls below?
of_node_put(cru_node);if (!i2s_tdm->cru_base) {dev_err(i2s_tdm->dev,"Missing or unsupported rockchip,cru node\n");return -ENOENT;}i2s_tdm->tx_reset_id = of_i2s_resetid_get(node, "tx-m");i2s_tdm->rx_reset_id = of_i2s_resetid_get(node, "rx-m");- }
- i2s_tdm->tx_reset = devm_reset_control_get(&pdev->dev, "tx-m");
Please use devm_reset_control_get_exclusive() instead.
- if (IS_ERR(i2s_tdm->tx_reset)) {
ret = PTR_ERR(i2s_tdm->tx_reset);if (ret != -ENOENT)return dev_err_probe(i2s_tdm->dev, ret,"Error in tx-m reset control\n");- }
- i2s_tdm->rx_reset = devm_reset_control_get(&pdev->dev, "rx-m");
- if (IS_ERR(i2s_tdm->rx_reset)) {
ret = PTR_ERR(i2s_tdm->rx_reset);if (ret != -ENOENT)
Why is -ENOENT acceptable? If you want these to be optional, use devm_reset_control_get_optional_exclusive() [1] instead. That will return NULL if the reset is not specified in the device tree, which will be ignored by reset_control_(de)assert().
[1] https://www.kernel.org/doc/html/latest/driver-api/reset.html#c.devm_reset_co...
return dev_err_probe(i2s_tdm->dev, ret,"Error in rx-m reset control\n");- }
- i2s_tdm->hclk = devm_clk_get(&pdev->dev, "hclk");
- if (IS_ERR(i2s_tdm->hclk)) {
return dev_err_probe(i2s_tdm->dev, PTR_ERR(i2s_tdm->hclk),"Failed to get clock hclk\n");- }
- ret = clk_prepare_enable(i2s_tdm->hclk);
- if (ret) {
return dev_err_probe(i2s_tdm->dev, ret,"Failed to enable clock hclk\n");- }
This clock is left enabled in all the error paths below. You could install a cleanup action with devm_add_action_or_reset(), or better start enabling the hardware only after acquisition of all required resources has succeeded.
[...]
+static int rockchip_i2s_tdm_remove(struct platform_device *pdev) +{
- struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(&pdev->dev);
- pm_runtime_disable(&pdev->dev);
- if (!pm_runtime_status_suspended(&pdev->dev))
i2s_tdm_runtime_suspend(&pdev->dev);- if (!IS_ERR(i2s_tdm->mclk_tx))
clk_prepare_enable(i2s_tdm->mclk_tx);- if (!IS_ERR(i2s_tdm->mclk_rx))
clk_prepare_enable(i2s_tdm->mclk_rx);
Why are we enabling these clocks now?
- if (!IS_ERR(i2s_tdm->hclk))
clk_disable_unprepare(i2s_tdm->hclk);- return 0;
+}
regards Philipp