Hi David,
On Mon, 13 Feb 2012 11:09:55 +0100, David Jander wrote:
> On Sun, 12 Feb 2012 11:09:44 +0100
> Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>
> > Based on a patch by David Jander that mostly did s/mc13783/mc13xxx/ .
> >
> > Additionally use dev_get_drvdata instead of to_platform_device +
> > platform_get_drvdata in mc13783_adc_read (spotted by Jean Delvare).
> >
> > Cc: David Jander <david.jander@xxxxxxxxxxx>
> > Acked-by: Jean Delvare <khali@xxxxxxxxxxxx>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > ---
> > changes since v4:
> > - provide touchscreen inputs on mc13892, too
> > changes since v3:
> > - drop _adc in Documentation/hwmon/mc13783-adc, too
> > - use dev_get_drvdata instead of to_platform_device +
> > platform_get_drvdata
> > changes since v2:
> > - make code generating name attribute more robust and strip [-_]adc
> > - consistenly use MC13783_ADC_16CHANS / mc13783_attr_16chans
> > ---
> > Documentation/hwmon/mc13783-adc | 50 +++++++++++++-----
> > drivers/hwmon/Kconfig | 6 +-
> > drivers/hwmon/mc13783-adc.c | 107 +++++++++++++++++++++++++++++---------
> > 3 files changed, 121 insertions(+), 42 deletions(-)
> >
> > diff --git a/Documentation/hwmon/mc13783-adc b/Documentation/hwmon/mc13783-adc
> > index 044531a..356b10a 100644
> > --- a/Documentation/hwmon/mc13783-adc
> > +++ b/Documentation/hwmon/mc13783-adc
> > (...)
> > +See the following tables for the meaning of the different channels and their
> > +chip internal scaling:
> > +
> > +MC13783:
> > Channel Signal Input Range Scaling
> > -------------------------------------------------------------------------------
> > 0 Battery Voltage (BATT) 2.50 - 4.65V -2.40V
> > @@ -34,7 +38,7 @@ Channel Signal Input Range Scaling
> > 2 Application Supply (BP) 2.50 - 4.65V -2.40V
> > 3 Charger Voltage (CHRGRAW) 0 - 10V / /5
> > 0 - 20V /10
> > -4 Charger Current (CHRGISNSP-CHRGISNSN) -0.25V - 0.25V x4
> > +4 Charger Current (CHRGISNSP-CHRGISNSN) -0.25 - 0.25V x4
> > 5 General Purpose ADIN5 / Battery Pack Thermistor 0 - 2.30V No
> > 6 General Purpose ADIN6 / Backup Voltage (LICELL) 0 - 2.30V / No /
> > 1.50 - 3.50V -1.20V
> > @@ -48,3 +52,23 @@ Channel Signal Input Range Scaling
> > 13 General Purpose TSX2 / Touchscreen X-plate 2 0 - 2.30V No
> > 14 General Purpose TSY1 / Touchscreen Y-plate 1 0 - 2.30V No
> > 15 General Purpose TSY2 / Touchscreen Y-plate 2 0 - 2.30V No
> > +
> > +MC13892:
> > +Channel Signal Input Range Scaling
> > +-------------------------------------------------------------------------------
> > +0 Battery Voltage (BATT) 0 - 4.8V /2
> > +1 Battery Current (BATT - BATTISNSCC) -60 - 60 mV x20
> > +2 Application Supply (BPSNS) 0 - 4.8V /2
> > +3 Charger Voltage (CHRGRAW) 0 - 12V / /5
> > + 0 - 20V /10
> > +4 Charger Current (CHRGISNS-BPSNS) / -0.3 - 0.3V / x4 /
> > + Touchscreen X-plate 1 0 - 2.4V No
> > +5 General Purpose ADIN5 / Battery Pack Thermistor 0 - 2.4V No
> > +6 General Purpose ADIN6 / Backup Voltage (LICELL) 0 - 2.4V / No
> > + Backup Voltage (LICELL) 0 - 3.6V x2/3
> > +7 General Purpose ADIN7 / UID / Die Temperature 0 - 2.4V / No /
> > + 0 - 4.8V /2
> > +12 Touchscreen X-plate 1 0 - 2.4V No
> > +13 Touchscreen X-plate 2 0 - 2.4V No
> > +14 Touchscreen Y-plate 1 0 - 2.4V No
> > +15 Touchscreen Y-plate 2 0 - 2.4V No
>
> Why two different tables?
> Both ADC blocks are almost identical.
I see a lot of differences.
> IMHO it would be better to only
> highlight the minor differences, instead of making two almost identical tables
> that are hard to compare this way.
Both can be useful. From a driver maintenance perspective, a list of
differences is indeed very useful. But for someone with one specific
system with one of these chips, a table summarizing the properties of
each input has more value.
> AFAICR the only differences between the two ADC's are as follows:
>
> - ADC channel 7 has slightly different general-purpose functionality.
> - ADC channels 8...11 are not externally accessible on 13892.
> - Scaling factors on channels 0, 2 and 6 are actually offsets on the 13783 (Do
> scaling and offsets belong here or in lm_sensors?)
Scaling and offsets done inside chips belong to the driver. If done
outside the chip (using external resistors and/or an amplifier) it is
board-specific and thus belongs to libsensors configuration file.
> - In touchscreen mode, on the 13892 the 3rd sample is invalid and needs to be
> discarded, whereas on the 13783 it could be used to get one more sample to
> average.
>
> That's about it, correct me if I'm wrong.
>From the two tables, it seems that the input range is different for
almost every input, even though most differences are small (e.g. 0 -
2.3V vs. 0 - 2.4V.) So your list is interesting because it underlines
the most important differences, but it isn't exhaustive.
Feel free to send a patch adding the above to the documentation. But
don't delete the tables that we have today, please.
> > (...)
> > @@ -219,12 +261,26 @@ static int __devexit mc13783_adc_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +static const struct platform_device_id mc13783_adc_idtable[] = {
> > + {
> > + .name = "mc13783-adc",
> > + .driver_data = MC13783_ADC_16CHANS,
> > + }, {
> > + .name = "mc13892-adc",
> > + .driver_data = MC13783_ADC_BPDIV2,
>
> I am quite clueless about the meaning of these defines here... are you using
> two of the specific differences as a flag to decide which chip we are on?
No. There are two ways to handle multiple chip types in a driver,
either by carrying a chip ID or by attaching a set of feature flags to
each device type. The latter was chosen here. It happens that each
supported device at the moment has a single feature flag, but this
could change.
Note that Guenter also did not like the feature flags approach, see:
http://lists.lm-sensors.org/pipermail/lm-sensors/2011-September/033933.html
However this approach is just as valid as the other one, and with such
a small number of chip types supported and feature flags, both are
possible and this is really a matter of personal taste. So I see no
reason to change it now. We can change it latter if differences between
chips become too numerous to be easily encoded as feature flags.
> Sorry if I'm late to chime in...
A little, but better late than never, the code isn't merged upstream
yet.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
