Re: [PATCH 2/2] pwm: meson: Add support for Amlogic S7

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

Hi Martin,
    Thanks for your review.

On 2026/3/31 05:44, Martin Blumenstingl wrote:
> Hi Xianwei Zhao,
> 
> On Thu, Mar 26, 2026 at 7:35 AM Xianwei Zhao via B4 Relay
> <devnull+xianwei.zhao.amlogic.com@kernel.org>  wrote:
>> From: Xianwei Zhao<xianwei.zhao@amlogic.com>
>>
>> Add support for Amlogic S7 PWM. Amlogic S7 different from the
>> previous SoCs, a controller includes one pwm, at the same time,
>> the controller has only one input clock source.
>>
>> Signed-off-by: Xianwei Zhao<xianwei.zhao@amlogic.com>
>> ---
>>   drivers/pwm/pwm-meson.c | 32 ++++++++++++++++++++++++++++++--
>>   1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index 8c6bf3d49753..3d16694e254e 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -113,6 +113,7 @@ struct meson_pwm_data {
>>          int (*channels_init)(struct pwm_chip *chip);
>>          bool has_constant;
>>          bool has_polarity;
>> +       bool single_pwm;
> At first I wasn't sure about this and thought we should replace it
> with a num_pwms (or similar) variable.
> However, I think it will be hard to add a third (or even more)
> channels to the PWM controller (not just from driver perspective but
> also from hardware perspective). So I think this is good enough as the
> choice will only be 1 or 2.
> > [...]

This is not a third channel added here.
Compared with the previous controller having two channels, here the 
control has only one channel. It's equivalent to the first channel 
before, while the second channel is reserved.

>> +static const struct meson_pwm_data pwm_s7_data = {
>> +       .channels_init = meson_pwm_init_channels_s7,
> I think you can use .channels_init = meson_pwm_init_channels_s4, if
> you change the code inside that function from:
>      for (i = 0; i < MESON_NUM_PWMS; i++) {
> to:
>      for (i = 0; i < chip->npwm; i++) {
> 
> [...]

The method you suggested was exactly what I did in the first version, 
but after my subsequent optimization, it's what you see now.

Since initialization only involves obtaining the clock, I modify the 
code less in this way and the logic is also simpler.

>> @@ -650,9 +674,13 @@ static int meson_pwm_probe(struct platform_device *pdev)
>>   {
>>          struct pwm_chip *chip;
>>          struct meson_pwm *meson;
>> +       const struct meson_pwm_data *pdata = of_device_get_match_data(&pdev->dev);
>>          int err;
>>
>> -       chip = devm_pwmchip_alloc(&pdev->dev, MESON_NUM_PWMS, sizeof(*meson));
>> +       if (pdata->single_pwm)
>> +               chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*meson));
>> +       else
>> +               chip = devm_pwmchip_alloc(&pdev->dev, MESON_NUM_PWMS, sizeof(*meson));
> I don't think this code is too bad for now.
> However, I'm wondering if you want to make "channels" from struct
> meson_pwm a flexible array member in a future patch. In that case it
> will be helpful to have an "unsigned int npwm = pdata->single_pwm ? 1
> : MESON_NUM_PWMS;" (or similar) variable to future-proof your code.
> What do you think?

I considered this, but chose the current implementation. I will switch 
to your suggestion in the next version.

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic