| Message ID | 20210531194947.10770-2-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/7] docs: firmware-guide: ACPI: Add a PWM example | expand |
Hello Andy, 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. > Thus always require PWM flags to be provided. I'm not sure I want to follow that conclusion. Most consumers only need a single PWM and then not providing flags is fine, isn't it? Or does this change fix an error? Best regards Uwe
On Sun, Jun 06, 2021 at 11:30:54PM +0200, Uwe Kleine-König wrote: > Hello Andy, > > 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. > > Thus always require PWM flags to be provided. > > I'm not sure I want to follow that conclusion. Most consumers only need > a single PWM and then not providing flags is fine, isn't it? Or does > this change fix an error? The current ACPI DSD implementation in the Linux kernel doesn't allow to use variadic arguments in the list of tuples. So, the current code states that we may use (x, y, z) or (x, y, z, t). That's true as long as that is the last tuple in the array (*). However, the case (x1, y1, z1, x2, y2, z2, t2) is not supported. It can be either (x1, y1, z1, t1, x2, y2, z2) or (x1, y1, z1, t1, x2, y2, z2, t2). But this makes a little sense and Linux APIs in use and first ABI that uses that API (GPIO) has never been designed for a such. What is allowed is to skip tuple in a form of NULL:ifying, like (0, x2, y2, z2, t2). *) Yes, the comments said that PWM supports only a single reference, but this may be expanded in the future and this patch allows to do that (it's not a fix per se, but rather a good clarification to the APIs/ABIs).
Hi Andy, 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: > > Hello Andy, > > > > 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). Best regards Uwe
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
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.
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?
Hello Andy, On Mon, Jun 07, 2021 at 02:49:01PM +0300, Andy Shevchenko wrote: > 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? This isn't technically the initial state. It's a hint to the consumer which period to pick. The duty-cycle is usually variable, but if I designed the binding today I would not include the period in the pwm handle. But to discuss this is moot---the binding is as it is. Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index c165c5822703..25f7b3370672 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -852,8 +852,10 @@ static struct pwm_chip *device_to_pwmchip(struct device *dev) * * This is analogous to of_pwm_get() except con_id is not yet supported. * ACPI entries must look like - * Package () {"pwms", Package () - * { <PWM device reference>, <PWM index>, <PWM period> [, <PWM flags>]}} + * Package () { "pwms", Package () { + * <PWM device reference>, <PWM index>, <PWM period>, <PWM flags>, + * } + * } * * Returns: A pointer to the requested PWM device or an ERR_PTR()-encoded * error code on failure. @@ -877,7 +879,7 @@ static struct pwm_device *acpi_pwm_get(struct fwnode_handle *fwnode) if (!acpi) return ERR_PTR(-EINVAL); - if (args.nargs < 2) + if (args.nargs < 3) return ERR_PTR(-EPROTO); chip = device_to_pwmchip(&acpi->dev); @@ -891,7 +893,7 @@ static struct pwm_device *acpi_pwm_get(struct fwnode_handle *fwnode) pwm->args.period = args.args[1]; pwm->args.polarity = PWM_POLARITY_NORMAL; - if (args.nargs > 2 && args.args[2] & PWM_POLARITY_INVERTED) + if (args.args[2] & PWM_POLARITY_INVERTED) pwm->args.polarity = PWM_POLARITY_INVERSED; #endif
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. Thus always require PWM flags to be provided. Fixes: 4a6ef8e37c4d ("pwm: Add support referencing PWMs from ACPI") Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- v2: no change drivers/pwm/core.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)