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

From: <Marius.Cristea@microchip.com>
To: <conor@kernel.org>
Cc: <linux-iio@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<lars@metafoo.de>, <linux-kernel@vger.kernel.org>,
	<jic23@kernel.org>, <conor+dt@kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>, <robh+dt@kernel.org>
Subject: Re: [PATCH v2 1/2] dt-bindings: iio: adc: adding dt-bindings for PAC193X
Date: Thu, 26 Oct 2023 15:23:46 +0000	[thread overview]
Message-ID: <937af3ec4012c6ec1d66285660d8c56dcf356703.camel@microchip.com> (raw)
In-Reply-To: <20231025-cheddar-tucking-b2ea777ed4f9@spud>

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>
> > 
> > This is the device tree schema for iio driver for
> > Microchip PAC193X series of Power Monitors with Accumulator.
> > 
> > Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> > ---
> >  .../bindings/iio/adc/microchip,pac1934.yaml   | 146
> > ++++++++++++++++++
> >  1 file changed, 146 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > new file mode 100644
> > index 000000000000..837053ed8a71
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml
> > @@ -0,0 +1,146 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$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?
> 
> Cheers,
> Conor.

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.

"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).

The system could work fine without this pin. The driver doesn't use
interrupt at this time, but it could be extended.

Thanks,
Marius