| Message ID | 1433250883-32245-1-git-send-email-frans.klaver@xsens.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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__);
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(-)