Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754033Ab2BFRzE (ORCPT ); Mon, 6 Feb 2012 12:55:04 -0500 Received: from imr4.ericy.com ([198.24.6.9]:59547 "EHLO imr4.ericy.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750844Ab2BFRzA (ORCPT ); Mon, 6 Feb 2012 12:55:00 -0500 Message-ID: <1328550761.2261.243.camel@groeck-laptop> Subject: Re: [PATCH 02/07] HWMON: DA9052/53 hwmon driver v1 From: Guenter Roeck Reply-To: guenter.roeck@ericsson.com To: Ashish Jangam CC: "randy.dunlap@oracle.com" , "linaro-dev@lists.linaro.org" , "dchen@diasemi.com" , "linux-kernel@vger.kernel.org" , "lm-sensors@lm-sensors.org" Date: Mon, 6 Feb 2012 09:52:41 -0800 In-Reply-To: <1328531989.30549.72.camel@dhruva> References: <1328531989.30549.72.camel@dhruva> Organization: Ericsson Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.2- Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 19253 Lines: 537 On Mon, 2012-02-06 at 07:39 -0500, Ashish Jangam wrote: > The DA9052 PMIC provides an Analogue to Digital Converter with 10 bits > resolution and 10 channels. > > This patch montiors the DA9052 PMIC's ADC channels mostly for battery > parameters like battery temperature, junction temperature, battery > current etc. > > This patch is functionally tested on Samsung SMDKV6410. > Hi Ashish, Looks pretty good. Couple of comments below. > Signed-off-by: David Dajun Chen > Signed-off-by: Ashish Jangam > --- > Documentation/hwmon/da9052 | 59 +++++++ > drivers/hwmon/Kconfig | 10 ++ > drivers/hwmon/Makefile | 1 + > drivers/hwmon/da9052-hwmon.c | 343 ++++++++++++++++++++++++++++++++++++++++++ Can you consider adding yourself or David as maintainer ? Not a must, but no one else would really be able to support the driver. > 4 files changed, 413 insertions(+), 0 deletions(-) > create mode 100644 Documentation/hwmon/da9052 > create mode 100644 drivers/hwmon/da9052-hwmon.c > diff --git a/Documentation/hwmon/da9052 b/Documentation/hwmon/da9052 > new file mode 100644 > index 0000000..3035bae > --- /dev/null > +++ b/Documentation/hwmon/da9052 > @@ -0,0 +1,59 @@ > +Supported chips: > + * Dialog Semiconductors DA9052-BC and DA9053-AA/Bx PMICs > + Prefix: 'da9052' > + Datasheet: Kindly visit www.dialog-semiconductor.com and request for the > + official datasheet. > + Maybe, but that doesn't seem to be straightforward. The web site has no search field, and I am not sure who to contact in the contact list. And since there does not seem to be support in the US, I am not sure if one is expected to contact someone in Europe or Taiwan or some other place. Maybe you can let me know how exactly to get the datasheets - I like to keep those around for chips we are supporting. Just saying ... for now I gave up trying, meaning some my feedback is guesswork and may be wrong. > +Authors: David Dajun Chen > + > +Description > +----------- > + > +The DA9052/53 provides an Analogue to Digital Converter (ADC) with 10 bits > +resolution and track and hold circuitry combined with an analogue input > +multiplexer. The analogue input multiplexer will allow conversion of up to 10 > +different inputs. The track and hold circuit ensures stable input voltages at > +the input of the ADC during the conversion. > + > +The ADC is used to measure the following inputs: > +Channel 0: VDDOUT - measurement of the system voltage > +Channel 1: ICH - internal battery charger current measurement > +Channel 2: TBAT - output from the battery NTC > +Channel 3: VBAT - measurement of the battery voltage > +Channel 4: ADC_IN4 - high impedance input (0 - 2.5V) > +Channel 5: ADC_IN5 - high impedance input (0 - 2.5V) > +Channel 6: ADC_IN6 - high impedance input (0 - 2.5V) > +Channel 7: XY - TSI interface to measure the X and Y voltage of the touch > +screen resistive potentiometers > +Channel 8: Internal Tjunc. - sense (internal temp. sensor) > +Channel 9: VBBAT - measurement of the backup battery voltage > + > +By using sysfs attributes we can measure the system voltage VDDOUT, the battery > +charging current ICH, battery temperature TBAT, battery junction temperature > +TJUNC, battery voltage VBAT and the back up battery voltage VBBAT. > + > +Voltage Monitoring > +------------------ > + > +Voltages are sampled by a 10 bit ADC. ERROR: trailing whitespace #121: FILE: Documentation/hwmon/da9052:38: +Voltages are sampled by a 10 bit ADC. $ > + > +The battery voltage is calculated as: > + Milli volt = ((ADC value * 1000) / 512) + 2500 > + > +The voltages on ADC channels 4, 5 and 6 are calculated as: > + Milli volt = (ADC value * 2500) / 1023 > + > +Temperature Monitoring > +---------------------- > + > +Temperatures are sampled by a 10 bit ADC. Junction and battery temperatures > +are monitored by the ADC channels. > + > +The junction temperature is calculated: > + Degrees celsius = 1.708 * (TJUNC_RES - T_OFFSET) - 108.8 > +The junction temperature attribute is supported by the driver. > + > +The battery temperature is calculated: > + Degree Celcius = 1 / (t1 + 1/298)- 273 > +where t1 = (1/B)* ln(( ADCval * 2.5)/(R25*ITBAT*255)) > +Default values of R25, B, ITBAT are 10e3, 3380 and 50e-6 respectively. > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 24d584a..a35d914 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -313,6 +313,16 @@ config SENSORS_DS1621 > This driver can also be built as a module. If so, the module > will be called ds1621. > > +config SENSORS_DA9052_ADC > + tristate "Dialog DA9052/DA9053 HWMON" "HWMON" is a bit redundant here. Should probably be "ADC". > + depends on PMIC_DA9052 > + help > + Say y here to support the ADC found on Dialog Semiconductor > + DA9052-BC and DA9053-AA/Bx PMICs. > + > + This driver can also be built as module. If so, the module > + will be called da9052-hwmon. > + > config SENSORS_EXYNOS4_TMU > tristate "Temperature sensor on Samsung EXYNOS4" > depends on ARCH_EXYNOS4 > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 8251ce8..5fe96e5 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -42,6 +42,7 @@ obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o > 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_DME1737) += dme1737.o > obj-$(CONFIG_SENSORS_DS620) += ds620.o > obj-$(CONFIG_SENSORS_DS1621) += ds1621.o > diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c > new file mode 100644 > index 0000000..d83f988 > --- /dev/null > +++ b/drivers/hwmon/da9052-hwmon.c > @@ -0,0 +1,343 @@ > +/* > + * HWMON Driver for Dialog DA9052 > + * > + * Copyright(c) 2012 Dialog Semiconductor Ltd. > + * > + * Author: David Dajun Chen > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +struct da9052_hwmon { > + struct da9052 *da9052; > + struct device *class_device; > + struct mutex hwmon_lock; > +}; > + > +static const char * const input_names[] = { > + [DA9052_ADC_VDDOUT] = "VDDOUT", > + [DA9052_ADC_ICH] = "CHARGING CURRENT", > + [DA9052_ADC_TBAT] = "BATTERY TEMP", > + [DA9052_ADC_VBAT] = "BATTERY VOLTAGE", > + [DA9052_ADC_IN4] = "ADC IN4", > + [DA9052_ADC_IN5] = "ADC IN5", > + [DA9052_ADC_IN6] = "ADC IN6", > + [DA9052_ADC_TJUNC] = "BATTERY JUNCTION TEMP", > + [DA9052_ADC_VBBAT] = "BACK-UP BATTERY VOLTAGE", > +}; > + > +static inline int volt_reg_to_mV(int value) > +{ > + return ((value * 1000) / 512) + 2500; > +} > + > +static inline int input_volt_to_mV(int value) > +{ > + return (value * 2500) / 1023; > +} > + > +static int da9052_enable_vddout_channel(struct da9052 *da9052) > +{ > + int ret; > + > + ret = da9052_reg_read(da9052, DA9052_ADC_CONT_REG); > + if (ret < 0) > + return ret; > + > + ret |= DA9052_ADCCONT_AUTOVDDEN; > + > + return da9052_reg_write(da9052, DA9052_ADC_CONT_REG, ret); > +} > + > +static int da9052_disable_vddout_channel(struct da9052 *da9052) > +{ > + int ret; > + > + ret = da9052_reg_read(da9052, DA9052_ADC_CONT_REG); > + if (ret < 0) > + return ret; > + > + ret &= ~DA9052_ADCCONT_AUTOVDDEN; > + > + return da9052_reg_write(da9052, DA9052_ADC_CONT_REG, ret); > +} > + > +static ssize_t da9052_read_vddout(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct da9052_hwmon *hwmon = dev_get_drvdata(dev); > + int ret, vdd; > + > + mutex_lock(&hwmon->hwmon_lock); > + > + ret = da9052_enable_vddout_channel(hwmon->da9052); > + if (ret < 0) > + goto hwmon_err; > + > + vdd = da9052_reg_read(hwmon->da9052, DA9052_VDD_RES_REG); > + if (vdd < 0) { > + ret = vdd; > + goto hwmon_err_release; > + } > + > + ret = da9052_disable_vddout_channel(hwmon->da9052); > + if (ret < 0) > + goto hwmon_err; > + > + mutex_unlock(&hwmon->hwmon_lock); > + vdd = volt_reg_to_mV(vdd); > + return sprintf(buf, "%d\n", vdd); > + return sprintf(buf, "%d\n", volt_reg_to_mV(vdd)); would be a bit simpler. Same for similar code below. > +hwmon_err_release: > + da9052_disable_vddout_channel(hwmon->da9052); > +hwmon_err: > + mutex_unlock(&hwmon->hwmon_lock); > + return ret; > +} > + > +static ssize_t da9052_read_ich(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct da9052_hwmon *hwmon = dev_get_drvdata(dev); > + int ret; > + > + ret = da9052_reg_read(hwmon->da9052, DA9052_ICHG_AV_REG); > + if (ret < 0) > + return ret; > + > + /* Equivalent to 3.9mA/bit in register ICHG_AV */ > + return sprintf(buf, "%d\n", DIV_ROUND_CLOSEST(ret * 39, 10)); > +} > + > +static ssize_t da9052_read_tbat(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct da9052_hwmon *hwmon = dev_get_drvdata(dev); > + > + return sprintf(buf, "%d\n", da9052_adc_read_temp(hwmon->da9052)); > +} > + > +static ssize_t da9052_read_vbat(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct da9052_hwmon *hwmon = dev_get_drvdata(dev); > + int ret; > + > + ret = da9052_adc_manual_read(hwmon->da9052, DA9052_ADC_VBAT); > + if (ret < 0) > + return ret; > + > + ret = volt_reg_to_mV(ret); > + return sprintf(buf, "%d\n", ret); > +} > + > +static ssize_t da9052_read_misc_channel(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + struct da9052_hwmon *hwmon = dev_get_drvdata(dev); > + int channel = to_sensor_dev_attr(devattr)->index; > + int ret; > + > + ret = da9052_adc_manual_read(hwmon->da9052, channel); > + if (ret < 0) > + return ret; > + if (ret) > + ret = input_volt_to_mV(ret); That is just an unnecessary optimization. input_volt_to_mV() returns 0 if 0 is passed to it. > + > + return sprintf(buf, "%d\n", ret); > +} > + > +static ssize_t da9052_read_tjunc(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct da9052_hwmon *hwmon = dev_get_drvdata(dev); > + int tjunc, toffset, result; > + > + tjunc = da9052_reg_read(hwmon->da9052, DA9052_TJUNC_RES_REG); > + if (tjunc < 0) > + return tjunc; > + > + toffset = da9052_reg_read(hwmon->da9052, DA9052_T_OFFSET_REG); > + if (toffset < 0) > + return toffset; > + > + if ((tjunc - toffset) == 0) Unnecessary ( ) > + result = 0; > + else > + /* Degrees celsius = 1.708 * (TJUNC_RES - T_OFFSET) - 108.8 */ > + result = 1708 * (tjunc - toffset) - 108800; > + Hmm. tjunc = 1, toffset = 2: return 1708 * (-1) - 108800 = -108800 - 1708 = -110508 tjunc = 2, toffset = 2: return 0 tjunc = 3, toffset = 2: return 1780 * 1 - 108800 = -108800 + 1708 = -107092 I am not saying this is wrong, not having access to the data sheet, but it is a bit odd. Why is the 0-difference case handled differently, and why does it return 0 and not, say, -108800 ? If tjunc - toffset < 0 is invalid (no idea if it is, of course), it might make sense to detect that condition. > + return sprintf(buf, "%d\n", result); > +} > + > +static ssize_t da9052_read_vbbat(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct da9052_hwmon *hwmon = dev_get_drvdata(dev); > + int ret; > + > + ret = da9052_adc_manual_read(hwmon->da9052, DA9052_ADC_VBBAT); > + if (ret < 0) > + return ret; > + > + return sprintf(buf, "%d\n", ret); No conversion needed here ? > +} > + > +static ssize_t da9052_hwmon_show_name(struct device *dev, > + struct device_attribute *devattr, > + char *buf) > +{ > + return sprintf(buf, "da9052-hwmon\n"); > +} > + > +static ssize_t show_label(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + return sprintf(buf, "%s\n", > + input_names[to_sensor_dev_attr(devattr)->index]); > +} > + > +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, da9052_read_vddout, NULL, > + DA9052_ADC_VDDOUT); > +static SENSOR_DEVICE_ATTR(in0_label, S_IRUGO, show_label, NULL, > + DA9052_ADC_VDDOUT); > +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, da9052_read_vbat, NULL, > + DA9052_ADC_VBAT); > +static SENSOR_DEVICE_ATTR(in3_label, S_IRUGO, show_label, NULL, > + DA9052_ADC_VBAT); > +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, da9052_read_misc_channel, NULL, > + DA9052_ADC_IN4); > +static SENSOR_DEVICE_ATTR(in4_label, S_IRUGO, show_label, NULL, > + DA9052_ADC_IN4); > +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, da9052_read_misc_channel, NULL, > + DA9052_ADC_IN5); > +static SENSOR_DEVICE_ATTR(in5_label, S_IRUGO, show_label, NULL, > + DA9052_ADC_IN5); > +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, da9052_read_misc_channel, NULL, > + DA9052_ADC_IN6); > +static SENSOR_DEVICE_ATTR(in6_label, S_IRUGO, show_label, NULL, > + DA9052_ADC_IN6); > +static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO, da9052_read_vbbat, NULL, > + DA9052_ADC_VBBAT); > +static SENSOR_DEVICE_ATTR(in9_label, S_IRUGO, show_label, NULL, > + DA9052_ADC_VBBAT); > + > +static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, da9052_read_ich, NULL, > + DA9052_ADC_ICH); > +static SENSOR_DEVICE_ATTR(curr1_label, S_IRUGO, show_label, NULL, > + DA9052_ADC_ICH); > + > +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, da9052_read_tbat, NULL, > + DA9052_ADC_TBAT); > +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_label, NULL, > + DA9052_ADC_TBAT); > +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, da9052_read_tjunc, NULL, > + DA9052_ADC_TJUNC); > +static SENSOR_DEVICE_ATTR(temp2_label, S_IRUGO, show_label, NULL, > + DA9052_ADC_TJUNC); > + Since voltages and current follow the sensor index, it might make sense to do the same with temperatures, ie use temp2 and temp8. > +static DEVICE_ATTR(name, S_IRUGO, da9052_hwmon_show_name, NULL); > + > +static struct attribute *da9052_attr[] = { > + &dev_attr_name.attr, > + &sensor_dev_attr_in0_input.dev_attr.attr, > + &sensor_dev_attr_in0_label.dev_attr.attr, > + &sensor_dev_attr_in3_input.dev_attr.attr, > + &sensor_dev_attr_in3_label.dev_attr.attr, > + &sensor_dev_attr_in4_input.dev_attr.attr, > + &sensor_dev_attr_in4_label.dev_attr.attr, > + &sensor_dev_attr_in5_input.dev_attr.attr, > + &sensor_dev_attr_in5_label.dev_attr.attr, > + &sensor_dev_attr_in6_input.dev_attr.attr, > + &sensor_dev_attr_in6_label.dev_attr.attr, > + &sensor_dev_attr_in9_input.dev_attr.attr, > + &sensor_dev_attr_in9_label.dev_attr.attr, > + &sensor_dev_attr_curr1_input.dev_attr.attr, > + &sensor_dev_attr_curr1_label.dev_attr.attr, > + &sensor_dev_attr_temp1_input.dev_attr.attr, > + &sensor_dev_attr_temp1_label.dev_attr.attr, > + &sensor_dev_attr_temp2_input.dev_attr.attr, > + &sensor_dev_attr_temp2_label.dev_attr.attr, > + NULL > +}; > + > +static const struct attribute_group da9052_attr_group = {.attrs = da9052_attr}; > + > +static int __init da9052_hwmon_probe(struct platform_device *pdev) > +{ > + struct da9052_hwmon *hwmon; > + int ret; > + > + hwmon = devm_kzalloc(&pdev->dev, sizeof(struct da9052_hwmon), > + GFP_KERNEL); > + if (!hwmon) > + return -ENOMEM; > + > + mutex_init(&hwmon->hwmon_lock); > + hwmon->da9052 = dev_get_drvdata(pdev->dev.parent); > + > + platform_set_drvdata(pdev, hwmon); > + > + ret = sysfs_create_group(&pdev->dev.kobj, &da9052_attr_group); > + if (ret) > + goto err_mem; > + > + hwmon->class_device = hwmon_device_register(&pdev->dev); > + if (IS_ERR(hwmon->class_device)) { > + ret = PTR_ERR(hwmon->class_device); > + goto err_sysfs; > + } > + > + return 0; > + > +err_sysfs: > + sysfs_remove_group(&pdev->dev.kobj, &da9052_attr_group); > +err_mem: > + devm_kfree(&pdev->dev, hwmon); > + return ret; > +} > + > +static int __devexit da9052_hwmon_remove(struct platform_device *pdev) > +{ > + struct da9052_hwmon *hwmon = platform_get_drvdata(pdev); > + > + hwmon_device_unregister(hwmon->class_device); > + sysfs_remove_group(&pdev->dev.kobj, &da9052_attr_group); > + devm_kfree(&pdev->dev, hwmon); > + > + return 0; > +} > + > +static struct platform_driver da9052_hwmon_driver = { > + .probe = da9052_hwmon_probe, > + .remove = __devexit_p(da9052_hwmon_remove), > + .driver = { > + .name = "da9052-hwmon", > + .owner = THIS_MODULE, > + }, > +}; > + > +module_platform_driver(da9052_hwmon_driver); > + > +MODULE_AUTHOR("David Dajun Chen "); > +MODULE_DESCRIPTION("DA9052 HWMON driver"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:da9052-hwmon"); > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/