[PATCH] sbs-battery: add option to always register battery

[PATCH] sbs-battery: add option to always register battery

[Frans Klaver]

Commit a22b41a31e53 ("sbs-battery: Probe should try talking to the
device") introduced a step in probing the SBS battery, that tries to
talk to the device before actually registering it, saying:

    this driver doesn't actually try talking to the device at probe
    time, so if it's incorrectly configured in the device tree or
    platform data (or if the battery has been removed from the system),
    then probe will succeed and every access will sit there and time
    out. The end result is a possibly laggy system that thinks it has a
    battery but can never read status, which isn't very useful.

Which is of course reasonable. However, it is also very well possible
for a device to boot up on wall-power and be connected to a battery
later on. The current advice in this situation is to probe the device
from userspace if you expect the battery to come on at some point in the
future. The downside of this approach is that userspace needs to be
aware of the backend of its powersupply, which is inconvenient and going
against the point of hardware abstraction.

In some of these cases you do want to register a battery, even if none
are attached at the moment. To facilitate this, add a configuration
option to try to talk to the device, defaulting to y, thus keeping the
current behavior. If unset, the battery will always be registered
without checking the sanity of the connection.

Signed-off-by: Frans Klaver <frans.klaver@xsens.com>
---
If there's a better place to arrange for this all to happen, or to make this
more common across power supplies, I'm perfectly happy to do that work instead.
For now this seems like the logical step to take, especially since using device
tree was (sensibly) shot down last september [0].

[0] https://lkml.org/lkml/2014/9/24/479

 drivers/power/Kconfig       | 8 ++++++++
 drivers/power/sbs-battery.c | 8 ++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 87f8cb01fbee..9cb74ce26629 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -157,6 +157,14 @@ config BATTERY_SBS
 	  Say Y to include support for SBS battery driver for SBS-compliant
 	  gas gauges.
 
+config BATTERY_SBS_TRY_PROBE
+	bool "Try communicating with SBS battery during probe"
+	depends on BATTERY_SBS
+	default y
+	help
+	  Say Y to try to communicate with the SBS battery during probe.
+	  Otherwise, always register the power supply.
+
 config BATTERY_BQ27x00
 	tristate "BQ27x00 battery driver"
 	depends on I2C || I2C=n
diff --git a/drivers/power/sbs-battery.c b/drivers/power/sbs-battery.c
index c7b7b4018df3..5582d5d49ab8 100644
--- a/drivers/power/sbs-battery.c
+++ b/drivers/power/sbs-battery.c
@@ -882,10 +882,14 @@ static int sbs_probe(struct i2c_client *client,
 
 skip_gpio:
 	/*
-	 * Before we register, we need to make sure we can actually talk
+	 * Before we register, we might need to make sure we can actually talk
 	 * to the battery.
 	 */
-	rc = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
+	if (IS_ENABLED(CONFIG_BATTERY_SBS_TRY_PROBE))
+		rc = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
+	else
+		rc = 0;
+
 	if (rc < 0) {
 		dev_err(&client->dev, "%s: Failed to get device status\n",
 			__func__);
-- 
2.3.4

Re: [PATCH] sbs-battery: add option to always register battery

[Sebastian Reichel]

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

Hi,

On Tue, Jun 02, 2015 at 03:14:43PM +0200, Frans Klaver wrote:
> Commit a22b41a31e53 ("sbs-battery: Probe should try talking to the
> device") introduced a step in probing the SBS battery, that tries to
> talk to the device before actually registering it, saying:
> 
>     this driver doesn't actually try talking to the device at probe
>     time, so if it's incorrectly configured in the device tree or
>     platform data (or if the battery has been removed from the system),
>     then probe will succeed and every access will sit there and time
>     out. The end result is a possibly laggy system that thinks it has a
>     battery but can never read status, which isn't very useful.
> 
> Which is of course reasonable. However, it is also very well possible
> for a device to boot up on wall-power and be connected to a battery
> later on. The current advice in this situation is to probe the device
> from userspace if you expect the battery to come on at some point in the
> future. The downside of this approach is that userspace needs to be
> aware of the backend of its powersupply, which is inconvenient and going
> against the point of hardware abstraction.
>
> In some of these cases you do want to register a battery, even if none
> are attached at the moment. To facilitate this, add a configuration
> option to try to talk to the device, defaulting to y, thus keeping the
> current behavior. If unset, the battery will always be registered
> without checking the sanity of the connection.

From my POV registering a power supply driver for an unconnected
chip is wrong.  It implies to the system, that there actually is
a battery connected.

Also how do you know, that a sbs based battery is connected at some
point and not some other battery (e.g. a bq27x00 based)? How does
the system know, that the battery becomes available?

Note, that userspace interaction does not mean missing hardware
abstraction. It means missing support for automatic device
identification. The same is true for serial connected bluetooth
devices.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] sbs-battery: add option to always register battery

[Frans Klaver]

On Tue, Jun 02, 2015 at 09:50:37PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Jun 02, 2015 at 03:14:43PM +0200, Frans Klaver wrote:
> >
> > In some of these cases you do want to register a battery, even if none
> > are attached at the moment. To facilitate this, add a configuration
> > option to try to talk to the device, defaulting to y, thus keeping the
> > current behavior. If unset, the battery will always be registered
> > without checking the sanity of the connection.
> 
> From my POV registering a power supply driver for an unconnected
> chip is wrong.  It implies to the system, that there actually is
> a battery connected.

I can see your point. The sbs-battery driver already is capable of
detecting whether or not the battery is present, so in a way the system
already knows there is no such battery. The communication test in probe
was added as a way to reduce lag (which our system doesn't suffer from),
and the optional and non-default behavior I propose isn't any different
from what the driver would have done before that patch.


> Also how do you know, that a sbs based battery is connected at some
> point and not some other battery (e.g. a bq27x00 based)?

We know because we specified so. Other batteries will simply not be
detected. Besides that, their controller might register to an i2c
address that is different from our current one, so I'm not sure what
you're aiming for here.

In a way we're implying to the system that a battery _could_ be
available. If there's going to be a battery, it's that one.

This is a situation that is currently easily triggered by booting up
with battery and removing the battery afterwards, by the way. So
behavior can be considered somewhat inconsistent there. How can we know
the sbs-battery will be attached again, rather than some other battery?

As I understand, i2c devices are expected to either be there or not be
there. A removable i2c battery doesn't really fit that expectation,
unless we can rely on the drivers ability to detect device presence,
which at least sbs-battery seems to be doing rather well. Besides that,
I already have to tell the kernel an sbs-battery may be available on
some i2c bus/address through device tree.


> How does the system know, that the battery becomes available?

The sbs-battery driver seems to detect that nicely. I don't know how
bq27x00 behaves, as I don't have that hardware available. That's why I
mentioned that I'd like to know if this should be something that affects
all power sources.


> Note, that userspace interaction does not mean missing hardware
> abstraction. It means missing support for automatic device
> identification. The same is true for serial connected bluetooth
> devices.

I'm not sure. User space interaction is quite broad. I would have much
less trouble accepting 'try to see if this battery is available', rather
than the current 'try to see if something is available on i2c bus 1
address 0x0b (that happens to be the battery that I've already
configured)'. I do not so much oppose the fact that userspace has to
tell when to try and probe. I do have a (conceptual) problem with
userspace having to know stuff that's already passed to the kernel by
different means.

Thanks,
Frans

Re: [PATCH] sbs-battery: add option to always register battery

[Sebastian Reichel]

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

Hi Frans,

On Tue, Jun 02, 2015 at 03:14:43PM +0200, Frans Klaver wrote:
> Commit a22b41a31e53 ("sbs-battery: Probe should try talking to the
> device") introduced a step in probing the SBS battery, that tries to
> talk to the device before actually registering it, saying:
> 
>     this driver doesn't actually try talking to the device at probe
>     time, so if it's incorrectly configured in the device tree or
>     platform data (or if the battery has been removed from the system),
>     then probe will succeed and every access will sit there and time
>     out. The end result is a possibly laggy system that thinks it has a
>     battery but can never read status, which isn't very useful.
> 
> Which is of course reasonable. However, it is also very well possible
> for a device to boot up on wall-power and be connected to a battery
> later on. The current advice in this situation is to probe the device
> from userspace if you expect the battery to come on at some point in the
> future. The downside of this approach is that userspace needs to be
> aware of the backend of its powersupply, which is inconvenient and going
> against the point of hardware abstraction.
> 
> In some of these cases you do want to register a battery, even if none
> are attached at the moment. To facilitate this, add a configuration
> option to try to talk to the device, defaulting to y, thus keeping the
> current behavior. If unset, the battery will always be registered
> without checking the sanity of the connection.
> 
> Signed-off-by: Frans Klaver <frans.klaver@xsens.com>
> ---
> If there's a better place to arrange for this all to happen, or to make this
> more common across power supplies, I'm perfectly happy to do that work instead.
> For now this seems like the logical step to take, especially since using device
> tree was (sensibly) shot down last september [0].

While I still think, that the HW design is bad, I'm basically fine
with this change based upon your comments. I think it's better to
make this into a module parameter, though, since that moves the
decision about this feature from compilation time to module load
time. This will make it possible to use a generic kernel on your
device. Maybe something like this could be used:

module_param(force_load, bool, 0444);
MODULE_PARM_DESC(force_load,
		 "Attempts to load the driver even if the "
		 "battery is not connected");

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] sbs-battery: add option to always register battery

[Frans Klaver]

On Wed, Jun 03, 2015 at 03:57:51PM +0200, Sebastian Reichel wrote:
> Hi Frans,
> 
> On Tue, Jun 02, 2015 at 03:14:43PM +0200, Frans Klaver wrote:
> > Commit a22b41a31e53 ("sbs-battery: Probe should try talking to the
> > device") introduced a step in probing the SBS battery, that tries to
> > talk to the device before actually registering it, saying:
> > 
> >     this driver doesn't actually try talking to the device at probe
> >     time, so if it's incorrectly configured in the device tree or
> >     platform data (or if the battery has been removed from the system),
> >     then probe will succeed and every access will sit there and time
> >     out. The end result is a possibly laggy system that thinks it has a
> >     battery but can never read status, which isn't very useful.
> > 
> > Which is of course reasonable. However, it is also very well possible
> > for a device to boot up on wall-power and be connected to a battery
> > later on. The current advice in this situation is to probe the device
> > from userspace if you expect the battery to come on at some point in the
> > future. The downside of this approach is that userspace needs to be
> > aware of the backend of its powersupply, which is inconvenient and going
> > against the point of hardware abstraction.
> > 
> > In some of these cases you do want to register a battery, even if none
> > are attached at the moment. To facilitate this, add a configuration
> > option to try to talk to the device, defaulting to y, thus keeping the
> > current behavior. If unset, the battery will always be registered
> > without checking the sanity of the connection.
> > 
> > Signed-off-by: Frans Klaver <frans.klaver@xsens.com>
> > ---
> > If there's a better place to arrange for this all to happen, or to make this
> > more common across power supplies, I'm perfectly happy to do that work instead.
> > For now this seems like the logical step to take, especially since using device
> > tree was (sensibly) shot down last september [0].
> 
> While I still think, that the HW design is bad,

I'm still interested in learning how we could improve the HW design in
your opinion. Would you say we should be using a non-removable battery?


> I'm basically fine
> with this change based upon your comments. I think it's better to
> make this into a module parameter, though, since that moves the
> decision about this feature from compilation time to module load
> time. This will make it possible to use a generic kernel on your
> device. Maybe something like this could be used:
> 
> module_param(force_load, bool, 0444);
> MODULE_PARM_DESC(force_load,
> 		 "Attempts to load the driver even if the "
> 		 "battery is not connected");

That makes sense. We can work with that.

Thanks,
Frans

Re: [PATCH] sbs-battery: add option to always register battery

[Sebastian Reichel]

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

Hi,

On Wed, Jun 03, 2015 at 04:10:35PM +0200, Frans Klaver wrote:
> > While I still think, that the HW design is bad,
> 
> I'm still interested in learning how we could improve the HW design in
> your opinion. Would you say we should be using a non-removable battery?

No I would say the battery should be able to identifiy itself. So if
the device is not connected, there is no battery device and when you
connect it the battery is registered. This could be done for example
using Device Tree overlays. iirc something like that is planned for
beagle bone capes.

> > I'm basically fine
> > with this change based upon your comments. I think it's better to
> > make this into a module parameter, though, since that moves the
> > decision about this feature from compilation time to module load
> > time. This will make it possible to use a generic kernel on your
> > device. Maybe something like this could be used:
> > 
> > module_param(force_load, bool, 0444);
> > MODULE_PARM_DESC(force_load,
> > 		 "Attempts to load the driver even if the "
> > 		 "battery is not connected");
> 
> That makes sense. We can work with that.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]