On 2018-08-02 02:21, Jonathan Cameron wrote:

On Tue, 31 Jul 2018 11:08:13 -0700
[off-list ref] wrote:

quoted

On 2018-07-28 04:08, Jonathan Cameron wrote:

quoted

On Wed, 25 Jul 2018 17:09:29 -0700
Siddartha Mohanadoss [off-list ref] wrote:

quoted

This patch adds support for QCOM SPMI PMIC5 family
of ADC driver that supports hardware based offset and
gain compensation. The ADC peripheral can measure both
voltage and current channels whose input signal is
connected to the PMIC ADC AMUX.

The register set and configuration has been refreshed
compared to the prior QCOM PMIC ADC family. Register
ADC5 as part of the IIO framework.

Signed-off-by: Siddartha Mohanadoss <redacted>

Hi Siddartha,

My main questions inline are around providing both PROCESSED and
RAW readouts for the channels.   Generally this is something we
don't ever do (as there is little point and it increases the exposed
ABI).  Now the oddity here is you've copied from the
qcom-spmi-vadc driver which does this and IIRC that was because
the initial submission didn't do any of the complex maths to get
to the PROCESSED values.  That was introduced later, leaving us with
a mess as we couldn't remove the existing ABI in case someone was
using it.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ba71704af4a0aae0d9e5812dbdd7bca95e181b14

So... I'm not convinced either way yet on whether we should let
this one through as a continuation of the exception we made there
or not.  Does this matter to you, or can you drop the RAW interface?

Hi Jonathan,

There could be few use cases that would be useful for the client to
have an option to read only the ADC code.

a) Few clients can request the ADC conversion and directly program
the raw code in the hardware.

Why?  Typically it's of no use to them unless they have 'magically' 
gotten
the conversion routines from somewhere.  So this never applies to 
general
purpose code, just to stuff tuned for the particular hardware platform.
Not a zero sized pool of applications, but I'm not convinced we need
to put much effort into support them (unless as I said earlier they are
already out there for this platform due to similarities to the ones 
where
we already provide this).

The use case would typically be for clients that want to be aware when
small changes in temperature occur so they could use a threshold
monitor to alarm them. If they have the raw code vs temperature it
would be easier to set the threshold monitor for notification.
It does require knowledge of scaling and mapping table.

There is also a problem - how does software know if the threshold is in
raw units or processed ones?  Normal assumption (I hope we documented 
it
somewhere) is that thresholds are matched to channel read outs. So if
the channel is processed, the threshold is in the right units, if raw
then it's in raw units.

Yes.

quoted

b) Few clients can program the thresholds in hardware with ADC code to
receive notification on a threshold crossing.

This one is interesting.  Mostly the client needs the conversion 
information
to know how to set that threshold.  Hence it is better off setting it 
in
real world units and letting the driver match that to the actual ADC 
threshold.
There is one common exception to this.  Light sensors typically use 
multiple
photodiodes to figure out illuminance and thresholds tend to based on 
one
of these raw readings.

Normally we get around this one by exposing the 'intensity' channel for 
the
photodiode in question as _raw only so the threshold can be applied 
against it.

Ok. We could use the same and expose just the _raw option for that 
channel.

quoted

c) Some clients may want to know when small movements occur,
so it would be useful for the client to measure the ADC code and they
could add a delta for the next threshold crossing.

It can still do that, it's just not as 'cheap' as it can apply those 
thresholds
as 'processed' units.  Otherwise small is extremely poorly defined so 
how
would userspace set it?

Agree, for userspace it would be better for have them set the thresholds 
in
processed units and have a generic driver handing the threshold monitor 
to scale
it to raw units and program the hardware.

quoted

d) If a client wants to do their own math and apply their own scaling 
i
can see
them requesting only the ADC code. They could add the scaling in the 
ADC
driver
but if they choose add offset to the raw code and program the hardware
then
providing only the code would be useful.

Works for specific devices, but is unusable for generic software. If we 
go
this way there is little point in having a generic subsystem which is 
one
of the reasons I resist providing this.

quoted

e) The raw ADC code is useful for debugging purpose. This point is
optional
as it can also be done by logging the ADC code with a pr_debug.

Sure, or debugfs.

quoted

quoted

There other bits inline are trivial enough I would just have
ignored or fixed them when applying if it weren't for this question.

(apologies if we have been round this before - I may have forgotten
or I may have been dozing during previous review.  This question has
come up for a few drivers recently so I'm more aware of it now)

...

quoted

quoted

quoted

+#define ADC5_CHAN(_dname, _type, _mask, _pre, _scale)			\
+	{								\
+		.datasheet_name = (_dname),				\
+		.prescale_index = _pre,					\
+		.type = _type,						\
+		.info_mask = _mask,					\
+		.scale_fn_type = _scale,				\
+	},								\
+
+#define ADC5_CHAN_TEMP(_dname, _pre, _scale)				\
+	ADC5_CHAN(_dname, IIO_TEMP,					\
+		BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_PROCESSED),	\

I'm fairly sure we've been round this before.  A device should not
supply
both raw and processed values without very very good reasons.
So far the only reasons we have had that I consider valid are:

1. We got it wrong initially and output raw values, only to have
processed
   added later to support something non linear.  We are stuck with
supporting
   raw by existing ABI.

2. We have a non linear channel that needs processed values and has
events.
   For some reason we can't map the event controls from processed back
   to raw (weird case but who knows) so we have to support both.


Now this case 'kind' of falls into case 1 as that is what I think
happened
with the qcom-spmi-vadc driver and lead to both being there.

Clients could also request the reference channels ADC code and do 
their
own
math and scaling. If there are any offsets that may be added to the 
ADC
code
then this can be done within the client driver that programs its
hardware
for any threshold crossing notification.

Hmm. I'm having trouble being convinced.  The whole point is that a 
client
should not be doing it's own scaling.  If it has an offset or similar
to apply it should be applied to the 'real world' value that is a 
standard
interface, not magically combined with it to generate the answer.

Ok. I will remove support for the raw code option and add only the
processed results for the subsequent patch. If there is a hard use case
we could use the _raw option for the specific channel like you had
listed earlier. If there are different clients and one wants only
the _raw option while the other client wants processed value then
we can revisit to check for any other alternatives.

We go through this question every couple of months, and I try to remain
consistent in blocking drivers from providing both interfaces simply 
because
it makes it very hard for generic userspace.

Jonathan

quoted

quoted

Hmm. This is awkward as in theory we are adding another 'broken'
interface
here, but it is reasonable to assume that there might be code that
requires
this interface on such a similar chip.

Do you definitely need to support both for some applications?
Technically
we would not be causing a regression if we don't support _raw as it
never worked for this particular device, but I can sympathise (and be
persuaded)
to let it go here if there is a strong usecase.

quoted

+		_pre, _scale)						\

...

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html