On Fri, Aug 24, 2012 at 03:05:00PM +0100, Krystian Garbaciak wrote:
> DA906x PMIC provides ADC for voltage and temperature monitoring.
>
Hi Krystian,
DA906x seems to be a bad choice for a name. It covers DA906[0-9] and possibly
even DA906[A-Z]. The only chip really in existence seems to be DA9064.
I personally think it is a bad idea to have an 'x' in a driver name. What if
DA9069 shows up at some point and is completely different ? I think the driver
should be named for the first supported chip; reference the others in the code
and documentation.
Worse, I find no information anywhere in your patch set indicating which chips
are actually supported. I don't know how other subsystems handle this, but for
hwmon this is a no-go.
> The driver provides results of following ADC channels:
> - in0 - system voltage (2500 - 5500 mV)
> - in1 - universal voltage channel #1 (0 - 2500 mV)
> - in2 - universal voltage channel #2 (0 - 2500 mV)
> - in3 - universal voltage channel #3 (0 - 2500 mV)
> - in4 - backup battery voltage (0 - 5000 mV)
> - temp1 - PMIC internal junction temperature (-88 - 333 Celcius degrees)
>
> Signed-off-by: Krystian Garbaciak <krystian.garbaciak@xxxxxxxxxxx>
> ---
> drivers/hwmon/Kconfig | 6 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/da906x-hwmon.c | 393 ++++++++++++++++++++++++++++++++++++++++++
Also please provide Documentation/hwmon/da906x-hwmon.
> 3 files changed, 400 insertions(+), 0 deletions(-)
> create mode 100644 drivers/hwmon/da906x-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index b0a2e4c..7abc9a0 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1373,6 +1373,12 @@ config SENSORS_WM8350
> This driver can also be built as a module. If so, the module
> will be called wm8350-hwmon.
>
> +config SENSORS_DA906X
> + tristate "DA906X HWMON device drivers"
> + depends on MFD_DA906X
> + help
> + Support for the HWMON DA906X device driver.
> +
Alphabetical order, please, and describe which chip(s) are supported. And you
don't really support a device driver, the device driver presumably supports a
chip. Since it is tristate, we also expect you to provide the module name if the
driver is built as module.
> config SENSORS_ULTRA45
> tristate "Sun Ultra45 PIC16F747"
> depends on SPARC64
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 7aa9811..ffbe151 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -43,6 +43,7 @@ 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_DA906X) += da906x-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/da906x-hwmon.c b/drivers/hwmon/da906x-hwmon.c
> new file mode 100644
> index 0000000..8ece931
> --- /dev/null
> +++ b/drivers/hwmon/da906x-hwmon.c
> @@ -0,0 +1,393 @@
> +/*
> + * HW Monitor support for Dialog DA906X PMIC series
> + *
> + * Copyright 2012 Dialog Semiconductor Ltd.
> + *
> + * Author: Krystian Garbaciak <krystian.garbaciak@xxxxxxxxxxx>,
> + * Michal Hajduk <michal.hajduk@xxxxxxxxxxx>
> + *
> + * 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/platform_device.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/mfd/da906x/core.h>
> +#include <linux/mfd/da906x/pdata.h>
> +
> +/* ADC resolutions for manual and auto modes */
> +#define DA906X_ADC_RES \
> + (1 << (DA906X_ADC_RES_L_BITS + DA906X_ADC_RES_M_BITS))
> +#define DA906X_ADC_MAX (DA906X_ADC_RES - 1)
> +#define DA906X_ADC_AUTO_RES (1 << DA906X_ADC_RES_M_BITS)
> +#define DA906X_ADC_AUTO_MAX (DA906X_ADC_AUTO_RES - 1)
> +
> +/* Define interpolation table to calculate ADC values */
> +struct i_table {
> + int x0;
> + int a;
> + int b;
> +};
> +#define ILINE(x1, x2, y1, y2) { \
> + .x0 = (x1), \
> + .a = ((y2) - (y1)) * DA906X_ADC_RES / ((x2) - (x1)), \
> + .b = (y1) - ((y2) - (y1)) * (x1) / ((x2) - (x1)), \
> + }
> +
> +struct channel_info {
> + const char *name;
> + const struct i_table *tbl;
> + int tbl_max;
> + u16 reg_auto_en;
> +};
> +
> +enum da906x_adc {
> + DA906X_VSYS,
> + DA906X_ADCIN1,
> + DA906X_ADCIN2,
> + DA906X_ADCIN3,
> + DA906X_TJUNC,
> + DA906X_VBBAT,
> +
> + DA906X_CHAN_NUM
> +};
> +
> +static const struct i_table vsys_tbl[] = {
> + ILINE(0, DA906X_ADC_MAX, 2500, 5500)
> +};
> +
> +static const struct i_table adcin_tbl[] = {
> + ILINE(0, DA906X_ADC_MAX, 0, 2500)
> +};
> +
> +static const struct i_table tjunc_tbl[] = {
> + ILINE(0, DA906X_ADC_MAX, 333, -86)
> +};
> +
> +static const struct i_table vbbat_tbl[] = {
> + ILINE(0, DA906X_ADC_MAX, 0, 5000)
> +};
Since the first parameter to ILINE is always 0, it is not needed. This means
that x0 in itable is also always 0 and thus not needed.
> +
> +static const struct channel_info da906x_channels[] = {
> + [DA906X_VSYS] = { "VSYS",
> + vsys_tbl, ARRAY_SIZE(vsys_tbl) - 1,
> + DA906X_REG_VSYS_RES },
> + [DA906X_ADCIN1] = { "ADCIN1",
> + adcin_tbl, ARRAY_SIZE(adcin_tbl) - 1,
> + DA906X_REG_ADCIN1_RES },
> + [DA906X_ADCIN2] = { "ADCIN2",
> + adcin_tbl, ARRAY_SIZE(adcin_tbl) - 1,
> + DA906X_REG_ADCIN2_RES },
> + [DA906X_ADCIN3] = { "ADCIN3",
> + adcin_tbl, ARRAY_SIZE(adcin_tbl) - 1,
> + DA906X_REG_ADCIN3_RES },
> + [DA906X_TJUNC] = { "TJUNC",
> + tjunc_tbl, ARRAY_SIZE(tjunc_tbl) - 1 },
> + [DA906X_VBBAT] = { "VBBAT",
> + vbbat_tbl, ARRAY_SIZE(vbbat_tbl) - 1}
s/1}/1 }/
> +};
You lost me here a bit (I am missing something ?).
Seems to be each table has exactly one entry. Since the table size is 1,
ARRAY_SIZE(vbbat_tbl) - 1 is 0, and ...
> +#define DA906X_ADC_AUTO_MODE_SUPPORT_MASK (DA906X_ADC_AUTO_VSYS_EN | \
> + DA906X_ADC_AUTO_AD1_EN | \
> + DA906X_ADC_AUTO_AD2_EN | \
> + DA906X_ADC_AUTO_AD3_EN)
> +
> +struct da906x_hwmon {
> + struct da906x *da906x;
> + struct device *class_dev;
> + struct completion man_adc_rdy; /* Manual read completion flag */
> + struct mutex hwmon_mutex; /* Queue concurent manual reads */
> + int irq;
> + u8 adc_auto_en; /* Bitmask of channels with auto mode enabled */
> + s8 tjunc_offset; /* Calibration offset for junction temperature */
> +};
> +
> +int da906x_adc_convert(int channel, int x)
> +{
> + const struct channel_info *info = &da906x_channels[channel];
> + int i, ret;
> +
> + for (i = info->tbl_max; i > 0; i--)
> + if (info->tbl[i].x0 <= x)
> + break;
... this loop never does anything because info->tbl_max is always 0.
Besides, even if the loop was used, x0 is always 0 anyway, so you might well
compare against 0 instead which doesn't make sense.
So what is the point for making the code that complex ? For me it just adds a
lot of confusion.
> +
> + ret = info->tbl[i].a * x;
> + if (ret >= 0)
> + ret += DA906X_ADC_RES / 2;
> + else
> + ret -= DA906X_ADC_RES / 2;
> + ret = ret / DA906X_ADC_RES + info->tbl[i].b;
ret = DIV_ROUND_CLOSEST(ret, DA906X_ADC_RES) + info->tbl[i].b;
has the same effect as the 5 lines above and is simpler.
> + return ret;
> +}
> +
> +static int da906x_adc_manual_read(struct da906x_hwmon *hwmon, int channel)
> +{
> + int ret;
> + u8 data[2];
> +
> + mutex_lock(&hwmon->hwmon_mutex);
> +
> + init_completion(&hwmon->man_adc_rdy);
> +
> + /* Start measurment on selected channel */
> + data[0] = (channel << DA906X_ADC_MUX_SHIFT) & DA906X_ADC_MUX_MASK;
> + data[0] |= DA906X_ADC_MAN;
> + ret = da906x_reg_update(hwmon->da906x, DA906X_REG_ADC_MAN,
> + DA906X_ADC_MUX_MASK | DA906X_ADC_MAN, data[0]);
> + if (ret < 0)
> + goto out;
> +
> + /* Wait for interrupt from ADC */
> + ret = wait_for_completion_timeout(&hwmon->man_adc_rdy,
> + msecs_to_jiffies(1000));
> + if (ret == 0) {
> + ret = -EBUSY;
Should this be -ETIMEDOUT ?
> + goto out;
> + }
> +
> + /* Get results */
> + ret = da906x_block_read(hwmon->da906x, DA906X_REG_ADC_RES_L, 2, data);
> + if (ret < 0)
> + goto out;
> + ret = (data[0] & DA906X_ADC_RES_L_MASK) >> DA906X_ADC_RES_L_SHIFT;
> + ret |= data[1] << DA906X_ADC_RES_L_BITS;
> +out:
> + mutex_unlock(&hwmon->hwmon_mutex);
> + return ret;
> +}
> +
> +static int da906x_adc_auto_read(struct da906x *da906x, int channel)
> +{
> + const struct channel_info *info = &da906x_channels[channel];
> +
> + return da906x_reg_read(da906x, info->reg_auto_en);
> +}
> +
> +static irqreturn_t da906x_hwmon_irq_handler(int irq, void *irq_data)
> +{
> + struct da906x_hwmon *hwmon = irq_data;
> +
> + complete(&hwmon->man_adc_rdy);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static ssize_t da906x_adc_read(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct da906x_hwmon *hwmon = dev_get_drvdata(dev);
> + int channel = to_sensor_dev_attr(devattr)->index;
> + int val;
> +
> + if (hwmon->adc_auto_en & (1 << channel)) {
> + val = da906x_adc_auto_read(hwmon->da906x, channel);
> + if (val < 0)
> + return val;
> +
> + val *= DA906X_ADC_RES / DA906X_ADC_AUTO_RES;
> + val = da906x_adc_convert(channel, val);
> + } else {
> + val = da906x_adc_manual_read(hwmon, channel);
> + if (val < 0)
> + return val;
> +
> + if (channel == DA906X_TJUNC)
> + val += hwmon->tjunc_offset;
> + val = da906x_adc_convert(channel, val);
This call is really the same for both the if and else path. Might as well put it
after the conditional code.
> + }
> +
> + return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t da906x_show_name(struct device *dev,
> + struct device_attribute *attr, char *buf)
Please align all multi-line function parameters with the opening (.
> +{
> + return sprintf(buf, DA906X_DRVNAME_HWMON "\n");
> +}
> +
> +static ssize_t da906x_show_label(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + const struct channel_info *info;
> +
> + info = &da906x_channels[to_sensor_dev_attr(devattr)->index];
> + return sprintf(buf, "%s\n", info->name);
> +}
> +
> +/* Vsys voltage */
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO,
> + da906x_adc_read, NULL, DA906X_VSYS);
> +static SENSOR_DEVICE_ATTR(in0_label, S_IRUGO,
> + da906x_show_label, NULL, DA906X_VSYS);
> +
> +/* Universal ADC channel #1 */
> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO,
> + da906x_adc_read, NULL, DA906X_ADCIN1);
> +static SENSOR_DEVICE_ATTR(in1_label, S_IRUGO,
> + da906x_show_label, NULL, DA906X_ADCIN1);
> +
> +/* Universal ADC channel #2 */
> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO,
> + da906x_adc_read, NULL,
> + DA906X_ADCIN2);
> +static SENSOR_DEVICE_ATTR(in2_label, S_IRUGO,
> + da906x_show_label, NULL, DA906X_ADCIN2);
> +
> +/* Universal ADC channel #3 */
> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO,
> + da906x_adc_read, NULL, DA906X_ADCIN3);
> +static SENSOR_DEVICE_ATTR(in3_label, S_IRUGO,
> + da906x_show_label, NULL, DA906X_ADCIN3);
> +
> +/* Backup battery voltage */
> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO,
> + da906x_adc_read, NULL, DA906X_VBBAT);
> +static SENSOR_DEVICE_ATTR(in4_label, S_IRUGO,
> + da906x_show_label, NULL, DA906X_VBBAT);
> +
> +/* Junction temperature */
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
> + da906x_adc_read, NULL, DA906X_TJUNC);
> +
> +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO,
> + da906x_show_label, NULL, DA906X_TJUNC);
> +
> +/* Device name */
> +static DEVICE_ATTR(name, S_IRUGO, da906x_show_name, NULL);
> +
> +static struct attribute *da906x_attributes[] = {
> + &dev_attr_name.attr,
> + &sensor_dev_attr_in0_input.dev_attr.attr,
> + &sensor_dev_attr_in0_label.dev_attr.attr,
> + &sensor_dev_attr_in1_input.dev_attr.attr,
> + &sensor_dev_attr_in1_label.dev_attr.attr,
> + &sensor_dev_attr_in2_input.dev_attr.attr,
> + &sensor_dev_attr_in2_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_temp1_input.dev_attr.attr,
> + &sensor_dev_attr_temp1_label.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group da906x_attr_group = {
> + .attrs = da906x_attributes,
> +};
> +
> +static int __devinit da906x_hwmon_probe(struct platform_device *pdev)
> +{
> + struct da906x *da906x = dev_get_drvdata(pdev->dev.parent);
> + struct da906x_pdata *da906x_pdata = dev_get_platdata(da906x->dev);
> + struct da906x_hwmon *hwmon;
> + int ret;
> +
> + hwmon = devm_kzalloc(&pdev->dev, sizeof(struct da906x_hwmon),
> + GFP_KERNEL);
> + if (!hwmon)
> + return -ENOMEM;
> +
> + mutex_init(&hwmon->hwmon_mutex);
> + init_completion(&hwmon->man_adc_rdy);
> + hwmon->da906x = da906x;
> +
> + ret = da906x_reg_read(da906x, DA906X_REG_ADC_CONT);
> + if (ret < 0)
> + return ret;
> + hwmon->adc_auto_en = ret & DA906X_ADC_AUTO_MODE_SUPPORT_MASK;
> +
> + if (da906x_pdata->flags & DA906X_FLG_FORCE_IN0_MANUAL_MODE)
You should check if da906x_pdata is NULL before using it.
> + hwmon->adc_auto_en &= ~DA906X_ADC_AUTO_VSYS_EN;
> + else if (da906x_pdata->flags & DA906X_FLG_FORCE_IN0_AUTO_MODE)
> + hwmon->adc_auto_en |= DA906X_ADC_AUTO_VSYS_EN;
> +
> + if (da906x_pdata->flags & DA906X_FLG_FORCE_IN1_MANUAL_MODE)
> + hwmon->adc_auto_en &= ~DA906X_ADC_AUTO_AD1_EN;
> + else if (da906x_pdata->flags & DA906X_FLG_FORCE_IN1_AUTO_MODE)
> + hwmon->adc_auto_en |= DA906X_ADC_AUTO_AD1_EN;
> +
> + if (da906x_pdata->flags & DA906X_FLG_FORCE_IN2_MANUAL_MODE)
> + hwmon->adc_auto_en &= ~DA906X_ADC_AUTO_AD2_EN;
> + else if (da906x_pdata->flags & DA906X_FLG_FORCE_IN2_AUTO_MODE)
> + hwmon->adc_auto_en |= DA906X_ADC_AUTO_AD2_EN;
> +
> + if (da906x_pdata->flags & DA906X_FLG_FORCE_IN3_MANUAL_MODE)
> + hwmon->adc_auto_en &= ~DA906X_ADC_AUTO_AD3_EN;
> + else if (da906x_pdata->flags & DA906X_FLG_FORCE_IN3_AUTO_MODE)
> + hwmon->adc_auto_en |= DA906X_ADC_AUTO_AD3_EN;
> +
> + ret = da906x_reg_update(da906x, DA906X_REG_ADC_CONT,
> + DA906X_ADC_AUTO_MODE_SUPPORT_MASK,
> + hwmon->adc_auto_en);
> + if (ret < 0)
> + return ret;
> +
> + hwmon->class_dev = hwmon_device_register(&pdev->dev);
> + if (IS_ERR(hwmon->class_dev))
> + return PTR_ERR(hwmon->class_dev);
> +
hwmon registration has to be the last call, after sysfs attributes exist.
sysfs entries are expected to exist when the hwmon device is registered.
> + hwmon->irq = platform_get_irq_byname(pdev, DA906X_DRVNAME_HWMON);
This function can return an error (-ENXIO).
> + ret = devm_request_threaded_irq(&pdev->dev, hwmon->irq, NULL,
> + da906x_hwmon_irq_handler,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + "HWMON", hwmon);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to request IRQ.\n");
> + goto err;
> + }
> +
> + platform_set_drvdata(pdev, hwmon);
> +
> + ret = da906x_reg_read(da906x, DA906X_REG_T_OFFSET);
> + if (ret < 0)
> + dev_warn(&pdev->dev, "Could not read temp1 callibration offset.\n");
s/callibration/calibration/
> + else
> + hwmon->tjunc_offset = (s8)ret;
> +
> + ret = sysfs_create_group(&pdev->dev.kobj, &da906x_attr_group);
> + if (ret)
> + goto err;
> +
> + return 0;
> +
> +err:
> + hwmon_device_unregister(hwmon->class_dev);
> + return ret;
> +}
> +
> +static int __devexit da906x_hwmon_remove(struct platform_device *pdev)
> +{
> + struct da906x_hwmon *hwmon = platform_get_drvdata(pdev);
> +
> + hwmon_device_unregister(hwmon->class_dev);
> + sysfs_remove_group(&pdev->dev.kobj, &da906x_attr_group);
> +
> + return 0;
> +}
> +
> +static struct platform_driver da906x_hwmon_driver = {
> + .probe = da906x_hwmon_probe,
> + .remove = __devexit_p(da906x_hwmon_remove),
> + .driver = {
> + .name = DA906X_DRVNAME_HWMON,
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +module_platform_driver(da906x_hwmon_driver);
> +
> +MODULE_DESCRIPTION("DA906X Hardware monitoring");
> +MODULE_AUTHOR("Krystian Garbaciak <krystian.garbaciak@xxxxxxxxxxx>, Michal Hajduk <michal.hajduk@xxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("paltform:" DA906X_DRVNAME_HWMON);
s/paltform/platform/
> --
> 1.7.0.4
>
>
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
