Re: [PATCH 3/7] pwm: dwc: add of/platform support

[Ben Dooks <ben.dooks@sifive.com>]

On 13/07/2022 16:07, Uwe Kleine-König wrote:
> On Wed, Jul 13, 2022 at 03:30:07PM +0100, Ben Dooks wrote:
>> On 13/07/2022 14:52, Uwe Kleine-König wrote:
>>> On Wed, Jul 13, 2022 at 12:56:55PM +0100, Ben Dooks wrote:
>>>> On 12/07/2022 23:17, Rob Herring wrote:
>>>>> On Tue, Jul 12, 2022 at 11:01:09AM +0100, Ben Dooks wrote:
>>>>>> The dwc pwm controller can be used in non-PCI systems, so allow
>>>>>> either platform or OF based probing.
>>>>>>
>>>>>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>>
>> [snip]
>>
>>>>>> +properties:
>>>>>> +  "#pwm-cells":
>>>>>> +    description: |
>>>>>> +      See pwm.yaml in this directory for a description of the cells format.
>>>>>
>>>>> pwm.yaml doesn't define how many cells. You need to. And you don't need
>>>>> generic descriptions.
>>>>
>>>>    "#pwm-cells":
>>>>       const: 1
>>>>
>>>> should be sufficient then?
>>>
>>> I would expect a value of (at least) 2 or (better) 3.
>>
>> OOPS, forgot the phandle.
>>
>> I will have to check if we have any support yet for dealing
>> with any of the pwm flags yet.
> 
> I didn't double check, but given that the driver only supports inversed
> polarity it might not even work without passing the flag for inversed
> polarity. Having said that, I expect you have to only add "#pwm-cells =
> <3>;" to your dts and then everything should work just fine.

I've gone back over the documentation we have for the block, and it
should have a count for high and a count for low in the PWM mode the
driver puts it into. I have no idea /why/ the driver is reporting it
as inversed, unless the PCI version has this automatically set....

I will go back and talk with the engineer who did the testing of the
PWM to get the test-bench re-set and check this, however my expectation
is we could easily do both and for the of/plat case we should just
report normal polarity (and we could deal with the inversed by simply
swapping the low and high values).

I also noted the v2 block supports 0 and 100% by setting a bit in the
control and the timers to a given value, so that can also be added to
the series (although this requires an IP generation option to be
set) which we can also add.

Thnak you for pointing this out, hopefully we can have this sorted
today and if so we will need to change this to a range of 2..3 for
the PWM cells.


> Best regards
> Uwe
>