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

[claudiu beznea <claudiu.beznea@tuxon.dev>]

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;
>   }