[PATCH v2 0/3] power: bq27xxx: add support for NVRAM R/W access

[PATCH v2 0/3] power: bq27xxx: add support for NVRAM R/W access

[Matt Ranostay]

Enable access to the NVRAM for the bq27425 chipset which allows to
update the battery metrics that are used in the state machine.

This allows adjustments of a termination voltage, design energy, and
design voltage.

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

Matt Ranostay (3):
  devicetree: property-units: add mWh and mAh units
  devicetree: bq27425: add documentation for bq27425 fuel gauge
  power: bq27xxx: add support for NVRAM R/W access

 .../devicetree/bindings/power/bq27425.txt          |  25 ++
 .../devicetree/bindings/property-units.txt         |   2 +
 drivers/power/supply/bq27xxx_battery_i2c.c         | 335 +++++++++++++++++++++
 include/linux/power/bq27xxx_battery.h              |   4 +
 4 files changed, 366 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/bq27425.txt

-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] devicetree: property-units: add mWh and mAh units

[Matt Ranostay]

Add entries for microwatt-hours and microamp-hours to property
units.

Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt b/Documentation/devicetree/bindings/property-units.txt
index 12278d79f6c0..5e8d220cc2b6 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 ----------------------------------------
 -microamp	: micro amps
+-microamp-hours : micro amp hours
 -ohms		: Ohms
 -micro-ohms	: micro Ohms
+-microwatt-hours: micro Watt hours
 -microvolt	: micro volts
 
 Temperature
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] devicetree: property-units: add mWh and mAh units

[Sebastian Reichel]

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

Hi,

On Wed, Jan 04, 2017 at 06:10:05PM -0800, Matt Ranostay wrote:
> Add entries for microwatt-hours and microamp-hours to property
> units.
> 
> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/property-units.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/property-units.txt b/Documentation/devicetree/bindings/property-units.txt
> index 12278d79f6c0..5e8d220cc2b6 100644
> --- a/Documentation/devicetree/bindings/property-units.txt
> +++ b/Documentation/devicetree/bindings/property-units.txt
> @@ -25,8 +25,10 @@ Distance
>  Electricity
>  ----------------------------------------
>  -microamp	: micro amps
> +-microamp-hours : micro amp hours
>  -ohms		: Ohms
>  -micro-ohms	: micro Ohms
> +-microwatt-hours: micro Watt hours
>  -microvolt	: micro volts
>  
>  Temperature

Acked-By: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

As there is no hard-dependency I expect this goes through the DT
subsystem independently of the remaining patchset. Please ping me,
if I should queue it via the power-supply tree instead.

@Matt: You miss a few CC for this patch:

./scripts/get_maintainer.pl -f Documentation/devicetree/bindings/property-units.txt
Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,commit_signer:1/1=100%,authored:1/1=100%,added_lines:39/39=100%)
Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,commit_signer:1/1=100%)
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list)

-- Sebastian

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

Re: [PATCH v2 1/3] devicetree: property-units: add mWh and mAh units

[Rob Herring]

On Wed, Jan 04, 2017 at 06:10:05PM -0800, Matt Ranostay wrote:
> Add entries for microwatt-hours and microamp-hours to property
> units.
> 
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
>  Documentation/devicetree/bindings/property-units.txt | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

This can go in with the rest of the series.

Rob

[PATCH v2 2/3] devicetree: bq27425: add documentation for bq27425 fuel gauge

[Matt Ranostay]

Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
---
 .../devicetree/bindings/power/bq27425.txt          | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/bq27425.txt

diff --git a/Documentation/devicetree/bindings/power/bq27425.txt b/Documentation/devicetree/bindings/power/bq27425.txt
new file mode 100644
index 000000000000..5d33b61cf9b7
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/bq27425.txt
@@ -0,0 +1,25 @@
+* TI BQ27425 Fuel Gauge
+
+http://www.ti.com/lit/ds/symlink/bq27425-g2a.pdf
+
+Please note that if any of the optional properties are defined
+then all settings must be.
+
+Required properties:
+- compatible: Should be "ti,bq27425"
+- reg: integer, I2C address of the device
+
+Optional properties:
+- ti,design-microamp-hours: integer of mAh of the battery
+- ti,design-microvolt-hours: integer of the mWh of the battery
+- ti,terminate-microvolt: integer of mV of the dead voltage of
+		the battery
+
+bq27425 {
+	compatible = "ti,bq27425";
+	reg = <0x55>;
+
+	ti,design-microamp-hours = <1360>;
+	ti,design-microwatt-hours = <4970>;
+	ti,terminate-microvolt = <3200>;
+};
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] devicetree: bq27425: add documentation for bq27425 fuel gauge

