Re: [PATCH v2 1/4] dt-bindings: hwmon: Add Aspeed AST2600 PWM/Fan

Re: [PATCH v2 1/4] dt-bindings: hwmon: Add Aspeed AST2600 PWM/Fan

[Rob Herring]

On Wed, 13 Jan 2021 07:08:45 +0000, Troy Lee wrote:
> We add binding for supporting a new AST2600 PWM/Fan hwmon driver.
> 
> Changes since v1:
> - dt binding with DT schema format
> 
> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> ---
>  .../hwmon/aspeed,ast2600-pwm-tachometer.yaml  | 137 ++++++++++++++++++
>  1 file changed, 137 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml:108:2: [warning] wrong indentation: expected 2 but found 1 (indentation)

dtschema/dtc warnings/errors:

See https://patchwork.ozlabs.org/patch/1425628

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] dt-bindings: hwmon: Add Aspeed AST2600 PWM/Fan

[Rob Herring]

On Wed, Jan 13, 2021 at 07:08:45AM +0000, Troy Lee wrote:
> We add binding for supporting a new AST2600 PWM/Fan hwmon driver.
> 
> Changes since v1:
> - dt binding with DT schema format
> 
> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> ---
>  .../hwmon/aspeed,ast2600-pwm-tachometer.yaml  | 137 ++++++++++++++++++
>  1 file changed, 137 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
> new file mode 100644
> index 000000000000..b84076a4a338
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
> @@ -0,0 +1,137 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-pwm-tachometer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ASPEED AST2600 PWM and Fan Tacho controller device driver
> +
> +maintainers:
> +  - Ryan Chen <ryan_chen@aspeedtech.com>
> +
> +description: |
> +  The ASPEED PWM controller can support upto 16 PWM outputs. The ASPEED Fan Tacho
> +  controller can support upto 16 Fan tachometer inputs.
> +  There can be upto 16 fans supported. Each fan can have one PWM output and
> +  one Fan tach inputs.
> +
> +properties:
> +  compatible:
> +    const: aspeed,ast2600-pwm-tachometer
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  "#cooling-cells":
> +    const: 2
> +
> +  reg:
> +    description:
> +      Address and length of the register set for the device.

No need for generic descriptions. That's every 'reg'.

What you need is how many entries and what each one is if more than 1. 
If only 1, then just 'maxItems: 1'

> +
> +  clocks:
> +    description:
> +      phandle to clock provider with the clock number in the second cell

Same here.

> +
> +  resets:
> +    description:
> +      phandle to reset controller with the reset number in the second cell

And here.

> +
> +patternProperties:
> +  "@[0-9]+$":

If every node is a fan and there are up to 16:

^fan@[0-9a-f]$

> +    type: object
> +    description:
> +      Under fan subnode there can upto 16 child nodes, with each child node
> +      representing a fan. There are 16 fans each fan can have one PWM port and one
> +      Fan tach inputs.
> +      For PWM port can be configured cooling-levels to create cooling device.
> +      Cooling device could be bound to a thermal zone for the thermal control.
> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 15
> +        description:
> +          This property identify the PWM control channel of this fan.
> +
> +      fan-tach-ch:
> +        $ref: /schemas/types.yaml#/definitions/uint8
> +        minimum: 0
> +        maximum: 15
> +        description:
> +          This property identify the fan tach input channel.
> +
> +      pulses-per-revolution:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        default: 2
> +        minimum: 1
> +        description:
> +          Specify tacho pulse per revolution of the fan.
> +
> +      cooling-levels:
> +        description:
> +          PWM duty cycle values in a range from 0 to 255
> +          which correspond to thermal cooling states.
> +
> +      aspeed,pwm-freq:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        default: 25000
> +        minimum: 24
> +        maximum: 780000
> +        description:
> +          Specify the frequency of PWM.

Units? Use a unit suffix and then drop the $ref.

> +
> +      aspeed,inverse-pin:
> +        type: boolean
> +        description:
> +          Inverse PWM output signal.
> +
> +      aspeed,falling-point:
> +        $ref: /schemas/types.yaml#/definitions/uint8
> +        default: 10
> +        minimum: 0
> +        maximum: 255

