Re: [PATCH v2 1/2] dt-bindings: iio: adc: adding dt-bindings for PAC193X

[<Marius.Cristea@microchip.com>]

Hi Jonathan,

On Fri, 2023-10-27 at 15:26 +0100, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Thu, 26 Oct 2023 17:08:07 +0100
> Conor Dooley <conor@kernel.org> wrote:
> 
> > On Thu, Oct 26, 2023 at 03:23:46PM +0000,
> > Marius.Cristea@microchip.com wrote:
> > > Hi Conor,
> > > 
> > > On Wed, 2023-10-25 at 16:08 +0100, Conor Dooley wrote:
> > > > Hey Marius,
> > > > 
> > > > On Wed, Oct 25, 2023 at 04:44:03PM +0300,
> > > > marius.cristea@microchip.com wrote:
> > > > > From: Marius Cristea <marius.cristea@microchip.com>
> > > > > 
.................
> > > > > +$id:
> > > > > http://devicetree.org/schemas/iio/adc/microchip,pac1934.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Microchip PAC1934 Power Monitors with Accumulator
> > > > > +
> > > > > +maintainers:
> > > > > +  - Marius Cristea <marius.cristea@microchip.com>
> > > > > +
> > > > > +description: |
> > > > > +  Bindings for the Microchip family of Power Monitors with
> > > > > Accumulator.
> > > > > +  The datasheet for PAC1931, PAC1932, PAC1933 and PAC1934
> > > > > can be
> > > > > found here:
> > > > > +
> > > > > https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/PAC1931-Family-Data-Sheet-DS20005850E.pdf
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - microchip,pac1931
> > > > > +      - microchip,pac1932
> > > > > +      - microchip,pac1933
> > > > > +      - microchip,pac1934
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  "#address-cells":
> > > > > +    const: 1
> > > > > +
> > > > > +  "#size-cells":
> > > > > +    const: 0
> > > > > +
> > > > > +  interrupts:
> > > > > +    description: IRQ line of the ADC
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  drive-open-drain:
> > > > > +    description: The IRQ signal is configured as open-drain.
> > > > > +    type: boolean
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  microchip,slow-io:
> > > > > +    type: boolean
> > > > > +    description: |
> > > > > +      A GPIO used to trigger a change is sampling rate
> > > > > (lowering
> > > > > the chip power consumption).
> > > > > +      In default mode, if this pin is forced high, sampling
> > > > > rate
> > > > > is forced to eight
> > > > > +      samples/second. When it is forced low, the sampling
> > > > > rate is
> > > > > 1024 samples/second unless
> > > > > +      a different sample rate has been programmed.
> > > > 
> > > > This description doesn't really make sense to me - if a GPIO is
> > > > used
> > > > to
> > > > drive the pin low or high, why do we need a property? A DT
> > > > property
> > > > implies that this is a static configuration depending on the
> > > > board,
> > > > but
> > > > reading the description this seems to be something that can be
> > > > toggled
> > > > at runtime.
> > > > I do note though, that this GPIO is not documented in the
> > > > binding, so
> > > > I
> > > > suppose what really needs to happen here is document the gpio
> > > > so that
> > > > the driver can determine at runtime what state this pin is in?
> > > > 
> > > > Also, you say "In default mode", but don't mention what the
> > > > non-
> > > > default
> > > > mode is. What happens in the other mode?
> > 
> > > This is a "double function" pin. On the PAC193x there is the
> > > SLOW/ALERT
> > > pin. At runtime this pin could be configured as an input to the
> > > PAC and
> > > the functionality will be "SLOW" that means if it is forced high,
> > > the
> > > PAC will work in low power mode by changing the sample rate to 8
> > > SPS.
> > > If it's forced low the PAC will work at it's full sample rate.
> > 
> > Since this is a runtime thing, it doesn't make sense to have a
> > property
> > that is set at dts creation time that decides what mode the pin is
> > in.
> > 
> > > "SLOW" is the default function of the pin but it may be
> > > programmed to
> > > function as ALERT pin (Open Collector when functioning as ALERT,
> > > requires pull-up resistor to VDD I/O). This time the pin will be
> > > set as
> > > output from PAC (ALERT functionality) to trigger an interrupt to
> > > the
> > > system (this is covered by the interrupts and drive-open-drain).
> > 
> > Hmm, at the risk of getting out of my depth with what the GPIO
> > subsystem
> > is capable of doing, I would expect to see something like
> > 
> > sampling-rate-gpios:
> >   description:
> >     <what you have above>
> >   maxItems: 1
> > 
> > Which would allow the driver to either drive this pin via the gpio
> > subsystem, or to use the interrupt property to use it as an
> > interrupt
> > instead.
> > 
> > Perhaps Jonathan etc knows better for these sort of dual mode pins.
> 
> Beyond them being a pain? The fun is they may get wired to interrupt
> controllers that are also GPIOs or they may not (and the other way
> around
> with them wired to GPIO pins that aren't interrupt pins).
> 
> I don't understand the usecase for the SLOW control.
> Given it seems software can override the use for SLOW I'd be tempted
> to
> always do that.
> Thus making this pin useable only as an optional interrupt.
> 

I was thinking to have something like interrupt or an equivalent to
"powerdown-gpios", "richtek,mute-enable", "shutdown-gpios", "mute-
gpios", "gain-gpios". I think the driver should know (from the Device
Tree) if the pin is routed to a gpio and it could be used as "SLOW"/low
power.

> If someone hard wires it to high or low that is harmless if we aren't
> letting it control anything.
> 
> > 
> > > The system could work fine without this pin. The driver doesn't
> > > use
> > > interrupt at this time, but it could be extended.
> > 
> > Cheers,
> > Conor.
> 
Thanks,
Marius