Re: [PATCH hwmon-next v2 2/3] dt-bindings: hwmon: add Microchip EMC2305 fan controller.

[Guenter Roeck <linux@roeck-us.net>]

On 5/25/22 19:11, Rob Herring wrote:
> On Tue, May 24, 2022 at 07:17:54PM +0300, michaelsh@nvidia.com wrote:
>> From: Michael Shych <michaelsh@nvidia.com>
>>
>> Add basic description of emc2305 driver device tree binding.
>>
>> Signed-off-by: Michael Shych <michaelsh@nvidia.com>
>> Reviewed-by: Vadim Pasternak <vadimp@nvidia.com>
>> ---
>> v1->v2
>> - Fix dt binding check errors;
>> - Add descriptions;
>> - Add missing fields;
>> - Change the patch subject name;
>> - Separate pwm-min, pwm-max per PWM channel.
>> ---
>>   .../bindings/hwmon/microchip,emc2305.yaml          | 87 ++++++++++++++++++++++
>>   1 file changed, 87 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml b/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
>> new file mode 100644
>> index 000000000000..0aa65dcfd238
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
>> @@ -0,0 +1,87 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +
>> +$id: http://devicetree.org/schemas/hwmon/microchip,emc2305.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip EMC2305 RPM-based PWM Fan Speed Controller
>> +
>> +maintainers:
>> +  - Michael Shych <michaelsh@nvidia.com>
>> +
>> +description: |
>> +  Microchip EMC2301/2/3/5 are RPM-based PWM Fan Controller.
>> +  The Fan Controller supports up to 5 independently controlled PWM fan drives.
>> +  Fan rotation speeds are reported in RPM.
>> +
>> +  Datasheet: https://www.microchip.com/en-us/product/EMC2305
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - microcip,emc2305
>> +      - microcip,emc2303
>> +      - microcip,emc2302
>> +      - microcip,emc2301
> 
> typo.
> 
>> +
>> +  microchip,pwm-channel:
>> +    description:
>> +      Max number of pwm channels.
>> +      Zero index is used in case of one common PWM setting.
>> +      Number of used separted pwm channels can be less than
>> +      maximum number of available pwm chnnels on this FAN controller.
>> +    $ref: /schemas/types.yaml#/definitions/uint8
>> +    items:
>> +      minimum: 0
>> +      maximum: 5
>> +      default: 0
>> +    maxItems: 1
>> +
>> +  microchip,cooling-levels:
>> +    description:
>> +      Quantity of cooling level state.
>> +    $ref: /schemas/types.yaml#/definitions/uint8
>> +    items:
>> +      minimum: 0
>> +      maximum: 10
>> +      default: 10
>> +    maxItems: 1
>> +
>> +  microchip,pwm-min:
>> +    description:
>> +      Min pwm of emc2305
>> +    $ref: /schemas/types.yaml#/definitions/uint8
>> +    default: 0
>> +    maxItems: 1
>> +
>> +  microchip,pwm-max:
>> +    description:
>> +      Max pwm of emc2305
>> +    $ref: /schemas/types.yaml#/definitions/uint8
>> +    default: 255
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    fan {
>> +        microchip,compatible = "microchip,emc2305";
> 
> microchip,compatible?? Again...
> 
> The bigger problem remains of this needing a common fan and fan
> controller bindings.
> 

I am more concerned that the bindings don't really reflect the fan controller
but thermal device configuration. Shouldn't that be handled with thermal
subsystem bindings ?

At the same time, there are no bindings suggested for the actual pwm
controller configuration (such as pwm inversion, pwm frequency, or pwm
output type), and there is nothing at all for the fan controller either.
I can understand that this is maybe not needed for the given application
(it seems to assume manual fan control driven by the thermal subsystem),
but the bindings have to take into account that there are other applications.

Also, the number of available pwm/fan control channels is determined by
the chip. If some of the channels are unused, that could be handled with
status properties for the individual channels. I don't see a need for
a "pwm-channel" property. Similar, the maximum pwm value is determined
by the chip; I don't see the point of the microchip,pwm-max property.
Again, this is a thermal subsystem property, and I would hope that the
range can be set with thermal subsystem properties.

Something like the following might do.

     fan-controller@1a {
	compatible = "microchip,emc2305";
	#address-cells = <1>;
         #size-cells = <0>;
	<chip properties, eg external clock>

	channel@0 {
	    reg = <0x0>;
	    pwm-frequency = < xxx >;
	    pwm-output-type = <0 | 1>;	// for open drain / push-pull
	    pwm-inverted;		// if pwm output is inverted
	    microchip,closed-loop;	// for closed loop fan control
	    fan-pulses = < 1..4 >;
	    ...
	};
	channel@1 {
	    ...
	};
	channel@4 {
	    ...
	    status = "disabled";	// if unused
	};
	...
     };

Thanks,
Guenter


> Rob
> 
>> +        microchip,pwm-channel = <5>;
>> +        microchip,cooling-levels = <10>;
>> +        microchip,pwms {
>> +            pwm@0 {
>> +                      pwm-min = <0>;
>> +                      pwm-max = <255>;
>> +            };
>> +            pwm@1 {
>> +                      pwm-min = <0>;
>> +                      pwm-max = <255>;
>> +            };
>> +        };
>> +    };
>> +
>> -- 
>> 2.14.1
>>
>>