Re: [PATCH v4 1/2] pwm: meson: Add support for Amlogic S4 PWM

[George Stark <gnstark@salutedevices.com>]

Hello Jerome

On 4/24/24 13:32, Jerome Brunet wrote:
> 
> On Wed 24 Apr 2024 at 18:28, Kelvin Zhang via B4 Relay <devnull+kelvin.zhang.amlogic.com@kernel.org> wrote:
> 
>> From: Junyi Zhao <junyi.zhao@amlogic.com>
>>
>> This patch adds support for Amlogic S4 PWM.
>>
>> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>> ---
>>   drivers/pwm/pwm-meson.c | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index ea96c5973488..6abc823745e4 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -462,6 +462,35 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
>>   	return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
>>   }
>>   
>> +static int meson_pwm_init_channels_meson_s4(struct pwm_chip *chip)
>> +{
>> +	int i, ret;
>> +	struct device *dev = pwmchip_parent(chip);
>> +	struct device_node *np = dev->of_node;
>> +	struct meson_pwm *meson = to_meson_pwm(chip);
>> +	struct meson_pwm_channel *channel;
>> +
>> +	for (i = 0; i < MESON_NUM_PWMS; i++) {
>> +		channel = &meson->channels[i];
>> +		channel->clk = of_clk_get(np, i);
>> +		if (IS_ERR(channel->clk)) {
>> +			ret = PTR_ERR(channel->clk);
>> +			dev_err_probe(dev, ret, "Failed to get clk\n");
>> +			goto err;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> +err:
>> +	while (--i >= 0) {
>> +		channel = &meson->channels[i];
>> +		clk_put(channel->clk);
> 
> Fine on error but leaks on module unload.
> 
> Same as George,
> 
> Add the devm variant of of_clk_get() if you must.
> Use devm_add_action_or_reset() otherwise
> 
> Could please synchronize this series with George and deal with all the
> supported SoCs ? a1, s4, t7, c3 ...

If the chipmaker eagers to support s4 himself we're ok :)
But since I sent my patch first I think it'd be fair if this
single patch have my tag:
Co-Developed-by: George Stark <gnstark@salutedevices.com>

I'll help to review the patch too.

Jerome could we split support for all mentioned socs into different series?
e.g.
1. Junyi finishes the driver's base patch and s4 dtsi patch
2. I send a1 dt-bindings and a1 dtsi patches
3. Someone later sends t7/c3 dt-bindings + dtsi

The reason is to apply what we have on hand now due to meson-pwm is 
under heavy development more than a year already.

> 
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   static const struct meson_pwm_data pwm_meson8b_data = {
>>   	.parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
>>   	.channels_init = meson_pwm_init_channels_meson8b_legacy,
>> @@ -500,6 +529,10 @@ static const struct meson_pwm_data pwm_meson8_v2_data = {
>>   	.channels_init = meson_pwm_init_channels_meson8b_v2,
>>   };
>>   
>> +static const struct meson_pwm_data pwm_meson_s4_data = {
>> +	.channels_init = meson_pwm_init_channels_meson_s4,
>> +};
>> +
>>   static const struct of_device_id meson_pwm_matches[] = {
>>   	{
>>   		.compatible = "amlogic,meson8-pwm-v2",
>> @@ -538,6 +571,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,meson-s4-pwm",
>> +		.data = &pwm_meson_s4_data
>> +	},
>>   	{},
>>   };
>>   MODULE_DEVICE_TABLE(of, meson_pwm_matches);
> 
> 

-- 
Best regards
George