Re: [PATCH 2/3] dt-bindings: hwmon: g76x: Add support for g761

[Christian Marangi <ansuelsmth@gmail.com>]

On Sat, May 25, 2024 at 07:32:04AM -0700, Guenter Roeck wrote:
> On 5/25/24 03:29, Christian Marangi wrote:
> > Add support for g761 PWM Fan controller. This is an exact copy of g763
> > with the difference that it does also support an internal clock
> > oscillators.
> > 
> > Add required logic to support this additional feature with the property
> > gmt,internal-clock and reject invalid schema that define both
> > internal-clock property and external clocks.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >   .../devicetree/bindings/hwmon/gmt,g76x.yaml   | 43 +++++++++++++++++--
> >   1 file changed, 40 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/gmt,g76x.yaml b/Documentation/devicetree/bindings/hwmon/gmt,g76x.yaml
> > index bfefe49f54bf..d6e80392d045 100644
> > --- a/Documentation/devicetree/bindings/hwmon/gmt,g76x.yaml
> > +++ b/Documentation/devicetree/bindings/hwmon/gmt,g76x.yaml
> > @@ -4,13 +4,13 @@
> >   $id: http://devicetree.org/schemas/hwmon/gmt,g76x.yaml#
> >   $schema: http://devicetree.org/meta-schemas/core.yaml#
> > -title: GMT G762/G763 PWM Fan controller
> > +title: GMT G761/G762/G763 PWM Fan controller
> >   maintainers:
> >     - Christian Marangi <ansuelsmth@gmail.com>
> >   description: |
> > -  GMT G762/G763 PWM Fan controller.
> > +  GMT G761/G762/G763 PWM Fan controller.
> >     If an optional property is not set in DT, then current value is kept
> >     unmodified (e.g. bootloader installed value).
> > @@ -22,6 +22,7 @@ description: |
> >   properties:
> >     compatible:
> >       enum:
> > +      - gmt,g761
> >         - gmt,g762
> >         - gmt,g763
> > @@ -48,10 +49,37 @@ properties:
> >       $ref: /schemas/types.yaml#/definitions/uint32
> >       enum: [0, 1, 2]
> > +  gmt,internal-clock:
> > +    description: Use the Internal clock instead of externally attached one
> > +      via the CLK pin.
> > +    type: boolean
> > +
> >   required:
> >     - compatible
> >     - reg
> > -  - clocks
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - gmt,g762
> > +              - gmt,g763
> > +    then:
> > +      properties:
> > +        gmt,internal-clock: false
> > +
> > +      required:
> > +        - clocks
> 
> Is the new property even necessary ? The absence of an external clock on G761
> could be used to imply that the internal clock is used.
>

Well with how the driver works, if a property is not defined, then the
value is not set and the one set by the bootloader or from device reset
is keept.

This conflicts with the logic of no clock = internal.

But yes if asked I can drop that, I can totally see your point since the
clocks is a required property anyway so it's always set.

> 
> > +
> > +  - if:
> > +      required:
> > +        - gmt,internal-clock
> > +
> > +    then:
> > +      properties:
> > +        clocks: false
> >   additionalProperties: false
> > @@ -80,4 +108,13 @@ examples:
> >               fan_startv = <1>;
> >               pwm_polarity = <0>;
> >           };
> > +
> > +        g761@1e {
> > +            compatible = "gmt,g761";
> > +            reg = <0x1e>;
> > +            gmt,internal-clock;
> > +            fan_gear_mode = <0>;
> > +            fan_startv = <1>;
> > +            pwm_polarity = <0>;
> > +        };
> >       };
> 

-- 
	Ansuel