Message-ID: <20140223220140.GA1258@earth.universe>
Date:	Sun, 23 Feb 2014 23:01:40 +0100
From:	Sebastian Reichel <sre@...ian.org>
To:	Jonathan Cameron <jic23@...nel.org>,
	Marek Belisko <marek@...delico.com>
Cc:	Lee Jones <lee.jones@...aro.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Grant Likely <grant.likely@...aro.org>,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linux-iio@...r.kernel.org
Subject: Re: [RFCv1 4/4] mfd: twl4030-madc: Move driver to drivers/iio/adc

Hi,

On Sun, Feb 23, 2014 at 11:02:14AM +0000, Jonathan Cameron wrote:
> On 23/02/14 00:35, Sebastian Reichel wrote:
> >I think the optimal workflow is:
> >
> >1. convert madc driver to IIO
> >2. convert twl4030-madc-battery driver to IIO API
> >3. convert rx51-battery to IIO API
> >4. convert twl4030-madc-hwmon to IIO API / deprecate it
> >5. remove old in-kernel ABI from madc
> >6. cleanup/simplify madc
> >
> >I guess its much simpler to do the driver cleanup/simplification
> >once we can get rid of the old API.
> 
> Perhaps.  I guess it depends on your planned schedule.  If you are
> intending to plough through the above in the near future then fair
> enough.  If the chances are it'll take a while, I'd be inclined to
> fix some of the more glarringly hideous bits now (such as error
> code reporting) and then visit your list above over time.

I plan to update the rx51-battery driver, since that's the only one
I can test. Actually I only did the changes to the twl4030-madc
driver, so that I can use the standard DT API for AD converters for
rx51-battery. I hope to get it updated ASAP.

Marek seems to work on getting DT support for the other two
consumers (twl4030-madc-battery and twl4030-madc-hwmon), so chances
are, that the old API can be removed in the near future :)

> Note that to my mind it is converted drivers like this that sneak some
> nasty code into an a subsystem I look after.  We have a lot of 'interesting'
> code out in staging still, but for new submissions we are being increasingly
> fussy about little bits.  So I hope I don't come across as being too mean
> about this driver.  I'm glad you are putting in the work to convert
> it over.  There are a quite a few similar drivers sculking through the tree
> so it's nice that someone has gotten started on bringing one over to IIO.

If it makes you feel more at ease I will try to forward the errors
and replace the error code with 0 at the end.

> [...]
>
> >>Is the average have an adjustable number of samples?
> >
> >The TWL supports reading a value directly (1 sample) or reading
> >the average of 4 samples.
> >
> >>If not I'd be tempted to use the more common option of IIO_CHAN_INFO_RAW
> >>rather than the average version (tends only to be used in fairly obscure
> >>cases where both a raw access and an averaged one are available).
> >
> >currently some drivers use the averaged read and some use the direct
> >read.
> >
> 
> In that case I'd expose both the 1 sample and averaged versions via the
> IIO interfaces. If you want to pick 1 then do IIO_CHAN_INFO_RAW not the
> averaged version.

That's what I intended to do. Now I see, that I forgot to expose
IIO_CHAN_INFO_RAW. Sorry for the confusion.

> >>>+static const struct iio_chan_spec twl4030_madc_iio_channels[] = {
> >>>+	ADC_CHANNEL(0, IIO_VOLTAGE, "ADCIN0", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(1, IIO_TEMP, "ADCIN1", BIT(IIO_CHAN_INFO_PROCESSED) |
> >>>+					   BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(2, IIO_VOLTAGE, "ADCIN2", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(3, IIO_VOLTAGE, "ADCIN3", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(4, IIO_VOLTAGE, "ADCIN4", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(5, IIO_VOLTAGE, "ADCIN5", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(6, IIO_VOLTAGE, "ADCIN6", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(7, IIO_VOLTAGE, "ADCIN7", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(8, IIO_VOLTAGE, "ADCIN8", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(9, IIO_VOLTAGE, "ADCIN9", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(10, IIO_CURRENT, "ADCIN10", BIT(IIO_CHAN_INFO_PROCESSED) |
> >>>+						BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(11, IIO_VOLTAGE, "ADCIN11", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(12, IIO_VOLTAGE, "ADCIN12", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(13, IIO_VOLTAGE, "ADCIN13", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(14, IIO_VOLTAGE, "ADCIN14", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+	ADC_CHANNEL(15, IIO_VOLTAGE, "ADCIN15", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >>>+};
> >>>+
> >>Why the artificial limitation of one of these devices?  I guess this is to
> >>allow the exported function to work without needing to be associated with
> >>any particular device...  Hmm.
> >
> >Artificial limitation? The madc is a ADC, which has some special
> >functions defined for some pins in the datasheet:
> >
> >ADC0 = Battery Voltage
> >ADC1 = Battery Temperature
> >ADC10 = Battery Current
> >
> >Nokia misused some of those pins on the Nokia N900, though. For
> >example they have their own conversion tables for the temperature,
> >which is not compatible with the generic one for the madc module.
> 
> *laughs*.  If someone documents a pin as having a particular purpose
> but it isn't wired inside a package. Someone will always think they know
> better.  Basic principal is to never limit ourselves to one instance of
> a chip because the chances of someone deciding to do something interesting
> with multiple chips is way to high.

;)

> >
> >I planned to convert the rx51-battery driver to simply read the raw
> >values. All other users can use the processed information.
> Makes sense.

ok :)

> [...]
> >>>+	if (count_req)
> >>>+		dev_err(madc->dev, "%d channel conversion failed\n", count_req);
> >>>+
> >>>+	return count;
> >>This is already apparant from the errors emmited earlier. I'd drop this
> >>and the count_req counter in general.
> >>Personally I'd error out in those cases anyway rather than carrying on
> >>with the rest of the channels. If it's worth of a dev_err it's worthy
> >>of getting out as fast as possible rather than trying to muddle on.
> >
> >I also think this should be postponed until the old API is removed.
> I'd prefer to see standalone cleanups and fixes integrated first as I suspect
> it may be a little while before we manage to fully drop the old API.

ok. I have included this change in the fixup patch. This slightly
changes the old API (callback may be called with negative error
values), but the callback feature doesn't seem to be used by any
mainline user.

> [...]
> >>Can this and the previous loop not be combined thus simplifying some tests?
> >
> >It probably can be, but I think this can wait until the old API is
> >removed.
> Hmm. As I state below, if this is moving over to IIO (which is sensible) it
> becomes my problem so I reserve the right to be fussy about less than optimal
> code.  If it was coming in clean as a new driver I'd push back hard on a lot
> of this stuff before merging it.  I have plenty of slightly uggly drivers
> in staging to look after as it is!

I would feel more comfortable leaving the second loop as is for now.
It's a speed optimization, which acknowledges all IRQs as fast as
possible (=> before triggering further conversion of the raw data).

> [...]
> >>>+static int twl4030_madc_start_conversion(struct twl4030_madc_data *madc,
> >>>+					 int conv_method)
> >>>+{
> >>>+	const struct twl4030_madc_conversion_method *method;
> >>>+	int ret = 0;
> >>>+	method = &twl4030_conversion_methods[conv_method];
> >>>+	switch (conv_method) {
> >>Can we get here via any paths where these aren't the methods set?
> >
> >conv_method is set from outside of the driver, so it's probably
> >safer to check. This can be simplified once the old API is removed.
> Then we should be returning an error if it's not one of these.

right and reading the code more carefully it should be tested before
using it for array access.

> >I assume, that the driver will get a huge cleanup once all consumers
> >of the old API are converted. Maybe a deprecation flag should be
> >added together with this patchset?
> Probably not worth bothering for an inkernel interface that seems to have
> relatively few users.  How many out of tree users will be moving to
> new kernels and using this device?

At least no new twl4030-madc consumer has been posted to
linux-kernel or linux-omap in the last months. So I will
ignore this.

> [...]
> >>Personally I'd rank the driver as rather to vebose on read error messages
> >>given they are pretty unusual.  Ah well I guess each to their own.
> >
> >All of this is not written by me.
> Sure.  Doesn't stop me moaning :)  At the end of the day you are proposing moving
> this driver into a place where it becomes at least partly my problem.  As such
> I'm keen on any tidying / cleaning occuring ASAP.

Fair enough.

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists