[1/5] pwm: atmel-tcb: Harmonize resource allocation order

Allocate driver data as first resource in the probe function. This way it
can be used during allocation of the other resources (instead of assigning
these to local variables first and update driver data only when it's
allocated). Also as driver data is allocated using a devm function this
should happen first to have the order of freeing resources in the error
path and the remove function in reverse.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-atmel-tcb.c | 49 +++++++++++++++----------------------
 1 file changed, 20 insertions(+), 29 deletions(-)

On 19.07.2023 22:20, Uwe Kleine-König wrote:
> Allocate driver data as first resource in the probe function. This way it
> can be used during allocation of the other resources (instead of assigning
> these to local variables first and update driver data only when it's
> allocated). Also as driver data is allocated using a devm function this
> should happen first to have the order of freeing resources in the error
> path and the remove function in reverse.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   drivers/pwm/pwm-atmel-tcb.c | 49 +++++++++++++++----------------------
>   1 file changed, 20 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
> index 4a116dc44f6e..613dd1810fb5 100644
> --- a/drivers/pwm/pwm-atmel-tcb.c
> +++ b/drivers/pwm/pwm-atmel-tcb.c
> @@ -422,13 +422,14 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>   	struct atmel_tcb_pwm_chip *tcbpwm;
>   	const struct atmel_tcb_config *config;
>   	struct device_node *np = pdev->dev.of_node;
> -	struct regmap *regmap;
> -	struct clk *clk, *gclk = NULL;
> -	struct clk *slow_clk;
>   	char clk_name[] = "t0_clk";
>   	int err;
>   	int channel;
>   
> +	tcbpwm = devm_kzalloc(&pdev->dev, sizeof(*tcbpwm), GFP_KERNEL);
> +	if (tcbpwm == NULL)

I know this was previously but maybe we can change it like this now:
	if (!tcbpwm)

this is how is done in most of the memory alloc failure checks (AFAICT).

> +		return -ENOMEM;
> +
>   	err = of_property_read_u32(np, "reg", &channel);
>   	if (err < 0) {
>   		dev_err(&pdev->dev,
> @@ -437,47 +438,37 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>   		return err;
>   	}
>   
> -	regmap = syscon_node_to_regmap(np->parent);
> -	if (IS_ERR(regmap))
> -		return PTR_ERR(regmap);
> +	tcbpwm->regmap = syscon_node_to_regmap(np->parent);
> +	if (IS_ERR(tcbpwm->regmap))
> +		return PTR_ERR(tcbpwm->regmap);
>   
> -	slow_clk = of_clk_get_by_name(np->parent, "slow_clk");
> -	if (IS_ERR(slow_clk))
> -		return PTR_ERR(slow_clk);
> +	tcbpwm->slow_clk = of_clk_get_by_name(np->parent, "slow_clk");
> +	if (IS_ERR(tcbpwm->slow_clk))
> +		return PTR_ERR(tcbpwm->slow_clk);
>   
>   	clk_name[1] += channel;
> -	clk = of_clk_get_by_name(np->parent, clk_name);
> -	if (IS_ERR(clk))
> -		clk = of_clk_get_by_name(np->parent, "t0_clk");
> -	if (IS_ERR(clk))
> -		return PTR_ERR(clk);
> +	tcbpwm->clk = of_clk_get_by_name(np->parent, clk_name);
> +	if (IS_ERR(tcbpwm->clk))
> +		tcbpwm->clk = of_clk_get_by_name(np->parent, "t0_clk");
> +	if (IS_ERR(tcbpwm->clk))
> +		return PTR_ERR(tcbpwm->clk);
>   
>   	match = of_match_node(atmel_tcb_of_match, np->parent);
>   	config = match->data;
>   
>   	if (config->has_gclk) {
> -		gclk = of_clk_get_by_name(np->parent, "gclk");
> -		if (IS_ERR(gclk))
> -			return PTR_ERR(gclk);
> -	}
> -
> -	tcbpwm = devm_kzalloc(&pdev->dev, sizeof(*tcbpwm), GFP_KERNEL);
> -	if (tcbpwm == NULL) {
> -		err = -ENOMEM;
> -		goto err_slow_clk;
> +		tcbpwm->gclk = of_clk_get_by_name(np->parent, "gclk");
> +		if (IS_ERR(tcbpwm->gclk))
> +			return PTR_ERR(tcbpwm->gclk);
>   	}
>   
>   	tcbpwm->chip.dev = &pdev->dev;
>   	tcbpwm->chip.ops = &atmel_tcb_pwm_ops;
>   	tcbpwm->chip.npwm = NPWM;
>   	tcbpwm->channel = channel;
> -	tcbpwm->regmap = regmap;
> -	tcbpwm->clk = clk;
> -	tcbpwm->gclk = gclk;
> -	tcbpwm->slow_clk = slow_clk;
>   	tcbpwm->width = config->counter_width;
>   
> -	err = clk_prepare_enable(slow_clk);
> +	err = clk_prepare_enable(tcbpwm->slow_clk);
>   	if (err)
>   		goto err_slow_clk;
>   
> @@ -495,7 +486,7 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>   	clk_disable_unprepare(tcbpwm->slow_clk);
>   
>   err_slow_clk:
> -	clk_put(slow_clk);
> +	clk_put(tcbpwm->slow_clk);
>   
>   	return err;
>   }

Hello claudiu,

On Thu, Jul 27, 2023 at 09:00:28AM +0300, claudiu beznea wrote:
> On 19.07.2023 22:20, Uwe Kleine-König wrote:
> > +	tcbpwm = devm_kzalloc(&pdev->dev, sizeof(*tcbpwm), GFP_KERNEL);
> > +	if (tcbpwm == NULL)
> 
> I know this was previously but maybe we can change it like this now:
> 	if (!tcbpwm)
> 
> this is how is done in most of the memory alloc failure checks (AFAICT).
> 
> > ...
> > -	tcbpwm = devm_kzalloc(&pdev->dev, sizeof(*tcbpwm), GFP_KERNEL);
> > -	if (tcbpwm == NULL) {

I don't care much. If and when I resend I will change to !tcbpwm.

Best regards
Uwe