| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <562A9A0A.5040503@baylibre.com>
Date: Fri, 23 Oct 2015 22:35:22 +0200
From: Marc Titinger <mtitinger@...libre.com>
To: Guenter Roeck <linux@...ck-us.net>, jdelvare@...e.com
Cc: lm-sensors@...sensors.org, linux-kernel@...r.kernel.org,
mturquette@...libre.com, bcousson@...libre.com
Subject: Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth.
Hi Guenter
thanks for the review, answers bellow.
Marc.
Le 23/10/2015 18:52, Guenter Roeck a écrit :
> On 10/23/2015 09:13 AM, Marc Titinger wrote:
>> With the current implementation, the driver will prevent a readout at a
>> pace faster than the default conversion time (2ms) times the averaging
>> setting, min AVG being 1:1.
>>
>> Any sysfs "show" read access from the client app faster than 500 Hz
>> will be
>
> 500 Hz ?
The current code checks for the elapsed time since the last read. If
this time is less than the default Conversion Time (CT = 2.2ms) times
the averaging setting, then the driver will not actually access the
registers.
Hence the user can at best read fresh values every 2.2 x 1 ms, roughly
500Hz.
Note that since the driver does bulk-read of all 8 registers (on
ina226), it takes about 0.8 x 8 = 6.4 ms to update all/any values on
BeagleBoneBlack, hence a client app cannot read fresh values faster than
160Hz per seconds.
This is the problem that motivates this rework.
>
>
>> 'cached' by the driver, but actually since do_update reads all 8
>> registers,
>> the best achievable measurement rate is roughly 8*800 us (for the time
>> spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.
>>
>> Registers are now accessed individually through the regmap facility.
>> For four measurements the readout rate is now around
>> (1/(4*800us) = 312 Hz.
>
> I'll need some time to go through the patch in detail Some quick
> comments below.
>
ok
>>
>> Signed-off-by: Marc Titinger <mtitinger@...libre.com>
>> ---
>> drivers/hwmon/ina2xx.c | 359
>> ++++++++++++++++++++++---------------------------
>> 1 file changed, 159 insertions(+), 200 deletions(-)
>>
>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>> index 4d28150..e7c1aaa 100644
>> --- a/drivers/hwmon/ina2xx.c
>> +++ b/drivers/hwmon/ina2xx.c
>> @@ -37,6 +37,7 @@
>> #include <linux/of.h>
>> #include <linux/delay.h>
>> #include <linux/util_macros.h>
>> +#include <linux/regmap.h>
>>
>> #include <linux/platform_data/ina2xx.h>
>>
>> @@ -48,32 +49,25 @@
>> #define INA2XX_CURRENT 0x04 /* readonly */
>> #define INA2XX_CALIBRATION 0x05
>>
>> -/* INA226 register definitions */
>> -#define INA226_MASK_ENABLE 0x06
>> -#define INA226_ALERT_LIMIT 0x07
>> -#define INA226_DIE_ID 0xFF
>> -
>> -/* register count */
>> -#define INA219_REGISTERS 6
>> -#define INA226_REGISTERS 8
>> -
>> -#define INA2XX_MAX_REGISTERS 8
>> +/* CONFIG register fields */
>> +#define INA2XX_AVG_MASK 0x0E00
>> +#define INA2XX_AVG_SHFT 9
>>
>> /* settings - depend on use case */
>> #define INA219_CONFIG_DEFAULT 0x399F /* PGA=8 */
>> #define INA226_CONFIG_DEFAULT 0x4527 /* averages=16 */
>>
>> /* worst case is 68.10 ms (~14.6Hz, ina219) */
>> -#define INA2XX_CONVERSION_RATE 15
>> #define INA2XX_MAX_DELAY 69 /* worst case delay in ms */
>>
>> #define INA2XX_RSHUNT_DEFAULT 10000
>>
>> -/* bit mask for reading the averaging setting in the configuration
>> register */
>> -#define INA226_AVG_RD_MASK 0x0E00
>> -
>> -#define INA226_READ_AVG(reg) (((reg) & INA226_AVG_RD_MASK) >> 9)
>> -#define INA226_SHIFT_AVG(val) ((val) << 9)
>> +/* Currently only handling common register set */
>> +static const struct regmap_config INA2XX_regmap_config = {
>> + .reg_bits = 8,
>> + .val_bits = 16,
>> + .max_register = INA2XX_CALIBRATION,
>> +};
>>
>> /* common attrs, ina226 attrs and NULL */
>> #define INA2XX_MAX_ATTRIBUTE_GROUPS 3
>> @@ -89,7 +83,6 @@ enum ina2xx_ids { ina219, ina226 };
>> struct ina2xx_config {
>> u16 config_default;
>> int calibration_factor;
>> - int registers;
>> int shunt_div;
>> int bus_voltage_shift;
>> int bus_voltage_lsb; /* uV */
>> @@ -99,25 +92,16 @@ struct ina2xx_config {
>> struct ina2xx_data {
>> struct i2c_client *client;
>> const struct ina2xx_config *config;
>> -
>> + struct regmap *regmap;
>> long rshunt;
>> - u16 curr_config;
>> -
>> - struct mutex update_lock;
>> - bool valid;
>> - unsigned long last_updated;
>> - int update_interval; /* in jiffies */
>> -
>> - int kind;
>> + int valid;
>
> bool, please. But then valid was supposed to be used to indicate
> if the cache is valid, and should no longer be needed since there
> is no more cache. I'll have to spend more time to understand if and
> why it is still needed and how it is used.
Ah yes thanks, re-reading the initial code, I figure I 'invented'
another need for this by mistake...
I'm happy to remove it. Good catch!
>
>> const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
>> - u16 regs[INA2XX_MAX_REGISTERS];
>> };
>>
>> static const struct ina2xx_config ina2xx_config[] = {
>> [ina219] = {
>> .config_default = INA219_CONFIG_DEFAULT,
>> .calibration_factor = 40960000,
>> - .registers = INA219_REGISTERS,
>> .shunt_div = 100,
>> .bus_voltage_shift = 3,
>> .bus_voltage_lsb = 4000,
>> @@ -126,7 +110,6 @@ static const struct ina2xx_config ina2xx_config[]
>> = {
>> [ina226] = {
>> .config_default = INA226_CONFIG_DEFAULT,
>> .calibration_factor = 5120000,
>> - .registers = INA226_REGISTERS,
>> .shunt_div = 400,
>> .bus_voltage_shift = 0,
>> .bus_voltage_lsb = 1250,
>> @@ -142,9 +125,9 @@ static const struct ina2xx_config ina2xx_config[]
>> = {
>> */
>> static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512,
>> 1024 };
>>
>> -static int ina226_reg_to_interval(u16 config)
>> +static int ina226_field_to_interval(int field)
>> {
>> - int avg = ina226_avg_tab[INA226_READ_AVG(config)];
>> + int avg = ina226_avg_tab[field];
>>
>> /*
>> * Multiply the total conversion time by the number of averages.
>> @@ -153,24 +136,11 @@ static int ina226_reg_to_interval(u16 config)
>> return DIV_ROUND_CLOSEST(avg * INA226_TOTAL_CONV_TIME_DEFAULT,
>> 1000);
>> }
>>
>> -static u16 ina226_interval_to_reg(int interval, u16 config)
>> -{
>> - int avg, avg_bits;
>> -
>> - avg = DIV_ROUND_CLOSEST(interval * 1000,
>> - INA226_TOTAL_CONV_TIME_DEFAULT);
>> - avg_bits = find_closest(avg, ina226_avg_tab,
>> - ARRAY_SIZE(ina226_avg_tab));
>> -
>> - return (config & ~INA226_AVG_RD_MASK) | INA226_SHIFT_AVG(avg_bits);
>> -}
>> -
>> -static void ina226_set_update_interval(struct ina2xx_data *data)
>> +static int ina226_interval_to_field(int interval)
>> {
>> - int ms;
>> -
>> - ms = ina226_reg_to_interval(data->curr_config);
>> - data->update_interval = msecs_to_jiffies(ms);
>> + int avg = DIV_ROUND_CLOSEST(interval * 1000,
>> + INA226_TOTAL_CONV_TIME_DEFAULT);
>> + return find_closest(avg, ina226_avg_tab,
>> ARRAY_SIZE(ina226_avg_tab));
>> }
>>
>> static int ina2xx_calibrate(struct ina2xx_data *data)
>> @@ -178,8 +148,7 @@ static int ina2xx_calibrate(struct ina2xx_data
>> *data)
>> u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
>> data->rshunt);
>>
>> - return i2c_smbus_write_word_swapped(data->client,
>> - INA2XX_CALIBRATION, val);
>> + return regmap_write(data->regmap, INA2XX_CALIBRATION, val);
>> }
>>
>> /*
>> @@ -187,15 +156,10 @@ static int ina2xx_calibrate(struct ina2xx_data
>> *data)
>> */
>> static int ina2xx_init(struct ina2xx_data *data)
>> {
>> - struct i2c_client *client = data->client;
>> - int ret;
>> -
>> - /* device configuration */
>> - ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
>> - data->curr_config);
>> - if (ret < 0)
>> + int ret = regmap_write(data->regmap, INA2XX_CONFIG,
>> + data->config->config_default);
>> + if (ret)
>> return ret;
>> -
>> /*
>> * Set current LSB to 1mA, shunt is in uOhms
>> * (equation 13 in datasheet).
>> @@ -203,22 +167,22 @@ static int ina2xx_init(struct ina2xx_data *data)
>> return ina2xx_calibrate(data);
>> }
>>
>> -static int ina2xx_do_update(struct device *dev)
>> +static int ina2xx_show_common(struct device *dev, int index, int *val)
>> {
>> struct ina2xx_data *data = dev_get_drvdata(dev);
>> - struct i2c_client *client = data->client;
>> - int i, rv, retry;
>> + int err, retry;
>>
>> - dev_dbg(&client->dev, "Starting ina2xx update\n");
>> + if (unlikely(!val))
>> + return -EINVAL;
>
> Please no unnecessary parameter checks. If val is NULL here,
> something is wrong with the implementation and needs to get fixed.
>
> Besides, the return value is only negative on errors. There is no need
> to pass an extra pointer to the value. Furthermore, regmap_read expects
> a pointer to an unsigned int, not a pointer to an int.
>
> Just return an error or the value returned by regmap. The calling code
> can determine if there is an error by checking if the returned value
> is < 0.
>
>>
>> for (retry = 5; retry; retry--) {
>> - /* Read all registers */
>> - for (i = 0; i < data->config->registers; i++) {
>> - rv = i2c_smbus_read_word_swapped(client, i);
>> - if (rv < 0)
>> - return rv;
>> - data->regs[i] = rv;
>> - }
>> +
>> + /* Check for remaining registers in mask. */
>> + err = regmap_read(data->regmap, index, val);
>> + if (err)
>> + return err;
>> +
>> + dev_dbg(dev, "read %d, val = 0x%04x\n", index, *val);
>>
>> /*
>> * If the current value in the calibration register is 0, the
>> @@ -226,24 +190,24 @@ static int ina2xx_do_update(struct device *dev)
>> * the chip has been reset let's check the calibration
>> * register and reinitialize if needed.
>> */
>> - if (data->regs[INA2XX_CALIBRATION] == 0) {
>> - dev_warn(dev, "chip not calibrated, reinitializing\n");
>> -
>> - rv = ina2xx_init(data);
>> - if (rv < 0)
>> - return rv;
>> + if (!data->valid) {
>> + dev_warn(dev,
>> + "chip needs calibration, reinitializing\n");
>>
>> + err = ina2xx_calibrate(data);
>> + if (err)
>> + return err;
>
> Ah, that is the context of "valid". I don't immediately see why moving
> its scope
> out of this function, and setting it to false in the calling code, is
> beneficial here.
>
> This also changes the dynamic of the driver. Previously it was
> specifically able
> to detect if the chip was power cycled (for whatever reason) and it
> would re-initialize
> itself. Now you use 'valid' and you defer (re-)initialization until
> chip registers
> are read. I don't immediately see if and how this is beneficial.
Agreed, as I said, it "morphed" the use of valid because of my earlier
patch...
>> /*
>> * Let's make sure the power and current registers
>> * have been updated before trying again.
>> */
>> msleep(INA2XX_MAX_DELAY);
>> +
>> + /* data valid once we have a cal value. */
>> + regmap_read(data->regmap, INA2XX_CALIBRATION,
>> + &data->valid);
>> continue;
>> }
>> -
>> - data->last_updated = jiffies;
>> - data->valid = 1;
>> -
>> return 0;
>> }
>>
>> @@ -256,86 +220,89 @@ static int ina2xx_do_update(struct device *dev)
>> return -ENODEV;
>> }
>>
>> -static struct ina2xx_data *ina2xx_update_device(struct device *dev)
>> +static ssize_t ina2xx_show_shunt(struct device *dev,
>> + struct device_attribute *da, char *buf)
>> {
>> struct ina2xx_data *data = dev_get_drvdata(dev);
>> - struct ina2xx_data *ret = data;
>> - unsigned long after;
>> - int rv;
>> + int val, err;
>>
>> - mutex_lock(&data->update_lock);
>> + err = ina2xx_show_common(dev, INA2XX_SHUNT_VOLTAGE, &val);
>> + if (err)
>> + return err;
>>
>> - after = data->last_updated + data->update_interval;
>> - if (time_after(jiffies, after) || !data->valid) {
>> - rv = ina2xx_do_update(dev);
>> - if (rv < 0)
>> - ret = ERR_PTR(rv);
>> - }
>> + val = DIV_ROUND_CLOSEST((s16) val, data->config->shunt_div);
>> + return snprintf(buf, PAGE_SIZE, "%d\n", val);
>> +}
>> +
>> +static ssize_t ina2xx_show_bus(struct device *dev,
>> + struct device_attribute *da, char *buf)
>> +{
>> + struct ina2xx_data *data = dev_get_drvdata(dev);
>> + int val, err;
>> +
>> + err = ina2xx_show_common(dev, INA2XX_BUS_VOLTAGE, &val);
>> + if (err)
>> + return err;
>>
>> - mutex_unlock(&data->update_lock);
>> - return ret;
>> + val = (val >> data->config->bus_voltage_shift)
>> + * data->config->bus_voltage_lsb;
>> + val = DIV_ROUND_CLOSEST(val, 1000);
>> +
>> + return snprintf(buf, PAGE_SIZE, "%d\n", val);
>> }
>>
>> -static int ina2xx_get_value(struct ina2xx_data *data, u8 reg)
>> +static ssize_t ina2xx_show_pow(struct device *dev,
>> + struct device_attribute *da, char *buf)
>> {
>> - int val;
>> -
>> - switch (reg) {
>> - case INA2XX_SHUNT_VOLTAGE:
>> - /* signed register */
>> - val = DIV_ROUND_CLOSEST((s16)data->regs[reg],
>> - data->config->shunt_div);
>> - break;
>> - case INA2XX_BUS_VOLTAGE:
>> - val = (data->regs[reg] >> data->config->bus_voltage_shift)
>> - * data->config->bus_voltage_lsb;
>> - val = DIV_ROUND_CLOSEST(val, 1000);
>> - break;
>> - case INA2XX_POWER:
>> - val = data->regs[reg] * data->config->power_lsb;
>> - break;
>> - case INA2XX_CURRENT:
>> - /* signed register, LSB=1mA (selected), in mA */
>> - val = (s16)data->regs[reg];
>> - break;
>> - case INA2XX_CALIBRATION:
>> - val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
>> - data->regs[reg]);
>> - break;
>> - default:
>> - /* programmer goofed */
>> - WARN_ON_ONCE(1);
>> - val = 0;
>> - break;
>> - }
>> + struct ina2xx_data *data = dev_get_drvdata(dev);
>> + int val, err;
>> +
>> + err = ina2xx_show_common(dev, INA2XX_POWER, &val);
>> + if (err)
>> + return err;
>> +
>> + val *= data->config->power_lsb;
>>
>> - return val;
>> + return snprintf(buf, PAGE_SIZE, "%d\n", val);
>> }
>>
>> -static ssize_t ina2xx_show_value(struct device *dev,
>> - struct device_attribute *da, char *buf)
>> +static ssize_t ina2xx_show_curr(struct device *dev,
>> + struct device_attribute *da, char *buf)
>> {
>> - struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>> - struct ina2xx_data *data = ina2xx_update_device(dev);
>> + int val, err;
>>
>> - if (IS_ERR(data))
>> - return PTR_ERR(data);
>> + err = ina2xx_show_common(dev, INA2XX_CURRENT, &val);
>> + if (err)
>> + return err;
>>
>> - return snprintf(buf, PAGE_SIZE, "%d\n",
>> - ina2xx_get_value(data, attr->index));
>> + /* signed register, LSB=1mA (selected), in mA */
>> + return snprintf(buf, PAGE_SIZE, "%d\n", val);
>> +}
>> +
>> +static ssize_t ina2xx_show_cal(struct device *dev,
>> + struct device_attribute *da, char *buf)
>> +{
>> + struct ina2xx_data *data = dev_get_drvdata(dev);
>> + int val, err;
>> +
>> + err = ina2xx_show_common(dev, INA2XX_CALIBRATION, &val);
>> + if (err)
>> + return err;
>> +
>> + val = DIV_ROUND_CLOSEST(data->config->calibration_factor, val);
>> +
>> + return snprintf(buf, PAGE_SIZE, "%d\n", val);
>> }
>>
>> static ssize_t ina2xx_set_shunt(struct device *dev,
>> struct device_attribute *da,
>> const char *buf, size_t count)
>> {
>> - struct ina2xx_data *data = ina2xx_update_device(dev);
>> + struct ina2xx_data *data = dev_get_drvdata(dev);
>> +
>> unsigned long val;
>> int status;
>>
>> - if (IS_ERR(data))
>> - return PTR_ERR(data);
>> -
>> status = kstrtoul(buf, 10, &val);
>> if (status < 0)
>> return status;
>> @@ -345,12 +312,8 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
>> val > data->config->calibration_factor)
>> return -EINVAL;
>>
>> - mutex_lock(&data->update_lock);
>> data->rshunt = val;
>> - status = ina2xx_calibrate(data);
>> - mutex_unlock(&data->update_lock);
>> - if (status < 0)
>> - return status;
>> + data->valid = 0;
>>
>> return count;
>> }
>> @@ -370,65 +333,58 @@ static ssize_t ina226_set_interval(struct
>> device *dev,
>> if (val > INT_MAX || val == 0)
>> return -EINVAL;
>>
>> - mutex_lock(&data->update_lock);
>> - data->curr_config = ina226_interval_to_reg(val,
>> - data->regs[INA2XX_CONFIG]);
>> - status = i2c_smbus_write_word_swapped(data->client,
>> - INA2XX_CONFIG,
>> - data->curr_config);
>> + val = ina226_interval_to_field(val);
>>
>> - ina226_set_update_interval(data);
>> - /* Make sure the next access re-reads all registers. */
>> - data->valid = 0;
>> - mutex_unlock(&data->update_lock);
>> + status = regmap_update_bits(data->regmap, INA2XX_CONFIG,
>> + INA2XX_AVG_MASK, (val << INA2XX_AVG_SHFT));
>> if (status < 0)
>> return status;
>>
>> + data->valid = 0;
>> +
>
> This now causes re-calibration. Why ?
Mistake.
>> return count;
>> }
>>
>> static ssize_t ina226_show_interval(struct device *dev,
>> struct device_attribute *da, char *buf)
>> {
>> - struct ina2xx_data *data = ina2xx_update_device(dev);
>> + struct ina2xx_data *data = dev_get_drvdata(dev);
>> + int status, val;
>>
>> - if (IS_ERR(data))
>> - return PTR_ERR(data);
>> + status = regmap_read(data->regmap, INA2XX_CONFIG, &val);
>> + if (status)
>> + return status;
>> +
>> + val = (val & INA2XX_AVG_MASK) >> INA2XX_AVG_SHFT;
>> + val = ina226_field_to_interval(val);
>>
>> /*
>> - * We don't use data->update_interval here as we want to display
>> - * the actual interval used by the chip and jiffies_to_msecs()
>> - * doesn't seem to be accurate enough.
>> + * We want to display the actual interval used by the chip.
>> */
>> - return snprintf(buf, PAGE_SIZE, "%d\n",
>> - ina226_reg_to_interval(data->regs[INA2XX_CONFIG]));
>> + return snprintf(buf, PAGE_SIZE, "%d\n", val);
>> }
>>
>> /* shunt voltage */
>> -static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
>> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_shunt, NULL,
>> INA2XX_SHUNT_VOLTAGE);
>
> If you no longer use the register parameter, there is no need to
> provide it.
> But then I'll have to spend some time trying to understand _why_ you
> don't
> use it anymore and why you introduced separate show functions.
> Some explanation might help.
>
The interval value is no longer needed to compute a read "gard" delay,
but the client Application may still need to set a different averaging
value through this interval setting, based on the DUT characteristics.
As of using separate functions, it fits better in the new logic of
"per-register" accesses. It does not bloat the code either.
>>
>> /* bus voltage */
>> -static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ina2xx_show_value, NULL,
>> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ina2xx_show_bus, NULL,
>> INA2XX_BUS_VOLTAGE);
>>
>> /* calculated current */
>> -static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ina2xx_show_value,
>> NULL,
>> +static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ina2xx_show_curr, NULL,
>> INA2XX_CURRENT);
>>
>> /* calculated power */
>> -static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value,
>> NULL,
>> +static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_pow, NULL,
>> INA2XX_POWER);
>>
>> /* shunt resistance */
>> static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR,
>> - ina2xx_show_value, ina2xx_set_shunt,
>> + ina2xx_show_cal, ina2xx_set_shunt,
>> INA2XX_CALIBRATION);
>>
>> -/* update interval (ina226 only) */
>> -static SENSOR_DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
>> - ina226_show_interval, ina226_set_interval, 0);
>> -
>> /* pointers to created device attributes */
>> static struct attribute *ina2xx_attrs[] = {
>> &sensor_dev_attr_in0_input.dev_attr.attr,
>> @@ -443,6 +399,10 @@ static const struct attribute_group ina2xx_group
>> = {
>> .attrs = ina2xx_attrs,
>> };
>>
>> +/* update interval (ina226 only) */
>> +static SENSOR_DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
>> + ina226_show_interval, ina226_set_interval, 0);
>> +
>
> I don't really see the value of moving this code around.
> Please no unnecessary changes.
>
>> static struct attribute *ina226_attrs[] = {
>> &sensor_dev_attr_update_interval.dev_attr.attr,
>> NULL,
>> @@ -452,65 +412,65 @@ static const struct attribute_group
>> ina226_group = {
>> .attrs = ina226_attrs,
>> };
>>
>> +
>> static int ina2xx_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> - struct i2c_adapter *adapter = client->adapter;
>> struct ina2xx_platform_data *pdata;
>> struct device *dev = &client->dev;
>> struct ina2xx_data *data;
>> struct device *hwmon_dev;
>> + struct regmap *regmap;
>> u32 val;
>> int ret, group = 0;
>>
>> - if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
>> - return -ENODEV;
>> + /* Register regmap */
>> + regmap = devm_regmap_init_i2c(client, &INA2XX_regmap_config);
>> + if (IS_ERR(regmap)) {
>> + dev_err(dev, "failed to allocate register map\n");
>> + return PTR_ERR(regmap);
>> + }
>>
>> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> if (!data)
>> return -ENOMEM;
>>
>> - if (dev_get_platdata(dev)) {
>> - pdata = dev_get_platdata(dev);
>> - data->rshunt = pdata->shunt_uohms;
>> - } else if (!of_property_read_u32(dev->of_node,
>> - "shunt-resistor", &val)) {
>> - data->rshunt = val;
>> - } else {
>> - data->rshunt = INA2XX_RSHUNT_DEFAULT;
>> - }
>> + data->regmap = regmap;
>>
>> - /* set the device type */
>> - data->kind = id->driver_data;
>> - data->config = &ina2xx_config[data->kind];
>> - data->curr_config = data->config->config_default;
>> + /* Set config according to device type. */
>> + data->config = &ina2xx_config[id->driver_data];
>> data->client = client;
>>
>> - /*
>> - * Ina226 has a variable update_interval. For ina219 we
>> - * use a constant value.
>> + /* Check for shunt resistor value.
>> + * Give precedence to device tree over must-recompile.
>> */
>> - if (data->kind == ina226)
>> - ina226_set_update_interval(data);
>> - else
>> - data->update_interval = HZ / INA2XX_CONVERSION_RATE;
>> + if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) <
>> 0) {
>> + pdata = dev_get_platdata(dev);
>> + if (pdata)
>> + val = pdata->shunt_uohms;
>> + else
>> + val = INA2XX_RSHUNT_DEFAULT;
>> + }
>
> This changes priority from platform data first to devicetree
> configuration first.
> As such, it is an unrelated change. If needed, split into a separate
> patch, and
Yes I would do a separate patch normaly, agreed.
> explain the reasoning, please.
Changing the platform data requires changes in the kernel code, and
hence recompilation. It seems a bit unexpected that setting a new value
in the dtb will be ignored because there is a compiled-in platform data.
Should'nt the dtb allow to override platform data ?
>
>>
>> - if (data->rshunt <= 0 ||
>> - data->rshunt > data->config->calibration_factor)
>> + if (val <= 0 || val > data->config->calibration_factor) {
>> + dev_err(dev, "Invalid shunt resistor value %li", val);
>> return -ENODEV;
>> + }
>> + data->rshunt = val;
>>
>> + /* Write config to chip, and calibrate */
>
> That comment, if needed, would be more appropriate at the top of the
> called function.
>
>> ret = ina2xx_init(data);
>> - if (ret < 0) {
>> - dev_err(dev, "error configuring the device: %d\n", ret);
>> - return -ENODEV;
>> + if (ret) {
>> + dev_err(dev, "error configuring the device.\n");
>> + return ret;
>
> Please explain whuy this code is better than before. To me it looks like
> personal preference, and I don't see the improvement.
>
It's not, it came from successive changes, will revert...
>> }
>>
>> - mutex_init(&data->update_lock);
>> -
>> + /* Set sensor group according to device type. */
>
> Seems to be obvious to me.
>
>> data->groups[group++] = &ina2xx_group;
>> - if (data->kind == ina226)
>> + if (ina226 == id->driver_data)
>
> Not likely to accept Yoda programming I am.
>
Appropriate it is, In some instances. Usually I dislike it too, question
of style and opinion really I'd say.
>> data->groups[group++] = &ina226_group;
>>
>> + /* register to hwmon */
>
> Isn't that obvious ? I don't see the value of this comment.
>
>> hwmon_dev = devm_hwmon_device_register_with_groups(dev,
>> client->name,
>> data, data->groups);
>> if (IS_ERR(hwmon_dev))
>> @@ -541,7 +501,6 @@ static struct i2c_driver ina2xx_driver = {
>> };
>>
>> module_i2c_driver(ina2xx_driver);
>> -
>
> Does this change fix a checkpatch problem ? If so, please submit as
> separate patch.
> If not, please no unnecessary whitespace changes.
Ok.
>
>> MODULE_AUTHOR("Lothar Felten <l-felten@...com>");
>> MODULE_DESCRIPTION("ina2xx driver");
>> MODULE_LICENSE("GPL");
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists