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

[Naresh Solanki]

Hi Krzysztof,

On 04-05-2023 04:17 pm, Krzysztof Kozlowski wrote:
> On 04/05/2023 11:47, Naresh Solanki wrote:
> > Hi Krzysztof,
> >
> > On 03-05-2023 09:48 pm, Krzysztof Kozlowski wrote:
> > > On 03/05/2023 10:26, Naresh Solanki wrote:
> > > > Hi Krzysztof,
> > > >
> > > > On 24-04-2023 03:23 pm, Naresh Solanki wrote:
> > > > > 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.
> > > > Please let me know if this approach is acceptable, or if there are any
> > > > other suggestions or concerns that you have.
> > >
> > > You are adding new bindings, so what does the driver has to do with it?
> > The reason for adding these new bindings is to enable the use of the
> > driver on my machine. Without the compatible string, it would not be
> > possible to use the driver.
> >
> > Currently, the driver initializes the device with defaults, which is
> > good enough for my application. Also, as you previously pointed out, it
> > uses the optional 'fan-supply' which will be included in the next patch
> > revision.
> >
> > I hope this clarifies my reasoning. Could you kindly confirm if we can
> > proceed with this approach?
>
> No, we cannot, because we asked you to fix things there. Your entire
> explanation about compatible and driver is not related to the comment
> you received: bindings should be complete. You argue that bindings do
> not have to be complete, because of something with driver. This is not
> related. Bindings are not for driver.

I understand that complete bindings are important, but as the driver is 
already merged and functional, my immediate goal is to enable its use on 
my machine. I will work on a separate patch to include the interrupts in 
both binding & driver.

> Best regards,
> Krzysztof