Re: [PATCH v7 1/3] dt-bindings: pwm: Add Raspberry Pi RP1 PWM controller

[Andrea della Porta <andrea.porta@suse.com>]

Hi,

On 09:50 Mon 20 Jul     , sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The binding description states the device acts as a syscon provider for shared registers, but the schema prohibits the use of the "syscon" compatible string.
> --
> 
> --- Patch [1]: [PATCH v7 1/3] dt-bindings: pwm: Add Raspberry Pi RP1 PWM controller ---
> commit 06ebdbe78362ab02ea5e88d503ad7c179855b48a
> Author: Naushir Patuck <naush@raspberrypi.com>
> 
> dt-bindings: pwm: Add Raspberry Pi RP1 PWM controller
> 
> This commit adds the devicetree binding documentation for the PWM controller
> found in the Raspberry Pi RP1 chipset.
> 
> > diff --git a/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml b/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
> > new file mode 100644
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/raspberrypi,rp1-pwm.yaml
> > @@ -0,0 +1,66 @@
> 
> [ ... ]
> 
> > +description: |
> > +  The PWM peripheral is a flexible waveform generator with a
> > +  variety of operational modes. It has the following features:
> > +   - four independent output channels
> > +   - 32-bit counter widths
> > +   - Seven output generation modes
> > +   - Optional per-channel output inversion
> > +   - Optional duty-cycle data FIFO with DMA support
> > +   - Optional sigma-delta noise shaping engine
> > +  Serves as a fan speed provider to other nodes for a PWM-connected
> > +  fan using shared registers (syscon).
> > +
> > +properties:
> > +  compatible:
> > +    const: raspberrypi,rp1-pwm
> 
> [Severity: Medium]
> The description states this device acts as a syscon provider for shared
> registers, but the compatible property is restricted to a single string.
> 
> Will device tree validation fail if a user includes the "syscon" string 
> normally required for standard kernel syscon lookup APIs alongside the
> "raspberrypi,rp1-pwm" compatible string?

I deliberately chose not to include 'syscon' in the 'compatible' property
to retain the flexibility to constrain the regmap exported through syscon
in the future.
Including 'syscon' in the producer's compatible string causes the syscon
framework to register the entire 'reg' property range, whereas the driver
uses of_syscon_register_regmap(), which could restrict the regmap later on.

Additionally, the fact that a specific register is used as a tachometer
depends solely on the firmware; if this changes in the future, the syscon
requirement might go away. Therefore, we might not want 'syscon' in the
compatible string at all.

Thanks,
Andrea

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784539788.git.andrea.porta@suse.com?part=1