Re: [PATCH v2 2/7] pwm: core: Always require PWM flags to be provided

[Andy Shevchenko]

On Mon, Jun 07, 2021 at 01:21:01PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 07, 2021 at 01:15:20PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 07, 2021 at 11:53:24AM +0200, Uwe Kleine-König wrote:
> > > On Mon, Jun 07, 2021 at 12:02:37PM +0300, Andy Shevchenko wrote:
> > > > On Sun, Jun 06, 2021 at 11:30:54PM +0200, Uwe Kleine-König wrote:
> > > > > On Mon, May 31, 2021 at 10:49:42PM +0300, Andy Shevchenko wrote:
> > > > > > It makes little sense to make PWM flags optional since in case
> > > > > > of multi-channel consumer the flags can be optional only for
> > > > > > the last listed channel.
> > > > > 
> > > > > I think the same holds true for dt references.
> > > > 
> > > > Can you elaborate this? I haven't got what you are talking about, not a DT
> > > > expert here.
> > > 
> > > Ah no, I mixed that up. While the function that parses the phandle is
> > > flexible, for each pwm controller the number of arguments is fixed, so
> > > 
> > > 	pwms = <&pwm1 100000 &pwm2 100000 &pwm3 1000000>;
> > > 
> > > cannot be interpreted as 3-argument references to two PWMs. This is
> > > different to ACPI (I guess, not an ACPI expert here :-) because &pwm1
> > > "knows" if it needs 1 or 2 additional parameters (#pwm-cells).
> > 
> > It's not about ACPI, it's about "the ACPI glue layer in Linux kernel".
> > Used API is a part of it and it does allow only two cases, either NULL entry
> > (by having 0 as an argument) or full-length supplied tuple (in case of PWM it's
> > 3, so, means 4 parameters.
> > 
> > Let's consider examples:
> > 
> > (0, 0, x3, y3, z3, t3) // NULL, NULL, PWM3
> > (x1, y1, z1, t1, 0, x3, y3, z3, t3) // PWM1, NULL, PWM3
> > 
> > So, making last parameter "flexible" will work only for the last tuple in the
> > array.
> > 
> > Read this [1] for further information.
> > 
> > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/acpi/property.c#L629
> 
> Hmm... I have read the actual implementation and it seems it's possible to have
> flexible array, so this patch needs to be reconsidered.

I was thinking more about it and what we have here is positional-dependent
arguments. Either way we might end up in the same situation (when we need to
parse arguments based on their positions, rather than always have them being
present). So, while I won't change documentation example (to be more stricter
there), I will drop this change.

Also, the PWM initial state doesn't include duty cycle. Any explanations why is
that?

-- 
With Best Regards,
Andy Shevchenko