0-255 is already the range of uint8, so drop.

> +        description:
> +          Initialize the pulse width.
> +
> +    required:
> +      - fan-tach-ch
> +      - reg
> +
> +    additionalProperties: true
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pwm_tacho: pwm-tacho-controller@1e610000 {
> +        compatible = "aspeed,ast2600-pwm-tachometer";
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        reg = <0x1e610000 0x100>;
> +
> +        fan@1 {
> +            reg = <0x00>;
> +            aspeed,pwm-freq = <25000>;
> +            cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
> +            fan-tach-ch = /bits/ 8 <0x00>;
> +            pulses-per-revolution = <2>;
> +        };
> +
> +        fan@2 {
> +            reg = <0x01>;
> +            aspeed,pwm-freq = <25000>;
> +            cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
> +            fan-tach-ch = /bits/ 8 <0x01>;
> +            pulses-per-revolution = <2>;
> +        };
> +    };
> +...
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] dt-bindings: hwmon: Add Aspeed AST2600 PWM/Fan

[Troy Lee]

Hi Rob,

Thanks for reviewing.

The 01/14/2021 22:13, Rob Herring wrote:
> On Wed, Jan 13, 2021 at 07:08:45AM +0000, Troy Lee wrote:
> > We add binding for supporting a new AST2600 PWM/Fan hwmon driver.
> > 
> > Changes since v1:
> > - dt binding with DT schema format
> > 
> > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> > ---
> >  .../hwmon/aspeed,ast2600-pwm-tachometer.yaml  | 137 ++++++++++++++++++
> >  1 file changed, 137 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
> > new file mode 100644
> > index 000000000000..b84076a4a338
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
> > @@ -0,0 +1,137 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +
> > +$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-pwm-tachometer.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ASPEED AST2600 PWM and Fan Tacho controller device driver
> > +
> > +maintainers:
> > +  - Ryan Chen <ryan_chen@aspeedtech.com>
> > +
> > +description: |
> > +  The ASPEED PWM controller can support upto 16 PWM outputs. The ASPEED Fan Tacho
> > +  controller can support upto 16 Fan tachometer inputs.
> > +  There can be upto 16 fans supported. Each fan can have one PWM output and
> > +  one Fan tach inputs.
> > +
> > +properties:
> > +  compatible:
> > +    const: aspeed,ast2600-pwm-tachometer
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  "#cooling-cells":
> > +    const: 2
> > +
> > +  reg:
> > +    description:
> > +      Address and length of the register set for the device.
> 
> No need for generic descriptions. That's every 'reg'.
> 
> What you need is how many entries and what each one is if more than 1. 
> If only 1, then just 'maxItems: 1'
> 
> > +
> > +  clocks:
> > +    description:
> > +      phandle to clock provider with the clock number in the second cell
> 
> Same here.
> 
> > +
> > +  resets:
> > +    description:
> > +      phandle to reset controller with the reset number in the second cell
> 
> And here.
> 
> > +
> > +patternProperties:
> > +  "@[0-9]+$":
> 
> If every node is a fan and there are up to 16:
> 
> ^fan@[0-9a-f]$
> 
I will update these in v3 patch set.

> > +    type: object
> > +    description:
> > +      Under fan subnode there can upto 16 child nodes, with each child node
> > +      representing a fan. There are 16 fans each fan can have one PWM port and one
> > +      Fan tach inputs.
> > +      For PWM port can be configured cooling-levels to create cooling device.
> > +      Cooling device could be bound to a thermal zone for the thermal control.
> > +
> > +    properties:
> > +      reg:
> > +        minimum: 0
> > +        maximum: 15
> > +        description:
> > +          This property identify the PWM control channel of this fan.
> > +
> > +      fan-tach-ch:
> > +        $ref: /schemas/types.yaml#/definitions/uint8
> > +        minimum: 0
> > +        maximum: 15
> > +        description:
> > +          This property identify the fan tach input channel.
> > +
> > +      pulses-per-revolution:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        default: 2
> > +        minimum: 1
> > +        description:
> > +          Specify tacho pulse per revolution of the fan.
> > +
> > +      cooling-levels:
> > +        description:
> > +          PWM duty cycle values in a range from 0 to 255
> > +          which correspond to thermal cooling states.
> > +
> > +      aspeed,pwm-freq:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        default: 25000
> > +        minimum: 24
> > +        maximum: 780000
> > +        description:
> > +          Specify the frequency of PWM.
> 
> Units? Use a unit suffix and then drop the $ref.
> 
I'll change it to pwm-freq-hz.

