On Wed, 10 Aug 2022 16:07:24 +0200
Julien Panis <jpanis@xxxxxxxxxxxx> wrote:
> ECAP hardware on AM62x SoC supports capture feature. It can be used
> to timestamp events (falling/rising edges) detected on signal input pin.
>
> This commit adds capture driver support for ECAP hardware on AM62x SoC.
>
> In the ECAP hardware, capture pin can also be configured to be in
> PWM mode. Current implementation only supports capture operating mode.
> Hardware also supports timebase sync between multiple instances, but
> this driver supports simple independent capture functionality.
>
> Signed-off-by: Julien Panis <jpanis@xxxxxxxxxxxx>
> ---
Hi Julien,
Been too long since I reviewed a counter driver, so I'll have
to leave that side of things to William (or find a spare few days
to remind myself of the details!)
A few superficial things I notice below whilst taking a quick look.
It might be worth adding a bit of description around what the
result of the various runtime pm calls is (what's actually getting
powered down?)
Jonathan
> +static int ecap_cnt_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct ecap_cnt_dev *ecap_dev;
> + struct counter_device *counter_dev;
> + void __iomem *mmio_base;
> + int ret;
> +
> + counter_dev = devm_counter_alloc(dev, sizeof(*ecap_dev));
> + if (IS_ERR(counter_dev))
> + return PTR_ERR(counter_dev);
> +
> + counter_dev->name = ECAP_DRV_NAME;
> + counter_dev->parent = dev;
> + counter_dev->ops = &ecap_cnt_ops;
> + counter_dev->signals = ecap_cnt_signals;
> + counter_dev->num_signals = ARRAY_SIZE(ecap_cnt_signals);
> + counter_dev->counts = ecap_cnt_counts;
> + counter_dev->num_counts = ARRAY_SIZE(ecap_cnt_counts);
> +
> + ecap_dev = counter_priv(counter_dev);
> +
> + ecap_dev->clk = devm_clk_get(dev, "fck");
> + if (IS_ERR(ecap_dev->clk))
> + return dev_err_probe(dev, PTR_ERR(ecap_dev->clk), "failed to get clock\n");
> +
> + ret = clk_prepare_enable(ecap_dev->clk);
We now have dev_clk_get_enabled() in upstream - finally!
Text book usecase for it here.
> + if (ret) {
> + dev_err(dev, "failed to enable clock\n");
> + return ret;
> + }
> +
> + ret = devm_add_action_or_reset(dev, ecap_cnt_clk_disable, ecap_dev->clk);
> + if (ret) {
> + dev_err(dev, "failed to add clock disable action\n");
> + return ret;
> + }
> +
> + ecap_dev->clk_rate = clk_get_rate(ecap_dev->clk);
> + if (!ecap_dev->clk_rate) {
> + dev_err(dev, "failed to get clock rate\n");
> + return -EINVAL;
> + }
> +
> + mmio_base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(mmio_base))
> + return PTR_ERR(mmio_base);
> +
> + ecap_dev->regmap = devm_regmap_init_mmio(dev, mmio_base, &ecap_cnt_regmap_config);
> + if (IS_ERR(ecap_dev->regmap))
> + return dev_err_probe(dev, PTR_ERR(ecap_dev->regmap), "failed to init regmap\n");
> +
> + spin_lock_init(&ecap_dev->lock);
> +
> + ret = platform_get_irq(pdev, 0);
> + if (ret < 0) {
> + dev_err(dev, "failed to get irq\n");
> + return ret;
> + }
> +
> + ret = devm_request_irq(dev, ret, ecap_cnt_isr, 0, pdev->name, counter_dev);
> + if (ret) {
> + dev_err(dev, "failed to request irq\n");
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, counter_dev);
> + pm_runtime_enable(dev);
> +
> + ecap_dev->enabled = 0;
> + ecap_cnt_capture_set_evmode(counter_dev, 0);
> +
> + ret = devm_counter_add(dev, counter_dev);
> + if (ret) {
> + dev_err(dev, "failed to add counter\n");
> + pm_runtime_disable(dev);
Unless there is a very good reason to mix devm and unmanaged
code, it's best to not do so as it leads to much head scratching over
whether there are race conditions. Here I can't see a reason not
to use devm_add_action_or_reset() to make the pm_runtime_disabled()
into devm managed.
Any setting of enabled occured after probe() was done so that's
fine as unmanaged or you could register another devm_ callback
if you prefer, but with a comment explaining the path to it needing
to do anything.
> + }
> +
> + return ret;
> +}
> +
> +static int ecap_cnt_remove(struct platform_device *pdev)
> +{
> + struct counter_device *counter_dev = platform_get_drvdata(pdev);
> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> +
> + if (ecap_dev->enabled)
> + ecap_cnt_capture_disable(counter_dev);
> +
> + pm_runtime_disable(&pdev->dev);
> +
> + return 0;
> +}
> +
