| Message ID | 20240910-pwm-v3-3-fbb047896618@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pwm: imx: add 32k clock for 8qm/qxp | expand |
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.
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@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. 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.
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@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. > > 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 ?
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); + chip->ops = &pwm_imx27_ops; imx->mmio_base = devm_platform_ioremap_resource(pdev, 0);