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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZuILmHRO9rIXfxIm@lizhi-Precision-Tower-5810>
Date: Wed, 11 Sep 2024 17:28:56 -0400
From: Frank Li <Frank.li@....com>
To: Marek Vasut <marex@...x.de>
Cc: Uwe Kleine-König <ukleinek@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Philipp Zabel <p.zabel@...gutronix.de>, linux-pwm@...r.kernel.org,
	devicetree@...r.kernel.org, imx@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	pratikmanvar09@...il.com, francesco@...cini.it,
	Liu Ying <victor.liu@....com>
Subject: Re: [PATCH v3 3/3] pwm: imx27: Add optional 32k clock for pwm in
 i.MX8QXP MIPI subsystem

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@....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@....com>
> > Signed-off-by: Frank Li <Frank.Li@....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.

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.

>
> 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.