Re: [PATCH v3 07/13] media: meson-ir-tx: Ensure clock is disabled on unbind

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the error code from `devm_clk_get_enabled()` is unconditionally masked with `-ENODEV`, which breaks probe deferral.
- [High] This problem wasn't introduced by this patch, but the error return from `platform_get_irq()` is masked by returning `-ENODEV`, breaking probe deferral.
--

--- Patch [7]: [PATCH v3 07/13] media: meson-ir-tx: Ensure clock is disabled on unbind ---
commit 902362d56de8e550d8a16fef5053c172039aa2ab
Author: Sean Young <[email protected]>
Subject: media: meson-ir-tx: Ensure clock is disabled on unbind

This commit replaces the manual devm_clk_get and clk_prepare_enable calls
with devm_clk_get_enabled. This ensures that the clock is properly
disabled when the driver is unbound.

> diff --git a/drivers/media/rc/meson-ir-tx.c b/drivers/media/rc/meson-ir-tx.c
> index fded2c256f2a6..e7bb107e6a843 100644
> --- a/drivers/media/rc/meson-ir-tx.c
> +++ b/drivers/media/rc/meson-ir-tx.c
> @@ -288,8 +288,8 @@ static int meson_irtx_mod_clock_probe(struct meson_irtx *ir,
>  	if (!np)
>  		return -ENODEV;
>  
> -	clock = devm_clk_get(ir->dev, "xtal");
> -	if (IS_ERR(clock) || clk_prepare_enable(clock))
> +	clock = devm_clk_get_enabled(ir->dev, "xtal");
> +	if (IS_ERR(clock))
>  		return -ENODEV;

[Severity: High]
This is a pre-existing issue, but does dropping the actual error code from
devm_clk_get_enabled() and returning -ENODEV break probe deferral?

If the clock provider isn't ready, devm_clk_get_enabled() returns
-EPROBE_DEFER, which gets masked here. Should this return PTR_ERR(clock)
instead?

[ ... ]

While reviewing this file, I also noticed another pre-existing issue in
the same driver:

drivers/media/rc/meson-ir-tx.c:meson_irtx_probe() {
    ...
    irq = platform_get_irq(pdev, 0);
    if (irq < 0)
        return -ENODEV;
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but would returning a hardcoded
-ENODEV here break probe deferral if the interrupt controller is not yet
ready?

When platform_get_irq() returns -EPROBE_DEFER, it seems the code currently
discards it. Would returning the actual error code be more appropriate?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic