[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <6686e0d41dd61_397df3708e@njaxe.notmuch>
Date: Thu, 04 Jul 2024 19:50:12 +0200
From: Matteo Martelli <matteomartelli3@...il.com>
To: Conor Dooley <conor.dooley@...rochip.com>,
Matteo Martelli <matteomartelli3@...il.com>
Cc: Conor Dooley <conor@...nel.org>,
Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
linux-iio@...r.kernel.org,
devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: iio: adc: add binding for pac1921
Conor Dooley wrote:
> On Thu, Jul 04, 2024 at 12:06:31PM +0200, Matteo Martelli wrote:
> > Conor Dooley wrote:
> > > > +
> > > > + microchip,dv-gain:
> > > > + description:
> > > > + Digital multiplier to control the effective bus voltage gain. The gain
> > > > + value of 1 is the setting for the full-scale range and it can be increased
> > > > + when the system is designed for a lower VBUS range.
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + enum: [1, 2, 4, 8, 16, 32]
> > > > + default: 1
> > > > +
> > > > + microchip,di-gain:
> > >
> > > Why is this gain a fixed property in the devicetree, rather than
> > > something the user can control? Feels like it should be user
> > > controllable.
> >
> > Gains are user controllable via the IIO_CHAN_INFO_HARDWAREGAIN. I also added
> > them as DT properties thinking that they could be pre-set depending on hardware
> > specifications: for instance by board design the monitored section is already
> > known to be in a particular voltage/current range (datasheet specifies
> > gains-ranges mapping at table 4-6 and table 4-7). Then, even if gains are
> > pre-set, the user can change them at runtime for instance by scaling them down
> > upon an overflow event. However, I can get rid of those gain properties if they
> > are out of the DT scope.
>
> Usually gain values are left out of DT entirely, unless the gain is
> something set by the board, for example, whether or not some input pins
> are tied high or low.
>
> > > > + description:
> > > > + Digital multiplier to control the effective current gain. The gain
> > > > + value of 1 is the setting for the full-scale range and it can be
> > > > + increased when the system is designed for a lower VSENSE range.
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + enum: [1, 2, 4, 8, 16, 32, 64, 128]
> > > > + default: 1
> > > > +
> > > > +required:
> > > > + - compatible
> > > > + - reg
> > > > + - shunt-resistor-micro-ohms
> > >
> > > You're missing a vdd-supply btw and the !read/int pin isn't described
> > > here either. I think the latter needs a property to control it (probably
> > > a GPIO since it is intended for host control) and a default value for if
> > > the GPIO isn't provided?
> >
> > The driver does not currently handle the vdd regulator nor the gpio for the
> > !read/int pin. Should they be added to the DT schema anyway?
>
> Yes.
>
> > I think I can add the vdd regulator handling with little effort, my guess is
> > that the "vdd-supply" property can be optional and defined as "vdd-supply: true"
> > in the DT schema. Then the driver, if the vdd-supply property is present in the
> > DT, would enable the regulator during device initialization and PM resume, and
> > disable it on driver removal and PM suspend.
>
> Nah, the regulator should be marked required in the binding, since
> without power the device cannot function, right? The regulator core will
> create a dummy register if one is not provided in the device tree, so
> you don't need to add any conditional logic around regulator actions.
>
> > Reguarding the !read/int pin, the current driver overrides it with a register
> > bit so it would not be considered at all by the device.
>
> We should fully describe devices, where possible, even if the driver for
> the device doesn't use it.
>
> Cheers,
> Conor.
>
Thanks Conor, I sent a patch v2 addressing these points.
Link to v2: https://lore.kernel.org/linux-iio/20240704-iio-pac1921-v2-0-0deb95a48409@gmail.com
Matteo
Powered by blists - more mailing lists