[PATCH 0/5] Improvements to bq27000 management on OMAP

[PATCH 0/5] Improvements to bq27000 management on OMAP

[NeilBrown]

[re-sent with multiple To: headers to placate vger.kernel.org]

This series makes the bq27000 battery monitor work well
when driven by the HDQ bus on OMAP devices.

I realise that there could be separate maintainers for each
file but figured that the series makes most sense as a whole
and am sending it as such.  If people could take or ACK the
patches relevant to them I would appreciate it.

The first 3 should be uncontroversial.

The fourth:
      omap_hdq: handle case where isr sees a 0 status byte.
fixes a problem that I have experienced, but I don't understand
why it happens.  As the commentary in that patch says, the interrupt
service routine sometimes sees the status as '0' meaning that nothing
caused an interrupt (Inconceivable!!).  This seems to happen in batches
resulting in timeouts that produce many-second delays in reading
battery status from /sys/class/bq27x00_battery/*.

The fifth:
      bq27x00 - don't report power-supply change so often.

Causes the driver to be less noisy to udev.  udev events should be
"uncommon" but the current code triggers a change event (almost) every
time the battery status is read, or every 6 minutes, as it is almost
certain that some value has changed (typically capacity but even
VOLTAGE_NOW can change a little bit).

Thanks,
NeilBrown

---

NeilBrown (5):
      bq27x00 - don't report power-supply change so often.
      omap_hdq: handle case where isr sees a 0 status byte.
      omap_hdq: use wait_event_timeout to wait for read to complete.
      omap_hdq: Fix some error/debug handling.
      Fix w1_bq27000


 drivers/power/bq27x00_battery.c |   15 ++++++++--
 drivers/w1/masters/omap_hdq.c   |   56 +++++++++++++++++++++++++++------------
 drivers/w1/slaves/w1_bq27000.c  |   17 ++++++++----
 3 files changed, 63 insertions(+), 25 deletions(-)

-- 
Signature

[PATCH 1/5] Fix w1_bq27000

[NeilBrown]

w1_bq27000 adds a bq27000-battery platform device but does not provide
platform data for it. This causes the bq27x00 driver to dereference a NULL
pointer.
So provide the appropriate platform data.  This requires modifying
w1_bq27000_read so that it find the w1 device as the parent of the bq device.

Also there is no point exporting w1_bq27000_read as nothing else uses it
or could use it.  So make it static.

Finally, as there is no way to track how many batteries have been found, and
we will probably only find one, use an id number of '-1' to assert that this
is a unique instance.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 drivers/w1/slaves/w1_bq27000.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/w1/slaves/w1_bq27000.c b/drivers/w1/slaves/w1_bq27000.c
index 8f4c91f..8f10fd2 100644
--- a/drivers/w1/slaves/w1_bq27000.c
+++ b/drivers/w1/slaves/w1_bq27000.c
@@ -15,6 +15,7 @@
 #include <linux/types.h>
 #include <linux/platform_device.h>
 #include <linux/mutex.h>
+#include <linux/power/bq27x00_battery.h>
 
 #include "../w1.h"
 #include "../w1_int.h"
@@ -39,10 +40,10 @@ void w1_bq27000_write(struct device *dev, u8 buf, u8 reg)
 }
 EXPORT_SYMBOL(w1_bq27000_write);
 
-int w1_bq27000_read(struct device *dev, u8 reg)
+static int w1_bq27000_read(struct device *dev, unsigned int reg)
 {
 	u8 val;
-	struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
+	struct w1_slave *sl = container_of(dev->parent, struct w1_slave, dev);
 
 	if (!dev)
 		return 0;
@@ -52,19 +53,25 @@ int w1_bq27000_read(struct device *dev, u8 reg)
 
 	return val;
 }
-EXPORT_SYMBOL(w1_bq27000_read);
+
+static struct bq27000_platform_data bq27000_battery_info = {
+	.read   = w1_bq27000_read,
+	.name   = "bq27000-battery",
+};
 
 static int w1_bq27000_add_slave(struct w1_slave *sl)
 {
 	int ret;
-	int id = 1;
 	struct platform_device *pdev;
 
-	pdev = platform_device_alloc("bq27000-battery", id);
+	pdev = platform_device_alloc("bq27000-battery", -1);
 	if (!pdev) {
 		ret = -ENOMEM;
 		return ret;
 	}
+	ret = platform_device_add_data(pdev,
+				       &bq27000_battery_info,
+				       sizeof(bq27000_battery_info));
 	pdev->dev.parent = &sl->dev;
 
 	ret = platform_device_add(pdev);

[PATCH 2/5] omap_hdq: Fix some error/debug handling.

[NeilBrown]

- some debug messages missed spaces
- sometimes no error was returned when it should have been
- sometimes a message is printed when there is no error, rather
  than when there is one.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 drivers/w1/masters/omap_hdq.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/w1/masters/omap_hdq.c b/drivers/w1/masters/omap_hdq.c
index 5ef385b..3036b61 100644
--- a/drivers/w1/masters/omap_hdq.c
+++ b/drivers/w1/masters/omap_hdq.c
@@ -180,6 +180,7 @@ static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status)
 		hdq_data->hdq_irqstatus, OMAP_HDQ_TIMEOUT);
 	if (ret == 0) {
 		dev_dbg(hdq_data->dev, "TX wait elapsed\n");
+		ret = -ETIMEDOUT;
 		goto out;
 	}
 
@@ -187,7 +188,7 @@ static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status)
 	/* check irqstatus */
 	if (!(*status & OMAP_HDQ_INT_STATUS_TXCOMPLETE)) {
 		dev_dbg(hdq_data->dev, "timeout waiting for"
-			"TXCOMPLETE/RXCOMPLETE, %x", *status);
+			" TXCOMPLETE/RXCOMPLETE, %x", *status);
 		ret = -ETIMEDOUT;
 		goto out;
 	}
@@ -198,7 +199,7 @@ static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status)
 			OMAP_HDQ_FLAG_CLEAR, &tmp_status);
 	if (ret) {
 		dev_dbg(hdq_data->dev, "timeout waiting GO bit"
-			"return to zero, %x", tmp_status);
+			" return to zero, %x", tmp_status);
 	}
 
 out:
@@ -341,7 +342,7 @@ static int omap_hdq_break(struct hdq_data *hdq_data)
 			&tmp_status);
 	if (ret)
 		dev_dbg(hdq_data->dev, "timeout waiting INIT&GO bits"
-			"return to zero, %x", tmp_status);
+			" return to zero, %x", tmp_status);
 
 out:
 	mutex_unlock(&hdq_data->hdq_mutex);
@@ -386,7 +387,7 @@ static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
 		/* check irqstatus */
 		if (!(status & OMAP_HDQ_INT_STATUS_RXCOMPLETE)) {
 			dev_dbg(hdq_data->dev, "timeout waiting for"
-				"RXCOMPLETE, %x", status);
+				" RXCOMPLETE, %x", status);
 			ret = -ETIMEDOUT;
 			goto out;
 		}
@@ -396,7 +397,7 @@ static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
 out:
 	mutex_unlock(&hdq_data->hdq_mutex);
 rtn:
-	return 0;
+	return ret;
 
 }
 
@@ -470,7 +471,7 @@ static int omap_hdq_put(struct hdq_data *hdq_data)
 
 	if (0 == hdq_data->hdq_usecount) {
 		dev_dbg(hdq_data->dev, "attempt to decrement use count"
-			"when it is zero");
+			" when it is zero");
 		ret = -EINVAL;
 	} else {
 		hdq_data->hdq_usecount--;
@@ -540,7 +541,7 @@ static void omap_w1_write_byte(void *_hdq, u8 byte)
 	mutex_unlock(&hdq_data->hdq_mutex);
 
 	ret = hdq_write_byte(hdq_data, byte, &status);
-	if (ret == 0) {
+	if (ret < 0) {
 		dev_dbg(hdq_data->dev, "TX failure:Ctrl status %x\n", status);
 		return;
 	}

[PATCH 4/5] omap_hdq: handle case where isr sees a 0 status byte.

[NeilBrown]

According to the documentation, when an isr happens one of the
3 bits in INT_STATUS must be set to say why.
But I sometimes see a value of zero.

The only explanation I can think of is that someone else reads the
register and so clears the bits.  But I cannot find that someone.

So until we do, intuit what must have caused the interrupt.
If we were trying to write, assume it completed.
If we were trying to read and go something other than 0xFF,
assume the read completed.
If we read 0xFF, assume the read timed-out.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 drivers/w1/masters/omap_hdq.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/drivers/w1/masters/omap_hdq.c b/drivers/w1/masters/omap_hdq.c
index 848399b..6b82469 100644
--- a/drivers/w1/masters/omap_hdq.c
+++ b/drivers/w1/masters/omap_hdq.c
@@ -211,9 +211,33 @@ static irqreturn_t hdq_isr(int irq, void *_hdq)
 {
 	struct hdq_data *hdq_data = _hdq;
 	unsigned long irqflags;
+	u8 status;
 
 	spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
-	hdq_data->hdq_irqstatus = hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
+	status = hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
+	if (!status) {
+		/* I sometimes see a zero status even though the read
+		 * seems to have been successful.  Very strange.
+		 * But we did get an interrupt, so figure out what
+		 * must have happened.
+		 */
+		u8 ctrl = hdq_reg_in(hdq_data, OMAP_HDQ_CTRL_STATUS);
+		dev_dbg(hdq_data->dev, "hdr_isr: INT=0!  CTRL=0x%02x\n", ctrl);
+		if (!(ctrl & OMAP_HDQ_CTRL_STATUS_GO)) {
+			/* It has definitely finished.. */
+			if (ctrl & OMAP_HDQ_CTRL_STATUS_DIR) {
+				/* A read finished or timed-out */
+				u8 data = hdq_reg_in(hdq_data,
+						      OMAP_HDQ_RX_DATA);
+				if (data == 0xFF)
+					status = OMAP_HDQ_INT_STATUS_TIMEOUT;
+				else
+					status = OMAP_HDQ_INT_STATUS_RXCOMPLETE;
+			} else
+				status = OMAP_HDQ_INT_STATUS_TXCOMPLETE;
+		}
+	}
+	hdq_data->hdq_irqstatus = status;
 	spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
 	dev_dbg(hdq_data->dev, "hdq_isr: %x", hdq_data->hdq_irqstatus);
 

[PATCH 3/5] omap_hdq: use wait_event_timeout to wait for read to complete.

[NeilBrown]

There is no gain in having a loop - there is no risk of missing the
interrupt with wait_event_timeout.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 drivers/w1/masters/omap_hdq.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/w1/masters/omap_hdq.c b/drivers/w1/masters/omap_hdq.c
index 3036b61..848399b 100644
--- a/drivers/w1/masters/omap_hdq.c
+++ b/drivers/w1/masters/omap_hdq.c
@@ -354,7 +354,6 @@ static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
 {
 	int ret = 0;
 	u8 status;
-	unsigned long timeout = jiffies + OMAP_HDQ_TIMEOUT;
 
 	ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
 	if (ret < 0) {
@@ -372,15 +371,13 @@ static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
 			OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO,
 			OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO);
 		/*
-		 * The RX comes immediately after TX. It
-		 * triggers another interrupt before we
-		 * sleep. So we have to wait for RXCOMPLETE bit.
+		 * The RX comes immediately after TX.
 		 */
-		while (!(hdq_data->hdq_irqstatus
-			& OMAP_HDQ_INT_STATUS_RXCOMPLETE)
-			&& time_before(jiffies, timeout)) {
-			schedule_timeout_uninterruptible(1);
-		}
+		wait_event_timeout(hdq_wait_queue,
+				   (hdq_data->hdq_irqstatus
+				    & OMAP_HDQ_INT_STATUS_RXCOMPLETE),
+				   OMAP_HDQ_TIMEOUT);
+
 		hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, 0,
 			OMAP_HDQ_CTRL_STATUS_DIR);
 		status = hdq_data->hdq_irqstatus;

[PATCH 5/5] bq27x00 - don't report power-supply change so often.

[NeilBrown]

A power_supply_changed should only be reported on significant changes
such as transition between charging and not.  Incremental changes
such as charge increasing should not be reported - that can easily
be polled for.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 drivers/power/bq27x00_battery.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/power/bq27x00_battery.c b/drivers/power/bq27x00_battery.c
index bb16f5b..7993a17 100644
--- a/drivers/power/bq27x00_battery.c
+++ b/drivers/power/bq27x00_battery.c
@@ -57,11 +57,15 @@
 #define BQ27000_FLAG_CHGS		BIT(7)
 #define BQ27000_FLAG_FC			BIT(5)
 
+#define BQ27000_FLAGS_IMPORTANT		(BQ27000_FLAG_FC|BQ27000_FLAG_CHGS|BIT(31))
+
 #define BQ27500_REG_SOC			0x2C
 #define BQ27500_REG_DCAP		0x3C /* Design capacity */
 #define BQ27500_FLAG_DSC		BIT(0)
 #define BQ27500_FLAG_FC			BIT(9)
 
+#define BQ27500_FLAGS_IMPORTANT		(BQ27500_FLAG_FC|BQ27500_FLAG_DSC|BIT(31))
+
 #define BQ27000_RS			20 /* Resistor sense */
 
 struct bq27x00_device_info;
@@ -259,6 +263,7 @@ static void bq27x00_update(struct bq27x00_device_info *di)
 {
 	struct bq27x00_reg_cache cache = {0, };
 	bool is_bq27500 = di->chip == BQ27500;
+	int flags_changed;
 
 	cache.flags = bq27x00_read(di, BQ27x00_REG_FLAGS, is_bq27500);
 	if (cache.flags >= 0) {
@@ -280,10 +285,14 @@ static void bq27x00_update(struct bq27x00_device_info *di)
 
 	/* Ignore current_now which is a snapshot of the current battery state
 	 * and is likely to be different even between two consecutive reads */
-	if (memcmp(&di->cache, &cache, sizeof(cache) - sizeof(int)) != 0) {
-		di->cache = cache;
+	flags_changed = di->cache.flags ^ cache.flags;
+	di->cache = cache;
+	if (is_bq27500)
+		flags_changed &= BQ27500_FLAGS_IMPORTANT;
+	else
+		flags_changed &= BQ27000_FLAGS_IMPORTANT;
+	if (flags_changed)
 		power_supply_changed(&di->bat);
-	}
 
 	di->last_update = jiffies;
 }

Re: [PATCH 5/5] bq27x00 - don't report power-supply change so often.

[Grazvydas Ignotas]

CCing Lars who added this. I vaguely recall something about generating
events to make some battery monitors update but I forget the details
now, maybe it was about something else. Also CCing Anton (the
maintainer).

On Fri, Dec 30, 2011 at 2:58 AM, NeilBrown <neilb@suse.de> wrote:
> A power_supply_changed should only be reported on significant changes
> such as transition between charging and not.  Incremental changes
> such as charge increasing should not be reported - that can easily
> be polled for.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>
>  drivers/power/bq27x00_battery.c |   15 ++++++++++++---
>  1 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/power/bq27x00_battery.c b/drivers/power/bq27x00_battery.c
> index bb16f5b..7993a17 100644
> --- a/drivers/power/bq27x00_battery.c
> +++ b/drivers/power/bq27x00_battery.c
> @@ -57,11 +57,15 @@
>  #define BQ27000_FLAG_CHGS              BIT(7)
>  #define BQ27000_FLAG_FC                        BIT(5)
>
> +#define BQ27000_FLAGS_IMPORTANT                (BQ27000_FLAG_FC|BQ27000_FLAG_CHGS|BIT(31))
> +
>  #define BQ27500_REG_SOC                        0x2C
>  #define BQ27500_REG_DCAP               0x3C /* Design capacity */
>  #define BQ27500_FLAG_DSC               BIT(0)
>  #define BQ27500_FLAG_FC                        BIT(9)
>
> +#define BQ27500_FLAGS_IMPORTANT                (BQ27500_FLAG_FC|BQ27500_FLAG_DSC|BIT(31))
> +
>  #define BQ27000_RS                     20 /* Resistor sense */
>
>  struct bq27x00_device_info;
> @@ -259,6 +263,7 @@ static void bq27x00_update(struct bq27x00_device_info *di)
>  {
>        struct bq27x00_reg_cache cache = {0, };
>        bool is_bq27500 = di->chip == BQ27500;
> +       int flags_changed;
>
>        cache.flags = bq27x00_read(di, BQ27x00_REG_FLAGS, is_bq27500);
>        if (cache.flags >= 0) {
> @@ -280,10 +285,14 @@ static void bq27x00_update(struct bq27x00_device_info *di)
>
>        /* Ignore current_now which is a snapshot of the current battery state
>         * and is likely to be different even between two consecutive reads */
> -       if (memcmp(&di->cache, &cache, sizeof(cache) - sizeof(int)) != 0) {
> -               di->cache = cache;
> +       flags_changed = di->cache.flags ^ cache.flags;
> +       di->cache = cache;
> +       if (is_bq27500)
> +               flags_changed &= BQ27500_FLAGS_IMPORTANT;
> +       else
> +               flags_changed &= BQ27000_FLAGS_IMPORTANT;
> +       if (flags_changed)
>                power_supply_changed(&di->bat);
> -       }
>
>        di->last_update = jiffies;
>  }
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Gražvydas

Re: [PATCH 5/5] bq27x00 - don't report power-supply change so often.

[Lars-Peter Clausen]

On 12/30/2011 12:13 PM, Grazvydas Ignotas wrote:
> CCing Lars who added this. I vaguely recall something about generating
> events to make some battery monitors update but I forget the details
> now, maybe it was about something else. Also CCing Anton (the
> maintainer).
> 
> On Fri, Dec 30, 2011 at 2:58 AM, NeilBrown <neilb@suse.de> wrote:
>> A power_supply_changed should only be reported on significant changes
>> such as transition between charging and not.  Incremental changes
>> such as charge increasing should not be reported - that can easily
>> be polled for.

Well, you can also poll for those "significant" changes, too. The point of
adding this was to have a centralized point where polling takes place instead
of letting each battery monitor do this on its own. Though if, as you wrote in
the cover letter, the some properties change every time the values are read it
might makes sense to exclude these from the comparison. On the other hand one
event every 6 minutes doesn't really sound harmful and I would suspect that
battery monitors will use a similar interval when manually polling the device.

- Lars

>>
>> Signed-off-by: NeilBrown <neilb@suse.de>
>> ---
>>
>>  drivers/power/bq27x00_battery.c |   15 ++++++++++++---
>>  1 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/power/bq27x00_battery.c b/drivers/power/bq27x00_battery.c
>> index bb16f5b..7993a17 100644
>> --- a/drivers/power/bq27x00_battery.c
>> +++ b/drivers/power/bq27x00_battery.c
>> @@ -57,11 +57,15 @@
>>  #define BQ27000_FLAG_CHGS              BIT(7)
>>  #define BQ27000_FLAG_FC                        BIT(5)
>>
>> +#define BQ27000_FLAGS_IMPORTANT                (BQ27000_FLAG_FC|BQ27000_FLAG_CHGS|BIT(31))
>> +
>>  #define BQ27500_REG_SOC                        0x2C
>>  #define BQ27500_REG_DCAP               0x3C /* Design capacity */
>>  #define BQ27500_FLAG_DSC               BIT(0)
>>  #define BQ27500_FLAG_FC                        BIT(9)
>>
>> +#define BQ27500_FLAGS_IMPORTANT                (BQ27500_FLAG_FC|BQ27500_FLAG_DSC|BIT(31))
>> +
>>  #define BQ27000_RS                     20 /* Resistor sense */
>>
>>  struct bq27x00_device_info;
>> @@ -259,6 +263,7 @@ static void bq27x00_update(struct bq27x00_device_info *di)
>>  {
>>        struct bq27x00_reg_cache cache = {0, };
>>        bool is_bq27500 = di->chip == BQ27500;
>> +       int flags_changed;
>>
>>        cache.flags = bq27x00_read(di, BQ27x00_REG_FLAGS, is_bq27500);
>>        if (cache.flags >= 0) {
>> @@ -280,10 +285,14 @@ static void bq27x00_update(struct bq27x00_device_info *di)
>>
>>        /* Ignore current_now which is a snapshot of the current battery state
>>         * and is likely to be different even between two consecutive reads */
>> -       if (memcmp(&di->cache, &cache, sizeof(cache) - sizeof(int)) != 0) {
>> -               di->cache = cache;
>> +       flags_changed = di->cache.flags ^ cache.flags;
>> +       di->cache = cache;
>> +       if (is_bq27500)
>> +               flags_changed &= BQ27500_FLAGS_IMPORTANT;
>> +       else
>> +               flags_changed &= BQ27000_FLAGS_IMPORTANT;
>> +       if (flags_changed)
>>                power_supply_changed(&di->bat);
>> -       }
>>
>>        di->last_update = jiffies;
>>  }

Re: [PATCH 5/5] bq27x00 - don't report power-supply change so often.

[NeilBrown]

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

On Sat, 31 Dec 2011 12:27:43 +0100 Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 12/30/2011 12:13 PM, Grazvydas Ignotas wrote:
> > CCing Lars who added this. I vaguely recall something about generating
> > events to make some battery monitors update but I forget the details
> > now, maybe it was about something else. Also CCing Anton (the
> > maintainer).
> > 
> > On Fri, Dec 30, 2011 at 2:58 AM, NeilBrown <neilb@suse.de> wrote:
> >> A power_supply_changed should only be reported on significant changes
> >> such as transition between charging and not.  Incremental changes
> >> such as charge increasing should not be reported - that can easily
> >> be polled for.
> 
> Well, you can also poll for those "significant" changes, too. The point of
> adding this was to have a centralized point where polling takes place instead
> of letting each battery monitor do this on its own. Though if, as you wrote in
> the cover letter, the some properties change every time the values are read it
> might makes sense to exclude these from the comparison. On the other hand one
> event every 6 minutes doesn't really sound harmful and I would suspect that
> battery monitors will use a similar interval when manually polling the device.

Hi,

 That is a very good point.  user-space could poll for all of the changes and
 polling the kernel provides no real benefit.

 I don't really see the problem with each battery monitor doing their own
 polling.  At least that way they would have control over when polling
 happens.

 Polling every 6 minutes does not really seem like a lot - except that
 polling at all in the kernel should be avoided.  If the system is idle and
 has managed to get into a lower power state, waking up just to check on the
 battery would be the wrong thing to do.

 A battery monitor could notice that no-one cared (e.g. A X11 client not
 getting any 'expose' events) and could stop polling.  The kernel cannot do
 that.

 However the part of the current code that really bothered me was that a
 'change' event is generated every time anyone polls the battery status - but
 at most every 5 seconds.  These extra change events really aren't wanted.

 I think we should always be cautious of adding change events.  They are
 not there just to report when any detail changed, else a 'keyboard' device
 would report a 'change' event on every keystroke.
 The main purpose of uevents are to report 'add' and 'remove' of devices.
 'change' is for situations where a device changes in a way that is very
 similar to an 'add' or a 'remove' but isn't implemented as a new device.
 A simple example is inserting a CD into a CD drive.  Before the device
 couldn't do anything useful, now it can.  It is a new medium, which is
 almost like a new device but just not implemented that way.  So it deserves a
 change event.
 Or when a power supply gets plugged in.  We really have a new thing here -
 we have added power.  But as the power isn't a device we cannot have an
 'add' uevent, so we have a 'change' uevent on the power supply.

 So I would be in favour of removing the in-kernel polling altogether.
 That puts complete control where it belongs: is the app that is monitoring
 the battery.

 Thoughts??

Thanks,
NeilBrown


> 
> - Lars
> 
> >>
> >> Signed-off-by: NeilBrown <neilb@suse.de>
> >> ---
> >>
> >>  drivers/power/bq27x00_battery.c |   15 ++++++++++++---
> >>  1 files changed, 12 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/power/bq27x00_battery.c b/drivers/power/bq27x00_battery.c
> >> index bb16f5b..7993a17 100644
> >> --- a/drivers/power/bq27x00_battery.c
> >> +++ b/drivers/power/bq27x00_battery.c
> >> @@ -57,11 +57,15 @@
> >>  #define BQ27000_FLAG_CHGS              BIT(7)
> >>  #define BQ27000_FLAG_FC                        BIT(5)
> >>
> >> +#define BQ27000_FLAGS_IMPORTANT                (BQ27000_FLAG_FC|BQ27000_FLAG_CHGS|BIT(31))
> >> +
> >>  #define BQ27500_REG_SOC                        0x2C
> >>  #define BQ27500_REG_DCAP               0x3C /* Design capacity */
> >>  #define BQ27500_FLAG_DSC               BIT(0)
> >>  #define BQ27500_FLAG_FC                        BIT(9)
> >>
> >> +#define BQ27500_FLAGS_IMPORTANT                (BQ27500_FLAG_FC|BQ27500_FLAG_DSC|BIT(31))
> >> +
> >>  #define BQ27000_RS                     20 /* Resistor sense */
> >>
> >>  struct bq27x00_device_info;
> >> @@ -259,6 +263,7 @@ static void bq27x00_update(struct bq27x00_device_info *di)
> >>  {
> >>        struct bq27x00_reg_cache cache = {0, };
> >>        bool is_bq27500 = di->chip == BQ27500;
> >> +       int flags_changed;
> >>
> >>        cache.flags = bq27x00_read(di, BQ27x00_REG_FLAGS, is_bq27500);
> >>        if (cache.flags >= 0) {
> >> @@ -280,10 +285,14 @@ static void bq27x00_update(struct bq27x00_device_info *di)
> >>
> >>        /* Ignore current_now which is a snapshot of the current battery state
> >>         * and is likely to be different even between two consecutive reads */
> >> -       if (memcmp(&di->cache, &cache, sizeof(cache) - sizeof(int)) != 0) {
> >> -               di->cache = cache;
> >> +       flags_changed = di->cache.flags ^ cache.flags;
> >> +       di->cache = cache;
> >> +       if (is_bq27500)
> >> +               flags_changed &= BQ27500_FLAGS_IMPORTANT;
> >> +       else
> >> +               flags_changed &= BQ27000_FLAGS_IMPORTANT;
> >> +       if (flags_changed)
> >>                power_supply_changed(&di->bat);
> >> -       }
> >>
> >>        di->last_update = jiffies;
> >>  }


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

Re: [PATCH 1/5] Fix w1_bq27000

[Thomas Weber]

Hello Neil,

On 30.12.2011 01:58, NeilBrown wrote:
> w1_bq27000 adds a bq27000-battery platform device but does not provide
> platform data for it. This causes the bq27x00 driver to dereference a NULL
> pointer.
> So provide the appropriate platform data.  This requires modifying
> w1_bq27000_read so that it find the w1 device as the parent of the bq device.
> 
> Also there is no point exporting w1_bq27000_read as nothing else uses it
> or could use it.  So make it static.
> 
> Finally, as there is no way to track how many batteries have been found, and
> we will probably only find one, use an id number of '-1' to assert that this
> is a unique instance.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> 
>  drivers/w1/slaves/w1_bq27000.c |   17 ++++++++++++-----
>  1 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/w1/slaves/w1_bq27000.c b/drivers/w1/slaves/w1_bq27000.c
> index 8f4c91f..8f10fd2 100644
> --- a/drivers/w1/slaves/w1_bq27000.c
> +++ b/drivers/w1/slaves/w1_bq27000.c
> @@ -15,6 +15,7 @@
>  #include <linux/types.h>
>  #include <linux/platform_device.h>
>  #include <linux/mutex.h>
> +#include <linux/power/bq27x00_battery.h>
>  
>  #include "../w1.h"
>  #include "../w1_int.h"
> @@ -39,10 +40,10 @@ void w1_bq27000_write(struct device *dev, u8 buf, u8 reg)
>  }
>  EXPORT_SYMBOL(w1_bq27000_write);
>  
> -int w1_bq27000_read(struct device *dev, u8 reg)
> +static int w1_bq27000_read(struct device *dev, unsigned int reg)
>  {
>  	u8 val;
> -	struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
> +	struct w1_slave *sl = container_of(dev->parent, struct w1_slave, dev);
>  
>  	if (!dev)
>  		return 0;
> @@ -52,19 +53,25 @@ int w1_bq27000_read(struct device *dev, u8 reg)
>  
>  	return val;
>  }
> -EXPORT_SYMBOL(w1_bq27000_read);
> +
> +static struct bq27000_platform_data bq27000_battery_info = {
> +	.read   = w1_bq27000_read,
> +	.name   = "bq27000-battery",
> +};
>  
>  static int w1_bq27000_add_slave(struct w1_slave *sl)
>  {
>  	int ret;
> -	int id = 1;
>  	struct platform_device *pdev;
>  
> -	pdev = platform_device_alloc("bq27000-battery", id);
> +	pdev = platform_device_alloc("bq27000-battery", -1);
>  	if (!pdev) {
>  		ret = -ENOMEM;
>  		return ret;
>  	}
> +	ret = platform_device_add_data(pdev,
> +				       &bq27000_battery_info,
> +				       sizeof(bq27000_battery_info));
>  	pdev->dev.parent = &sl->dev;
>  
>  	ret = platform_device_add(pdev);
> 
> 

Tested-by: Thomas Weber <weber@corscience.de>

Thanks for the patch, now the platform data of the bq27000 are found.

before:

omap_hdq omap_hdq.0: OMAP HDQ Hardware Rev 0.5. Driver in Interrupt mode
w1_master_driver w1_bus_master1: Family 1 for 01.000000000000.3d is not
registered.
bq27000-battery bq27000-battery.1: no platform_data supplied
bq27000-battery: probe of bq27000-battery.1 failed with error -22

after:

omap_hdq omap_hdq.0: OMAP HDQ Hardware Rev 0.5. Driver in Interrupt mode
w1_master_driver w1_bus_master1: Family 1 for 01.000000000000.3d is not
registered.
bq27000-battery bq27000-battery: support ver. 1.2.0 enabled

Regards,
Thomas

Re: [PATCH 1/5] Fix w1_bq27000

[NeilBrown]

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

On Wed, 15 Feb 2012 16:36:09 +0100 Thomas Weber <weber@corscience.de> wrote:

> Hello Neil,
> 
> On 30.12.2011 01:58, NeilBrown wrote:
> > w1_bq27000 adds a bq27000-battery platform device but does not provide
> > platform data for it. This causes the bq27x00 driver to dereference a NULL
> > pointer.
> > So provide the appropriate platform data.  This requires modifying
> > w1_bq27000_read so that it find the w1 device as the parent of the bq device.
> > 
> > Also there is no point exporting w1_bq27000_read as nothing else uses it
> > or could use it.  So make it static.
> > 
> > Finally, as there is no way to track how many batteries have been found, and
> > we will probably only find one, use an id number of '-1' to assert that this
> > is a unique instance.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> > 
> >  drivers/w1/slaves/w1_bq27000.c |   17 ++++++++++++-----
> >  1 files changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/w1/slaves/w1_bq27000.c b/drivers/w1/slaves/w1_bq27000.c
> > index 8f4c91f..8f10fd2 100644
> > --- a/drivers/w1/slaves/w1_bq27000.c
> > +++ b/drivers/w1/slaves/w1_bq27000.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/types.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/mutex.h>
> > +#include <linux/power/bq27x00_battery.h>
> >  
> >  #include "../w1.h"
> >  #include "../w1_int.h"
> > @@ -39,10 +40,10 @@ void w1_bq27000_write(struct device *dev, u8 buf, u8 reg)
> >  }
> >  EXPORT_SYMBOL(w1_bq27000_write);
> >  
> > -int w1_bq27000_read(struct device *dev, u8 reg)
> > +static int w1_bq27000_read(struct device *dev, unsigned int reg)
> >  {
> >  	u8 val;
> > -	struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
> > +	struct w1_slave *sl = container_of(dev->parent, struct w1_slave, dev);
> >  
> >  	if (!dev)
> >  		return 0;
> > @@ -52,19 +53,25 @@ int w1_bq27000_read(struct device *dev, u8 reg)
> >  
> >  	return val;
> >  }
> > -EXPORT_SYMBOL(w1_bq27000_read);
> > +
> > +static struct bq27000_platform_data bq27000_battery_info = {
> > +	.read   = w1_bq27000_read,
> > +	.name   = "bq27000-battery",
> > +};
> >  
> >  static int w1_bq27000_add_slave(struct w1_slave *sl)
> >  {
> >  	int ret;
> > -	int id = 1;
> >  	struct platform_device *pdev;
> >  
> > -	pdev = platform_device_alloc("bq27000-battery", id);
> > +	pdev = platform_device_alloc("bq27000-battery", -1);
> >  	if (!pdev) {
> >  		ret = -ENOMEM;
> >  		return ret;
> >  	}
> > +	ret = platform_device_add_data(pdev,
> > +				       &bq27000_battery_info,
> > +				       sizeof(bq27000_battery_info));
> >  	pdev->dev.parent = &sl->dev;
> >  
> >  	ret = platform_device_add(pdev);
> > 
> > 
> 
> Tested-by: Thomas Weber <weber@corscience.de>
> 
> Thanks for the patch, now the platform data of the bq27000 are found.
> 
> before:
> 
> omap_hdq omap_hdq.0: OMAP HDQ Hardware Rev 0.5. Driver in Interrupt mode
> w1_master_driver w1_bus_master1: Family 1 for 01.000000000000.3d is not
> registered.
> bq27000-battery bq27000-battery.1: no platform_data supplied
> bq27000-battery: probe of bq27000-battery.1 failed with error -22
> 
> after:
> 
> omap_hdq omap_hdq.0: OMAP HDQ Hardware Rev 0.5. Driver in Interrupt mode
> w1_master_driver w1_bus_master1: Family 1 for 01.000000000000.3d is not
> registered.
> bq27000-battery bq27000-battery: support ver. 1.2.0 enabled
> 

Hi Thomas,
 thanks for testing and forwarding to Greg - I appreciate it.

NeilBrown

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