| Message ID | 20170612150356.wvehmijba6xbdwvb@lenoch (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
See below for my comment... On 12-6-2017 17:03, Ladislav Michl wrote: > Add LTC2942 registers definitions. No functional changes. > > Signed-off-by: Ladislav Michl <ladis@linux-mips.org> > --- > drivers/power/supply/ltc2941-battery-gauge.c | 27 +++++++++++++-------------- > 1 file changed, 13 insertions(+), 14 deletions(-) > > diff --git a/drivers/power/supply/ltc2941-battery-gauge.c b/drivers/power/supply/ltc2941-battery-gauge.c > index 7efb908f4451..0a1b69bbca7f 100644 > --- a/drivers/power/supply/ltc2941-battery-gauge.c > +++ b/drivers/power/supply/ltc2941-battery-gauge.c > @@ -34,16 +34,14 @@ enum ltc294x_reg { > LTC294X_REG_CONTROL = 0x01, > LTC294X_REG_ACC_CHARGE_MSB = 0x02, > LTC294X_REG_ACC_CHARGE_LSB = 0x03, > - LTC294X_REG_THRESH_HIGH_MSB = 0x04, > - LTC294X_REG_THRESH_HIGH_LSB = 0x05, > - LTC294X_REG_THRESH_LOW_MSB = 0x06, > - LTC294X_REG_THRESH_LOW_LSB = 0x07, > - LTC294X_REG_VOLTAGE_MSB = 0x08, > - LTC294X_REG_VOLTAGE_LSB = 0x09, > - LTC294X_REG_CURRENT_MSB = 0x0E, > - LTC294X_REG_CURRENT_LSB = 0x0F, > - LTC294X_REG_TEMPERATURE_MSB = 0x14, > - LTC294X_REG_TEMPERATURE_LSB = 0x15, > + LTC294X_REG_VOLTAGE_MSB = 0x08, > + LTC294X_REG_VOLTAGE_LSB = 0x09, > + LTC2942_REG_TEMPERATURE_MSB = 0x0C, > + LTC2942_REG_TEMPERATURE_LSB = 0x0D, > + LTC2943_REG_CURRENT_MSB = 0x0E, > + LTC2943_REG_CURRENT_LSB = 0x0F, > + LTC2943_REG_TEMPERATURE_MSB = 0x14, > + LTC2943_REG_TEMPERATURE_LSB = 0x15, > }; I don't see the advantage of renaming these, I'd rather see the LTC294X naming throughout, making it easier to locate them in the enum as they all share the exact same prefix. But if you do want this renaming, I guess you should also rename the LTC2941 registers. I'm neutral to either naming, linux-pm maintainer will have the final word I guess. > > #define LTC2943_REG_CONTROL_MODE_MASK (BIT(7) | BIT(6)) > @@ -55,6 +53,7 @@ enum ltc294x_reg { > #define LTC294X_REG_CONTROL_ALCC_CONFIG_DISABLED 0 > > #define LTC2941_NUM_REGS 0x08 > +#define LTC2942_NUM_REGS 0x10 > #define LTC2943_NUM_REGS 0x18 > > struct ltc294x_info { > @@ -263,7 +262,7 @@ static int ltc294x_get_current(const struct ltc294x_info *info, int *val) > s32 value; > > ret = ltc294x_read_regs(info->client, > - LTC294X_REG_CURRENT_MSB, &datar[0], 2); > + LTC2943_REG_CURRENT_MSB, &datar[0], 2); > value = (datar[0] << 8) | datar[1]; > value -= 0x7FFF; > /* Value is in range -32k..+32k, r_sense is usually 10..50 mOhm, > @@ -280,7 +279,7 @@ static int ltc294x_get_temperature(const struct ltc294x_info *info, int *val) > u32 value; > > ret = ltc294x_read_regs(info->client, > - LTC294X_REG_TEMPERATURE_MSB, &datar[0], 2); > + LTC2943_REG_TEMPERATURE_MSB, &datar[0], 2); > value = (datar[0] << 8) | datar[1]; > /* Full-scale is 510 Kelvin, convert to centidegrees */ > *val = (((51000 * value) / 0xFFFF) - 27215); > @@ -424,10 +423,10 @@ static int ltc294x_i2c_probe(struct i2c_client *client, > info->client = client; > info->supply_desc.type = POWER_SUPPLY_TYPE_BATTERY; > info->supply_desc.properties = ltc294x_properties; > - if (info->num_regs >= LTC294X_REG_TEMPERATURE_LSB) > + if (info->num_regs >= LTC2943_REG_TEMPERATURE_LSB) > info->supply_desc.num_properties = > ARRAY_SIZE(ltc294x_properties); > - else if (info->num_regs >= LTC294X_REG_CURRENT_LSB) > + else if (info->num_regs >= LTC2943_REG_CURRENT_LSB) > info->supply_desc.num_properties = > ARRAY_SIZE(ltc294x_properties) - 1; > else if (info->num_regs >= LTC294X_REG_VOLTAGE_LSB) >
On Mon, Jun 12, 2017 at 06:26:17PM +0200, Mike Looijmans wrote: > On 12-6-2017 17:03, Ladislav Michl wrote: > > Add LTC2942 registers definitions. No functional changes. > > > > Signed-off-by: Ladislav Michl <ladis@linux-mips.org> > > --- > > drivers/power/supply/ltc2941-battery-gauge.c | 27 +++++++++++++-------------- > > 1 file changed, 13 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/power/supply/ltc2941-battery-gauge.c b/drivers/power/supply/ltc2941-battery-gauge.c > > index 7efb908f4451..0a1b69bbca7f 100644 > > --- a/drivers/power/supply/ltc2941-battery-gauge.c > > +++ b/drivers/power/supply/ltc2941-battery-gauge.c > > @@ -34,16 +34,14 @@ enum ltc294x_reg { > > LTC294X_REG_CONTROL = 0x01, > > LTC294X_REG_ACC_CHARGE_MSB = 0x02, > > LTC294X_REG_ACC_CHARGE_LSB = 0x03, > > - LTC294X_REG_THRESH_HIGH_MSB = 0x04, > > - LTC294X_REG_THRESH_HIGH_LSB = 0x05, > > - LTC294X_REG_THRESH_LOW_MSB = 0x06, > > - LTC294X_REG_THRESH_LOW_LSB = 0x07, > > - LTC294X_REG_VOLTAGE_MSB = 0x08, > > - LTC294X_REG_VOLTAGE_LSB = 0x09, > > - LTC294X_REG_CURRENT_MSB = 0x0E, > > - LTC294X_REG_CURRENT_LSB = 0x0F, > > - LTC294X_REG_TEMPERATURE_MSB = 0x14, > > - LTC294X_REG_TEMPERATURE_LSB = 0x15, > > + LTC294X_REG_VOLTAGE_MSB = 0x08, > > + LTC294X_REG_VOLTAGE_LSB = 0x09, > > + LTC2942_REG_TEMPERATURE_MSB = 0x0C, > > + LTC2942_REG_TEMPERATURE_LSB = 0x0D, > > + LTC2943_REG_CURRENT_MSB = 0x0E, > > + LTC2943_REG_CURRENT_LSB = 0x0F, > > + LTC2943_REG_TEMPERATURE_MSB = 0x14, > > + LTC2943_REG_TEMPERATURE_LSB = 0x15, > > }; > > I don't see the advantage of renaming these, I'd rather see the LTC294X > naming throughout, making it easier to locate them in the enum as they all > share the exact same prefix. LTC2942 and LTC2943 are using different register offsets for temperature and current register is LTC2943 specific. Hence the rename, otherwise you won't be able to distinguish between temperature registers in different chips. > But if you do want this renaming, I guess you should also rename the LTC2941 > registers. Well, naming convention is to use LTC294X prefix only for registers common to all chips. Chip specific registers are using chip specific prefixes. Also I'm not big fan of having register offsets as enums - offset is a number and should be represented as such. > I'm neutral to either naming, linux-pm maintainer will have the final word I > guess. I have no problem rolling change back with exception of tepmerature registers (LTC294X_LTC2942_REG_TEMPERATURE_MSB would probably be an option, but such a long identifier hurts my eyes). Let's wait for maintainer's comment. Best regards, ladis
diff --git a/drivers/power/supply/ltc2941-battery-gauge.c b/drivers/power/supply/ltc2941-battery-gauge.c index 7efb908f4451..0a1b69bbca7f 100644 --- a/drivers/power/supply/ltc2941-battery-gauge.c +++ b/drivers/power/supply/ltc2941-battery-gauge.c @@ -34,16 +34,14 @@ enum ltc294x_reg { LTC294X_REG_CONTROL = 0x01, LTC294X_REG_ACC_CHARGE_MSB = 0x02, LTC294X_REG_ACC_CHARGE_LSB = 0x03, - LTC294X_REG_THRESH_HIGH_MSB = 0x04, - LTC294X_REG_THRESH_HIGH_LSB = 0x05, - LTC294X_REG_THRESH_LOW_MSB = 0x06, - LTC294X_REG_THRESH_LOW_LSB = 0x07, - LTC294X_REG_VOLTAGE_MSB = 0x08, - LTC294X_REG_VOLTAGE_LSB = 0x09, - LTC294X_REG_CURRENT_MSB = 0x0E, - LTC294X_REG_CURRENT_LSB = 0x0F, - LTC294X_REG_TEMPERATURE_MSB = 0x14, - LTC294X_REG_TEMPERATURE_LSB = 0x15, + LTC294X_REG_VOLTAGE_MSB = 0x08, + LTC294X_REG_VOLTAGE_LSB = 0x09, + LTC2942_REG_TEMPERATURE_MSB = 0x0C, + LTC2942_REG_TEMPERATURE_LSB = 0x0D, + LTC2943_REG_CURRENT_MSB = 0x0E, + LTC2943_REG_CURRENT_LSB = 0x0F, + LTC2943_REG_TEMPERATURE_MSB = 0x14, + LTC2943_REG_TEMPERATURE_LSB = 0x15, }; #define LTC2943_REG_CONTROL_MODE_MASK (BIT(7) | BIT(6)) @@ -55,6 +53,7 @@ enum ltc294x_reg { #define LTC294X_REG_CONTROL_ALCC_CONFIG_DISABLED 0 #define LTC2941_NUM_REGS 0x08 +#define LTC2942_NUM_REGS 0x10 #define LTC2943_NUM_REGS 0x18 struct ltc294x_info { @@ -263,7 +262,7 @@ static int ltc294x_get_current(const struct ltc294x_info *info, int *val) s32 value; ret = ltc294x_read_regs(info->client, - LTC294X_REG_CURRENT_MSB, &datar[0], 2); + LTC2943_REG_CURRENT_MSB, &datar[0], 2); value = (datar[0] << 8) | datar[1]; value -= 0x7FFF; /* Value is in range -32k..+32k, r_sense is usually 10..50 mOhm, @@ -280,7 +279,7 @@ static int ltc294x_get_temperature(const struct ltc294x_info *info, int *val) u32 value; ret = ltc294x_read_regs(info->client, - LTC294X_REG_TEMPERATURE_MSB, &datar[0], 2); + LTC2943_REG_TEMPERATURE_MSB, &datar[0], 2); value = (datar[0] << 8) | datar[1]; /* Full-scale is 510 Kelvin, convert to centidegrees */ *val = (((51000 * value) / 0xFFFF) - 27215); @@ -424,10 +423,10 @@ static int ltc294x_i2c_probe(struct i2c_client *client, info->client = client; info->supply_desc.type = POWER_SUPPLY_TYPE_BATTERY; info->supply_desc.properties = ltc294x_properties; - if (info->num_regs >= LTC294X_REG_TEMPERATURE_LSB) + if (info->num_regs >= LTC2943_REG_TEMPERATURE_LSB) info->supply_desc.num_properties = ARRAY_SIZE(ltc294x_properties); - else if (info->num_regs >= LTC294X_REG_CURRENT_LSB) + else if (info->num_regs >= LTC2943_REG_CURRENT_LSB) info->supply_desc.num_properties = ARRAY_SIZE(ltc294x_properties) - 1; else if (info->num_regs >= LTC294X_REG_VOLTAGE_LSB)
Add LTC2942 registers definitions. No functional changes. Signed-off-by: Ladislav Michl <ladis@linux-mips.org> --- drivers/power/supply/ltc2941-battery-gauge.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)