Re: [PATCH v2 07/11] ASoC: qcom: Add QAIF regmap, DT parsing and platform init

[Harendra Gautam]

On Thu, Jul 2, 2026 at 12:37 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On Wed, Jul 01, 2026 at 04:27:53PM +0530, Harendra Gautam wrote:
> > +/**
> > + * asoc_qcom_qaif_cpu_platform_probe - Probe the QAIF CPU and platform driver
> > + * @pdev: Platform device
> > + *
> > + * Initialises the QAIF regmap, parses DT, sets up clocks and registers
> > + * the CPU DAI component and PCM platform.
> > + *
> > + * Return: 0 on success, negative error code on failure.
> > + */
> > +int asoc_qcom_qaif_cpu_platform_probe(struct platform_device *pdev)
> > +{
> > +     struct qaif_drv_data *drvdata;
> > +     struct resource *res;
> > +     const struct qaif_variant *variant;
> > +     struct device *dev = &pdev->dev;
> > +     const struct of_device_id *match;
> > +     int ret, i, dai_id, idx;
> > +     bool variant_init_done = false;
> > +
> > +     drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > +     if (!drvdata)
> > +             return -ENOMEM;
> > +     platform_set_drvdata(pdev, drvdata);
> > +
> > +     match = of_match_device(dev->driver->of_match_table, dev);
> > +     if (!match || !match->data)
> > +             return -EINVAL;
> > +
> > +     drvdata->variant = match->data;
> > +     variant = drvdata->variant;
> > +
> > +     ret = of_qaif_parse_aif_intf_cfg(dev, drvdata);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "Failed to parse aif interfaces\n");
> > +
> > +     drvdata->audio_qaif =
> > +                     devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(drvdata->audio_qaif))
> > +             return PTR_ERR(drvdata->audio_qaif);
> > +
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     if (!res)
> > +             return -EINVAL;
> > +
> > +     audio_qaif_regmap_config.max_register = resource_size(res);
>
> Why are you modifiying static variable? So if second instance is probed,
> how is this supposed to work?
>
> > +     drvdata->audio_qaif_map =
> > +             devm_regmap_init_mmio(dev, drvdata->audio_qaif,
> > +                                   &audio_qaif_regmap_config);
> > +     if (IS_ERR(drvdata->audio_qaif_map))
> > +             return PTR_ERR(drvdata->audio_qaif_map);
> > +
> > +     ret = of_qaif_cdc_dma_clks_parse(dev, drvdata);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "Failed to get cdc dma clocks\n");
> > +
> > +     if (variant->clk_init) {
> > +             ret = variant->clk_init(pdev);
> > +             if (ret)
> > +                     return dev_err_probe(dev, ret, "Failed to initialize variant\n");
> > +             variant_init_done = true;
> > +     }
> > +
> > +     for (i = 0; i < variant->num_dai; i++) {
> > +             dai_id = variant->dai_driver[i].id;
> > +             if (is_cif_dma_port(dai_id))
> > +                     continue;
> > +             idx = variant->get_dma_idx(dai_id);
> > +             if (idx < 0)
> > +                     continue;
> > +
> > +             drvdata->mi2s_bit_clk[idx] = devm_clk_get(dev,
> > +                                                       variant->dai_bit_clk_names[idx]);
> > +             if (IS_ERR(drvdata->mi2s_bit_clk[idx])) {
> > +                     ret = PTR_ERR(drvdata->mi2s_bit_clk[idx]);
>
> Wrong syntax. Please use dev_err_probe.
>
>
> > +                     dev_err_probe(dev, ret, "error getting %s\n",
> > +                                   variant->dai_bit_clk_names[idx]);
> > +                     goto err;
> > +             }
> > +     }
> > +
> > +     ret = qaif_aif_cpu_init_bitfields(dev, drvdata->audio_qaif_map);
> > +     if (ret) {
> > +             dev_err_probe(dev, ret, "error init aif bitfields\n");
> > +             goto err;
> > +     }
> > +
> > +     ret = qaif_aif_cfg_cpu_init_bitfields(dev, drvdata->audio_qaif_map);
> > +     if (ret) {
> > +             dev_err_probe(dev, ret, "error init aif intfctl fields\n");
> > +             goto err;
> > +     }
> > +
> > +     ret = qaif_cif_cpu_init_bitfields(dev, drvdata->audio_qaif_map);
> > +     if (ret) {
> > +             dev_err_probe(dev, ret, "error init cif bitfields\n");
> > +             goto err;
> > +     }
>
> Why do you need to enable the clocks before all this init? Do you
> actually access the device here (like its registers)?
>
> > +
> > +     ret = devm_snd_soc_register_component(dev,
> > +                                           &qaif_cpu_comp_driver,
> > +                                           variant->dai_driver,
> > +                                           variant->num_dai);
> > +     if (ret) {
> > +             dev_err_probe(dev, ret, "error registering cpu driver\n");
> > +             goto err;
> > +     }
> > +
> > +     ret = asoc_qcom_qaif_platform_register(pdev);
> > +     if (ret) {
> > +             dev_err_probe(dev, ret, "error registering platform driver\n");
> > +             goto err;
> > +     }
> > +err:
> > +     if (ret && variant_init_done && variant->clk_deinit)
> > +             variant->clk_deinit(pdev);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(asoc_qcom_qaif_cpu_platform_probe);
> > +
> > +/**
> > + * asoc_qcom_qaif_cpu_platform_remove - Remove the QAIF CPU and platform driver
> > + * @pdev: Platform device
> > + */
> > +void asoc_qcom_qaif_cpu_platform_remove(struct platform_device *pdev)
> > +{
> > +     struct qaif_drv_data *drvdata = platform_get_drvdata(pdev);
> > +
> > +     if (drvdata->variant->clk_deinit)
> > +             drvdata->variant->clk_deinit(pdev);
> > +}
> > +EXPORT_SYMBOL_GPL(asoc_qcom_qaif_cpu_platform_remove);
> > +
> > +/**
> > + * asoc_qcom_qaif_cpu_platform_shutdown - Shutdown the QAIF CPU and platform driver
> > + * @pdev: Platform device
> > + */
> > +void asoc_qcom_qaif_cpu_platform_shutdown(struct platform_device *pdev)
> > +{
> > +     struct qaif_drv_data *drvdata = platform_get_drvdata(pdev);
> > +
> > +     if (drvdata->variant->clk_deinit)
> > +             drvdata->variant->clk_deinit(pdev);
> > +}
> > +EXPORT_SYMBOL_GPL(asoc_qcom_qaif_cpu_platform_shutdown);
>
> I do not understand why do you need wrappers over single call of single
> driver. You do not have different variants. And even if you had
> different variants, I claim they will have exactly the same clk init and
> deinit.
>
> Again, this looks awfully like sending us a dowsntream driver which is a
> known antipattern of upstreaming.
>
>
> Best regards,
> Krzysztof
>
Addressed comments, will raise as part of the next patch.
-Harendra