Message-ID: <20231204094157.775f604e@jic23-huawei>
Date:   Mon, 4 Dec 2023 09:41:57 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     <Marius.Cristea@...rochip.com>
Cc:     <conor@...nel.org>, <robh+dt@...nel.org>, <conor+dt@...nel.org>,
        <devicetree@...r.kernel.org>, <lars@...afoo.de>,
        <linux-iio@...r.kernel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] dt-bindings: iio: adc: adding dt-bindings for
 PAC193X

On Tue, 7 Nov 2023 08:55:48 +0000
<Marius.Cristea@...rochip.com> wrote:

> Hi
> 
> 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@...nel.org> wrote:
> >   
> > > On Thu, Oct 26, 2023 at 03:23:46PM +0000,
> > > Marius.Cristea@...rochip.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@...rochip.com wrote:  
> > > > > > From: Marius Cristea <marius.cristea@...rochip.com>
> > > > > > 
> > > > > > This is the device tree schema for iio driver for
> > > > > > Microchip PAC193X series of Power Monitors with Accumulator.
> > > > > > 
> > > > > >   
> ......
> > > > > > +
> > > > > > +  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.
> > 
> > If someone hard wires it to high or low that is harmless if we aren't
> > letting it control anything.
> >   
> 
> Here I was trying to define/describe 3 possible situations:
> - 1) the pin is not used at all, so it doesn't matter if it's connected
> somewhere
> 
> - 2) the pin is user configured as "interrupt" and it's connected to
> the interrupt controller (this case is not supported in the driver
> right now)
> 
> - 3) the pin is user configured as "SLOW" (this case is not supported
> in the driver right now). That means it should be connected to a GPIO
> pin. This function (SLOW control) will automatically change the PAC
> internal sampling frequency to lower the PAC internal power
> consumption. For example, the PAC could be configured to a sample rate
> of 1024 samples/s (it will consume maximum current). Using the SLOW
> control, the chip will internally change to 8 samples/s but the math
> internally will "behave" as the 1024 samples/s but at a much lower
> power consumption. It's very useful in case the system wants to lower
> power consumption (we still need to measure battery power consumption
> even if the system is put into a low power state). PAC internal power
> consumption is proportional to the number of channels used and also the
> sampling frequency.

So far so good, but how does 3 differ from just setting the chip to sample
at 8 samples/s, which I believe we can do from software?

Anyhow, for a DT binding, provide both gpio and interrupt as optional
and the driver can make up it's mind on what to do if both are provided.

Jonathan

> 
> 
> 
> > >   
> > > > 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

Powered by blists - more mailing lists