Re: [RFC PATCH 7/7] dt-bindings: motion: Add motion-simple-pwm bindings

Re: [RFC PATCH 7/7] dt-bindings: motion: Add motion-simple-pwm bindings

[David Jander]

On Fri, 28 Feb 2025 10:37:48 +0100
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 28/02/2025 10:22, David Jander wrote:
> >   
> >>> +
> >>> +  motion,pwm-inverted:
> >>> +    $ref: /schemas/types.yaml#/definitions/flag    
> >>
> >> And PWM flag does not work?  
> > 
> > I have seen PWM controllers that don't seem to support the
> > PWM_POLARITY_INVERTED flag and those where it just doesn't work. Should all  
> 
> 
> Shouldn't the controllers be fixed? Or let's rephrase the question: why
> only this PWM consumer needs this property and none of others need it?

CCing Uwe Kleine-Koenig and linux-pwm mailing list.

I know that at least in kernel 6.11 the pwm-stm32.c PWM driver doesn't
properly invert the PWM signal when specifying PWM_POLARITY_INVERTED. I agree
this is a probably bug that needs fixing if still present in 6.14-rc. Besides
that, if linux-pwm agrees that every single PWM driver _must_ properly support
this flag, I will drop this consumer flag an start fixing broken PWM drivers
that I encounter. I agree that it makes more sense this way, but I wanted to
be sure.

Best regards,

-- 
David Jander

Re: [RFC PATCH 7/7] dt-bindings: motion: Add motion-simple-pwm bindings

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]

Hey David,

On Fri, Feb 28, 2025 at 11:09:31AM +0100, David Jander wrote:
> On Fri, 28 Feb 2025 10:37:48 +0100
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
> > On 28/02/2025 10:22, David Jander wrote:
> > >   
> > >>> +
> > >>> +  motion,pwm-inverted:
> > >>> +    $ref: /schemas/types.yaml#/definitions/flag    
> > >>
> > >> And PWM flag does not work?  
> > > 
> > > I have seen PWM controllers that don't seem to support the
> > > PWM_POLARITY_INVERTED flag and those where it just doesn't work. Should all  
> > 
> > 
> > Shouldn't the controllers be fixed? Or let's rephrase the question: why
> > only this PWM consumer needs this property and none of others need it?
> 
> CCing Uwe Kleine-Koenig and linux-pwm mailing list.
> 
> I know that at least in kernel 6.11 the pwm-stm32.c PWM driver doesn't
> properly invert the PWM signal when specifying PWM_POLARITY_INVERTED. I agree
> this is a probably bug that needs fixing if still present in 6.14-rc. Besides
> that, if linux-pwm agrees that every single PWM driver _must_ properly support
> this flag, I will drop this consumer flag an start fixing broken PWM drivers
> that I encounter. I agree that it makes more sense this way, but I wanted to
> be sure.

Some hardwares cannot support PWM_POLARITY_INVERTED. Affected drivers
include:

	pwm-adp5585
	pwm-ntxec
	pwm-raspberrypi-poe
	pwm-rz-mtu3 (software limitation only)
	pwm-sunplus
	pwm-twl-led (not completely sure, that one is strange)

. ISTR that there is a driver that does only support inverted polarity,
but I don't find it. For an overview I recommend reading through the
output of:

	for f in drivers/pwm/pwm-*; do
		echo $f;
		sed -rn '/Limitations:/,/\*\/?$/p' $f;
		echo;
	done | less

. (Note not all drivers have commentary in the right format to unveil
their limitations.)

For most use-cases you can just do

	.duty_cycle = .period - .duty_cycle

instead of inverting polarity, but there is no abstraction in the PWM
bindings for that and also no helpers in the PWM framework. The problem
is more or less ignored, so if you have a device with

	pwms = <&pwm0 0 PWM_POLARITY_INVERTED>;

and the PWM chip in question doesn't support that, the pwm API functions
will fail. So the system designer better makes sure that the PWM
hardware can cope with the needed polarity.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 7/7] dt-bindings: motion: Add motion-simple-pwm bindings

[Maud Spierings]

