| Message ID | 20220808224153.3634-1-povik+lin@cutebit.org |
|---|---|
| Headers | show |
| Series | ASoC platform driver for Apple MCA | expand |
Hi Martin, On Di, 2022-08-09 at 00:41 +0200, Martin Povišer wrote: > + mca->rstc = devm_reset_control_get_shared(&pdev->dev, NULL); > + if (IS_ERR(mca->rstc)) { > + dev_dbg(&pdev->dev, "couldn't obtain reset control: %pe\n", mca->rstc); > + mca->rstc = NULL; > + } Please don't ignore errors, this could be -ENOMEM. For optional resets, use devm_reset_control_get_optional_shared(), which returns NULL if there is no resets property in the device tree. regards Philipp
On 09/08/2022 01:41, Martin Povišer wrote: > Add ASoC platform driver for the MCA peripheral found on Apple M1 and > other chips. > > Signed-off-by: Martin Povišer <povik+lin@cutebit.org> > +static int apple_mca_probe(struct platform_device *pdev) > +{ > + struct mca_data *mca; > + struct mca_cluster *clusters; > + struct snd_soc_dai_driver *dai_drivers; > + struct resource *res; > + void __iomem *base; > + int nclusters; > + int ret, i; > + > + base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + if (resource_size(res) < CLUSTER_STRIDE) > + return -EINVAL; > + nclusters = (resource_size(res) - CLUSTER_STRIDE) / CLUSTER_STRIDE + 1; > + > + mca = devm_kzalloc(&pdev->dev, struct_size(mca, clusters, nclusters), > + GFP_KERNEL); > + if (!mca) > + return -ENOMEM; > + mca->dev = &pdev->dev; > + mca->nclusters = nclusters; > + platform_set_drvdata(pdev, mca); > + clusters = mca->clusters; > + > + mca->switch_base = > + devm_platform_ioremap_resource_byname(pdev, "switch"); > + if (IS_ERR(mca->switch_base)) > + return PTR_ERR(mca->switch_base); How does it work exactly? There is no such property... Can you submit also DTS using the bindings so we can validate they are real/correct? > + > + mca->rstc = devm_reset_control_get_shared(&pdev->dev, NULL); > + if (IS_ERR(mca->rstc)) { > + dev_dbg(&pdev->dev, "couldn't obtain reset control: %pe\n", mca->rstc); > + mca->rstc = NULL; > + } Similar question. > + > + dai_drivers = devm_kzalloc( > + &pdev->dev, sizeof(*dai_drivers) * 2 * nclusters, GFP_KERNEL); > + if (!dai_drivers) > + return -ENOMEM; > + > + mca->pd_dev = dev_pm_domain_attach_by_id(&pdev->dev, 0); > + if (IS_ERR(mca->pd_dev)) > + return -EINVAL; > + > + mca->pd_link = device_link_add(&pdev->dev, mca->pd_dev, > + DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | > + DL_FLAG_RPM_ACTIVE); > + if (!mca->pd_link) { > + ret = -EINVAL; > + /* Prevent an unbalanced reset assert */ > + mca->rstc = NULL; > + goto err_release; > + } > + > + reset_control_deassert(mca->rstc); > + > + for (i = 0; i < nclusters; i++) { > + struct mca_cluster *cl = &clusters[i]; > + struct snd_soc_dai_driver *fe = > + &dai_drivers[mca->nclusters + i]; > + struct snd_soc_dai_driver *be = &dai_drivers[i]; > + int stream; > + > + cl->host = mca; > + cl->no = i; > + cl->base = base + CLUSTER_STRIDE * i; > + cl->port_driver = -1; > + cl->clk_parent = of_clk_get(pdev->dev.of_node, i); > + if (IS_ERR(cl->clk_parent)) { > + dev_err(&pdev->dev, "unable to obtain clock %d: %ld\n", > + i, PTR_ERR(cl->clk_parent)); > + ret = PTR_ERR(cl->clk_parent); > + goto err_release; > + } > + cl->pd_dev = dev_pm_domain_attach_by_id(&pdev->dev, i + 1); > + if (IS_ERR(cl->pd_dev)) { > + dev_err(&pdev->dev, > + "unable to obtain cluster %d PD: %ld\n", i, > + PTR_ERR(cl->pd_dev)); > + ret = PTR_ERR(cl->pd_dev); > + goto err_release; > + } > + > + for_each_pcm_streams(stream) { > + struct dma_chan *chan; > + bool is_tx = (stream == SNDRV_PCM_STREAM_PLAYBACK); > +#ifndef USE_RXB_FOR_CAPTURE > + char *name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + is_tx ? "tx%da" : "rx%da", > + i); > +#else > + char *name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + is_tx ? "tx%da" : "rx%db", > + i); > +#endif > + > + chan = of_dma_request_slave_channel(pdev->dev.of_node, > + name); > + if (IS_ERR(chan)) { > + if (PTR_ERR(chan) != -EPROBE_DEFER) > + dev_err(&pdev->dev, > + "no %s DMA channel: %ld\n", > + name, PTR_ERR(chan)); > + > + ret = PTR_ERR(chan); > + goto err_release; > + } > + > + cl->dma_chans[stream] = chan; > + } > + > + fe->id = i; > + fe->name = > + devm_kasprintf(&pdev->dev, GFP_KERNEL, "mca-pcm-%d", i); > + if (!fe->name) { > + ret = -ENOMEM; > + goto err_release; > + } > + fe->ops = &mca_fe_ops; > + fe->playback.channels_min = 1; > + fe->playback.channels_max = 32; > + fe->playback.rates = SNDRV_PCM_RATE_8000_192000; > + fe->playback.formats = APPLE_MCA_FMTBITS; > + fe->capture.channels_min = 1; > + fe->capture.channels_max = 32; > + fe->capture.rates = SNDRV_PCM_RATE_8000_192000; > + fe->capture.formats = APPLE_MCA_FMTBITS; > + fe->symmetric_rate = 1; > + > + fe->playback.stream_name = > + devm_kasprintf(&pdev->dev, GFP_KERNEL, "PCM%d TX", i); > + fe->capture.stream_name = > + devm_kasprintf(&pdev->dev, GFP_KERNEL, "PCM%d RX", i); > + > + if (!fe->playback.stream_name || !fe->capture.stream_name) { > + ret = -ENOMEM; > + goto err_release; > + } > + > + be->id = i + nclusters; > + be->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "mca-i2s-%d", i); > + if (!be->name) { > + ret = -ENOMEM; > + goto err_release; > + } > + be->ops = &mca_be_ops; > + be->playback.channels_min = 1; > + be->playback.channels_max = 32; > + be->playback.rates = SNDRV_PCM_RATE_8000_192000; > + be->playback.formats = APPLE_MCA_FMTBITS; > + be->capture.channels_min = 1; > + be->capture.channels_max = 32; > + be->capture.rates = SNDRV_PCM_RATE_8000_192000; > + be->capture.formats = APPLE_MCA_FMTBITS; > + > + be->playback.stream_name = > + devm_kasprintf(&pdev->dev, GFP_KERNEL, "I2S%d TX", i); > + be->capture.stream_name = > + devm_kasprintf(&pdev->dev, GFP_KERNEL, "I2S%d RX", i); > + if (!be->playback.stream_name || !be->capture.stream_name) { > + ret = -ENOMEM; > + goto err_release; > + } > + } > + > + ret = devm_snd_soc_register_component(&pdev->dev, &mca_component, > + dai_drivers, nclusters * 2); > + if (ret) { > + dev_err(&pdev->dev, "unable to register ASoC component: %d\n", > + ret); > + goto err_release; > + } > + > + return 0; > + > +err_release: > + apple_mca_release(mca); > + return ret; > +} > + > +static int apple_mca_remove(struct platform_device *pdev) > +{ > + struct mca_data *mca = platform_get_drvdata(pdev); > + > + apple_mca_release(mca); > + return 0; > +} > + > +static const struct of_device_id apple_mca_of_match[] = { > + { .compatible = "apple,mca", }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, apple_mca_of_match); > + > +static struct platform_driver apple_mca_driver = { > + .driver = { > + .name = "apple-mca", > + .owner = THIS_MODULE, No need for this. Run coccinelle. Best regards, Krzysztof
> On 9. 8. 2022, at 10:47, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 09/08/2022 01:41, Martin Povišer wrote: >> + mca->switch_base = >> + devm_platform_ioremap_resource_byname(pdev, "switch"); >> + if (IS_ERR(mca->switch_base)) >> + return PTR_ERR(mca->switch_base); > > How does it work exactly? There is no such property... Can you submit > also DTS using the bindings so we can validate they are real/correct? Ah, I thought I fixed that. There’s supposed to be mca->switch_base = devm_platform_ioremap_resource(pdev, 1); of course. My bad, I guess didn’t reexport the patches after these last minute changes. >> + >> + mca->rstc = devm_reset_control_get_shared(&pdev->dev, NULL); >> + if (IS_ERR(mca->rstc)) { >> + dev_dbg(&pdev->dev, "couldn't obtain reset control: %pe\n", mca->rstc); >> + mca->rstc = NULL; >> + } > > Similar question. Same as above, there’s supposed to be resets: maxItems: 1 in the schema. > Best regards, > Krzysztof > Martin
> On 9. 8. 2022, at 10:54, Martin Povišer <povik+lin@cutebit.org> wrote: > > >> On 9. 8. 2022, at 10:47, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >> >> On 09/08/2022 01:41, Martin Povišer wrote: (...) >>> + >>> + mca->rstc = devm_reset_control_get_shared(&pdev->dev, NULL); >>> + if (IS_ERR(mca->rstc)) { >>> + dev_dbg(&pdev->dev, "couldn't obtain reset control: %pe\n", mca->rstc); >>> + mca->rstc = NULL; >>> + } >> >> Similar question. > > Same as above, there’s supposed to be > > resets: > maxItems: 1 > > in the schema. Preparing an iteration of the series, I see there *was* the reset in the schema. Let me know if there is some issue with it. Martin > >> Best regards, >> Krzysztof >> > > Martin >
On 18/08/2022 20:54, Martin Povišer wrote: >> >> Same as above, there’s supposed to be >> >> resets: >> maxItems: 1 >> >> in the schema. > > Preparing an iteration of the series, I see there *was* the reset in > the schema. Let me know if there is some issue with it. > Indeed there is one, I missed it. It's OK. Best regards, Krzysztof