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

[Marek Vasut]

On 9/11/24 11:28 PM, Frank Li wrote:
> On Wed, Sep 11, 2024 at 10:31:40PM +0200, Marek Vasut wrote:
> > On 9/10/24 9:07 PM, Frank Li wrote:
> > > From: Liu Ying <victor.liu@xxxxxxx>
> > >
> > > PWM in i.MX8QXP MIPI subsystem needs the clock '32k'. Use it if the DTS
> > > provides that.
> > >
> > > Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> > > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > > ---
> > > 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.
>
> I have not touch regular PWM clock's code. Just add more optional clocks.
>
> devm_clk_bulk_get(imx->clks);
> devm_clk_bulk_get_optional(imx->clks + required_cnt);
>
> so imx->clks have two part {required_part, optional_part};
>
> require part is the same as the before. If it invalidate or not present,
> driver will fail probe.

Ah, understood, thank you for clarifying.

> > Also, it is not clear whether the 32kHz clock are really supplying the PWM,
> > see my comment on 1/3 in this series.
>
> Yes, it is for pwm.
Do the older SoCs (iMX8M or iMX6 or such) also need 32kHz clock for PWM?

I think what I am asking is, what exactly does consume the 32kHz clock 
in the PWM IP ?