[Sebastian Reichel]

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

Hi,

On Wed, Jan 04, 2017 at 06:10:06PM -0800, Matt Ranostay wrote:
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
>  .../devicetree/bindings/power/bq27425.txt          | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/bq27425.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/bq27425.txt b/Documentation/devicetree/bindings/power/bq27425.txt
> new file mode 100644
> index 000000000000..5d33b61cf9b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/bq27425.txt
> @@ -0,0 +1,25 @@
> +* TI BQ27425 Fuel Gauge
> +
> +http://www.ti.com/lit/ds/symlink/bq27425-g2a.pdf
> +
> +Please note that if any of the optional properties are defined
> +then all settings must be.
> +
> +Required properties:
> +- compatible: Should be "ti,bq27425"
> +- reg: integer, I2C address of the device
> +
> +Optional properties:
> +- ti,design-microamp-hours: integer of mAh of the battery
> +- ti,design-microvolt-hours: integer of the mWh of the battery
> +- ti,terminate-microvolt: integer of mV of the dead voltage of
> +		the battery
> +
> +bq27425 {
> +	compatible = "ti,bq27425";
> +	reg = <0x55>;
> +
> +	ti,design-microamp-hours = <1360>;
> +	ti,design-microwatt-hours = <4970>;
> +	ti,terminate-microvolt = <3200>;
> +};

The design capacity of the battery is not a property of the fuel
gauge. It should be modelled more like this:

/ {
    bat0: battery {
        /* Nokia BL-5J */
        nominal-microvolt = <3700000>;
        design-microwatt-hours = <4900000>;
        design-microamp-hours = <1320000>;
        chemistry-type = <POWER_SUPPLY_LI_ION>;

        /* other battery specific stuff */
    };
};

&i2c3 {
    bq27425 {
        compatible = "ti,bq27425";
        reg = <0x55>;

        monitored-battery = <&bat0>;

        /* other fuel-gauge specific stuff */
    };
};

Also there is another pending patch series, that documents bq27xxx.
bq27425 should be documented inside of it. The battery binding would
be generic, so it should be described in its own document.

-- Sebastian

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

Re: [PATCH v2 2/3] devicetree: bq27425: add documentation for bq27425 fuel gauge

[Rob Herring]

On Wed, Jan 04, 2017 at 06:10:06PM -0800, Matt Ranostay wrote:
> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
> ---
>  .../devicetree/bindings/power/bq27425.txt          | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/bq27425.txt

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] power: bq27xxx: add support for NVRAM R/W access

[Matt Ranostay]

Initial support for access and modification of the non-volatile regions
of the bq27425 fuel gauge DesignEnergy, DesignCapacity, and
TerminateVoltage settings.

This is intended for fine tuning the fuel gauge state machine for the
respective battery specifications.

Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
---
 drivers/power/supply/bq27xxx_battery_i2c.c | 335 +++++++++++++++++++++++++++++
 include/linux/power/bq27xxx_battery.h      |   4 +
 2 files changed, 339 insertions(+)

diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index 27143230839a..8ec278e15ab6 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -14,9 +14,11 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <asm/unaligned.h>
 
 #include <linux/power/bq27xxx_battery.h>
@@ -24,6 +26,48 @@
 static DEFINE_IDR(battery_id);
 static DEFINE_MUTEX(battery_mutex);
 
