Re: [PATCH v3 3/3] pwm: imx27: Add optional 32k clock for pwm in i.MX8QXP MIPI subsystem

[Marek Vasut <marex@denx.de>]

On 9/10/24 9:07 PM, Frank Li wrote:
> From: Liu Ying <victor.liu@nxp.com>
> 
> PWM in i.MX8QXP MIPI subsystem needs the clock '32k'. Use it if the DTS
> provides that.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v2 to v3
> - use buck clk API
> 
> Change from v1 to v2
> - remove if check for clk
> - use dev_err_probe
> - remove int val
> ---
>   drivers/pwm/pwm-imx27.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index ce9208540f1b8..2a9fba6f9d0a8 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -81,10 +81,11 @@
>   #define MX3_PWMPR_MAX			0xfffe
>   
>   static const char * const pwm_imx27_clks[] = {"ipg", "per"};
> +static const char * const pwm_imx27_opt_clks[] = {"32k"};
>   #define PWM_IMX27_PER			1
>   
>   struct pwm_imx27_chip {
> -	struct clk_bulk_data clks[ARRAY_SIZE(pwm_imx27_clks)];
> +	struct clk_bulk_data clks[ARRAY_SIZE(pwm_imx27_clks) + ARRAY_SIZE(pwm_imx27_opt_clks)];
>   	int clks_cnt;
>   	void __iomem	*mmio_base;
>   
> @@ -371,6 +372,16 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>   		return dev_err_probe(&pdev->dev, ret,
>   				     "getting clocks failed\n");
>   
> +	for (i = 0; i < ARRAY_SIZE(pwm_imx27_opt_clks); i++)
> +		imx->clks[i + imx->clks_cnt].id = pwm_imx27_opt_clks[i];
> +
> +	ret = devm_clk_bulk_get_optional(&pdev->dev, ARRAY_SIZE(pwm_imx27_opt_clks),
> +					 imx->clks + imx->clks_cnt);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "get optional clocks failed\n");
> +
> +	imx->clks_cnt += ARRAY_SIZE(pwm_imx27_opt_clks);
> +

This will succeed even if the regular PWM clock are invalid or not 
present, wouldn't it? I don't think removing that protection is an 
improvement.

Also, it is not clear whether the 32kHz clock are really supplying the 
PWM, see my comment on 1/3 in this series.