Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
References: <20231016052457.1191838-1-junyi.zhao@amlogic.com>
 <10dcc778-f165-407e-b765-760d277d5e35@salutedevices.com>
User-agent: mu4e 1.8.13; emacs 29.1
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     George Stark <gnstark@salutedevices.com>,
        JunYi Zhao <junyi.zhao@amlogic.com>, thierry.reding@gmail.com,
        u.kleine-koenig@pengutronix.de, neil.armstrong@linaro.org,
        khilman@baylibre.com, martin.blumenstingl@googlemail.com,
        linux-pwm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH V3 RESEND] pwm: meson: add pwm support for S4
Date:   Fri, 27 Oct 2023 12:00:50 +0200
In-reply-to: <10dcc778-f165-407e-b765-760d277d5e35@salutedevices.com>
Message-ID: <1jzg041ina.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


On Mon 23 Oct 2023 at 23:07, George Stark <gnstark@salutedevices.com> wrote:

> Hello JunYi Zhao
>
> On 10/16/23 08:24, JunYi Zhao wrote:
>> From: "junyi.zhao" <junyi.zhao@amlogic.com>
>> Support PWM for S4 soc.
>> Now the PWM clock input is done in independent CLKCTRL registers.
>> And no more in the PWM registers.
>> PWM needs to obtain an external clock source.
>> Signed-off-by: junyi.zhao <junyi.zhao@amlogic.com>
>> ---
>> V2 -> V3:
>> Rebase and Review the latest upstream code again.
>> After reconstruction, stick to the previous code as much as possible.
>>   drivers/pwm/pwm-meson.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index 25519cddc2a9..fe9fd75747c4 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -99,6 +99,7 @@ struct meson_pwm_channel {
>>   struct meson_pwm_data {
>>   	const char * const *parent_names;
>>   	unsigned int num_parents;
>> +	unsigned int extern_clk;
> may be bool extern_clk;
>>   };
>>     struct meson_pwm {
>> @@ -396,6 +397,10 @@ static const struct meson_pwm_data pwm_g12a_ao_cd_data = {
>>   	.num_parents = ARRAY_SIZE(pwm_g12a_ao_cd_parent_names),
>>   };
>>   +static const struct meson_pwm_data pwm_s4_data = {
>> +	.extern_clk = true,
>> +};
>> +
>>   static const struct of_device_id meson_pwm_matches[] = {
>>   	{
>>   		.compatible = "amlogic,meson8b-pwm",
>> @@ -429,6 +434,10 @@ static const struct of_device_id meson_pwm_matches[] = {
>>   		.compatible = "amlogic,meson-g12a-ao-pwm-cd",
>>   		.data = &pwm_g12a_ao_cd_data
>>   	},
>> +	{
>> +		.compatible = "amlogic,s4-pwm",
>> +		.data = &pwm_s4_data,
>> +	},
>>   	{},
>>   };
>>   MODULE_DEVICE_TABLE(of, meson_pwm_matches);
>> @@ -451,6 +460,16 @@ static int meson_pwm_init_channels(struct meson_pwm *meson)
>>   		struct clk_parent_data div_parent = {}, gate_parent = {};
>>   		struct clk_init_data init = {};
>>   +		if (meson->data->extern_clk) {

Instead of hacking through the existing registration function, it be
much better to provide the clock registration function as on ops in dt data.

Also, as Neil pointed out on the v2 [1], the meaning of clkin0/1 is changed
on this SoC.
* On previous SoC, it was a reference to clock input the PWM block
  should select from the hard-coded list it has (should fix that
  someday)
* Now it is directly the input the PWM block must claim.

You need to update the bindings accordingly for the S4

[1]: https://lore.kernel.org/linux-amlogic/07581fb8-0cd9-5b76-6fa3-1d1a7353d944@baylibre.com

>> +			snprintf(name, sizeof(name), "clkin%u", i);
>> +			channel->clk = devm_clk_get(dev, name);
>> +			if (IS_ERR(channel->clk)) {
>> +				dev_err(meson->chip.dev, "can't get device clock\n");
>> +				return PTR_ERR(channel->clk);
>> +			}
>> +			continue;
>> +		}
>> +
>>   		snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i);
>>     		init.name = name;
>> base-commit: 4d2c646ac07cf4a35ef1c4a935a1a4fd6c6b1a36