linux-kernel - Re: [PATCH v5 06/12] dt-bindings: pwm: add microchip corepwm binding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 2 Feb 2022 14:37:42 +0000
From:   <Conor.Dooley@...rochip.com>
To:     <geert@...ux-m68k.org>
CC:     <u.kleine-koenig@...gutronix.de>, <a.zummo@...ertech.it>,
        <alexandre.belloni@...tlin.com>, <aou@...s.berkeley.edu>,
        <atishp@...osinc.com>, <bin.meng@...driver.com>, <brgl@...ev.pl>,
        <Daire.McNamara@...rochip.com>, <devicetree@...r.kernel.org>,
        <heiko@...ech.de>, <Ivan.Griffin@...rochip.com>,
        <jassisinghbrar@...il.com>, <krzysztof.kozlowski@...onical.com>,
        <lee.jones@...aro.org>, <Lewis.Hanly@...rochip.com>,
        <linus.walleij@...aro.org>, <linux-gpio@...r.kernel.org>,
        <linux-i2c@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-pwm@...r.kernel.org>, <linux-riscv@...ts.infradead.org>,
        <linux-rtc@...r.kernel.org>, <palmer@...belt.com>,
        <paul.walmsley@...ive.com>, <robh+dt@...nel.org>,
        <robh@...nel.org>, <thierry.reding@...il.com>
Subject: Re: [PATCH v5 06/12] dt-bindings: pwm: add microchip corepwm binding

On 02/02/2022 14:02, Geert Uytterhoeven wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> On Wed, Feb 2, 2022 at 2:46 PM <Conor.Dooley@...rochip.com> wrote:
>> On 02/02/2022 13:28, Geert Uytterhoeven wrote:
>>> On Wed, Feb 2, 2022 at 1:33 PM <conor.dooley@...rochip.com> wrote:
>>>>> On 01/02/2022 07:58, Uwe Kleine-König wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>> On Mon, Jan 31, 2022 at 11:47:21AM +0000, conor.dooley@...rochip.com wrote:
>>>>>> From: Conor Dooley <conor.dooley@...rochip.com>
>>>>>>
>>>>>> Add device tree bindings for the Microchip fpga fabric based "core" PWM
>>>>>> controller.
>>>>>>
>>>>>> Reviewed-by: Rob Herring <robh@...nel.org>
>>>>>>
>>>>>> Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
>>>>>> ---
>>>>>> .../bindings/pwm/microchip,corepwm.yaml       | 75 +++++++++++++++++++
>>>
>>>>>> +  microchip,sync-update:
>>>>>> +    description: |
>>>>>> +      In synchronous mode, all channels are updated at the beginning of the PWM period.
>>>>>> +      Asynchronous mode is relevant to applications such as LED control, where
>>>>>> +      synchronous updates are not required. Asynchronous mode lowers the area size,
>>>>>> +      reducing shadow register requirements. This can be set at run time, provided
>>>>>> +      SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed
>>>>>> +      to the device.
>>>>>> +      Each bit corresponds to a PWM channel & represents whether synchronous mode is
>>>>>> +      possible for the PWM channel.
>>>>>> +
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint16
>>>>>> +    default: 0
>>>>>
>>>>> I'm not sure I understand this correctly. This is a soft-core and you
>>>>> can synthesize it either with or without the ability to do synchronous
>>>>> updates or not, right? All 16 channels share the same period length and
>>>>> in the simple implementation changing the duty cycle is done at once
>>>>> (maybe introducing a glitch) and in the more expensive implementation
>>>>> there is a register to implement both variants?
>>>>
>>>> Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both
>>>> registers that control the duty cycle for channel x have a second
>>>> "shadow reg" synthesised. At runtime a bit wide register exposed to
>>>> APB can be used to toggle on/off synchronised mode for all channels
>>>> it has been synthesised for.
>>>>
>>>> I will reword this description since it is not clear.
>>>
>>> Shouldn't it use a different compatible value instead?
>>> Differentiation by properties is not recommended, as it's easy to
>>> miss a difference.
>>
>> Either you have something in mind that I've not thought of, or I've done
>> a bad job of explaining again. The buffer/"shadow" registers are
>> synthesised on a per channel basis, so any combination of the 16
>> channels may have this capability. The same applies to the DAC mode, per
>> channel there too.
> 
> Oops, hadn't noticed this is per channel. Indeed, then a different
> compatible value is futile.
> So since "microchip,sync-update" is a bitmask, perhaps it should be
> called "microchip,sync-update-mask"?
> Same for "microchip,dac-mode" -> "microchip,dac-mode-mask"?

Adding -mask sounds good to me.

> Also, using different integer sizes than uint32 is frowned upon, unless
> there is a very good reason to do so. I can imagine a future version
> would support more channels, and then uint16 becomes a limitation.

Sure, uint32 it is.

> For both: Rob?

Both of these properties fall under the "DO attempt to make bindings 
complete even if a driver doesn’t support some features" category, so I 
am perfectly happy to change these properties to whatever is convention 
(or ultimately drop them for the sake of the remainder of the series).

Thanks,
Conor.