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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Conor,

On Wed, Feb 2, 2022 at 2:46 PM <Conor.Dooley@microchip.com> wrote:
> On 02/02/2022 13:28, Geert Uytterhoeven wrote:
> > On Wed, Feb 2, 2022 at 1:33 PM <conor.dooley@microchip.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@microchip.com wrote:
> >>>> From: Conor Dooley <conor.dooley@microchip.com>
> >>>>
> >>>> Add device tree bindings for the Microchip fpga fabric based "core" PWM
> >>>> controller.
> >>>>
> >>>> Reviewed-by: Rob Herring <robh@kernel.org>
> >>>>
> >>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.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"?

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.

For both: Rob?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds