Re: [PATCH v5 1/2] dt-bindings: adc: provide max34408/9 device tree binding document

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 13 Oct 2023 10:35:49 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 13/10/2023 10:19, Jonathan Cameron wrote:
> > On Thu, 12 Oct 2023 19:27:33 +0300
> > Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
> >   
> >> On Thu, 2023-10-12 at 08:40 +0100, Jonathan Cameron wrote:  
> >>> On Tue, 10 Oct 2023 23:22:48 +0300
> >>> Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
> >>>     
> >>>> On Tue, 2023-10-10 at 15:40 +0100, Jonathan Cameron wrote:    
> >>>>> On Sun,  8 Oct 2023 02:48:37 +0300
> >>>>> Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
> >>>>>       
> >>>>>> The hardware binding for i2c current monitoring device with
> >>>>>> overcurrent
> >>>>>> control.
> >>>>>>
> >>>>>> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> >>>>>> ---
> >>>>>>  .../bindings/iio/adc/maxim,max34408.yaml      | 141
> >>>>>> ++++++++++++++++++
> >>>>>>  1 file changed, 141 insertions(+)
> >>>>>>  create mode 100644
> >>>>>> Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> >>>>>>
> >>>>>> diff --git
> >>>>>> a/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> >>>>>> b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..9749f1fd1802
> >>>>>> --- /dev/null
> >>>>>> +++
> >>>>>> b/Documentation/devicetree/bindings/iio/adc/maxim,max34408.yaml
> >>>>>> @@ -0,0 +1,141 @@
> >>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>>>>> +%YAML 1.2
> >>>>>> +---
> >>>>>> +$id:
> >>>>>> http://devicetree.org/schemas/iio/adc/maxim,max34408.yaml#
> >>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>> +
> >>>>>> +title: Two- and four-channel current monitors with overcurrent
> >>>>>> control
> >>>>>> +
> >>>>>> +maintainers:
> >>>>>> +  - Ivan Mikhaylov <fr0st61te@gmail.com>
> >>>>>> +
> >>>>>> +description: |
> >>>>>> +  The MAX34408/MAX34409 are two- and four-channel current
> >>>>>> monitors
> >>>>>> that are
> >>>>>> +  configured and monitored with a standard I2C/SMBus serial
> >>>>>> interface. Each
> >>>>>> +  unidirectional current sensor offers precision high-side
> >>>>>> operation with a
> >>>>>> +  low full-scale sense voltage. The devices automatically
> >>>>>> sequence
> >>>>>> through
> >>>>>> +  two or four channels and collect the current-sense samples
> >>>>>> and
> >>>>>> average them
> >>>>>> +  to reduce the effect of impulse noise. The raw ADC samples
> >>>>>> are
> >>>>>> compared to
> >>>>>> +  user-programmable digital thresholds to indicate overcurrent
> >>>>>> conditions.
> >>>>>> +  Overcurrent conditions trigger a hardware output to provide
> >>>>>> an
> >>>>>> immediate
> >>>>>> +  indication to shut down any necessary external circuitry.
> >>>>>> +
> >>>>>> +  Specifications about the devices can be found at:
> >>>>>> + 
> >>>>>> https://www.analog.com/media/en/technical-documentation/data-sheets/MAX34408-MAX34409.pdf
> >>>>>> +
> >>>>>> +properties:
> >>>>>> +  compatible:
> >>>>>> +    enum:
> >>>>>> +      - maxim,max34408
> >>>>>> +      - maxim,max34409
> >>>>>> +
> >>>>>> +  "#address-cells":
> >>>>>> +    const: 1
> >>>>>> +
> >>>>>> +  "#size-cells":
> >>>>>> +    const: 0
> >>>>>> +
> >>>>>> +  reg:
> >>>>>> +    maxItems: 1
> >>>>>> +
> >>>>>> +  interrupts:
> >>>>>> +    maxItems: 1
> >>>>>> +
> >>>>>> +  powerdown-gpios:
> >>>>>> +    description:
> >>>>>> +      Shutdown Output. Open-drain output. This output
> >>>>>> transitions
> >>>>>> to high impedance
> >>>>>> +      when any of the digital comparator thresholds are
> >>>>>> exceeded
> >>>>>> as long as the ENA
> >>>>>> +      pin is high.
> >>>>>> +    maxItems: 1
> >>>>>> +
> >>>>>> +  shtdn-enable-gpios:      
> >>>>>
> >>>>> I guess the review crossed with you sending v5.  There is some
> >>>>> feedback on v4 you need
> >>>>> to address here.      
> >>>>
> >>>> Jonathan, I thought I did, I've changed ena to powerdown-gpios from
> >>>> Krzysztof's comments but about this one pin I'm still not sure, it
> >>>> looks like *-enable-gpios (like in *-enable-gpios pins in
> >>>> iio/frequency/adi,adf4377.yaml) pin or is it not? Or maybe any
> >>>> other
> >>>> suggestions about naming of this one?
> >>>>
> >>>> Thanks.    
> >>>
> >>> shutdown-gpios and make the sense (active high / low) such that
> >>> setting
> >>> it results in teh device being shut down.
> >>> Or treat it as an enable and enable-gpios
> >>>
> >>> Something that indicates both shutdown and enable is confusing ;)
> >>>
> >>> Jonathan    
> >>
> >>
> >> Jonathan, then I make these changes:
> >>
> >> powerdown-gpios: -> output-enable:  
> > Needs to retain the gpios bit as we want the standard gpio stuff to pick
> > them up. I'm not that keen on output-enable-gpios though.  The activity
> > here is very much 'shutdown because of error or not enabled' I think.
> > So perhaps we flip the sense and document that it needs to be active low?
> >   
> >> shtdn-enable-gpios: -> enable-gpios:
> >>
> >> Is it ok?  
> > 
> > Conor, Rob, Krzysztof - you probably have a better insight into this than
> > I do.
> >   
> 
> "enable-gpios" are for turning on a specific feature, not powering
> on/off entire device. For example to enable regulator output.
> 
> "powerdown-gpios" are for turning device on/off.
> 
> I don't know what do you have in your device.
Ok. Sounds like that what is enable-gpios above should be shutdown-gpios.

The other case is a device output indicating whether the device is
shutdown.  That can happen because it was told to do so (via the other gpio),
or because it is in an error state. What's a good naming convention for that?

Thanks,

Jonathan

> 
> Best regards,
> Krzysztof
> 
>