> > +
> > +      aspeed,inverse-pin:
> > +        type: boolean
> > +        description:
> > +          Inverse PWM output signal.
> > +
> > +      aspeed,falling-point:
> > +        $ref: /schemas/types.yaml#/definitions/uint8
> > +        default: 10
> > +        minimum: 0
> > +        maximum: 255
> 
> 0-255 is already the range of uint8, so drop.
> 
I'll drop it in v3.

Thanks,
Troy Lee
> > +        description:
> > +          Initialize the pulse width.
> > +
> > +    required:
> > +      - fan-tach-ch
> > +      - reg
> > +
> > +    additionalProperties: true
> > +
> > +required:
> > + - compatible
> > + - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    pwm_tacho: pwm-tacho-controller@1e610000 {
> > +        compatible = "aspeed,ast2600-pwm-tachometer";
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        reg = <0x1e610000 0x100>;
> > +
> > +        fan@1 {
> > +            reg = <0x00>;
> > +            aspeed,pwm-freq = <25000>;
> > +            cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
> > +            fan-tach-ch = /bits/ 8 <0x00>;
> > +            pulses-per-revolution = <2>;
> > +        };
> > +
> > +        fan@2 {
> > +            reg = <0x01>;
> > +            aspeed,pwm-freq = <25000>;
> > +            cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
> > +            fan-tach-ch = /bits/ 8 <0x01>;
> > +            pulses-per-revolution = <2>;
> > +        };
> > +    };
> > +...
> > -- 
> > 2.25.1
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] hwmon: Support Aspeed AST2600 PWM/Fan tachometer

[Guenter Roeck]

On Wed, Jan 13, 2021 at 07:08:48AM +0000, Troy Lee wrote:
> Add Aspeed AST2600 PWM/Fan tacho driver. AST2600 has 16 PWM channel and
> 16 FAN tacho channel.
> 
> Changes since v1:
> - fixed review comments
> - fixed double-looped calculation of div_h and div_l
> - moving configuration to device tree
> - register hwmon driver with devm_hwmon_device_register_with_info()
> 
> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>

checkpatch says:

total: 0 errors, 9 warnings, 26 checks, 779 lines checked

This is a bit much. Please run checkpatch --strict and fix the issues
it reports. Please also fix the issues reported by 0-day as well as
the issues reported by the bindings robot, and resubmit.

Thanks,
Guenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] hwmon: Support Aspeed AST2600 PWM/Fan tachometer

[Troy Lee]

Hi Guenter,

The 01/24/2021 00:14, Guenter Roeck wrote:
> On Wed, Jan 13, 2021 at 07:08:48AM +0000, Troy Lee wrote:
> > Add Aspeed AST2600 PWM/Fan tacho driver. AST2600 has 16 PWM channel and
> > 16 FAN tacho channel.
> > 
> > Changes since v1:
> > - fixed review comments
> > - fixed double-looped calculation of div_h and div_l
> > - moving configuration to device tree
> > - register hwmon driver with devm_hwmon_device_register_with_info()
> > 
> > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> 
> checkpatch says:
> 
> total: 0 errors, 9 warnings, 26 checks, 779 lines checked
> 
> This is a bit much. Please run checkpatch --strict and fix the issues
> it reports. Please also fix the issues reported by 0-day as well as
> the issues reported by the bindings robot, and resubmit.
> 
> Thanks,
> Guenter

I'll fix the WARNINGs and CHECKs.

Thanks,
Troy Lee

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel