diff mbox series

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

Message ID 20210531194947.10770-2-andriy.shevchenko@linux.intel.com (mailing list archive)
State Superseded, archived
Headers show
Series [v2,1/7] docs: firmware-guide: ACPI: Add a PWM example | expand

Commit Message

Andy Shevchenko May 31, 2021, 7:49 p.m. UTC
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(-)

Comments

Uwe Kleine-König June 6, 2021, 9:30 p.m. UTC | #1
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
Andy Shevchenko June 7, 2021, 9:02 a.m. UTC | #2
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).
Uwe Kleine-König June 7, 2021, 9:53 a.m. UTC | #3
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
Andy Shevchenko June 7, 2021, 10:15 a.m. UTC | #4
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
Andy Shevchenko June 7, 2021, 10:21 a.m. UTC | #5
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.
Andy Shevchenko June 7, 2021, 11:49 a.m. UTC | #6
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?
Uwe Kleine-König June 7, 2021, 2:11 p.m. UTC | #7
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 mbox series

Patch

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