On Wed, Apr 17, 2013 at 05:33:36PM +0100, Anthony Olech wrote: > This patch is relative to linux next-20130417 > > This is the HWMON component driver of the Dialog DA9058 PMIC. > This driver is just one component of the whole DA9058 PMIC driver. > It depends on the CORE and ADC component drivers of the DA9058 MFD. > > Changes relative to V3 of this patch: > - rebased to latest tagged linux-next - previously relative to mainline > Documentation/hwmon/da9058 > - added final NL > drivers/hwmon/Kconfig > - changed dependancy from I2C to MFD > drivers/hwmon/Makefile > - put in alphabetical order > drivers/hwmon/da9058-hwmon.c > - aligned subsequent lines of function declarations > - used single function for all slow labels > - used recommended ..._label as function name > - error conditions are returned as negative integers > - chaned to suggested return value casting > - removed all constant sysfile atributes except the labels > - corrected parameter to adc read function to unsigned > - used suggest name 'input' instead of 'value' > - changed first temp attribute to temp1 > - fixed expression error to boolean and from bitwise and > - removed redundant return statement > - removed race condition by initializing before create sysfs > - corected alignments on broken long lines > > Signed-off-by: Anthony Olech <anthony.olech.opensource@xxxxxxxxxxx> > Signed-off-by: David Dajun Chen <david.chen@xxxxxxxxxxx> > --- > Documentation/hwmon/da9058 | 39 +++++ > drivers/hwmon/Kconfig | 10 ++ > drivers/hwmon/Makefile | 3 +- > drivers/hwmon/da9058-hwmon.c | 349 ++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 400 insertions(+), 1 deletion(-) > create mode 100644 Documentation/hwmon/da9058 > create mode 100644 drivers/hwmon/da9058-hwmon.c > > diff --git a/Documentation/hwmon/da9058 b/Documentation/hwmon/da9058 > new file mode 100644 > index 0000000..eaedfe7 > --- /dev/null > +++ b/Documentation/hwmon/da9058 > @@ -0,0 +1,39 @@ > +Kernel driver da9058-hwmon > +========================== > + > +Supported chips: > + * Dialog Semiconductor DA9058 PMIC > + Prefix: 'da9058' > + Datasheet: > + http://www.dialog-semiconductor.com/products/power-management/da9058 > + > +Authors: Opensource [Anthony Olech] <anthony.olech.opensource@xxxxxxxxxxx> > + > +Description > +----------- > + > +The DA9058 PMIC contains a 5 channel ADC which can be used to monitor a > +range of system operating parameters, including the battery voltage and > +temperature. The ADC measures voltage, but two of the ADC channels can > +be configured to supply a current, so that if an NTC termister is connected > +then the voltage reading can be converted to a temperature. Currently the > +driver provides reporting of all the input values but does not provide any > +alarms. > + > +Voltage Monitoring > +------------------ > + > +Voltages are sampled in either 'automatic' or 'manual' mode, which is an > +initialization parameter set in the platform data by the machine driver. > +In manual mode the ADC conversion is 12 bit and in automatic mode it is > +10 bit. However all the raw readings are reported as 12 bit numbers. > + > +Physical Limits > +--------------- > + > +vbat 2500 - 4500 milliVolts > +tbat 0 - 2500 milliVolts > +adc 0 - 2500 milliVolts > +vfpin 0 - 4095 milliVolts > +tjunc there is a correction factor programmed during manufacturing > + > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index da93094..8014af2 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -324,6 +324,16 @@ config SENSORS_ATXP1 > This driver can also be built as a module. If so, the module > will be called atxp1. > > +config SENSORS_DA9058 > + tristate "Dialog Semiconductor DA9058 ADC" > + depends on MFD_DA9058 && DA9058_ADC > + help > + If you say yes here you get support for the hardware monitoring > + functionality of the Dialog Semiconductor DA9058 PMIC. > + > + This driver can also be built as a module. If so, the module > + will be called da9058-hwmon. > + > config SENSORS_DS620 > tristate "Dallas Semiconductor DS620" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index c51b0dc..5b7705a 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -46,7 +46,8 @@ obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o > obj-$(CONFIG_SENSORS_ATXP1) += atxp1.o > obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o > obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o > -obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o > +obj-$(CONFIG_SENSORS_DA9055) += da9055-hwmon.o > +obj-$(CONFIG_SENSORS_DA9058) += da9058-hwmon.o > obj-$(CONFIG_SENSORS_DME1737) += dme1737.o > obj-$(CONFIG_SENSORS_DS620) += ds620.o > obj-$(CONFIG_SENSORS_DS1621) += ds1621.o > diff --git a/drivers/hwmon/da9058-hwmon.c b/drivers/hwmon/da9058-hwmon.c > new file mode 100644 > index 0000000..b273f58 > --- /dev/null > +++ b/drivers/hwmon/da9058-hwmon.c > @@ -0,0 +1,349 @@ > +/* > + * Copyright (C) 2012 Dialog Semiconductor Ltd. > + * 2012 - 2013 ? > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + */ > + > +#include <linux/module.h> > +#include <linux/err.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> > +#include <linux/regmap.h> Are you using any regmap code ? > +#include <linux/mfd/core.h> > + > +#include <linux/mfd/da9058/version.h> > +#include <linux/mfd/da9058/registers.h> > +#include <linux/mfd/da9058/core.h> > +#include <linux/mfd/da9058/hwmon.h> Why is the hwmon include file exported ? Is it needed outside this file ? If not, the defined should just be added here, without extra include file. If the defines are needed elsewhere, I would expect to be copied on the patch adding the file. Also, do you really need to include all those header files ? For example, I don't immediately see how you use version.h. > +static ssize_t da9058_vbat_show_adc(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + struct da9058_hwmon *hwmon = dev_get_drvdata(dev); > + unsigned int voltage; /* x000 .. xFFF = 2500 .. 4500 mV */ > + int ret; > + > + ret = da9058_adc_read(hwmon->da9058, DA9058_ADCMAN_MUXSEL_VBAT, > + hwmon->use_automatic_adc, &voltage); > + if (ret) > + return ret; > + > + return sprintf(buf, "%d\n", 2500 + voltage * 2000 / 0xFFF); > +} > + > +static ssize_t da9058_tbat_show_type(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + struct da9058_hwmon *hwmon = dev_get_drvdata(dev); > + > + return sprintf(buf, "%d\n", hwmon->battery_sensor_type); > +} > + > +static ssize_t da9058_tbat_show_adc(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + struct da9058_hwmon *hwmon = dev_get_drvdata(dev); > + unsigned int voltage; /* x000 .. xFFF = 0 .. 2500 mV */ > + int ret; > + > + ret = da9058_adc_read(hwmon->da9058, DA9058_ADCMAN_MUXSEL_TEMP, > + hwmon->use_automatic_adc, &voltage); > + if (ret) > + return ret; > + > + return sprintf(buf, "%d\n", voltage * 2500 / 0xFFF); Might be a good idea to use DIV_ROUND_CLOSEST for those divisions to improve rounding. > +} > + > +static ssize_t da9058_gp_show_adc(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + struct da9058_hwmon *hwmon = dev_get_drvdata(dev); > + unsigned int voltage; /* xFFF .. x800 = 0 .. 2500 mV */ > + int ret; > + > + ret = da9058_adc_read(hwmon->da9058, DA9058_ADCMAN_MUXSEL_ADCIN, > + hwmon->use_automatic_adc, &voltage); > + if (ret) > + return ret; > + > + return sprintf(buf, "%d\n", (0xFFF - voltage) * 2500 / 0x7FF); > +} > + > +static ssize_t da9058_tjunc_show_min(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + struct da9058_hwmon *hwmon = dev_get_drvdata(dev); > + unsigned int toffreg; > + int ret = da9058_reg_read(hwmon->da9058, DA9058_TOFFSET_REG, &toffreg); > + > + if (ret) > + return ret; > + > + return sprintf(buf, "%d\n", -(1708 * (s8)((u8)toffreg) + 108800)); > +} > + > +static ssize_t da9058_tjunc_show_max(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + struct da9058_hwmon *hwmon = dev_get_drvdata(dev); > + unsigned int toffreg; > + int ret = da9058_reg_read(hwmon->da9058, DA9058_TOFFSET_REG, &toffreg); > + > + if (ret) > + return ret; > + > + return sprintf(buf, "%d\n", 1708*(255 - (s8)((u8)toffreg)) - 108800); Please watch for coding style: space before and after '*'. You have several of those in the patch. Don't ask me why checkpatch doesn't complain. > +} > + > +/* > + * The algorithm for converting the value is > + * Degrees celsius = 1.708 * (TJUNC_RES - T_OFFSET) - 108.8 > + * T_OFFSET is a trim value used to improve accuracy of the result > + */ > +static ssize_t da9058_tjunc_show_adc(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + struct da9058_hwmon *hwmon = dev_get_drvdata(dev); > + int tjunc; > + unsigned int toffreg; > + int ret; > + > + ret = da9058_reg_read(hwmon->da9058, DA9058_TOFFSET_REG, &toffreg); > + if (ret < 0) > + return ret; > + > + ret = da9058_adc_read(hwmon->da9058, DA9058_ADCMAN_MUXSEL_TJUNC, > + hwmon->use_automatic_adc, &tjunc); > + if (ret < 0) > + return ret; > + > + tjunc >>= 4; /* recover most sig 8 bits as a pos/zero number */ "Recover" is a bit odd here. Is this what you mean ? > + > + return sprintf(buf, "%d\n", 1708*(tjunc - (s8)((u8)toffreg)) - 108800); Here again > +} > +static ssize_t da9058_tjunc_show_offset(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + struct da9058_hwmon *hwmon = dev_get_drvdata(dev); > + unsigned int toffreg; > + int ret; > + > + ret = da9058_reg_read(hwmon->da9058, DA9058_TOFFSET_REG, &toffreg); > + if (ret < 0) > + return ret; > + > + return sprintf(buf, "%d\n", -1708*(s8)((u8)toffreg) - 108800); And again > +} > + > +static ssize_t da9058_vfpin_show_adc(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + struct da9058_hwmon *hwmon = dev_get_drvdata(dev); > + unsigned int voltage; /* x000 .. xFFF = 0 .. 4095 mV */ > + int ret; > + > + ret = da9058_adc_read(hwmon->da9058, DA9058_ADCMAN_MUXSEL_VF, > + hwmon->use_automatic_adc, &voltage); > + if (ret) > + return ret; > + > + return sprintf(buf, "%d\n", voltage); > +} > + > +static ssize_t da9058_hwmon_show_name(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + return sprintf(buf, "da9058\n"); > +} > + > +static ssize_t da9058_show_label(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + int channel = to_sensor_dev_attr(devattr)->index; > + > + switch (channel) { > + case 0: return sprintf(buf, "vbat\n"); > + case 1: return sprintf(buf, "tbat\n"); > + case 2: return sprintf(buf, "vfpin\n"); > + case 3: return sprintf(buf, "adc\n"); > + case 4: return sprintf(buf, "tjunc\n"); > + default: return -EINVAL; One statement per line, please. On the other side, since the value range of channel is well known, it would be simpler to declare static const char * const da9058_labels[] = { "vbat", "tbat", "vfpin", "adc", "tjunc" }; ... return sprintf("%s\n", da9058_labels[channel]); and drop the case statement entirely. Plus, you can actually merge "da9058_hwmon_show_name" into the same function by just defining another index for it. > + } > +} > + > +static DEVICE_ATTR(name, S_IRUGO, da9058_hwmon_show_name, NULL); > + > +static SENSOR_DEVICE_ATTR(in0_label, S_IRUGO, da9058_show_label, NULL, 0); > +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, da9058_vbat_show_adc, NULL, 0); > + > +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, da9058_show_label, NULL, 1); > +static SENSOR_DEVICE_ATTR(temp1_type, S_IRUGO, da9058_tbat_show_type, NULL, 1); > +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, da9058_tbat_show_adc, NULL, 1); > + > +static SENSOR_DEVICE_ATTR(in1_label, S_IRUGO, da9058_show_label, NULL, 2); > +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, da9058_vfpin_show_adc, NULL, 2); > + > +static SENSOR_DEVICE_ATTR(in2_label, S_IRUGO, da9058_show_label, NULL, 3); > +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, da9058_gp_show_adc, NULL, 3); > + > +static SENSOR_DEVICE_ATTR(temp2_label, S_IRUGO, da9058_show_label, NULL, 4); > +static SENSOR_DEVICE_ATTR(temp2_min, S_IRUGO, da9058_tjunc_show_min, NULL, 4); > +static SENSOR_DEVICE_ATTR(temp2_max, S_IRUGO, da9058_tjunc_show_max, NULL, 4); > +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, da9058_tjunc_show_adc, NULL, 4); > +static SENSOR_DEVICE_ATTR(temp2_offset, S_IRUGO, da9058_tjunc_show_offset, NULL, > + 4); Alignment ? > + > +static struct attribute *da9058_attr[] = { > + &dev_attr_name.attr, > + &sensor_dev_attr_in0_label.dev_attr.attr, > + &sensor_dev_attr_in0_input.dev_attr.attr, > + &sensor_dev_attr_temp1_label.dev_attr.attr, > + &sensor_dev_attr_temp1_type.dev_attr.attr, > + &sensor_dev_attr_temp1_input.dev_attr.attr, > + &sensor_dev_attr_in1_label.dev_attr.attr, > + &sensor_dev_attr_in1_input.dev_attr.attr, > + &sensor_dev_attr_in2_label.dev_attr.attr, > + &sensor_dev_attr_in2_input.dev_attr.attr, > + &sensor_dev_attr_temp2_label.dev_attr.attr, > + &sensor_dev_attr_temp2_min.dev_attr.attr, > + &sensor_dev_attr_temp2_max.dev_attr.attr, > + &sensor_dev_attr_temp2_input.dev_attr.attr, > + &sensor_dev_attr_temp2_offset.dev_attr.attr, > + NULL > +}; > + > +static const struct attribute_group da9058_attr_group = {.attrs = da9058_attr}; Might be better to split this instead of packing it together to avoid the 80-column limit. > + > +static int da9058_hwmon_probe(struct platform_device *pdev) > +{ > + struct da9058 *da9058 = dev_get_drvdata(pdev->dev.parent); > + const struct mfd_cell *cell = mfd_get_cell(pdev); > + struct da9058_hwmon_pdata *hwmon_pdata; > + struct da9058_hwmon *hwmon; > + int ret; > + > + if (cell == NULL) { > + ret = -ENODEV; > + goto exit; Just return -ENODEV is good enough here. See CodingStyle, chapter 7. Same below where you can return directly. > + } > + > + hwmon_pdata = cell->platform_data; > + > + if (hwmon_pdata == NULL) { > + ret = -EINVAL; > + goto exit; > + } > + > + if (hwmon_pdata->use_automatic_adc && > + !hwmon_pdata->temp_adc_resistance) { > + ret = -EINVAL; /* impossible setting */ > + goto exit; > + } > + > + hwmon = devm_kzalloc(&pdev->dev, sizeof(struct da9058_hwmon), > + GFP_KERNEL); Is the alignment correct here ? > + if (!hwmon) { > + ret = -ENOMEM; > + goto exit; > + } > + > + platform_set_drvdata(pdev, hwmon); > + > + hwmon->da9058 = da9058; > + hwmon->pdev = pdev; > + hwmon->use_automatic_adc = hwmon_pdata->use_automatic_adc; > + hwmon->temp_adc_resistance = hwmon_pdata->temp_adc_resistance; > + hwmon->vf_adc_resistance = hwmon_pdata->vf_adc_resistance; > + hwmon->battery_sensor_type = hwmon_pdata->battery_sensor_type; > + > + if (hwmon->use_automatic_adc) { > + unsigned int mode = DA9058_ADCCONT_AUTOADCEN | > + DA9058_ADCCONT_TEMPISRCEN | > + DA9058_ADCCONT_AUTOVBATEN | > + DA9058_ADCCONT_AUTOVFEN | > + DA9058_ADCCONT_AUTOAINEN; > + > + if (hwmon->vf_adc_resistance) > + mode |= DA9058_ADCCONT_VFISRCEN; > + > + ret = da9058_reg_write(da9058, DA9058_ADCCONT_REG, mode); > + if (ret) > + goto failed_to_initialize_device; > + } else { > + unsigned int mode = 0; > + > + if (hwmon->temp_adc_resistance) > + mode |= DA9058_ADCCONT_TEMPISRCEN; > + if (hwmon->vf_adc_resistance) > + mode |= DA9058_ADCCONT_VFISRCEN; > + > + ret = da9058_reg_write(da9058, DA9058_ADCCONT_REG, mode); > + if (ret) > + goto failed_to_initialize_device; The last 6 lines are duplicate and can be outside the if/else statement. Just declare mode at the beginning of the function. > + } > + > + mutex_init(&hwmon->hwmon_lock); Any idea what you plan to use this lock for ? > + > + hwmon->class_device = hwmon_device_register(&pdev->dev); > + if (IS_ERR(hwmon->class_device)) { > + ret = PTR_ERR(hwmon->class_device); > + goto failed_to_register_device; > + } hwmon_device_register comes last to prevent race conditions where the hwmon device exists and is reported to user space, but the attributes are missing. > + > + ret = sysfs_create_group(&pdev->dev.kobj, &da9058_attr_group); > + if (ret) > + goto failed_to_create_sysfs_group; > + > + goto exit; Please just return 0; here. > + > +failed_to_create_sysfs_group: > + hwmon_device_unregister(hwmon->class_device); > +failed_to_register_device: > + sysfs_remove_group(&pdev->dev.kobj, &da9058_attr_group); > +failed_to_initialize_device: > +exit: Two labels are really unnecessary here. > + return ret; > +} > + > +static int da9058_hwmon_remove(struct platform_device *pdev) > +{ > + struct da9058_hwmon *hwmon = platform_get_drvdata(pdev); > + > + sysfs_remove_group(&pdev->dev.kobj, &da9058_attr_group); > + > + One empty line is enough. > + hwmon_device_unregister(hwmon->class_device); > + hwmon_device_unregister comes first to prevent race conditions on unload. > + return 0; > +} > + > +static struct platform_driver da9058_hwmon_driver = { > + .probe = da9058_hwmon_probe, > + .remove = da9058_hwmon_remove, > + .driver = { > + .name = "da9058-hwmon", > + .owner = THIS_MODULE, > + }, > +}; > + > +module_platform_driver(da9058_hwmon_driver); > + > +MODULE_DESCRIPTION("Dialog DA9058 PMIC HardWare Monitor Driver"); HardWare isn't really common english, not even as capital letters. > +MODULE_AUTHOR("Anthony Olech <Anthony.Olech@xxxxxxxxxxx>"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:da9058-hwmon"); > -- > end-of-patch for NEW DRIVER V5 > > _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
