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@xxxxxxxxxx> wrote: > > > On Thu, Oct 26, 2023 at 03:23:46PM +0000, > > Marius.Cristea@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx wrote: > > > > > From: Marius Cristea <marius.cristea@xxxxxxxxxxxxx> > > > > > ................. > > > > > +$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@xxxxxxxxxxxxx> > > > > > + > > > > > +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
