Hi Uwe, On Tue, 27 Sep 2011 23:16:09 +0200, Uwe Kleine-König wrote: > Based on a patch by David Jander that mostly did s/mc13783/mc13xxx/ > > Cc: David Jander <david.jander@xxxxxxxxxxx> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> > --- > changes since (implicit) v1: > - update documentation > - undo driver renaming > - fix show_name callback (not 100% sure it is correct to give a different name > on mc13892. I think name contains the prefix documented in > Documentation/hwmon/mc13783-adc?!) > - implement some differences between mc13783 and mc13892 that I noticed when > updating the docs > - different scaling for bp channel > - no general purpose channels 8-15 on mc13892 > - take over authorship as the patch got much more complicated now compared to > David's original. > > Documentation/hwmon/mc13783-adc | 48 ++++++++++++---- > drivers/hwmon/Kconfig | 6 +- > drivers/hwmon/mc13783-adc.c | 113 +++++++++++++++++++++++++++++---------- > 3 files changed, 123 insertions(+), 44 deletions(-) As merging this got stuck after Guenter's last review (thanks Guenter, BTW) I am picking up from there and hopefully we can get this upstream. Here I go with my review, mostly minor things: > > diff --git a/Documentation/hwmon/mc13783-adc b/Documentation/hwmon/mc13783-adc > index 044531a..8b717f5 100644 > --- a/Documentation/hwmon/mc13783-adc > +++ b/Documentation/hwmon/mc13783-adc > @@ -5,6 +5,9 @@ Supported chips: > * Freescale Atlas MC13783 > Prefix: 'mc13783_adc' > Datasheet: http://www.freescale.com/files/rf_if/doc/data_sheet/MC13783.pdf?fsrch=1 > + * Freescale Atlas MC13892 > + Prefix: 'mc13892_adc' > + Datasheet: http://cache.freescale.com/files/analog/doc/data_sheet/MC13892.pdf?fsrch=1&sr=1 > > Authors: > Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> > @@ -13,20 +16,21 @@ Authors: > Description > ----------- > > -The Freescale MC13783 is a Power Management and Audio Circuit. Among > -other things it contains a 10-bit A/D converter. The converter has 16 > -channels which can be used in different modes. > -The A/D converter has a resolution of 2.25mV. Channels 0-4 have > -a dedicated meaning with chip internal scaling applied. Channels 5-7 > -can be used as general purpose inputs or alternatively in a dedicated > -mode. Channels 12-15 are occupied by the touchscreen if it's active. > +The Freescale MC13783 and MC13892 are Power Management and Audio Circuits. > +Among other things they contain a 10-bit A/D converter. The converter has 16 > +(MC13783) resp. 12 (MC13892) channels which can be used in different modes. The > +A/D converter has a resolution of 2.25mV. > > -Currently the driver only supports channels 2 and 5-15 with no alternative > -modes for channels 5-7. > +Some channels can be used as General Purpose inputs or in a dedicated mode with > +a chip internal scaling applied . > > -See this table for the meaning of the different channels and their chip > -internal scaling: > +Currently the driver only supports BP, the General Purpose inputs and > +touchscreen. It might be a good idea to explain what BP is... Me, I have no idea. > > +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 > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 0b62c3c..e3e3bc0 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1322,10 +1322,10 @@ config SENSORS_APPLESMC > the awesome power of applesmc. > > config SENSORS_MC13783_ADC > - tristate "Freescale MC13783 ADC" > - depends on MFD_MC13783 BTW, isn't CONFIG_MFD_MC13783 supposed to go away now? It seems to be an alias for MFD_MC13XXX today. 4 drivers are still using it... > + tristate "Freescale MC13783/MC13892 ADC" > + depends on MFD_MC13XXX > help > - Support for the A/D converter on MC13783 PMIC. > + Support for the A/D converter on MC13783 and MC13892 PMIC. > > if ACPI > > diff --git a/drivers/hwmon/mc13783-adc.c b/drivers/hwmon/mc13783-adc.c > index ef65ab5..0d796f7 100644 > --- a/drivers/hwmon/mc13783-adc.c > +++ b/drivers/hwmon/mc13783-adc.c > @@ -1,5 +1,5 @@ > /* > - * Driver for the Freescale Semiconductor MC13783 adc. > + * Driver for the adc on Freescale Semiconductor MC13783 and MC13892 PMICs. Would be better spelled ADC, methinks. > * > * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved. > * Copyright (C) 2009 Sascha Hauer, Pengutronix > @@ -18,7 +18,7 @@ > * Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > */ > > -#include <linux/mfd/mc13783.h> > +#include <linux/mfd/mc13xxx.h> > #include <linux/platform_device.h> > #include <linux/hwmon-sysfs.h> > #include <linux/kernel.h> > @@ -28,7 +28,11 @@ > #include <linux/init.h> > #include <linux/err.h> > > -#define MC13783_ADC_NAME "mc13783-adc" > +#define DRIVER_NAME "mc13783-adc" > + > +/* platform device id driver data */ > +#define MC13783_ADC_16CHANS 1 > +#define MC13783_ADC_BPDIV2 2 > > struct mc13783_adc_priv { > struct mc13xxx *mc13xxx; > @@ -38,7 +42,13 @@ struct mc13783_adc_priv { > static ssize_t mc13783_adc_show_name(struct device *dev, struct device_attribute > *devattr, char *buf) > { > - return sprintf(buf, "mc13783_adc\n"); > + struct platform_device *pdev = to_platform_device(dev); > + ssize_t ret = sprintf(buf, "%s\n", pdev->name); > + > + if (ret > 7 && buf[7] == '-') > + buf[7] = '_'; > + > + return ret; > } I share Guenter's concerns about this code. This is simply too fragile. Storing the right string, or a pointer thereto, in struct mc13783_adc_priv would be better. Your argument about the wasted memory size doesn't really hold, we're talking about 30 bytes here, tops. Or if you really want to dynamically turn "-" into "_" then you should do that in a robust way, for example: char *dash; dash = strchr(buf, '-'); if (dash) *dash = '_'; That way you no longer depend on the length of the string nor the position of the dash. As a side note, I'm not quite sure why the _adc suffix was preserved in the name attribute in the first place. Given that this is a hwmon device attribute, it seems redundant. As another side note, if you really care about memory consumption, then you could call sysfs_remove_group() unconditionally everywhere, as it's OK to remove files which do not exist. This would let you mark mc13783_adc_use_touchscreen() as __init. It may also make sense to move file removal to a separate function to avoid code redundancy between mc13783_adc_probe() and mc13783_adc_remove() (or even adjust mc13783_adc_remove() so that it can be called straight from mc13783_adc_probe()'s error path. > > static int mc13783_adc_read(struct device *dev, > @@ -68,16 +78,21 @@ static ssize_t mc13783_adc_read_bp(struct device *dev, > struct device_attribute *devattr, char *buf) > { > unsigned val; > + struct platform_device *pdev = to_platform_device(dev); > + kernel_ulong_t driver_data = platform_get_device_id(pdev)->driver_data; > int ret = mc13783_adc_read(dev, devattr, &val); > > if (ret) > return ret; > > - /* > - * BP (channel 2) reports with offset 2.4V to the actual value to fit > - * the input range of the ADC. unit = 2.25mV = 9/4 mV. > - */ > - val = DIV_ROUND_CLOSEST(val * 9, 4) + 2400; > + if (driver_data & MC13783_ADC_BPDIV2) > + val = DIV_ROUND_CLOSEST(val * 9, 2); > + else > + /* > + * BP (channel 2) reports with offset 2.4V to the actual value > + * to fit the input range of the ADC. unit = 2.25mV = 9/4 mV. > + */ > + val = DIV_ROUND_CLOSEST(val * 9, 4) + 2400; > > return sprintf(buf, "%u\n", val); > } > @@ -114,12 +129,21 @@ static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO, mc13783_adc_read_gp, NULL, 13); > static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO, mc13783_adc_read_gp, NULL, 14); > static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO, mc13783_adc_read_gp, NULL, 15); > > -static struct attribute *mc13783_attr[] = { > +static struct attribute *mc13783_attr_base[] = { > &dev_attr_name.attr, > &sensor_dev_attr_in2_input.dev_attr.attr, > &sensor_dev_attr_in5_input.dev_attr.attr, > &sensor_dev_attr_in6_input.dev_attr.attr, > &sensor_dev_attr_in7_input.dev_attr.attr, > + NULL > +}; > + > +static const struct attribute_group mc13783_group_base = { > + .attrs = mc13783_attr_base, > +}; > + > +/* these are only used if MC13783_ADC_16CHANS is provided in driver data */ > +static struct attribute *mc13783_attr_16chan[] = { 16CHANS vs. 16chan. Would be good to decide for a spelling and stick to it. > &sensor_dev_attr_in8_input.dev_attr.attr, > &sensor_dev_attr_in9_input.dev_attr.attr, > &sensor_dev_attr_in10_input.dev_attr.attr, > @@ -127,8 +151,8 @@ static struct attribute *mc13783_attr[] = { > NULL > }; > > -static const struct attribute_group mc13783_group = { > - .attrs = mc13783_attr, > +static const struct attribute_group mc13783_group_16chan = { > + .attrs = mc13783_attr_16chan, > }; > > /* last four channels may be occupied by the touchscreen */ > @@ -156,6 +180,7 @@ static int __init mc13783_adc_probe(struct platform_device *pdev) > { > struct mc13783_adc_priv *priv; > int ret; > + kernel_ulong_t driver_data = platform_get_device_id(pdev)->driver_data; > > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > if (!priv) > @@ -166,14 +191,22 @@ static int __init mc13783_adc_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, priv); > > /* Register sysfs hooks */ > - ret = sysfs_create_group(&pdev->dev.kobj, &mc13783_group); > + ret = sysfs_create_group(&pdev->dev.kobj, &mc13783_group_base); > if (ret) > - goto out_err_create1; > + goto out_err_create_base; > > - if (!mc13783_adc_use_touchscreen(pdev)) { > - ret = sysfs_create_group(&pdev->dev.kobj, &mc13783_group_ts); > + if (driver_data & MC13783_ADC_16CHANS) { > + ret = sysfs_create_group(&pdev->dev.kobj, > + &mc13783_group_16chan); > if (ret) > - goto out_err_create2; > + goto out_err_create_16chan; > + > + if (!mc13783_adc_use_touchscreen(pdev)) { > + ret = sysfs_create_group(&pdev->dev.kobj, > + &mc13783_group_ts); > + if (ret) > + goto out_err_create_ts; > + } > } > > priv->hwmon_dev = hwmon_device_register(&pdev->dev); > @@ -184,17 +217,21 @@ static int __init mc13783_adc_probe(struct platform_device *pdev) > goto out_err_register; > } > > - > return 0; > > out_err_register: > > - if (!mc13783_adc_use_touchscreen(pdev)) > - sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_ts); > -out_err_create2: > + if (driver_data & MC13783_ADC_16CHANS) { > + if (!mc13783_adc_use_touchscreen(pdev)) > + sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_ts); > +out_err_create_ts: > + > + sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_16chan); > + } > +out_err_create_16chan: > > - sysfs_remove_group(&pdev->dev.kobj, &mc13783_group); > -out_err_create1: > + sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_base); > +out_err_create_base: > > platform_set_drvdata(pdev, NULL); > kfree(priv); > @@ -205,13 +242,18 @@ out_err_create1: > static int __devexit mc13783_adc_remove(struct platform_device *pdev) > { > struct mc13783_adc_priv *priv = platform_get_drvdata(pdev); > + kernel_ulong_t driver_data = platform_get_device_id(pdev)->driver_data; > > hwmon_device_unregister(priv->hwmon_dev); > > - if (!mc13783_adc_use_touchscreen(pdev)) > - sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_ts); > + if (driver_data & MC13783_ADC_16CHANS) { > + if (!mc13783_adc_use_touchscreen(pdev)) > + sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_ts); > > - sysfs_remove_group(&pdev->dev.kobj, &mc13783_group); > + sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_16chan); > + } > + > + sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_base); > > platform_set_drvdata(pdev, NULL); > kfree(priv); > @@ -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, > + }, { > + /* sentinel */ > + } > +}; > +MODULE_DEVICE_TABLE(platform, mc13783_adc_idtable); > + > static struct platform_driver mc13783_adc_driver = { > - .remove = __devexit_p(mc13783_adc_remove), > + .remove = __devexit_p(mc13783_adc_remove), > .driver = { > .owner = THIS_MODULE, > - .name = MC13783_ADC_NAME, > + .name = DRIVER_NAME, > }, > + .id_table = mc13783_adc_idtable, > }; > > static int __init mc13783_adc_init(void) > @@ -243,4 +299,3 @@ module_exit(mc13783_adc_exit); > MODULE_DESCRIPTION("MC13783 ADC driver"); > MODULE_AUTHOR("Luotao Fu <l.fu@xxxxxxxxxxxxxx>"); > MODULE_LICENSE("GPL"); > -MODULE_ALIAS("platform:" MC13783_ADC_NAME); Will you resend an updated patch, or should I update it myself with the changes above? -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
