Re: [PATCH v2 1/2] dt-bindings: hwmon: Add max6639

[Naresh Solanki]

Hi Krzysztof,

On 24-04-2023 03:12 pm, Krzysztof Kozlowski wrote:
> On 24/04/2023 11:18, Naresh Solanki wrote:
>
> > > > Changes in V2:
> > > > - Update subject
> > > > - Drop blank lines
> > > > ---
> > > >    .../bindings/hwmon/maxim,max6639.yaml         | 52 +++++++++++++++++++
> > > >    1 file changed, 52 insertions(+)
> > > >    create mode 100644 Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
> > > > new file mode 100644
> > > > index 000000000000..1aaedfd7cee0
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml
> > > > @@ -0,0 +1,52 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/hwmon/maxim,max6639.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Maxim max6639
> > >
> > > What is this device? fan controller?
> > Yes Fan controller.
> > Do you want me to update the title here as:
> > "Maxim MAC6639 2 channel fan controller & temperature monitor" ?
>
> Enough would be:
> Maxim MAX6639 Fan Controller
Ack
> > > > +
> > > > +maintainers:
> > > > +  - Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> > > > +
> > > > +description: |
> > > > +  The MAX6639 is a 2-channel temperature monitor with dual, automatic, PWM
> > > > +  fan-speed controller.  It monitors its own temperature and one external
> > > > +  diode-connected transistor or the temperatures of two external diode-connected
> > > > +  transistors, typically available in CPUs, FPGAs, or GPUs.
> > > > +
> > > > +  Datasheets:
> > > > +    https://datasheets.maximintegrated.com/en/ds/MAX6639-MAX6639F.pdf
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - maxim,max6639
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  '#address-cells':
> > > > +    const: 1
> > > > +
> > > > +  '#size-cells':
> > > > +    const: 0
> > >
> > > Why do you need these two properties?
> > Ack. Will remove them.
> > > Anyway, the binding looks incomplete. Where are the supplies? Interrupts?
> > This patch just adds basic support to the existing platform driver.
> > Intention is to be able to call the driver from DT with basic
> > initialization from driver the existing driver.
>
> Bindings should be rather complete. Here the datasheet is accessible and
> few properties quite obvious, so I don't see a reason to skip them.
I agree with you. But currently the driver which is already merged 
doesn't has it implemented.
And will be working on separate patch to include this later.
> Best regards,
> Krzysztof

Regards,
Naresh.