On 2/28/25 4:18 PM, Uwe Kleine-König wrote:
> Hey David,
>
> On Fri, Feb 28, 2025 at 11:09:31AM +0100, David Jander wrote:
>> On Fri, 28 Feb 2025 10:37:48 +0100
>> Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>>> On 28/02/2025 10:22, David Jander wrote:
>>>>    
>>>>>> +
>>>>>> +  motion,pwm-inverted:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>>>> And PWM flag does not work?
>>>> I have seen PWM controllers that don't seem to support the
>>>> PWM_POLARITY_INVERTED flag and those where it just doesn't work. Should all
>>>
>>> Shouldn't the controllers be fixed? Or let's rephrase the question: why
>>> only this PWM consumer needs this property and none of others need it?
>> CCing Uwe Kleine-Koenig and linux-pwm mailing list.
>>
>> I know that at least in kernel 6.11 the pwm-stm32.c PWM driver doesn't
>> properly invert the PWM signal when specifying PWM_POLARITY_INVERTED. I agree
>> this is a probably bug that needs fixing if still present in 6.14-rc. Besides
>> that, if linux-pwm agrees that every single PWM driver _must_ properly support
>> this flag, I will drop this consumer flag an start fixing broken PWM drivers
>> that I encounter. I agree that it makes more sense this way, but I wanted to
>> be sure.
> Some hardwares cannot support PWM_POLARITY_INVERTED. Affected drivers
> include:
>
> 	pwm-adp5585
> 	pwm-ntxec
> 	pwm-raspberrypi-poe
> 	pwm-rz-mtu3 (software limitation only)
> 	pwm-sunplus
> 	pwm-twl-led (not completely sure, that one is strange)
>
> . ISTR that there is a driver that does only support inverted polarity,
> but I don't find it. For an overview I recommend reading through the
> output of:


The only one that I know of is the opencores pwm driver that the 
starfive jh71xx uses, I remember talking with you there. That one does 
still need a proper review I believe:
https://lore.kernel.org/all/20250106103540.10079-1-william.qiu@starfivetech.com/

It is kind of in a limbo right now


>
> 	for f in drivers/pwm/pwm-*; do
> 		echo $f;
> 		sed -rn '/Limitations:/,/\*\/?$/p' $f;
> 		echo;
> 	done | less
>
> . (Note not all drivers have commentary in the right format to unveil
> their limitations.)
>
> For most use-cases you can just do
>
> 	.duty_cycle = .period - .duty_cycle
>
> instead of inverting polarity, but there is no abstraction in the PWM
> bindings for that and also no helpers in the PWM framework. The problem
> is more or less ignored, so if you have a device with
>
> 	pwms = <&pwm0 0 PWM_POLARITY_INVERTED>;
>
> and the PWM chip in question doesn't support that, the pwm API functions
> will fail. So the system designer better makes sure that the PWM
> hardware can cope with the needed polarity.
>
> Best regards
> Uwe

Re: [RFC PATCH 7/7] dt-bindings: motion: Add motion-simple-pwm bindings

[David Jander]

Dear Uwe,

Thanks for chiming in!

On Fri, 28 Feb 2025 16:18:05 +0100
Uwe Kleine-König <ukleinek@kernel.org> wrote:

