Hi Jean,
On Mon, 13 Feb 2012 18:12:29 +0100
Jean Delvare <khali@xxxxxxxxxxxx> wrote:
> 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.
Hmmm... ok.
> > 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.
Thanks for clarifying.
> > - 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.
Well, you are right. I forgot one more difference: The reference voltage for
the ADC is 2.3V for the 13783 and 2.4V for the 13892. Sorry for that omission.
That makes the tables indeed a little bit too different.
> Feel free to send a patch adding the above to the documentation. But
> don't delete the tables that we have today, please.
I get your point, thanks.
> > > (...)
> > > @@ -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
Actually I feel inclined to side on Guenter's opinion about this, but who am I
to say this is wrong, specially at this stage? Thanks for pointing out though.
> 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.
You convinced me. Just commit it as is, I am Ok with it.
Best regards,
--
David Jander
Protonic Holland.
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