+#define BQ27XXX_TERM_V_MIN	2800
+#define BQ27XXX_TERM_V_MAX	3700
+
+#define BQ27XXX_REG_CTRL		0
+
+#define BQ27XXX_BLOCK_DATA_CLASS	0x3E
+#define BQ27XXX_DATA_BLOCK		0x3F
+#define BQ27XXX_BLOCK_DATA		0x40
+#define BQ27XXX_BLOCK_DATA_CHECKSUM	0x60
+#define BQ27XXX_BLOCK_DATA_CONTROL	0x61
+#define BQ27XXX_SET_CFGUPDATE		0x13
+#define BQ27XXX_SOFT_RESET		0x42
+
+enum bq27xxx_dm_subclass_index {
+	BQ27XXX_DM_DESIGN_CAP = 0,
+	BQ27XXX_DM_DESIGN_ENERGY,
+	BQ27XXX_DM_TERMINATE_VOLTAGE,
+	BQ27XXX_NUM_IDX,
+};
+
+struct bq27xxx_dm_regs {
+	unsigned int subclass_id;
+	unsigned int offset;
+	char *name;
+};
+
+#define BQ27XXX_GAS_GAUGING_STATE_SUBCLASS	82
+
+static struct bq27xxx_dm_regs bq27425_dm_subclass_regs[] = {
+	{ BQ27XXX_GAS_GAUGING_STATE_SUBCLASS, 12, "design-capacity" },
+	{ BQ27XXX_GAS_GAUGING_STATE_SUBCLASS, 14, "design-energy" },
+	{ BQ27XXX_GAS_GAUGING_STATE_SUBCLASS, 18, "terminate-voltage" },
+};
+
+static struct bq27xxx_dm_regs *bq27xxx_dm_subclass_regs[] = {
+	[BQ27425] = bq27425_dm_subclass_regs,
+};
+
+static unsigned int bq27xxx_unseal_keys[] = {
+	[BQ27425] = 0x04143672,
+};
+
 static irqreturn_t bq27xxx_battery_irq_handler_thread(int irq, void *data)
 {
 	struct bq27xxx_device_info *di = data;
@@ -68,6 +112,289 @@ 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_set_seal_state(struct bq27xxx_device_info *di,
+					      bool state)
+{
+	unsigned int key = bq27xxx_unseal_keys[di->chip];
+	int ret;
+
+	if (state)
+		return di->bus.write(di, BQ27XXX_REG_CTRL, 0x20, false);
+
+	ret = di->bus.write(di, BQ27XXX_REG_CTRL, (key >> 16) & 0xffff, false);
+	if (ret < 0)
+		return ret;
+
+	return di->bus.write(di, BQ27XXX_REG_CTRL, key & 0xffff, false);
+}
+
+static int bq27xxx_battery_i2c_read_dm_block(struct bq27xxx_device_info *di,
+					     int subclass)
+{
+	int ret = di->bus.write(di, BQ27XXX_REG_CTRL, 0, false);
+
+	if (ret < 0)
+		return ret;
+
+	ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
+	if (ret < 0)
+		return ret;
+
+	ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CLASS, subclass, true);
+	if (ret < 0)
+		return ret;
+
+	ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, 0, true);
+	if (ret < 0)
+		return ret;
+
+	usleep_range(1000, 1500);
+
+	return di->bus.read_bulk(di, BQ27XXX_BLOCK_DATA,
+				(u8 *) &di->buffer, sizeof(di->buffer));
+}
+
+static int bq27xxx_battery_i2c_print_config(struct bq27xxx_device_info *di)
+{
+	struct bq27xxx_dm_regs *reg = bq27xxx_dm_subclass_regs[di->chip];
+	int ret, i;
+
+	ret = bq27xxx_battery_i2c_read_dm_block(di,
+			BQ27XXX_GAS_GAUGING_STATE_SUBCLASS);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < BQ27XXX_NUM_IDX; i++) {
+		int val;
+
+		if (reg->subclass_id != BQ27XXX_GAS_GAUGING_STATE_SUBCLASS)
+			continue;
+
+		val = be16_to_cpup((u16 *) &di->buffer[reg->offset]);
+
+		dev_info(di->dev, "settings for %s set at %d\n", reg->name, val);
+
+		reg++;
+	}
+
+	return 0;
+}
+
+static bool bq27xxx_battery_update_dm_setting(struct bq27xxx_device_info *di,
+			      unsigned int reg, unsigned int val)
+{
+	struct bq27xxx_dm_regs *dm_reg = &bq27xxx_dm_subclass_regs[di->chip][reg];
+	u16 *prev = (u16 *) &di->buffer[dm_reg->offset];
+
+	if (be16_to_cpup(prev) == val)
+		return false;
+
+	*prev = cpu_to_be16(val);
+
+	return true;
+}
+
+static u8 bq27xxx_battery_checksum(struct bq27xxx_device_info *di)
+{
+	u8 *data = (u8 *) &di->buffer;
+	u16 sum = 0;
+	int i;
+
+	for (i = 0; i < sizeof(di->buffer); i++) {
+		sum += data[i];
+		sum &= 0xff;
+	}
+
+	return 0xff - sum;
+}
+
+static int bq27xxx_battery_i2c_write_nvram(struct bq27xxx_device_info *di,
+					   unsigned int subclass)
+{
+	int ret;
+
+	ret = di->bus.write(di, BQ27XXX_REG_CTRL, BQ27XXX_SET_CFGUPDATE, false);
+	if (ret)
+		return ret;
+
+	ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
+	if (ret)
+		return ret;
+
+	ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CLASS, subclass, true);
+	if (ret)
+		return ret;
+
+	ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, 0, true);
+	if (ret)
+		return ret;
+
+	ret = di->bus.write_bulk(di, BQ27XXX_BLOCK_DATA,
+				(u8 *) &di->buffer, sizeof(di->buffer));
+	if (ret < 0)
+		return ret;
+
+	usleep_range(1000, 1500);
+
+	di->bus.write(di, BQ27XXX_BLOCK_DATA_CHECKSUM,
+				bq27xxx_battery_checksum(di), true);
+
+	usleep_range(1000, 1500);
+
+	di->bus.write(di, BQ27XXX_REG_CTRL, BQ27XXX_SOFT_RESET, false);
+
+	return 0;
+}
+
+static int bq27xxx_battery_i2c_set_config(struct bq27xxx_device_info *di,
+					  unsigned int cap, unsigned int energy,
+					  unsigned int voltage)
+{
+	int ret = bq27xxx_battery_i2c_read_dm_block(di,
+				BQ27XXX_GAS_GAUGING_STATE_SUBCLASS);
+
+	if (ret < 0)
+		return ret;
+
+	ret  = bq27xxx_battery_update_dm_setting(di, BQ27XXX_DM_DESIGN_CAP, cap);
+	ret |= bq27xxx_battery_update_dm_setting(di, BQ27XXX_DM_DESIGN_ENERGY,
+						 energy);
+	ret |= bq27xxx_battery_update_dm_setting(di, BQ27XXX_DM_TERMINATE_VOLTAGE,
+						 voltage);
+
+	if (ret) {
+		dev_info(di->dev, "updating NVM settings\n");
+		return bq27xxx_battery_i2c_write_nvram(di,
+				BQ27XXX_GAS_GAUGING_STATE_SUBCLASS);
+	}
+
+	return 0;
+}
+
+static int bq27xxx_battery_i2c_parse_dt(struct bq27xxx_device_info *di)
+{
+	struct device_node *np = di->dev->of_node;
+	int cap, energy, voltage = -EINVAL;
+	int ret = 0;
+
+	/* no settings to be set for this chipset so abort */
+	if (!bq27xxx_dm_subclass_regs[di->chip])
+		return 0;
+
+	bq27xxx_battery_i2c_set_seal_state(di, false);
+
+	if (np) {
+		ret = of_property_read_u32(np, "ti,design-microamp-hours", &cap);
+		if (ret < 0 || cap > 0x7fff) {
+			if (!ret)
+				dev_err(di->dev,
+					"invalid ti,design-microamp-hours %d\n",
+					cap);
+			cap = -EINVAL;
+		}
+
+		ret = of_property_read_u32(np, "ti,design-microwatt-hours",
+					   &energy);
+		if (ret < 0 || energy > 0x7fff) {
+			if (!ret)
+				dev_err(di->dev,
+					"invalid ti,design-microwatt-hours %d\n",
+					energy);
+			energy = -EINVAL;
+		}
+
+		ret = of_property_read_u32(np, "ti,terminate-microvolt", &voltage);
+		if (ret < 0 || voltage < BQ27XXX_TERM_V_MIN
+			    || voltage > BQ27XXX_TERM_V_MAX) {
+			if (!ret)
+				dev_err(di->dev,
+					"invalid ti,terminate-microvolt %d\n",
+					voltage);
+			voltage = -EINVAL;
+		}
+
+		/* assume that we want the defaults */
+		if (cap < 0 && energy < 0 && voltage < 0) {
+			ret = 0;
+			goto out;
+		}
+
+		/* we need all three settings for safety reasons */
+		if (cap < 0 || energy < 0 || voltage < 0) {
+			dev_err(di->dev,
+				"missing or invalid devicetree values; NVM not updated\n");
+			ret = -EINVAL;
+			goto out;
+		}
+
+		ret = bq27xxx_battery_i2c_set_config(di, cap, energy, voltage);
+	}
+
+out:
+	bq27xxx_battery_i2c_print_config(di);
+	bq27xxx_battery_i2c_set_seal_state(di, true);
+
+	return ret;
+}
+
 static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 				     const struct i2c_device_id *id)
 {
@@ -95,7 +422,15 @@ 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_i2c_parse_dt(di);
+	if (ret)
+		goto err_failed;
 
 	ret = bq27xxx_battery_setup(di);
 	if (ret)
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 14ecac158150..22b4cfc3acab 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -33,6 +33,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 {
@@ -62,6 +65,7 @@ struct bq27xxx_device_info {
 	struct power_supply *bat;
 	struct list_head list;
 	struct mutex lock;
+	u8 buffer[32];
 	u8 *regs;
 };
 
-- 
2.10.2

Re: [PATCH v2 3/3] power: bq27xxx: add support for NVRAM R/W access

[Sebastian Reichel]

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

On Wed, Jan 04, 2017 at 06:10:07PM -0800, Matt Ranostay wrote:
> Initial support for access and modification of the non-volatile regions
> of the bq27425 fuel gauge DesignEnergy, DesignCapacity, and
> TerminateVoltage settings.
> 
> This is intended for fine tuning the fuel gauge state machine for the
> respective battery specifications.
> 
> Cc: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
> ---
>  drivers/power/supply/bq27xxx_battery_i2c.c | 335 +++++++++++++++++++++++++++++
>  include/linux/power/bq27xxx_battery.h      |   4 +
>  2 files changed, 339 insertions(+)

I only skipped over this one, as the changed DT binding requires
quite some changes in this patch anyways. Here are a couple of
comments how I would like to see this implemented:

Add a patch for the power-supply core, which implements a
structure for the battery info. Something like this:

struct power_supply_battery_info {
    uint32 energy; /* µWh */
    uint32 power;  /* µAh */
    uint32 nominal_voltage; /* µV */
    /* ... */
};

And a function in the core framework, which gets the information
from DT. The function itself should *not* be DT specific, so that
ACPI/platformdata/whatever support can be added later without
modifying every single driver.

static struct power_supply_battery_info*
power_supply_get_battery_info(struct power_supply *psy) {
    if (psy->dt) {
        /* get battery phandle or return -ENXIO */
        /* fill and return struct */
    }

    return -ENOTSUP;
}

Then call power_supply_get_battery_info() during bq27xxx probe and
use the struct to initialize the registers. Also I expect that
functionality in bq27xxx_battery.c instead of bq27xxx_battery_i2c.c,
since it's not I²C specific.

-- Sebastian

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

Re: [PATCH v2 3/3] power: bq27xxx: add support for NVRAM R/W access

[Matt Ranostay]

On Thu, Jan 5, 2017 at 5:53 PM, Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Jan 04, 2017 at 06:10:07PM -0800, Matt Ranostay wrote:
>> Initial support for access and modification of the non-volatile regions
>> of the bq27425 fuel gauge DesignEnergy, DesignCapacity, and
>> TerminateVoltage settings.
>>
>> This is intended for fine tuning the fuel gauge state machine for the
>> respective battery specifications.
>>
>> Cc: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
>> ---
>>  drivers/power/supply/bq27xxx_battery_i2c.c | 335 +++++++++++++++++++++++++++++
>>  include/linux/power/bq27xxx_battery.h      |   4 +
>>  2 files changed, 339 insertions(+)
>
> I only skipped over this one, as the changed DT binding requires
> quite some changes in this patch anyways. Here are a couple of
> comments how I would like to see this implemented:
>
> Add a patch for the power-supply core, which implements a
> structure for the battery info. Something like this:
>
> struct power_supply_battery_info {
>     uint32 energy; /* µWh */
>     uint32 power;  /* µAh */
>     uint32 nominal_voltage; /* µV */
>     /* ... */
> };
>
> And a function in the core framework, which gets the information
> from DT. The function itself should *not* be DT specific, so that
> ACPI/platformdata/whatever support can be added later without
> modifying every single driver.
>
> static struct power_supply_battery_info*
> power_supply_get_battery_info(struct power_supply *psy) {
>     if (psy->dt) {
>         /* get battery phandle or return -ENXIO */
>         /* fill and return struct */
>     }
>
>     return -ENOTSUP;
> }
>
> Then call power_supply_get_battery_info() during bq27xxx probe and
> use the struct to initialize the registers. Also I expect that
> functionality in bq27xxx_battery.c instead of bq27xxx_battery_i2c.c,
> since it's not I²C specific.

They may not be i2c specific but they are only used by the i2c path of
the code currently. Do you think that platform data would ever have a
struct to pass with the respective data?

Also it would have to be in bq27xxx_battery_setup() and not
bq27xxx_battery_platform_probe() since the former is only thing called
by both code paths.


>
> -- Sebastian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] power: bq27xxx: add support for NVRAM R/W access

[Matt Ranostay]

On Thu, Jan 5, 2017 at 6:34 PM, Matt Ranostay <matt@ranostay.consulting> wrote:
> On Thu, Jan 5, 2017 at 5:53 PM, Sebastian Reichel <sre@kernel.org> wrote:
>> On Wed, Jan 04, 2017 at 06:10:07PM -0800, Matt Ranostay wrote:
>>> Initial support for access and modification of the non-volatile regions
>>> of the bq27425 fuel gauge DesignEnergy, DesignCapacity, and
>>> TerminateVoltage settings.
>>>
>>> This is intended for fine tuning the fuel gauge state machine for the
>>> respective battery specifications.
>>>
>>> Cc: Sebastian Reichel <sre@kernel.org>
>>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>>> ---
>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 335 +++++++++++++++++++++++++++++
>>>  include/linux/power/bq27xxx_battery.h      |   4 +
>>>  2 files changed, 339 insertions(+)
>>
>> I only skipped over this one, as the changed DT binding requires
>> quite some changes in this patch anyways. Here are a couple of
>> comments how I would like to see this implemented:
>>
>> Add a patch for the power-supply core, which implements a
>> structure for the battery info. Something like this:
>>
>> struct power_supply_battery_info {
>>     uint32 energy; /* µWh */
>>     uint32 power;  /* µAh */
>>     uint32 nominal_voltage; /* µV */
>>     /* ... */
>> };
>>
>> And a function in the core framework, which gets the information
>> from DT. The function itself should *not* be DT specific, so that
>> ACPI/platformdata/whatever support can be added later without
>> modifying every single driver.
>>
>> static struct power_supply_battery_info*
>> power_supply_get_battery_info(struct power_supply *psy) {
>>     if (psy->dt) {
>>         /* get battery phandle or return -ENXIO */
>>         /* fill and return struct */
>>     }
>>
>>     return -ENOTSUP;
>> }
>>
>> Then call power_supply_get_battery_info() during bq27xxx probe and
>> use the struct to initialize the registers. Also I expect that
>> functionality in bq27xxx_battery.c instead of bq27xxx_battery_i2c.c,
>> since it's not I²C specific.
>
> They may not be i2c specific but they are only used by the i2c path of
> the code currently. Do you think that platform data would ever have a
> struct to pass with the respective data?
>
> Also it would have to be in bq27xxx_battery_setup() and not
> bq27xxx_battery_platform_probe() since the former is only thing called
> by both code paths.

Also there is no bq27xxx_battery_platform_write to even write the
configuration data.

>
>
>>
>> -- Sebastian

Re: [PATCH v2 3/3] power: bq27xxx: add support for NVRAM R/W access

[Sebastian Reichel]

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

Hi,

On Thu, Jan 05, 2017 at 07:23:48PM -0800, Matt Ranostay wrote:
> On Thu, Jan 5, 2017 at 6:34 PM, Matt Ranostay <matt@ranostay.consulting> wrote:
> > On Thu, Jan 5, 2017 at 5:53 PM, Sebastian Reichel <sre@kernel.org> wrote:
> >> On Wed, Jan 04, 2017 at 06:10:07PM -0800, Matt Ranostay wrote:
> >>> Initial support for access and modification of the non-volatile regions
> >>> of the bq27425 fuel gauge DesignEnergy, DesignCapacity, and
> >>> TerminateVoltage settings.
> >>>
> >>> This is intended for fine tuning the fuel gauge state machine for the
> >>> respective battery specifications.
> >>>
> >>> Cc: Sebastian Reichel <sre@kernel.org>
> >>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> >>> ---
> >>>  drivers/power/supply/bq27xxx_battery_i2c.c | 335 +++++++++++++++++++++++++++++
> >>>  include/linux/power/bq27xxx_battery.h      |   4 +
> >>>  2 files changed, 339 insertions(+)
> >>
> >> I only skipped over this one, as the changed DT binding requires
> >> quite some changes in this patch anyways. Here are a couple of
> >> comments how I would like to see this implemented:
> >>
> >> Add a patch for the power-supply core, which implements a
> >> structure for the battery info. Something like this:
> >>
> >> struct power_supply_battery_info {
> >>     uint32 energy; /* µWh */
> >>     uint32 power;  /* µAh */
> >>     uint32 nominal_voltage; /* µV */
> >>     /* ... */
> >> };
> >>
> >> And a function in the core framework, which gets the information
> >> from DT. The function itself should *not* be DT specific, so that
> >> ACPI/platformdata/whatever support can be added later without
> >> modifying every single driver.
> >>
> >> static struct power_supply_battery_info*
> >> power_supply_get_battery_info(struct power_supply *psy) {
> >>     if (psy->dt) {
> >>         /* get battery phandle or return -ENXIO */
> >>         /* fill and return struct */
> >>     }
> >>
> >>     return -ENOTSUP;
> >> }
> >>
> >> Then call power_supply_get_battery_info() during bq27xxx probe and
> >> use the struct to initialize the registers. Also I expect that
> >> functionality in bq27xxx_battery.c instead of bq27xxx_battery_i2c.c,
> >> since it's not I²C specific.
> >
> > They may not be i2c specific but they are only used by the i2c path of
> > the code currently. Do you think that platform data would ever have a
> > struct to pass with the respective data?

I think it is possible, that there will be some non-i2c bq27xxx
device in the future, that needs this feature.

> > Also it would have to be in bq27xxx_battery_setup() and not
> > bq27xxx_battery_platform_probe() since the former is only thing called
> > by both code paths.

bq27xxx_battery_setup is fine.

> Also there is no bq27xxx_battery_platform_write to even write the
> configuration data.

Just check if the callback exists.

The plan is to convert the 1wire bq27xxx to regmap at some point, so
that the bus-specific bq27xxx files can be dropped by just using
regmap in the core bq27xxx driver.

-- Sebastian

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

Re: [PATCH v2 3/3] power: bq27xxx: add support for NVRAM R/W access

[kbuild test robot]

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

Hi Matt,

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.10-rc2 next-20170105]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matt-Ranostay/devicetree-property-units-add-mWh-and-mAh-units/20170106-112110
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: x86_64-randconfig-x008-201701 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> drivers/power/supply/bq27xxx_battery_i2c.c:64:3: error: 'BQ27425' undeclared here (not in a function)
     [BQ27425] = bq27425_dm_subclass_regs,
      ^~~~~~~
