[PATCH v1 0/2] Enable WDT reload feature

[Uwe Kleine-König]

Hello Billy,

On Fri, Oct 25, 2024 at 02:00:39AM +0000, Billy Tsai wrote:
> > On 10/24/24 08:40, Uwe Kleine-König wrote:
> > > On Thu, Oct 24, 2024 at 03:15:46PM +0800, Billy Tsai wrote:
> > >> Aspeed PWM controller has the WDT reload feature, which changes the duty
> > >> cycle to a preprogrammed value after a WDT/EXTRST#.
> > >>
> > >> Billy Tsai (2):
> > >>    hwmon: (aspeed-g6-pwm-tacho): Extend the #pwm-cells to 4
> > >>    hwmon: (aspeed-g6-pwm-tacho): Support the WDT reload
> > >
> > > Huh, I'm not convinced that extending #pwm-cells for that feature is a
> > > good idea. Unless I'm missing something none of the other supported PWM
> > > chips can do that, so I hesitate to change a standard for it. I suggest
> > > to make this a separate property instead.
>
> Using a separate property to enable this feature is a straightforward
> method, but I don’t understand why extending #pwm-cells isn’t a good
> idea in my situation. The feature ‘WDT reload’ can be set for
> individual PWM channels, and the PWM subsystem has the of_xlate
> callback hook, which allows each driver to define its arguments for
> the PWM consumer. I’m unsure if I misunderstood this callback usage,
> as I couldn’t find examples. If my understanding is correct, this
> method is better for adding our specific feature, rather than using
> child nodes or separate properties to indicate which PWM channel
> should enable this feature with the corresponding duty cycle values. I
> think using separate properties to achieve this feature would be quite
> cumbersome.
> As I know the arguments for this usage are as follows:
> First: PWM channel index
> Second: PWM period in ns
> Third: PWM polarity
> Therefore, I extended our feature to a fourth argument to avoid any confusion regarding usage and added the description in our yaml file.
> 
> If my thinking is incorrect or doesn’t make sense, please let me know.

It might make sense if your bubble only contains that single PWM
hardware. However if you extend the pwm cells semantic for your PWM to
mean "period if the PWM watchdog triggers", i can hardly refuse the next
developer who wants to extend for "period of the secondary output pin of
the PWM" or a step counter or some property that defines how the duty
cycle is modulated over time. And should the next one also use the 4th
u32 for his purpose, or should we consider it reserved now for your
purpose such that the duty_cycle modulation goes into the 7th cell?

Today the bindings are (well nearly) used in the same way for all
hardwares and I want to keep it that way. If your PWM has a special
feature, give it a speaking name that the occasional dts reader has a
chance to understand without reading HW docs or dt bindings.

Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20241025/57fa68ef/attachment.sig>