Re: [PATCH v4 6/8] power: bq27xxx_battery: add i2c bulk read/write functions

Re: [PATCH v4 6/8] power: bq27xxx_battery: add i2c bulk read/write functions

[Liam Breck]

On Sun, 2017-01-29 at 18:38:26, Sebastian Reichel wrote:
>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>> index 3f265dbf11af..581402380d6e 100644
>> --- a/include/linux/power/bq27xxx_battery.h
>> +++ b/include/linux/power/bq27xxx_battery.h
>> @@ -41,6 +41,9 @@ struct bq27xxx_platform_data {
>>  struct bq27xxx_device_info;
>>  struct bq27xxx_access_methods {
>>  	int (*read)(struct bq27xxx_device_info *di, u8 reg, bool single);
>> +	int (*write)(struct bq27xxx_device_info *di, u8 reg, int value, bool single);
>> +	int (*read_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
>> +	int (*write_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
>
>So I had a look at patch 8 and I think we should start with finally
>converting bq27xxx to regmap API and probably a second regmap for
>the block data stuff. That way you get all the debugging info in
>debugfs and the driver looks much cleaner.
										
Could we complete this patchset using the I2C api? We haven't added to it
extensively, and regmap is unrelated to the purpose of this patchset.

Also maybe the primary maintainers of BQ27xxx should be on the hook for
that api change :-)

Plus we're blocked here on devicetree support for BQ24190 waiting for
power_supply_battery_info.

Thanks

Re: [PATCH v4 6/8] power: bq27xxx_battery: add i2c bulk read/write functions

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2173 bytes --]

Hi,

On Sun, Jan 29, 2017 at 12:08:25PM -0800, Liam Breck wrote:
> On Sun, 2017-01-29 at 18:38:26, Sebastian Reichel wrote:
> >> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> >> index 3f265dbf11af..581402380d6e 100644
> >> --- a/include/linux/power/bq27xxx_battery.h
> >> +++ b/include/linux/power/bq27xxx_battery.h
> >> @@ -41,6 +41,9 @@ struct bq27xxx_platform_data {
> >>  struct bq27xxx_device_info;
> >>  struct bq27xxx_access_methods {
> >>  	int (*read)(struct bq27xxx_device_info *di, u8 reg, bool single);
> >> +	int (*write)(struct bq27xxx_device_info *di, u8 reg, int value, bool single);
> >> +	int (*read_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
> >> +	int (*write_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
> >
> >So I had a look at patch 8 and I think we should start with finally
> >converting bq27xxx to regmap API and probably a second regmap for
> >the block data stuff. That way you get all the debugging info in
> >debugfs and the driver looks much cleaner.
> 										
> Could we complete this patchset using the I2C api? We haven't added to it
> extensively, and regmap is unrelated to the purpose of this patchset.

Previously we had only a read operation, which could be easily
converted to regmap, since it accesses a single register. Afterwards
we have read,write,bulk_read and bulk_write. That's 400% and 2/4
functions, which do not follow the regmap style.

So what is your definition of extensively? :)

> Also maybe the primary maintainers of BQ27xxx should be on the hook for
> that api change :-)

https://lkml.org/lkml/2015/9/23/542

The w1 stuff is not that hard to implement actually by just using
the existing functions and devm_regmap_init().

> Plus we're blocked here on devicetree support for BQ24190 waiting for
> power_supply_battery_info.

Well this patch depends on the power_supply_battery_info patch, not
the other way around. I will apply the power_supply_battery_info
patch once its ok. No need to wait for this patch. So not really a
problem for your BQ24190 work?

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 6/8] power: bq27xxx_battery: add i2c bulk read/write functions

[Liam Breck]

On Sun, Jan 29, 2017 at 2:22 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Sun, Jan 29, 2017 at 12:08:25PM -0800, Liam Breck wrote:
>> On Sun, 2017-01-29 at 18:38:26, Sebastian Reichel wrote:
>> >> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>> >> index 3f265dbf11af..581402380d6e 100644
>> >> --- a/include/linux/power/bq27xxx_battery.h
>> >> +++ b/include/linux/power/bq27xxx_battery.h
>> >> @@ -41,6 +41,9 @@ struct bq27xxx_platform_data {
>> >>  struct bq27xxx_device_info;
>> >>  struct bq27xxx_access_methods {
>> >>    int (*read)(struct bq27xxx_device_info *di, u8 reg, bool single);
>> >> +  int (*write)(struct bq27xxx_device_info *di, u8 reg, int value, bool single);
>> >> +  int (*read_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
>> >> +  int (*write_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
>> >
>> >So I had a look at patch 8 and I think we should start with finally
>> >converting bq27xxx to regmap API and probably a second regmap for
>> >the block data stuff. That way you get all the debugging info in
>> >debugfs and the driver looks much cleaner.
>>
>> Could we complete this patchset using the I2C api? We haven't added to it
>> extensively, and regmap is unrelated to the purpose of this patchset.
>
> Previously we had only a read operation, which could be easily
> converted to regmap, since it accesses a single register. Afterwards
> we have read,write,bulk_read and bulk_write. That's 400% and 2/4
> functions, which do not follow the regmap style.
>
> So what is your definition of extensively? :)

We added all of 51 LOC, plus whitespace. Extensive is at least greater
than UCHAR_MAX :-)

>> Also maybe the primary maintainers of BQ27xxx should be on the hook for
>> that api change :-)
>
> https://lkml.org/lkml/2015/9/23/542
>
> The w1 stuff is not that hard to implement actually by just using
> the existing functions and devm_regmap_init().

We don't really have the bandwidth or budget for general driver
maintenance. There was no indication in the I2C code that it was
deprecated. Please ask TI to fund this, not us.

>> Plus we're blocked here on devicetree support for BQ24190 waiting for
>> power_supply_battery_info.
>
> Well this patch depends on the power_supply_battery_info patch, not
> the other way around. I will apply the power_supply_battery_info
> patch once its ok. No need to wait for this patch. So not really a
> problem for your BQ24190 work?

Usually features like that aren't added without a client?

[PATCH v4 0/8] power: bq27xxx: add support for NVRAM R/W access

[Matt Ranostay]

Changes from v1:
* add documentation for mWh and mAh property units
* change devicetree entries to match new property units

Changes from v2:
* split i2c changes into respective patches
* add documentation for battery information for fuel gauge
* rebased documentation patches on change on the list
* abstracted the battery configuration for the state machine
  to an generic struct and platform data access function

Changes from v3:
* add "fixed-battery" compatible field to be be more consistant
  with devicetree

Matt Ranostay (8):
  devicetree: property-units: add mWh and mAh units
  devicetree: power: add battery state machine documentation
  power: power_supply: add battery information struct
  power: power_supply: add battery info platform data retrieval
  power: bq27xxx_battery: add BQ27425 chip id
  power: bq27xxx_battery: add i2c bulk read/write functions
  devicetree: power: bq27xxx: add monitored battery documentation
  power: bq27xxx_battery: add initial state machine support

 .../devicetree/bindings/power/supply/battery.txt   |  24 ++
 .../devicetree/bindings/power/supply/bq27xxx.txt   |   8 +
 .../devicetree/bindings/property-units.txt         |   2 +
 drivers/power/supply/bq27xxx_battery.c             | 253 ++++++++++++++++++++-
 drivers/power/supply/bq27xxx_battery_i2c.c         |  64 +++++-
 drivers/power/supply/power_supply_core.c           |  41 ++++
 include/linux/power/bq27xxx_battery.h              |   7 +-
 include/linux/power_supply.h                       |  16 ++
 8 files changed, 411 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

-- 
2.10.2

[PATCH v4 6/8] power: bq27xxx_battery: add i2c bulk read/write functions

[Matt Ranostay]

Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
---
 drivers/power/supply/bq27xxx_battery_i2c.c | 62 ++++++++++++++++++++++++++++++
 include/linux/power/bq27xxx_battery.h      |  4 ++
 2 files changed, 66 insertions(+)

diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index 2ea2d0b06948..feeb04ed88c8 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -68,6 +68,64 @@ static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
 	return ret;
 }
 
+static int bq27xxx_battery_i2c_write(struct bq27xxx_device_info *di, u8 reg,
+				     int value, bool single)
+{
+	struct i2c_client *client = to_i2c_client(di->dev);
+	struct i2c_msg msg;
+	unsigned char data[4];
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	data[0] = reg;
+	if (single) {
+		data[1] = (unsigned char) value;
+		msg.len = 2;
+	} else {
+		put_unaligned_le16(value, &data[1]);
+		msg.len = 3;
+	}
+
+	msg.buf = data;
+	msg.addr = client->addr;
+	msg.flags = 0;
+
+	return i2c_transfer(client->adapter, &msg, 1) == 1 ? 0 : -EINVAL;
+}
+
+static int bq27xxx_battery_i2c_bulk_read(struct bq27xxx_device_info *di, u8 reg,
+					 u8 *data, int len)
+{
+	struct i2c_client *client = to_i2c_client(di->dev);
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	return i2c_smbus_read_i2c_block_data(client, reg, len, data);
+}
+
+static int bq27xxx_battery_i2c_bulk_write(struct bq27xxx_device_info *di,
+					  u8 reg, u8 *data, int len)
+{
+	struct i2c_client *client = to_i2c_client(di->dev);
+	struct i2c_msg msg;
+	u8 buf[33];
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	buf[0] = reg;
+	memcpy(&buf[1], data, len);
+
+	msg.buf = buf;
+	msg.addr = client->addr;
+	msg.flags = 0;
+	msg.len = len + 1;
+
+	return i2c_transfer(client->adapter, &msg, 1) == 1 ? 0 : -EINVAL;
+}
+
 static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 				     const struct i2c_device_id *id)
 {
@@ -95,7 +153,11 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 	di->dev = &client->dev;
 	di->chip = id->driver_data;
 	di->name = name;
+
 	di->bus.read = bq27xxx_battery_i2c_read;
+	di->bus.write = bq27xxx_battery_i2c_write;
+	di->bus.read_bulk = bq27xxx_battery_i2c_bulk_read;
+	di->bus.write_bulk = bq27xxx_battery_i2c_bulk_write;
 
 	ret = bq27xxx_battery_setup(di);
 	if (ret)
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 3f265dbf11af..581402380d6e 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -41,6 +41,9 @@ struct bq27xxx_platform_data {
 struct bq27xxx_device_info;
 struct bq27xxx_access_methods {
 	int (*read)(struct bq27xxx_device_info *di, u8 reg, bool single);
+	int (*write)(struct bq27xxx_device_info *di, u8 reg, int value, bool single);
+	int (*read_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
+	int (*write_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
 };
 
 struct bq27xxx_reg_cache {
@@ -71,6 +74,7 @@ struct bq27xxx_device_info {
 	struct list_head list;
 	struct mutex lock;
 	u8 *regs;
+	u8 buffer[32];
 };
 
 void bq27xxx_battery_update(struct bq27xxx_device_info *di);
-- 
2.10.2

Re: [PATCH v4 6/8] power: bq27xxx_battery: add i2c bulk read/write functions

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3938 bytes --]

Hi,

On Sat, Jan 21, 2017 at 11:14:02PM -0800, Matt Ranostay wrote:
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
>  drivers/power/supply/bq27xxx_battery_i2c.c | 62 ++++++++++++++++++++++++++++++
>  include/linux/power/bq27xxx_battery.h      |  4 ++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
> index 2ea2d0b06948..feeb04ed88c8 100644
> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
> @@ -68,6 +68,64 @@ static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
>  	return ret;
>  }
>  
> +static int bq27xxx_battery_i2c_write(struct bq27xxx_device_info *di, u8 reg,
> +				     int value, bool single)
> +{
> +	struct i2c_client *client = to_i2c_client(di->dev);
> +	struct i2c_msg msg;
> +	unsigned char data[4];
> +
> +	if (!client->adapter)
> +		return -ENODEV;
> +
> +	data[0] = reg;
> +	if (single) {
> +		data[1] = (unsigned char) value;
> +		msg.len = 2;
> +	} else {
> +		put_unaligned_le16(value, &data[1]);
> +		msg.len = 3;
> +	}
> +
> +	msg.buf = data;
> +	msg.addr = client->addr;
> +	msg.flags = 0;
> +
> +	return i2c_transfer(client->adapter, &msg, 1) == 1 ? 0 : -EINVAL;
> +}
> +
> +static int bq27xxx_battery_i2c_bulk_read(struct bq27xxx_device_info *di, u8 reg,
> +					 u8 *data, int len)
> +{
> +	struct i2c_client *client = to_i2c_client(di->dev);
> +
> +	if (!client->adapter)
> +		return -ENODEV;
> +
> +	return i2c_smbus_read_i2c_block_data(client, reg, len, data);
> +}
> +
> +static int bq27xxx_battery_i2c_bulk_write(struct bq27xxx_device_info *di,
> +					  u8 reg, u8 *data, int len)
> +{
> +	struct i2c_client *client = to_i2c_client(di->dev);
> +	struct i2c_msg msg;
> +	u8 buf[33];
> +
> +	if (!client->adapter)
> +		return -ENODEV;
> +
> +	buf[0] = reg;
> +	memcpy(&buf[1], data, len);
> +
> +	msg.buf = buf;
> +	msg.addr = client->addr;
> +	msg.flags = 0;
> +	msg.len = len + 1;
> +
> +	return i2c_transfer(client->adapter, &msg, 1) == 1 ? 0 : -EINVAL;
> +}
> +
>  static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
>  				     const struct i2c_device_id *id)
>  {
> @@ -95,7 +153,11 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
>  	di->dev = &client->dev;
>  	di->chip = id->driver_data;
>  	di->name = name;
> +
>  	di->bus.read = bq27xxx_battery_i2c_read;
> +	di->bus.write = bq27xxx_battery_i2c_write;
> +	di->bus.read_bulk = bq27xxx_battery_i2c_bulk_read;
> +	di->bus.write_bulk = bq27xxx_battery_i2c_bulk_write;
>  
>  	ret = bq27xxx_battery_setup(di);
>  	if (ret)
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index 3f265dbf11af..581402380d6e 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -41,6 +41,9 @@ struct bq27xxx_platform_data {
>  struct bq27xxx_device_info;
>  struct bq27xxx_access_methods {
>  	int (*read)(struct bq27xxx_device_info *di, u8 reg, bool single);
> +	int (*write)(struct bq27xxx_device_info *di, u8 reg, int value, bool single);
> +	int (*read_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
> +	int (*write_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);

So I had a look at patch 8 and I think we should start with finally
converting bq27xxx to regmap API and probably a second regmap for
the block data stuff. That way you get all the debugging info in
debugfs and the driver looks much cleaner.

>  };
>  
>  struct bq27xxx_reg_cache {
> @@ -71,6 +74,7 @@ struct bq27xxx_device_info {
>  	struct list_head list;
>  	struct mutex lock;
>  	u8 *regs;
> +	u8 buffer[32];

unrelated change.

>  };
>  
>  void bq27xxx_battery_update(struct bq27xxx_device_info *di);

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]