> Hey David,
> 
> On Fri, Feb 28, 2025 at 11:09:31AM +0100, David Jander wrote:
> > On Fri, 28 Feb 2025 10:37:48 +0100
> > Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >   
> > > On 28/02/2025 10:22, David Jander wrote:  
> > > >     
> > > >>> +
> > > >>> +  motion,pwm-inverted:
> > > >>> +    $ref: /schemas/types.yaml#/definitions/flag      
> > > >>
> > > >> And PWM flag does not work?    
> > > > 
> > > > I have seen PWM controllers that don't seem to support the
> > > > PWM_POLARITY_INVERTED flag and those where it just doesn't work. Should all    
> > > 
> > > 
> > > Shouldn't the controllers be fixed? Or let's rephrase the question: why
> > > only this PWM consumer needs this property and none of others need it?  
> > 
> > CCing Uwe Kleine-Koenig and linux-pwm mailing list.
> > 
> > I know that at least in kernel 6.11 the pwm-stm32.c PWM driver doesn't
> > properly invert the PWM signal when specifying PWM_POLARITY_INVERTED. I agree
> > this is a probably bug that needs fixing if still present in 6.14-rc. Besides
> > that, if linux-pwm agrees that every single PWM driver _must_ properly support
> > this flag, I will drop this consumer flag an start fixing broken PWM drivers
> > that I encounter. I agree that it makes more sense this way, but I wanted to
> > be sure.  
> 
> Some hardwares cannot support PWM_POLARITY_INVERTED. Affected drivers
> include:
> 
> 	pwm-adp5585
> 	pwm-ntxec
> 	pwm-raspberrypi-poe
> 	pwm-rz-mtu3 (software limitation only)
> 	pwm-sunplus
> 	pwm-twl-led (not completely sure, that one is strange)
> 
> . ISTR that there is a driver that does only support inverted polarity,
> but I don't find it. For an overview I recommend reading through the
> output of:
> 
> 	for f in drivers/pwm/pwm-*; do
> 		echo $f;
> 		sed -rn '/Limitations:/,/\*\/?$/p' $f;
> 		echo;
> 	done | less
> 
> . (Note not all drivers have commentary in the right format to unveil
> their limitations.)
> 
> For most use-cases you can just do
> 
> 	.duty_cycle = .period - .duty_cycle

Yes, that is exactly what the relevant code in motion/simple-pwm.c does when
the "pwm-inverted" flag is present in the DT node.

> instead of inverting polarity, but there is no abstraction in the PWM
> bindings for that and also no helpers in the PWM framework. The problem
> is more or less ignored, so if you have a device with
> 
> 	pwms = <&pwm0 0 PWM_POLARITY_INVERTED>;
> 
> and the PWM chip in question doesn't support that, the pwm API functions
> will fail. So the system designer better makes sure that the PWM
> hardware can cope with the needed polarity.

Thanks for clarifying this!

@Krzysztof, do you think that given this situation it is acceptable to include
the "pwm-inverted" flag in the dt-schema of the simple PWM motor driver?

The need for an inverted PWM signal is something very common in the case of
H-bridge motor drivers, where the PWM signal represents the actual logical
output level of each of the two halves of the bridge. Often the high-side
switches are used as the free-wheel position, so that 100% duty-cycle on both
channels is actually standstill, while 0% duty-cycle on one channel is full
speed in either direction. This isn't always the case though, hence the
importance for this to be able to be selected.

Best regards,

-- 
David Jander

Re: [RFC PATCH 7/7] dt-bindings: motion: Add motion-simple-pwm bindings

[Krzysztof Kozlowski]

On 03/03/2025 12:40, David Jander wrote:
>>
>> Some hardwares cannot support PWM_POLARITY_INVERTED. Affected drivers
>> include:
>>
>> 	pwm-adp5585
>> 	pwm-ntxec
>> 	pwm-raspberrypi-poe
>> 	pwm-rz-mtu3 (software limitation only)
>> 	pwm-sunplus
>> 	pwm-twl-led (not completely sure, that one is strange)
>>
>> . ISTR that there is a driver that does only support inverted polarity,
>> but I don't find it. For an overview I recommend reading through the
>> output of:
>>
>> 	for f in drivers/pwm/pwm-*; do
>> 		echo $f;
>> 		sed -rn '/Limitations:/,/\*\/?$/p' $f;
>> 		echo;
>> 	done | less
>>
>> . (Note not all drivers have commentary in the right format to unveil
>> their limitations.)
>>
>> For most use-cases you can just do
>>
>> 	.duty_cycle = .period - .duty_cycle
> 
> Yes, that is exactly what the relevant code in motion/simple-pwm.c does when
> the "pwm-inverted" flag is present in the DT node.
> 
>> instead of inverting polarity, but there is no abstraction in the PWM
>> bindings for that and also no helpers in the PWM framework. The problem
>> is more or less ignored, so if you have a device with
>>
>> 	pwms = <&pwm0 0 PWM_POLARITY_INVERTED>;
>>
>> and the PWM chip in question doesn't support that, the pwm API functions
>> will fail. So the system designer better makes sure that the PWM
>> hardware can cope with the needed polarity.
> 
> Thanks for clarifying this!
> 
> @Krzysztof, do you think that given this situation it is acceptable to include
> the "pwm-inverted" flag in the dt-schema of the simple PWM motor driver?

No, because that flag is already supported via PWM_POLARITY_INVERTED.

Do not tie bindings to specific implementation. If PWM core is changed
to always handle PWM_POLARITY_INVERTED even if controller does not
support it, would the binding became outdated?

> 
> The need for an inverted PWM signal is something very common in the case of
> H-bridge motor drivers, where the PWM signal represents the actual logical
> output level of each of the two halves of the bridge. Often the high-side
> switches are used as the free-wheel position, so that 100% duty-cycle on both
> channels is actually standstill, while 0% duty-cycle on one channel is full
> speed in either direction. This isn't always the case though, hence the
> importance for this to be able to be selected.

Sure and use existing bindings for that. If implementation has problems,
fix implementation.

Best regards,
Krzysztof

Re: [RFC PATCH 7/7] dt-bindings: motion: Add motion-simple-pwm bindings

[David Jander]

On Mon, 3 Mar 2025 15:18:40 +0100
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 03/03/2025 12:40, David Jander wrote:
> >>
> >> Some hardwares cannot support PWM_POLARITY_INVERTED. Affected drivers
> >> include:
> >>
> >> 	pwm-adp5585
> >> 	pwm-ntxec
> >> 	pwm-raspberrypi-poe
> >> 	pwm-rz-mtu3 (software limitation only)
> >> 	pwm-sunplus
> >> 	pwm-twl-led (not completely sure, that one is strange)
> >>
> >> . ISTR that there is a driver that does only support inverted polarity,
> >> but I don't find it. For an overview I recommend reading through the
> >> output of:
> >>
> >> 	for f in drivers/pwm/pwm-*; do
> >> 		echo $f;
> >> 		sed -rn '/Limitations:/,/\*\/?$/p' $f;
> >> 		echo;
> >> 	done | less
> >>
> >> . (Note not all drivers have commentary in the right format to unveil
> >> their limitations.)
> >>
> >> For most use-cases you can just do
> >>
> >> 	.duty_cycle = .period - .duty_cycle  
> > 
> > Yes, that is exactly what the relevant code in motion/simple-pwm.c does when
> > the "pwm-inverted" flag is present in the DT node.
> >   
> >> instead of inverting polarity, but there is no abstraction in the PWM
> >> bindings for that and also no helpers in the PWM framework. The problem
> >> is more or less ignored, so if you have a device with
> >>
> >> 	pwms = <&pwm0 0 PWM_POLARITY_INVERTED>;
> >>
> >> and the PWM chip in question doesn't support that, the pwm API functions
> >> will fail. So the system designer better makes sure that the PWM
> >> hardware can cope with the needed polarity.  
> > 
> > Thanks for clarifying this!
> > 
> > @Krzysztof, do you think that given this situation it is acceptable to include
> > the "pwm-inverted" flag in the dt-schema of the simple PWM motor driver?  
> 
> No, because that flag is already supported via PWM_POLARITY_INVERTED.
> 
> Do not tie bindings to specific implementation. If PWM core is changed
> to always handle PWM_POLARITY_INVERTED even if controller does not
> support it, would the binding became outdated?

Understood. I guess I will have to start fixing PWM drivers then... @uwe, do
you have objections to trying to add a (very trivial) helper for PWM
controllers that can't do inversion in hardware and start to patch some of
them if needed?

> > The need for an inverted PWM signal is something very common in the case of
> > H-bridge motor drivers, where the PWM signal represents the actual logical
> > output level of each of the two halves of the bridge. Often the high-side
> > switches are used as the free-wheel position, so that 100% duty-cycle on both
> > channels is actually standstill, while 0% duty-cycle on one channel is full
> > speed in either direction. This isn't always the case though, hence the
> > importance for this to be able to be selected.  
> 
> Sure and use existing bindings for that. If implementation has problems,
> fix implementation.

Sounds reasonable.

Best regards,

-- 
David Jander