[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <522db306-64b1-42f5-8446-e6dd56eea7ec@denx.de>
Date: Wed, 11 Sep 2024 23:33:02 +0200
From: Marek Vasut <marex@...x.de>
To: Frank Li <Frank.li@....com>
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 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@....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.
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 ?
Powered by blists - more mailing lists