Re: [hwmon-next PATCH v3 1/4] dt-bindings: hwmon: adi,adt7470

[Luiz Angelo Daros de Luca]

Em qui., 30 de jul. de 2026 às 12:38, Guenter Roeck
<linux@xxxxxxxxxxxx> escreveu:
>
> On 7/29/26 23:17, Krzysztof Kozlowski wrote:
> > On 29/07/2026 06:45, Luiz Angelo Daros de Luca wrote:
> >> Hi Krzysztof,
> >>
> >> Thanks for the review.
> >>
> >>> On Tue, Jul 28, 2026 at 12:07:09AM -0300, Luiz Angelo Daros de Luca wrote:
> >>>> The Analog Devices ADT7470 is a multichannel temperature sensor and
> >>>> PWM fan controller. It supports monitoring up to 10 external
> >>>> temperature sensors and controlling up to 4 fans.
> >>>>
> >>>> Add the device tree binding documentation for it. This includes
> >>>> support for the thermal framework by defining the "#thermal-sensor-cells"
> >>>> property, and models the fan control lines as PWM channels by
> >>>> defining the "#pwm-cells" property.
> >>>>
> >>>> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@xxxxxxxxx>
> >>>> ---
> >>>>   .../devicetree/bindings/hwmon/adi,adt7470.yaml     | 55 ++++++++++++++++++++++
> >>>>   1 file changed, 55 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml b/Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..bb2375a4a490
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/hwmon/adi,adt7470.yaml
> >>>> @@ -0,0 +1,55 @@
> >>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/hwmon/adi,adt7470.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: Analog Devices ADT7470 hwmon sensor
> >>>> +
> >>>> +maintainers:
> >>>> +  - Luiz Angelo Daros de Luca <luizluca@xxxxxxxxx>
> >>>> +
> >>>> +description:
> >>>> +  Multi-channel temperature monitor and PWM fan controller.
> >>>> +  It supports monitoring up to 10 external temperature sensors and
> >>>> +  controlling up to four fans. The fan control lines are modeled
> >>>> +  as standard PWM channels.
> >>>> +
> >>>> +properties:
> >>>> +  compatible:
> >>>> +    const: adi,adt7470
> >>>> +
> >>>> +  reg:
> >>>> +    maxItems: 1
> >>>> +
> >>>> +  "#pwm-cells":
> >>>> +    const: 3
> >>>> +    description:
> >>>> +      See Documentation/devicetree/bindings/pwm/pwm.yaml for the
> >>>> +      generic PWM binding description.
> >>>
> >>> Drop description.
> >>>
> >>> You OTOH, miss ref to pwm schema. Referencing here in description should
> >>> raise you questions... And then use unevaluatedProperties.
> >>
> >> I will drop the description and change to `unevaluatedProperties: false`.
> >>
> >> Regarding the reference to the PWM schema, adding `$ref:
> >> /schemas/pwm/pwm.yaml#` enforces the `$nodename` to match
> >> `^pwm(@.*|-([0-9a-f]+))?$`. Since the ADT7470 is primarily a hardware
> >> monitor, its node is typically named `sensor@...`,
> >> `fan-controller@...` or `hwmon@...`. Importing `pwm.yaml` causes
> >> `dtbs_check` to fail on existing node names because of this mismatch.
> >
> > If it is a fan-controller then it should use common fan schema, because
> > you might be missing several properties.
> >
> > Device having PWM outputs alone does not make it a fan controller, though.
>
> It is a fan controller. The logic used here is the opposite:
> It has pwm outputs to control the fans, therefore it should be modeled as
> pwm controller and use pwm controller bindings (to clarify, this is not
> my logic, but both DT and PWM maintainers insist on it).

Hi Guenter, Hi Krzysztof,

Thanks for your review. You are both right about the nature of this
device. While the manufacturer markets the ADT7470 as a "PWM fan
controller" (and it does have internal closed-loop capabilities), the
physical pins are multi-functional. They can act as generic PWM
outputs or even GPIOs. Because of this flexibility, tying the binding
to `fan-common.yaml` would be too restrictive.

On my specific hardware (Edgecore switches), the vendor bypassed the
ADT7470's internal temperature sensors entirely. The fans must be
controlled by the main SoC's thermal zone (or any other userland
daemon/script).

Regarding the architecture, while I looked at the `emc2305` approach,
delegating the actual fan management to the time-proven `pwm-fan`
driver allows us to avoid reinventing the wheel (like cooling device
registration and state management) inside the adt7470 driver. It also
keeps the standard PWM consumer/provider relationship clear in the DT.
It still feels strange that emc2305 declares #pwm-cells and its
subdriver fans pwm properties and they do not work as a pwm
controller. Other drivers, like aspeed-g6-pwm-tach (with similar
bindings) do both: use a fan subdriver controlled by hwmon sysfs and
register itself as pwmchip. But this is exactly what Guenter asked me
to avoid.

> > Since we cannot override the `$nodename` from an imported schema, what
> > is the preferred approach here? Should I skip the `$ref` to `pwm.yaml`
> > in this specific case, or is there an accepted workaround for sensors
> > acting as PWM providers?
>
> You can skip the pwm.yaml in such case.

OK. Thanks.

> >> You most likely miss ref to thermal-sensor.yaml as well, but that would
> >> change the ABI since hwmon would fail on missing cells, right?
> >
> > Yes, you are right. It would require #thermal-sensor-cells and the
> > device might not have a thermal sensor connected if it is used as a
> > pure fan controller. Likewise, it can also be used as a simple
> > analog-to-digital temperature adapter, with no fans connected (and
> > "#pwm-cells" might be missing).
> >
> > To address this without breaking the ABI, we could add the ref
> > conditionally if "#thermal-sensor-cells" is present.
> >
> > allOf:
> >  - if:
> >      required:
> >        - "#thermal-sensor-cells"
> >    then:
> >      $ref: /schemas/thermal/thermal-sensor.yaml#
> >
> > I just don't know if it is ok to ref thermal-sensor but not pwm.
>
> Feels ok.

Thanks.

I'm sending v5 soon with the current approach. If needed I can revert
back to the subdriver/cooling device directly in the adt7470 (as in my
v1). In any case, both approaches share the same #pwm-cells but the
subdriver would require a new fans property.

Best Regards,

Luiz