>> drivers/power/supply/bq27xxx_battery_i2c.c:64:3: error: array index in initializer not of integer type
   drivers/power/supply/bq27xxx_battery_i2c.c:64:3: note: (near initialization for 'bq27xxx_dm_subclass_regs')
   drivers/power/supply/bq27xxx_battery_i2c.c:68:3: error: array index in initializer not of integer type
     [BQ27425] = 0x04143672,
      ^~~~~~~
   drivers/power/supply/bq27xxx_battery_i2c.c:68:3: note: (near initialization for 'bq27xxx_unseal_keys')

vim +/BQ27425 +64 drivers/power/supply/bq27xxx_battery_i2c.c

    58		{ BQ27XXX_GAS_GAUGING_STATE_SUBCLASS, 12, "design-capacity" },
    59		{ BQ27XXX_GAS_GAUGING_STATE_SUBCLASS, 14, "design-energy" },
    60		{ BQ27XXX_GAS_GAUGING_STATE_SUBCLASS, 18, "terminate-voltage" },
    61	};
    62	
    63	static struct bq27xxx_dm_regs *bq27xxx_dm_subclass_regs[] = {
  > 64		[BQ27425] = bq27425_dm_subclass_regs,
    65	};
    66	
    67	static unsigned int bq27xxx_unseal_keys[] = {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27593 bytes --]