Re: [PATCH v3 1/2] hwmon: (ads7828) driver cleanup

From: Guenter Roeck
Date: Tue Oct 02 2012 - 20:28:20 EST

Next message: Ryan Mallon: "Re: [PATCH 3/7] staging: rts_pstor: reuse kbasename()"
Previous message: Kent Overstreet: "Re: [RFC, PATCH] Extensible AIO interface"
In reply to: Vivien Didelot: "[PATCH v3 2/2] hwmon: (ads7828) add support for ADS7830"
Next in thread: Vivien Didelot: "Re: [PATCH v3 1/2] hwmon: (ads7828) driver cleanup"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Oct 02, 2012 at 06:10:02PM -0400, Vivien Didelot wrote:
> * Remove module parameters, add a ads7828_platform_data;
> * Move driver declaration to avoid adding function prototypes;
> * Remove unused macros;
> * Coding Style fixes.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx>

Hi Vivien,

nice cleanup. Couple of minor comments below.

> ---
> Documentation/hwmon/ads7828 | 31 +++--
> drivers/hwmon/ads7828.c | 216 +++++++++++++++++-----------------
> include/linux/platform_data/ads7828.h | 29 +++++
> 3 files changed, 155 insertions(+), 121 deletions(-)
> create mode 100644 include/linux/platform_data/ads7828.h
>
> diff --git a/Documentation/hwmon/ads7828 b/Documentation/hwmon/ads7828
> index 2bbebe6..b35668c 100644
> --- a/Documentation/hwmon/ads7828
> +++ b/Documentation/hwmon/ads7828
> @@ -5,21 +5,32 @@ Supported chips:
> * Texas Instruments/Burr-Brown ADS7828
> Prefix: 'ads7828'
> Addresses scanned: I2C 0x48, 0x49, 0x4a, 0x4b
> - Datasheet: Publicly available at the Texas Instruments website :
> + Datasheet: Publicly available at the Texas Instruments website:
> http://focus.ti.com/lit/ds/symlink/ads7828.pdf
>
> Authors:
> Steve Hardy <shardy@xxxxxxxxxx>
>
> -Module Parameters
> ------------------
> -
> -* se_input: bool (default Y)
> - Single ended operation - set to N for differential mode
> -* int_vref: bool (default Y)
> - Operate with the internal 2.5V reference - set to N for external reference
> -* vref_mv: int (default 2500)
> - If using an external reference, set this to the reference voltage in mV
> +Platform data
> +-------------
> +
> +The ads7828 driver accepts an optional ads7828_platform_data structure (defined
> +in include/linux/platform_data/ads7828.h). If no structure is provided, the
> +configuration defaults to single ended operation and internal vref (2.5V).
> +
> +The structure fields are:
> +
> +* diff_input: bool
> + Differential operation - set to true for differential mode,
> + false for default single ended mode.
> +* ext_vref: bool
> + External reference - set to true if it operates with an external reference,
> + false for default internal reference.
> +* vref_mv: int
> + Voltage reference - if using an external reference, set this to the reference
> + voltage in mV, otherwise, it will default to the internal value (2500mV).
> + This value will be bounded with limits accepted by the chip, described in the
> + datasheet.
>
> Description
> -----------
> diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
> index bf3fdf4..0a13bf8 100644
> --- a/drivers/hwmon/ads7828.c
> +++ b/drivers/hwmon/ads7828.c
> @@ -6,7 +6,7 @@
> *
> * Written by Steve Hardy <shardy@xxxxxxxxxx>
> *
> - * Datasheet available at: http://focus.ti.com/lit/ds/symlink/ads7828.pdf
> + * For further information, see the Documentation/hwmon/ads7828 file.
> *
> * 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
> @@ -23,63 +23,48 @@
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> -#include <linux/module.h>
> -#include <linux/init.h>
> -#include <linux/slab.h>
> -#include <linux/jiffies.h>
> -#include <linux/i2c.h>
> +#include <linux/err.h>
> #include <linux/hwmon.h>
> #include <linux/hwmon-sysfs.h>
> -#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/platform_data/ads7828.h>
> +#include <linux/slab.h>
>
> /* The ADS7828 registers */
> -#define ADS7828_NCH 8 /* 8 channels of 12-bit A-D supported */
> -#define ADS7828_CMD_SD_SE 0x80 /* Single ended inputs */
> -#define ADS7828_CMD_SD_DIFF 0x00 /* Differential inputs */
> -#define ADS7828_CMD_PD0 0x0 /* Power Down between A-D conversions */
> -#define ADS7828_CMD_PD1 0x04 /* Internal ref OFF && A-D ON */
> -#define ADS7828_CMD_PD2 0x08 /* Internal ref ON && A-D OFF */
> -#define ADS7828_CMD_PD3 0x0C /* Internal ref ON && A-D ON */
> -#define ADS7828_INT_VREF_MV 2500 /* Internal vref is 2.5V, 2500mV */
> +#define ADS7828_NCH 8 /* 8 channels supported */
> +#define ADS7828_CMD_SD_SE 0x80 /* Single ended inputs */
> +#define ADS7828_CMD_PD1 0x04 /* Internal vref OFF && A/D ON */
> +#define ADS7828_CMD_PD3 0x0C /* Internal vref ON && A/D ON */
> +#define ADS7828_INT_VREF_MV 2500 /* Internal vref is 2.5V, 2500mV */
> +#define ADS7828_EXT_VREF_MV_MIN 50 /* External vref min value 0.05V */
> +#define ADS7828_EXT_VREF_MV_MAX 5250 /* External vref max value 5.25V */
>
> /* Addresses to scan */
> static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
> I2C_CLIENT_END };
>
> -/* Module parameters */
> -static bool se_input = 1; /* Default is SE, 0 == diff */
> -static bool int_vref = 1; /* Default is internal ref ON */
> -static int vref_mv = ADS7828_INT_VREF_MV; /* set if vref != 2.5V */
> -module_param(se_input, bool, S_IRUGO);
> -module_param(int_vref, bool, S_IRUGO);
> -module_param(vref_mv, int, S_IRUGO);
> -
> -/* Global Variables */
> -static u8 ads7828_cmd_byte; /* cmd byte without channel bits */
> -static unsigned int ads7828_lsb_resol; /* resolution of the ADC sample lsb */
> -
> -/* Each client has this additional data */
> +/* Client specific data */
> struct ads7828_data {
> struct device *hwmon_dev;
> - struct mutex update_lock; /* mutex protect updates */
> - char valid; /* !=0 if following fields are valid */
> - unsigned long last_updated; /* In jiffies */
> - u16 adc_input[ADS7828_NCH]; /* ADS7828_NCH 12-bit samples */
> + struct mutex update_lock; /* Mutex protecting updates */
> + unsigned long last_updated; /* Last updated time (in jiffies) */
> + u16 adc_input[ADS7828_NCH]; /* ADS7828_NCH samples */
> + bool valid; /* Validity flag */
> + bool diff_input; /* Differential input */
> + bool ext_vref; /* External voltage reference */
> + unsigned int vref_mv; /* voltage reference value */
> + u8 cmd_byte; /* Command byte without channel bits */
> + unsigned int lsb_resol; /* Resolution of the ADC sample LSB */
> };
>
> -/* Function declaration - necessary due to function dependencies */
> -static int ads7828_detect(struct i2c_client *client,
> - struct i2c_board_info *info);
> -static int ads7828_probe(struct i2c_client *client,
> - const struct i2c_device_id *id);
> -
> -static inline u8 channel_cmd_byte(int ch)
> +/* Command byte C2,C1,C0 - see datasheet */
> +static inline u8 ads7828_cmd_byte(u8 cmd, int ch)
> {
> - /* cmd byte C2,C1,C0 - see datasheet */
> - u8 cmd = (((ch>>1) | (ch&0x01)<<2)<<4);
> - cmd |= ads7828_cmd_byte;
> - return cmd;
> + return cmd | (((ch >> 1) | (ch & 0x01) << 2) << 4);
> }
>
> /* Update data for the device (all 8 channels) */
> @@ -96,12 +81,12 @@ static struct ads7828_data *ads7828_update_device(struct device *dev)
> dev_dbg(&client->dev, "Starting ads7828 update\n");
>
> for (ch = 0; ch < ADS7828_NCH; ch++) {
> - u8 cmd = channel_cmd_byte(ch);
> + u8 cmd = ads7828_cmd_byte(data->cmd_byte, ch);
> data->adc_input[ch] =
> i2c_smbus_read_word_swapped(client, cmd);
> }
> data->last_updated = jiffies;
> - data->valid = 1;
> + data->valid = true;
> }
>
> mutex_unlock(&data->update_lock);
> @@ -110,28 +95,25 @@ static struct ads7828_data *ads7828_update_device(struct device *dev)
> }
>
> /* sysfs callback function */
> -static ssize_t show_in(struct device *dev, struct device_attribute *da,
> - char *buf)
> +static ssize_t ads7828_show_in(struct device *dev, struct device_attribute *da,
> + char *buf)
> {
> struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> struct ads7828_data *data = ads7828_update_device(dev);
> - /* Print value (in mV as specified in sysfs-interface documentation) */
> - return sprintf(buf, "%d\n", (data->adc_input[attr->index] *
> - ads7828_lsb_resol)/1000);
> -}
> + unsigned int value = DIV_ROUND_CLOSEST(data->adc_input[attr->index] *
> + data->lsb_resol, 1000);
>
> -#define in_reg(offset)\
> -static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in,\
> - NULL, offset)
> + return sprintf(buf, "%d\n", value);
> +}
>
> -in_reg(0);
> -in_reg(1);
> -in_reg(2);
> -in_reg(3);
> -in_reg(4);
> -in_reg(5);
> -in_reg(6);
> -in_reg(7);
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ads7828_show_in, NULL, 0);
> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ads7828_show_in, NULL, 1);
> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, ads7828_show_in, NULL, 2);
> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, ads7828_show_in, NULL, 3);
> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, ads7828_show_in, NULL, 4);
> +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, ads7828_show_in, NULL, 5);
> +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, ads7828_show_in, NULL, 6);
> +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, ads7828_show_in, NULL, 7);
>
> static struct attribute *ads7828_attributes[] = {
> &sensor_dev_attr_in0_input.dev_attr.attr,
> @@ -152,36 +134,20 @@ static const struct attribute_group ads7828_group = {
> static int ads7828_remove(struct i2c_client *client)
> {
> struct ads7828_data *data = i2c_get_clientdata(client);
> +
> hwmon_device_unregister(data->hwmon_dev);
> sysfs_remove_group(&client->dev.kobj, &ads7828_group);
> - kfree(i2c_get_clientdata(client));
> + i2c_set_clientdata(client, NULL);

i2c_set_clientdata(client, NULL) is not necessary. The framework does that for you.

> +
> return 0;
> }
>
> -static const struct i2c_device_id ads7828_id[] = {
> - { "ads7828", 0 },
> - { }
> -};
> -MODULE_DEVICE_TABLE(i2c, ads7828_id);
> -
> -/* This is the driver that will be inserted */
> -static struct i2c_driver ads7828_driver = {
> - .class = I2C_CLASS_HWMON,
> - .driver = {
> - .name = "ads7828",
> - },
> - .probe = ads7828_probe,
> - .remove = ads7828_remove,
> - .id_table = ads7828_id,
> - .detect = ads7828_detect,
> - .address_list = normal_i2c,
> -};
> -
> /* Return 0 if detection is successful, -ENODEV otherwise */
> static int ads7828_detect(struct i2c_client *client,
> struct i2c_board_info *info)
> {
> struct i2c_adapter *adapter = client->adapter;
> + u8 default_cmd_byte = ADS7828_CMD_SD_SE | ADS7828_CMD_PD3;
> int ch;
>
> /* Check we have a valid client */
> @@ -196,9 +162,12 @@ static int ads7828_detect(struct i2c_client *client,
> * - Check the top 4 bits of each result are not set (12 data bits)
> */
> for (ch = 0; ch < ADS7828_NCH; ch++) {
> - u16 in_data;
> - u8 cmd = channel_cmd_byte(ch);
> - in_data = i2c_smbus_read_word_swapped(client, cmd);
> + u8 cmd = ads7828_cmd_byte(default_cmd_byte, ch);
> + u16 in_data = i2c_smbus_read_word_swapped(client, cmd);
> +
> + if (in_data < 0)
> + return -ENODEV;
> +
> if (in_data & 0xF000) {
> pr_debug("%s : Doesn't look like an ads7828 device\n",
> __func__);
> @@ -214,61 +183,86 @@ static int ads7828_detect(struct i2c_client *client,
> static int ads7828_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> - struct ads7828_data *data;
> int err;
> -
> - data = kzalloc(sizeof(struct ads7828_data), GFP_KERNEL);
> - if (!data) {
> - err = -ENOMEM;
> - goto exit;
> + struct ads7828_data *data;
> + struct ads7828_platform_data *pdata = client->dev.platform_data;
> +
> + data = devm_kzalloc(&client->dev, sizeof(struct ads7828_data),
> + GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
The above change (use of devm_kzalloc) is in the latest upstream code already.
Please reparent.

> + if (pdata) {
> + data->diff_input = pdata->diff_input;
> + data->ext_vref = pdata->ext_vref;
> + if (data->ext_vref)
> + data->vref_mv = pdata->vref_mv;
> }
>
> + /* Bound Vref with min/max values if it was provided */
> + if (data->vref_mv)
> + data->vref_mv = SENSORS_LIMIT(data->vref_mv,
> + ADS7828_EXT_VREF_MV_MIN,
> + ADS7828_EXT_VREF_MV_MAX);
> + else
> + data->vref_mv = ADS7828_INT_VREF_MV;
> +
> + data->lsb_resol = DIV_ROUND_CLOSEST(data->vref_mv * 1000, 4096);
> +
> + data->cmd_byte = data->ext_vref ? ADS7828_CMD_PD1 : ADS7828_CMD_PD3;
> + if (!data->diff_input)
> + data->cmd_byte |= ADS7828_CMD_SD_SE;
> +
> i2c_set_clientdata(client, data);
> mutex_init(&data->update_lock);
>
> - /* Register sysfs hooks */
> err = sysfs_create_group(&client->dev.kobj, &ads7828_group);
> if (err)
> - goto exit_free;
> + return err;
>
> data->hwmon_dev = hwmon_device_register(&client->dev);
> if (IS_ERR(data->hwmon_dev)) {
> err = PTR_ERR(data->hwmon_dev);
> - goto exit_remove;
> + goto error;
> }
>
> return 0;
>
> -exit_remove:
> +error:
> sysfs_remove_group(&client->dev.kobj, &ads7828_group);
> -exit_free:
> - kfree(data);
> -exit:
> return err;
> }
>
> -static int __init sensors_ads7828_init(void)
> -{
> - /* Initialize the command byte according to module parameters */
> - ads7828_cmd_byte = se_input ?
> - ADS7828_CMD_SD_SE : ADS7828_CMD_SD_DIFF;
> - ads7828_cmd_byte |= int_vref ?
> - ADS7828_CMD_PD3 : ADS7828_CMD_PD1;
> +static const struct i2c_device_id ads7828_ids[] = {
> + { "ads7828", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, ads7828_ids);
>
> - /* Calculate the LSB resolution */
> - ads7828_lsb_resol = (vref_mv*1000)/4096;
> +static struct i2c_driver ads7828_driver = {
> + .class = I2C_CLASS_HWMON,
> + .driver = {
> + .name = "ads7828",
> + },
> + .address_list = normal_i2c,
> + .detect = ads7828_detect,
> + .probe = ads7828_probe,
> + .remove = ads7828_remove,
> + .id_table = ads7828_ids,
> +};
>
> +static int __init sensors_ads7828_init(void)
> +{
> return i2c_add_driver(&ads7828_driver);
> }
> +module_init(sensors_ads7828_init);
>
> static void __exit sensors_ads7828_exit(void)
> {
> i2c_del_driver(&ads7828_driver);
> }
> +module_exit(sensors_ads7828_exit);
>
With the cleanup, you can now use the module_i2c_driver macro.

> MODULE_AUTHOR("Steve Hardy <shardy@xxxxxxxxxx>");
> -MODULE_DESCRIPTION("ADS7828 driver");
> +MODULE_DESCRIPTION("Driver for TI ADS7828 A/D converter");
> MODULE_LICENSE("GPL");
> -
> -module_init(sensors_ads7828_init);
> -module_exit(sensors_ads7828_exit);
> diff --git a/include/linux/platform_data/ads7828.h b/include/linux/platform_data/ads7828.h
> new file mode 100644
> index 0000000..3245f45
> --- /dev/null
> +++ b/include/linux/platform_data/ads7828.h
> @@ -0,0 +1,29 @@
> +/*
> + * TI ADS7828 A/D Converter platform data definition
> + *
> + * Copyright (c) 2012 Savoir-faire Linux Inc.
> + * Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx>
> + *
> + * For further information, see the Documentation/hwmon/ads7828 file.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _PDATA_ADS7828_H
> +#define _PDATA_ADS7828_H
> +
> +/**
> + * struct ads7828_platform_data - optional ADS7828 connectivity info
> + * @diff_input: Differential input mode.
> + * @ext_vref: Use an external voltage reference.
> + * @vref_mv: Voltage reference value, if external.
> + */
> +struct ads7828_platform_data {
> + bool diff_input;
> + bool ext_vref;
> + unsigned int vref_mv;
> +};
> +
> +#endif /* _PDATA_ADS7828_H */
> --
> 1.7.11.4
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Ryan Mallon: "Re: [PATCH 3/7] staging: rts_pstor: reuse kbasename()"
Previous message: Kent Overstreet: "Re: [RFC, PATCH] Extensible AIO interface"
In reply to: Vivien Didelot: "[PATCH v3 2/2] hwmon: (ads7828) add support for ADS7830"
Next in thread: Vivien Didelot: "Re: [PATCH v3 1/2] hwmon: (ads7828) driver cleanup"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]