Re: [PATCH 0/5] gpio: mvebu: Armada 8K/7K PWM support

[Baruch Siach <baruch@tkos.co.il>]

Hi Andrew,

Thanks for your review comments.

On Thu, Nov 19 2020, Andrew Lunn wrote:
> On Wed, Nov 18, 2020 at 12:30:41PM +0200, Baruch Siach wrote:
>> The gpio-mvebu driver supports the PWM functionality of the GPIO block for 
>> earlier Armada variants like XP, 370 and 38x. This series extends support to 
>> newer Armada variants that use CP11x and AP80x, like Armada 8K and 7K.
>> 
>> This series adds adds the 'pwm-offset' property to DT binding. 'pwm-offset' 
>
> One adds is enough.
>
>> points to the base of A/B counter registers that determine the PWM period and 
>> duty cycle.
>> 
>> The existing PWM DT binding reflects an arbitrary decision to allocate the A 
>> counter to the first GPIO block, and B counter to the other one.
>
> It was not arbitrary. I decided on KISS. The few devices i've seen
> using this have been for a single GPIO/PWN controlled fan. KISS was
> sufficient for that, so why make it more complex?

In saying "arbitrary" I don't mean to say it's not a good choice in the
context of the Linux PWM and GPIO subsystems. But this choice is still
arbitrary from hardware point of view. DT is meant to describe the
hardware. I think that coding the A/B counters allocation choice in DT
is not optimal in terms for hardware description. Both counters are
usable for both GPIO blocks.

I also don't see how this makes anything more complex. The driver code
is already aware of this A/B allocation (GPIO_BLINK_CNT_SELECT_OFF). My
suggestion is to keep this decision in the driver, and leave DT to
describe the hardware. This costs us a single code line (patch #3):

  mvpwm->offset += PWM_BLINK_COUNTER_B_OFF;

Does that make sense?

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -