> Subject: Re: [PATCH 01/21] pinctrl: ti: iodelay: Use scope based of_node_put()
> cleanups
>
> On Wed, May 01, 2024 at 08:55:59PM +0800, Peng Fan (OSS) wrote:
> > @@ -879,16 +874,12 @@ static int ti_iodelay_probe(struct
> platform_device *pdev)
> > ret = pinctrl_register_and_init(&iod->desc, dev, iod, &iod->pctl);
> > if (ret) {
> > dev_err(dev, "Failed to register pinctrl\n");
> > - goto exit_out;
> > + return ret;
> > }
> >
> > platform_set_drvdata(pdev, iod);
> >
> > return pinctrl_enable(iod->pctl);
> > -
> > -exit_out:
> > - of_node_put(np);
> > - return ret;
> > }
>
> This will call of_node_put() on the success path so it's a behavior change. The
> original code is buggy, it's supposed to call of_node_put() on the success path
> here or in ti_iodelay_remove().
>
> If it's supposed to call of_node_put() here, then fine, this is bugfix but if it's
> supposed to call it in ti_iodelay_remove() then we need to save the pointer
> somewhere using no_free_ptr(). Probably saving ->np is the safest choice?
>
> The original code is already a little bit buggy because it doesn't check for
> pinctrl_enable() errors and cleanup.
It was introduced by
commit 6118714275f0a313ecc296a87ed1af32d9691bed (tag: pinctrl-v4.11-4)
Author: Tony Lindgren <tony@xxxxxxxxxxx>
Date: Thu Mar 30 09:16:39 2017 -0700
pinctrl: core: Fix pinctrl_register_and_init() with pinctrl_enable()
of_node_put is expected in probe, not in remove.
Thanks,
Peng.
>
>
> diff --git a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c b/drivers/pinctrl/ti/pinctrl-ti-
> iodelay.c
> index 040f2c46a868..f40a1476e4ff 100644
> --- a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
> +++ b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
> @@ -156,6 +156,7 @@ struct ti_iodelay_device {
>
> const struct ti_iodelay_reg_data *reg_data;
> struct ti_iodelay_reg_values reg_init_conf_values;
> + struct device_node *np;
> };
>
> /**
> @@ -884,7 +885,12 @@ static int ti_iodelay_probe(struct platform_device
> *pdev)
>
> platform_set_drvdata(pdev, iod);
>
> - return pinctrl_enable(iod->pctl);
> + ret = pinctrl_enable(iod->pctl);
> + if (ret)
> + goto exit_out;
> +
> + iod->np = no_free_ptr(np);
> + return 0;
>
> exit_out:
> of_node_put(np);
> @@ -903,6 +909,7 @@ static void ti_iodelay_remove(struct platform_device
> *pdev)
> pinctrl_unregister(iod->pctl);
>
> ti_iodelay_pinconf_deinit_dev(iod);
> + of_node_put(iod->np);
>
> /* Expect other allocations to be freed by devm */ }
>
>
>
