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

Jonathan Cameron
2023-11-26T16:04:48Z

On Sun, 26 Nov 2023 11:24:56 +0000
Conor Dooley <conor@kernel.org> wrote:

> On Sat, Nov 25, 2023 at 07:47:54PM +0000, Jonathan Cameron wrote:
> > On Thu, 16 Nov 2023 18:21:33 +0000
> > Conor Dooley <conor@kernel.org> wrote:
> > > On Thu, Nov 16, 2023 at 04:01:43PM +0100, Krzysztof Kozlowski wrote:
> > > > On 15/11/2023 14:44, marius.cristea@microchip.com wrote:
> > > > > From: Marius Cristea <marius.cristea@microchip.com>
>
> > > > > +patternProperties:
> > > > > + "^channel@[1-4]+$":
> > > > > + type: object
> > > > > + $ref: adc.yaml
> > > > > + description: Represents the external channels which are connected to the ADC.
> > > > > +
> > > > > + properties:
> > > > > + reg:
> > > > > + items:
> > > > > + minimum: 1
> > > > > + maximum: 4
> > > > > +
> > > > > + shunt-resistor-micro-ohms:
> > > > > + description: |
> > > > > + Value in micro Ohms of the shunt resistor connected between
> > > > > + the SENSE+ and SENSE- inputs, across which the current is measured. Value
> > > > > + is needed to compute the scaling of the measured current.
> > > > > +
> > > > > + required:
> > > > > + - reg
> > > > > + - shunt-resistor-micro-ohms
> > > > > +
> > > > > + unevaluatedProperties: false
> > > > > +
> > > > > +required:
> > > > > + - compatible
> > > > > + - reg
> > > > > + - "#address-cells"
> > > > > + - "#size-cells"
> > > > > +
> > > > > +allOf:
> > > > > + - if:
> > > > > + properties:
> > > > > + compatible:
> > > > > + contains:
> > > > > + const: interrupts
> > > >
> > > >
> > > > I don't understand what do you want to say here. I am also 100% sure you
> > > > did not test it on a real case (maybe example passes but nothing more).
> > >
> > > As far as I understand, the same pin on the device is used for both an
> > > output or an input depending on the configuration. As an input, it is
> > > the "slow-io" control, and as an output it is an interrupt.
> > > I think Marius is trying to convey that either this pin can be in
> > > exclusively one state or another.
> > >
> > > _However_ I am not sure that that is really the right thing to do - they
> > > might well be mutually exclusive modes, but I think the decision can be
> > > made at runtime, rather than at devicetree creation time. Say for
> > > example the GPIO controller this is connected to is capable of acting as
> > > an interrupt controller. Unless I am misunderstanding the runtime
> > > configurability of this hardware, I think it is possible to actually
> > > provide a "slow-io-gpios" and an interrupt property & let the operating
> > > system decide at runtime which mode it wants to work in.
> >
> > I'll admit I've long forgotten what was going on here, but based just on
> > this bit of text I agree. There is nothing 'stopping' us having a pin
> > uses as either / or / both interrupt and gpio.
> >
> > It'll be a bit messy to support in the driver as IIRC there are some sanity
> > checks that limit combinations on IRQs and output GPIOS. Can't remember
> > how bad the dance to navigate it safely is.
> >
> > First version I'd just say pick one option if both are provided and
> > don't support configuring it at runtime.
>
> Just to be clear, you are suggesting having the
> "microchip,slow-io-gpios" and "interrupts" properties in the binding,
> but the driver will just (for example) put that pin into alert mode
> always & leave it there?

Yes.

> If that is what you are suggesting, that seems pragmatic to me.

If a use case to do something else comes along later, then we can
be smarter, but I'd like to keep it simple initially at least.

Jonathan

>
> Cheers,
> Conor.
>