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