On Wed, Oct 17, 2018 at 12:41:53PM +0000, Claudiu.Beznea at microchip.com wrote:

On 16.10.2018 15:03, Thierry Reding wrote:

quoted

On Fri, Sep 14, 2018 at 06:20:48PM +0200, Nicolas Ferre wrote:

quoted

Thierry,

On 28/08/2018 at 15:01, Claudiu Beznea wrote:

quoted

Hi,

Please give feedback on these patches which extends the PWM framework in
order to support multiple PWM modes of operations. This series is a rework
of [1] and [2].

This series started with a RFC back on 5 April 2017 "extend PWM framework to
support PWM modes". The continuous work starting with v2 of this series on
January 12, 2018.

Then Claudiu tried to address all comments up to v4 which didn't have any
more reviews. He posted a v5 without comments since May 22, 2018. This
series is basically a resent of the v5 (as said in the $subject).

We would like to know what is preventing this series to be included in the
PWM sub-system. Note that if some issue still remain with it, we are ready
to help to solve them.

Without feedback from you side, we fear that we would miss a merge window
again for no obvious reason (DT part is Acked by Rob: patch 5/9).

First off, apologies for not getting around to this earlier.

I think this series is mostly fine, but I still have doubts about the DT
aspects of this. In particular, Rob raised a concern about this here:

	https://lkml.org/lkml/2018/1/22/655

and it seems like that particular question was never fully resolved as
the discussion veered off that particular topic.

1/ If you are talking about this sentence:
"Yes, but you have to make "normal" be no bit set to be compatible with
everything already out there."

The current implementation consider that if no mode is provided then, the
old approach is considered, meaning the normal mode will be used by every
PWM in-kernel clients.

In of_pwm_xlate_with_flags() the pmw->args.mode is initialized with what
pwm_mode_get_valid() returns. In case of controllers which does not
implement something special for PWM modes the PWM normal mode will be
returned (pwmchip_get_default_caps() function has to be called in the end).
Otherwise the pwm->args.mode will be populated with what user provided as
input from DT, if what was provided from DT is valid for PWM channel.
Please see that pwm_mode_valid() is used to validate user input, otherwise
PWM normal mode will be used.

No, that part looks fine.

+	pwm->args.mode = pwm_mode_get_valid(pc, pwm);

-	if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
-		pwm->args.polarity = PWM_POLARITY_INVERSED;
+	if (args->args_count > 2) {
+		if (args->args[2] & PWM_POLARITY_INVERTED)
+			pwm->args.polarity = PWM_POLARITY_INVERSED;
+
+		for (modebit = PWMC_MODE_COMPLEMENTARY_BIT;
+		     modebit < PWMC_MODE_CNT; modebit++) {
+			unsigned long mode = BIT(modebit);
+
+			if ((args->args[2] & mode) &&
+			    pwm_mode_valid(pwm, mode)) {
+				pwm->args.mode = mode;
+				break;
+			}
+		}
+	}


2/ If you are talking about this sentence:
"Thinking about this some more, shouldn't the new modes just be
implied? A client is going to require one of these modes or it won't
work right."

As explained at point 1, if there is no mode requested from DT the default
mode for channel will be used, which, in case of PWM controller which are
not implementing the new modes, will be PWM normal mode.

I don't think that's an issue. I think what Rob was referring to and
which mirrors my concern is that these modes are a feature that doesn't
extend to typical use-cases. So for all existing use-cases (like LED or
backlight) we always assume a PWM running in normal mode. Now, if you
write a driver for some particular piece of hardware that needs a mode
that is not the normal mode, the question is: wouldn't that driver know
that it wants exactly push-pull or complementary mode? Wouldn't it have
to explicitly check that the PWM supports it and select it (i.e. in the
driver code)?

Say you have a driver that requires push-pull mode. It doesn't really
make sense to require the mode to be encoded in DT, because the driver
will only work with one specific mode anyway. So might as well require
it and have the driver check for support and fail if the PWM is not
compatible. This would likely never happen, because hardware engineers
couldn't have validated the design in that case, but there's no reason
for the mode to be specified in DT because it is fixed by the very use-
case anyway.

Also, leaving it out of DT simplifies things. If you allow the mode to
be specified in DT you could end up with a situation where the driver
required push-pull mode, but the DT specifies complementary mode. What
do you do in such a situation? Warn about it and just go with push-pull
anyway? Then why give the users a way of screwing things up in the first
place?

3/ If you are talking about:
"Also complementary mode could be accomplished with a single pwm output
and a board level inverter, right? How would that be handled when the
PWM driver doesn't support that mode?"
This complicates the things and I think it requires extra device tree
bindings to describe extra per-pwm channel capabilities. For the moment,
with this series I've stopped to only have the capabilities registered from
driver. My understanding is that this could be accomplished with extra
device tree bindings in PWM controller to describe PWM channels
capabilities. If you also want me to look into this direction please let me
know. And also, suggestions would be appreciated.

I think this is very interesting, but I don't think this needs to be
done as part of this series.

I know that Rob acked

quoted

the DT parts of this, but I suspect that this might have been glossed
over.

If this is about point 3 I've emphasize above I would like to have some
inputs from your side, if you agree with a behavior like having extra DT
bindings. The intention wasn't to left this over but to have a better
understanding of the subject. I'm thinking if it is ok to have modules
outside of the SoC that model a behavior that could not be controlled from
software (it could be only hardware) but to behave in a single way. Take
for instance this scenario:

- new DT bindings are implemented to specify this behavior per channel
- hardware modules are soldered outside of a PWM channel with one output
- the new DT bindings are not specified for the soldered PWM channel
- user enables this channel, it will have only normal mode available for it
to configure (because the new DT bindings were not provided)
- but the real output the user will see would be in complementary or even
push-pull mode.

I think we could possible model this as a "logical" PWM in DT. We could
for example have something like this:

	/ {
		...

		pwms {
			pwm at 0 {
				compatible = "pwm-fixed"; /* or whatever */
				pwms = <&pwm 0 40000>; /* input PWM */
				mode = <PWM_MODE_COMPLEMENTARY>;
			};

			...
		};

		...
	};

The above would model a logical PWM that is driven by the specified PWM
in normal mode but which is effectively complementary because of some
additional circuitry on the board.

quoted

To restate the concern: these extended modes have special uses and none
of the users in the kernel, other than sysfs, can use anything other
than the normal mode. They may work fine with other modes, but only if
they ignore the extras that come with them. Therefore I think it's safe
to say that anyone who would want to use these modes would want to
explicitly say so. For example the sysfs interface already does that by
changing the mode only after the "mode" attribute is written. Any users
for special use-cases would want to do the same thing, that is, drive a
PWM in a specific mode, on purpose. You wouldn't have a "generic" user
such as pwm-backlight or leds-pwm request anything other than the normal
mode.

So my question is, do we really need to represent these modes in DT? The
series currently doesn't contain any patches that add users of these new
modes. Are such patches available somewhere, or is the only user of this
supposed to be sysfs?

For the moment, no, there is no in-kernel user for this, only sysfs. I had
in mind to adapt the use of these new mode for PWM regulator for scenarios
described in [1] page 2126. Anyway, since these patches doesn't introduce
any user other that sysfs it will not disturbed me to drop the changes. By
the time I or someone else will do some other changes that requires this,
the DT part should also be introduced.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf

Yes, I'd like that. The half-bridge converter certainly sounds like
something that may be able to use the DT bindings that you specified,
but I'd be less concerned about these changes if we had actual users.

quoted

I'm hesitant to move forward with this as-is without seeing how it will
be used.

For the moment only sysfs is supposed to use this.

The PWM specifier flags are somewhat abused by adding modes to

quoted

them. 

I see.

I think this hasn't been completely thought through, because the

quoted

only reason to specify a mode is to actually set that mode.

Maybe it wasn't clear understand, with the current implementation if no
mode will be specified the default mode will be used. There is no impose to
specify a mode.

 But then the

quoted

DT ABI allows a bitmask of modes to be requested via DT. I know that
only the first of those modes will end up being used, but then why even
allow it in the first place?

I had in mind that I will change the PWM regulator driver to work in
scenarios like the one specified in the link above.

Yeah, that sounds like it would be reasonable from a quick look.
However, what I don't quite understand yet is why the mode in the PWM
specifier would need to be a bitmask. Take for example the pwm-regulator
case for a half-bridge converter. If your board uses such a setup, then
you absolutely must drive the PWM in push-pull mode, otherwise the
converter will not work, right? So you want exactly one mode to be
applied. Then why complicate matters by allowing the mode to be a
bitmask? We could just as easily reserve, say, 8 bits (24-31) for the
mode, which could then be identical to enum pwm_mode.

quoted

And again, even if we allow the mode to be specified in DT, how do the
consumer drivers know that the correct mode was being set in DT. 

PWM user will call at some point devm_pwm_get() which, in turn, will call
of_pwm_get() which in turn will initialize PWM args with inputs from DT.
After that PWM user will, at some point, apply a PWM state; if it is
initialized with PWM args initialized when devm_pwm_get() was called then
pwm_apply_state() would fail if no good mode is provided as input via DT.

Same thing happens if a bad period is provided via DT.

But that only checks that the DT specified a supported mode, it doesn't
mean that it's the correct one. For cases like pwm-regulator this may be
fine because the driver ultimately doesn't care about the exact mode. If
you have a driver that only works with a specific mode, however, it can
be problematic.

Let's

quoted

say we have a consumer that requires the PWM to be driven in
complementary mode. Should it rely on the DT to contain the correct
specification for the mode? And if it knows that it needs complementary
mode, why not just set that mode itself?

I'm thinking it's the same way as is with PWM period which could also be
provided from DT. In the end a bad period value could be provided from
device tree. E.g. Atmel PWM controller could generate PWM signals who's
periods could not be higher than ~0.6 seconds. If a bad value is provided
the pwm_apply_state() will fail.

I understand that. And it's good to validate these things in the driver.
However, the PWM driver can only validate for the PWM that it is
driving. It doesn't know if the consumer has any special requirements
regarding the mode. So if the PWM supports push-pull mode and the DT
contains PWM_MODE_PUSH_PULL, then everything is fine as far as the PWM
driver is concerned. However, if the consumer driver strictly requires
complementary mode, there's nothing the PWM driver can do about it. So
we either need the consumer to be able to query the mode if it has any
specific needs and refuse to use a PWM that has the wrong mode in the
specifier, or the consumer needs to explicitly set a mode, in which case
there's no need to have it in DT and the PWM driver needs to reject it
if the PWM doesn't support it.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181018/9c119ab4/attachment.sig>