[RFC PATCH 0/2] explicitly instantiate battery from device tree

[RFC PATCH 0/2] explicitly instantiate battery from device tree

[Frans Klaver]

Hi there,

Here's some patches that make it possible to instantiate an sbs battery without
it being present yet.

i2c devices are more or less presumed to be persistent. This is a fair
assumption, except for the case of batteries. It is perfectly sensible for a
device to boot up using wall power, but no battery attached. While later
attaching a battery and dropping the wall power. Detecting the device from
userspace is tedious requires you to scan for device presence yourself, or you
can ask the kernel to probe again, cluttering your logs with failed probes.

I'd like to hear what you think about this.

Thanks,
Frans

Frans Klaver (2):
  sbs-battery: add forced instantiation from device tree
  sbs-battery: dts: document always-present property

 .../devicetree/bindings/power_supply/sbs_sbs-battery.txt      |  2 ++
 drivers/power/sbs-battery.c                                   | 11 +++++++++--
 include/linux/power/sbs-battery.h                             |  3 +++
 4 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.1.0

[RFC PATCH 1/2] sbs-battery: add forced instantiation from device tree

[Frans Klaver]

In some cases you want to instantiate a battery even before it is
attached; it is perfectly reasonable for a device to start up on
wall-power and be connected to a battery later. The current advice is to
instantiate a device explicitly in the kernel, or probe for the device
from userspace. The downside of these approaches is that the user needs
to keep the information related to the i2c battery in different places,
which is inconvenient.

Add the "sbs,always-present" option to the device tree. If set, the
battery is instantiated without sanity checking the connection.

Signed-off-by: Frans Klaver <frans.klaver@xsens.com>
---
 drivers/power/sbs-battery.c       | 11 +++++++++--
 include/linux/power/sbs-battery.h |  3 +++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/power/sbs-battery.c b/drivers/power/sbs-battery.c
index b5f2a76..5d327b5 100644
--- a/drivers/power/sbs-battery.c
+++ b/drivers/power/sbs-battery.c
@@ -651,6 +651,9 @@ static struct sbs_platform_data *sbs_of_populate_pdata(
 	if (!rc)
 		pdata->poll_retry_count = prop;
 
+	pdata->always_present = of_property_read_bool(of_node,
+						      "sbs,always-present");
+
 	if (!of_get_property(of_node, "sbs,battery-detect-gpios", NULL)) {
 		pdata->battery_detect = -1;
 		goto of_out;
@@ -761,10 +764,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 (!pdata->always_present)
+		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__);
diff --git a/include/linux/power/sbs-battery.h b/include/linux/power/sbs-battery.h
index 2b0a9d9..724bd9b 100644
--- a/include/linux/power/sbs-battery.h
+++ b/include/linux/power/sbs-battery.h
@@ -31,12 +31,15 @@
  * @i2c_retry_count:		# of times to retry on i2c IO failure
  * @poll_retry_count:		# of times to retry looking for new status after
  *				external change notification
+ * @always_present:		the device is instantiated even if the battery
+ *				is not physically present
  */
 struct sbs_platform_data {
 	int battery_detect;
 	int battery_detect_present;
 	int i2c_retry_count;
 	int poll_retry_count;
+	bool always_present;
 };
 
 #endif
-- 
2.1.0

[PATCH 2/2] sbs-battery: dts: document always-present property

[Frans Klaver]

Document the sbs,always-present property of sbs batteries.

Signed-off-by: Frans Klaver <frans.klaver@xsens.com>
---
 Documentation/devicetree/bindings/power_supply/sbs_sbs-battery.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/power_supply/sbs_sbs-battery.txt b/Documentation/devicetree/bindings/power_supply/sbs_sbs-battery.txt
index c40e892..05bf09c 100644
--- a/Documentation/devicetree/bindings/power_supply/sbs_sbs-battery.txt
+++ b/Documentation/devicetree/bindings/power_supply/sbs_sbs-battery.txt
@@ -11,6 +11,8 @@ Optional properties :
    after an external change notification.
  - sbs,battery-detect-gpios : The gpio which signals battery detection and
    a flag specifying its polarity.
+ - sbs,always-present : If set, the battery is always registered even if
+   status cannot be read during probe
 
 Example:
 
-- 
2.1.0

Re: [RFC PATCH 1/2] sbs-battery: add forced instantiation from device tree

[Mark Rutland]

On Wed, Sep 24, 2014 at 02:11:17PM +0100, Frans Klaver wrote:
> In some cases you want to instantiate a battery even before it is
> attached; it is perfectly reasonable for a device to start up on
> wall-power and be connected to a battery later. The current advice is to
> instantiate a device explicitly in the kernel, or probe for the device
> from userspace. The downside of these approaches is that the user needs
> to keep the information related to the i2c battery in different places,
> which is inconvenient.

This really sounds like a Linux policy issue rather than something that
should be described in dt.

Presumably there's a reason we sanity cehck this in the first place.
What happens while the battery isn't plugged in? What can fail, and how?

Mark.

> Add the "sbs,always-present" option to the device tree. If set, the
> battery is instantiated without sanity checking the connection.

>From the description above, this name is incorrect. You're adding this
property to work around the battery _not_ being present at probe-time.
>From a binding point of view, "instantiated" is somewhat meaningless --
that's an OS level detail rather than a contract detail.

Mark.

> 
> Signed-off-by: Frans Klaver <frans.klaver@xsens.com>
> ---
>  drivers/power/sbs-battery.c       | 11 +++++++++--
>  include/linux/power/sbs-battery.h |  3 +++
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/sbs-battery.c b/drivers/power/sbs-battery.c
> index b5f2a76..5d327b5 100644
> --- a/drivers/power/sbs-battery.c
> +++ b/drivers/power/sbs-battery.c
> @@ -651,6 +651,9 @@ static struct sbs_platform_data *sbs_of_populate_pdata(
>  	if (!rc)
>  		pdata->poll_retry_count = prop;
>  
> +	pdata->always_present = of_property_read_bool(of_node,
> +						      "sbs,always-present");
> +
>  	if (!of_get_property(of_node, "sbs,battery-detect-gpios", NULL)) {
>  		pdata->battery_detect = -1;
>  		goto of_out;
> @@ -761,10 +764,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 (!pdata->always_present)
> +		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__);
> diff --git a/include/linux/power/sbs-battery.h b/include/linux/power/sbs-battery.h
> index 2b0a9d9..724bd9b 100644
> --- a/include/linux/power/sbs-battery.h
> +++ b/include/linux/power/sbs-battery.h
> @@ -31,12 +31,15 @@
>   * @i2c_retry_count:		# of times to retry on i2c IO failure
>   * @poll_retry_count:		# of times to retry looking for new status after
>   *				external change notification
> + * @always_present:		the device is instantiated even if the battery
> + *				is not physically present
>   */
>  struct sbs_platform_data {
>  	int battery_detect;
>  	int battery_detect_present;
>  	int i2c_retry_count;
>  	int poll_retry_count;
> +	bool always_present;
>  };
>  
>  #endif
> -- 
> 2.1.0
> 
> 

Re: [RFC PATCH 1/2] sbs-battery: add forced instantiation from device tree

[Frans Klaver]

On Wed, Sep 24, 2014 at 02:16:55PM +0100, Mark Rutland wrote:
> On Wed, Sep 24, 2014 at 02:11:17PM +0100, Frans Klaver wrote:
> > In some cases you want to instantiate a battery even before it is
> > attached; it is perfectly reasonable for a device to start up on
> > wall-power and be connected to a battery later. The current advice is to
> > instantiate a device explicitly in the kernel, or probe for the device
> > from userspace. The downside of these approaches is that the user needs
> > to keep the information related to the i2c battery in different places,
> > which is inconvenient.
> 
> This really sounds like a Linux policy issue rather than something that
> should be described in dt.
> 
> Presumably there's a reason we sanity cehck this in the first place.
> What happens while the battery isn't plugged in? What can fail, and how?

It was introduced in a22b41a31e53 "sbs-battery: Probe should try talking
to the device", 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."

That's a reasonable thing to do, but it breaks just the feature I need.
Besides that, the driver provides us with a gpio that indicates battery
presence, which will also be useless if the device isn't present at
probe time. That commit also doesn't take into account the fact that a
battery could be removed after probing without any problems, leaving the
system in the same state as before the probe change.

Now if the battery isn't plugged in, it is never detected after it has
been attached, unless you take action from userspace. Basically you
don't know your battery level until it has been explicitly probed.

We might also reduce the severity of the sanity check failure to produce
a warning instead of an error. This would achieve that a developer might
be warned that the battery isn't present, but also allow my use case
where the battery may not be present at boot time. Was that what you
meant with policy by the way?

> 
> > Add the "sbs,always-present" option to the device tree. If set, the
> > battery is instantiated without sanity checking the connection.
> 
> From the description above, this name is incorrect. You're adding this
> property to work around the battery _not_ being present at probe-time.
> From a binding point of view, "instantiated" is somewhat meaningless --
> that's an OS level detail rather than a contract detail.

That's fair. I wasn't entirely sure of the name myself, so feel free to
suggest some other names. It started out with "sbs,force-presence", but
I would be just as happy using "sbs,always-register",
"sbs,no-sanity-check" or something entirely different altogether.

If we go with the downgrade of the status read error, the device tree
requirement will be removed entirely.

Thanks,
Frans

Re: [RFC PATCH 1/2] sbs-battery: add forced instantiation from device tree

[Mark Rutland]

On Wed, Sep 24, 2014 at 03:22:22PM +0100, Frans Klaver wrote:
> On Wed, Sep 24, 2014 at 02:16:55PM +0100, Mark Rutland wrote:
> > On Wed, Sep 24, 2014 at 02:11:17PM +0100, Frans Klaver wrote:
> > > In some cases you want to instantiate a battery even before it is
> > > attached; it is perfectly reasonable for a device to start up on
> > > wall-power and be connected to a battery later. The current advice is to
> > > instantiate a device explicitly in the kernel, or probe for the device
> > > from userspace. The downside of these approaches is that the user needs
> > > to keep the information related to the i2c battery in different places,
> > > which is inconvenient.
> > 
> > This really sounds like a Linux policy issue rather than something that
> > should be described in dt.
> > 
> > Presumably there's a reason we sanity cehck this in the first place.
> > What happens while the battery isn't plugged in? What can fail, and how?
> 
> It was introduced in a22b41a31e53 "sbs-battery: Probe should try talking
> to the device", 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."
> 
> That's a reasonable thing to do, but it breaks just the feature I need.
> Besides that, the driver provides us with a gpio that indicates battery
> presence, which will also be useless if the device isn't present at
> probe time. That commit also doesn't take into account the fact that a
> battery could be removed after probing without any problems, leaving the
> system in the same state as before the probe change.
> 
> Now if the battery isn't plugged in, it is never detected after it has
> been attached, unless you take action from userspace. Basically you
> don't know your battery level until it has been explicitly probed.
> 
> We might also reduce the severity of the sanity check failure to produce
> a warning instead of an error. This would achieve that a developer might
> be warned that the battery isn't present, but also allow my use case
> where the battery may not be present at boot time. Was that what you
> meant with policy by the way?

In general, properties in general shouldn't tell the kernel what to do.
They should tell the kernel details of the hardware that it can then use
to make informed decisions. In this case, the suggested property is
purely a software detail, as the hardware isn't any different in
situations you would or would not want the property.

You mention that there's a GPIO that can be used to detect the battery
presence. Why can't the driver always probe and then on check for the
presence of the battery dynamically using that GPIO? That should cover
both cases.

Mark.

Re: [RFC PATCH 1/2] sbs-battery: add forced instantiation from device tree

[Frans Klaver]

On Wed, Sep 24, 2014 at 03:38:49PM +0100, Mark Rutland wrote:
> On Wed, Sep 24, 2014 at 03:22:22PM +0100, Frans Klaver wrote:
> > On Wed, Sep 24, 2014 at 02:16:55PM +0100, Mark Rutland wrote:
> > > On Wed, Sep 24, 2014 at 02:11:17PM +0100, Frans Klaver wrote:
> > > > In some cases you want to instantiate a battery even before it is
> > > > attached; it is perfectly reasonable for a device to start up on
> > > > wall-power and be connected to a battery later. The current advice is to
> > > > instantiate a device explicitly in the kernel, or probe for the device
> > > > from userspace. The downside of these approaches is that the user needs
> > > > to keep the information related to the i2c battery in different places,
> > > > which is inconvenient.
> > > 
> > > This really sounds like a Linux policy issue rather than something that
> > > should be described in dt.
> > > 
> > > Presumably there's a reason we sanity cehck this in the first place.
> > > What happens while the battery isn't plugged in? What can fail, and how?
> > 
> > It was introduced in a22b41a31e53 "sbs-battery: Probe should try talking
> > to the device", 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."
> > 
> > That's a reasonable thing to do, but it breaks just the feature I need.
> > Besides that, the driver provides us with a gpio that indicates battery
> > presence, which will also be useless if the device isn't present at
> > probe time. That commit also doesn't take into account the fact that a
> > battery could be removed after probing without any problems, leaving the
> > system in the same state as before the probe change.
> > 
> > Now if the battery isn't plugged in, it is never detected after it has
> > been attached, unless you take action from userspace. Basically you
> > don't know your battery level until it has been explicitly probed.
> > 
> > We might also reduce the severity of the sanity check failure to produce
> > a warning instead of an error. This would achieve that a developer might
> > be warned that the battery isn't present, but also allow my use case
> > where the battery may not be present at boot time. Was that what you
> > meant with policy by the way?
> 
> In general, properties in general shouldn't tell the kernel what to do.
> They should tell the kernel details of the hardware that it can then use
> to make informed decisions. In this case, the suggested property is
> purely a software detail, as the hardware isn't any different in
> situations you would or would not want the property.

Ok, that makes sense.

> You mention that there's a GPIO that can be used to detect the battery
> presence. Why can't the driver always probe and then on check for the
> presence of the battery dynamically using that GPIO? That should cover
> both cases.

I would say that this was the case before [1] was done. The GPIO is
optional and if not configured, the presence or absence of the battery
is detected by checking a status register much like probe() currently
does. It seems all cases were covered before that patch. If you worry
about speed, you should use the GPIO. I wonder if we might be able to
revert [1] without doing much harm.

Thanks,
Frans

[1] a22b41a31e53 "sbs-battery: Probe should try talking to the device"

Re: [RFC PATCH 1/2] sbs-battery: add forced instantiation from device tree

[Mark Rutland]

On Wed, Sep 24, 2014 at 04:14:48PM +0100, Frans Klaver wrote:
> On Wed, Sep 24, 2014 at 03:38:49PM +0100, Mark Rutland wrote:
> > On Wed, Sep 24, 2014 at 03:22:22PM +0100, Frans Klaver wrote:
> > > On Wed, Sep 24, 2014 at 02:16:55PM +0100, Mark Rutland wrote:
> > > > On Wed, Sep 24, 2014 at 02:11:17PM +0100, Frans Klaver wrote:
> > > > > In some cases you want to instantiate a battery even before it is
> > > > > attached; it is perfectly reasonable for a device to start up on
> > > > > wall-power and be connected to a battery later. The current advice is to
> > > > > instantiate a device explicitly in the kernel, or probe for the device
> > > > > from userspace. The downside of these approaches is that the user needs
> > > > > to keep the information related to the i2c battery in different places,
> > > > > which is inconvenient.
> > > > 
> > > > This really sounds like a Linux policy issue rather than something that
> > > > should be described in dt.
> > > > 
> > > > Presumably there's a reason we sanity cehck this in the first place.
> > > > What happens while the battery isn't plugged in? What can fail, and how?
> > > 
> > > It was introduced in a22b41a31e53 "sbs-battery: Probe should try talking
> > > to the device", 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."
> > > 
> > > That's a reasonable thing to do, but it breaks just the feature I need.
> > > Besides that, the driver provides us with a gpio that indicates battery
> > > presence, which will also be useless if the device isn't present at
> > > probe time. That commit also doesn't take into account the fact that a
> > > battery could be removed after probing without any problems, leaving the
> > > system in the same state as before the probe change.
> > > 
> > > Now if the battery isn't plugged in, it is never detected after it has
> > > been attached, unless you take action from userspace. Basically you
> > > don't know your battery level until it has been explicitly probed.
> > > 
> > > We might also reduce the severity of the sanity check failure to produce
> > > a warning instead of an error. This would achieve that a developer might
> > > be warned that the battery isn't present, but also allow my use case
> > > where the battery may not be present at boot time. Was that what you
> > > meant with policy by the way?
> > 
> > In general, properties in general shouldn't tell the kernel what to do.
> > They should tell the kernel details of the hardware that it can then use
> > to make informed decisions. In this case, the suggested property is
> > purely a software detail, as the hardware isn't any different in
> > situations you would or would not want the property.
> 
> Ok, that makes sense.
> 
> > You mention that there's a GPIO that can be used to detect the battery
> > presence. Why can't the driver always probe and then on check for the
> > presence of the battery dynamically using that GPIO? That should cover
> > both cases.
> 
> I would say that this was the case before [1] was done. The GPIO is
> optional and if not configured, the presence or absence of the battery
> is detected by checking a status register much like probe() currently
> does. It seems all cases were covered before that patch. If you worry
> about speed, you should use the GPIO. I wonder if we might be able to
> revert [1] without doing much harm.

But reverting that would re-introduce the lag on some systems, no? Given
the wording of the original commit I would guess that the GPIO wasn't
available. Perhaps Olof or Anton can enlighten us?

In the cases where a GPIO is available, I think we should be able to be
less pessimistic. Is a GPIO available in your case?

Mark.

> 
> Thanks,
> Frans
> 
> [1] a22b41a31e53 "sbs-battery: Probe should try talking to the device"
> 

Re: [RFC PATCH 1/2] sbs-battery: add forced instantiation from device tree

[Frans Klaver]

On Wed, Sep 24, 2014 at 04:34:32PM +0100, Mark Rutland wrote:
> On Wed, Sep 24, 2014 at 04:14:48PM +0100, Frans Klaver wrote:
> > On Wed, Sep 24, 2014 at 03:38:49PM +0100, Mark Rutland wrote:
> > > On Wed, Sep 24, 2014 at 03:22:22PM +0100, Frans Klaver wrote:
> > > You mention that there's a GPIO that can be used to detect the battery
> > > presence. Why can't the driver always probe and then on check for the
> > > presence of the battery dynamically using that GPIO? That should cover
> > > both cases.
> > 
> > I would say that this was the case before [1] was done. The GPIO is
> > optional and if not configured, the presence or absence of the battery
> > is detected by checking a status register much like probe() currently
> > does. It seems all cases were covered before that patch. If you worry
> > about speed, you should use the GPIO. I wonder if we might be able to
> > revert [1] without doing much harm.
> 
> But reverting that would re-introduce the lag on some systems, no? Given
> the wording of the original commit I would guess that the GPIO wasn't
> available. Perhaps Olof or Anton can enlighten us?

It probably would yes. The battery_detect gpio was last touched in 2011, the
probe check was added somewhere in 2012.

We could keep it as a compile option.

> In the cases where a GPIO is available, I think we should be able to be
> less pessimistic. Is a GPIO available in your case?

We don't have the battery_detect pin available. Incidentally, a bit of
lag reading out the battery is not a problem for us.

> Mark.
> 
> > 
> > Thanks,
> > Frans
> > 
> > [1] a22b41a31e53 "sbs-battery: Probe should try talking to the device"
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/2] sbs-battery: add forced instantiation from device tree

[Mark Rutland]

On Wed, Sep 24, 2014 at 07:59:34PM +0100, Frans Klaver wrote:
> On Wed, Sep 24, 2014 at 04:34:32PM +0100, Mark Rutland wrote:
> > On Wed, Sep 24, 2014 at 04:14:48PM +0100, Frans Klaver wrote:
> > > On Wed, Sep 24, 2014 at 03:38:49PM +0100, Mark Rutland wrote:
> > > > On Wed, Sep 24, 2014 at 03:22:22PM +0100, Frans Klaver wrote:
> > > > You mention that there's a GPIO that can be used to detect the battery
> > > > presence. Why can't the driver always probe and then on check for the
> > > > presence of the battery dynamically using that GPIO? That should cover
> > > > both cases.
> > > 
> > > I would say that this was the case before [1] was done. The GPIO is
> > > optional and if not configured, the presence or absence of the battery
> > > is detected by checking a status register much like probe() currently
> > > does. It seems all cases were covered before that patch. If you worry
> > > about speed, you should use the GPIO. I wonder if we might be able to
> > > revert [1] without doing much harm.
> > 
> > But reverting that would re-introduce the lag on some systems, no? Given
> > the wording of the original commit I would guess that the GPIO wasn't
> > available. Perhaps Olof or Anton can enlighten us?
> 
> It probably would yes. The battery_detect gpio was last touched in 2011, the
> probe check was added somewhere in 2012.

We can't revert it unless we know doing so won't reintroduce the
problem. From the above it sounds like we can't revert it.

> We could keep it as a compile option.

Perhaps.

> > In the cases where a GPIO is available, I think we should be able to be
> > less pessimistic. Is a GPIO available in your case?
> 
> We don't have the battery_detect pin available. Incidentally, a bit of
> lag reading out the battery is not a problem for us.

So now we're back at sqaure one. The hardware is likely identical in the
your case and the care-about-lag case. Whether or not you care about lag
is a property of the user rather than the HW, so I don't think that
belongs in the dt.

It would be interesting to know what the lag was adversely affecting.
Perhaps there's another way around this.

Mark.

Re: [RFC PATCH 1/2] sbs-battery: add forced instantiation from device tree

[Frans Klaver]

On Thu, Sep 25, 2014 at 11:27 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Sep 24, 2014 at 07:59:34PM +0100, Frans Klaver wrote:
>> On Wed, Sep 24, 2014 at 04:34:32PM +0100, Mark Rutland wrote:
>> > On Wed, Sep 24, 2014 at 04:14:48PM +0100, Frans Klaver wrote:
>> > > On Wed, Sep 24, 2014 at 03:38:49PM +0100, Mark Rutland wrote:
>> > > > On Wed, Sep 24, 2014 at 03:22:22PM +0100, Frans Klaver wrote:
>> > > > You mention that there's a GPIO that can be used to detect the battery
>> > > > presence. Why can't the driver always probe and then on check for the
>> > > > presence of the battery dynamically using that GPIO? That should cover
>> > > > both cases.
>> > >
>> > > I would say that this was the case before [1] was done. The GPIO is
>> > > optional and if not configured, the presence or absence of the battery
>> > > is detected by checking a status register much like probe() currently
>> > > does. It seems all cases were covered before that patch. If you worry
>> > > about speed, you should use the GPIO. I wonder if we might be able to
>> > > revert [1] without doing much harm.
>> >
>> > But reverting that would re-introduce the lag on some systems, no? Given
>> > the wording of the original commit I would guess that the GPIO wasn't
>> > available. Perhaps Olof or Anton can enlighten us?
>>
>> It probably would yes. The battery_detect gpio was last touched in 2011, the
>> probe check was added somewhere in 2012.
>
> We can't revert it unless we know doing so won't reintroduce the
> problem. From the above it sounds like we can't revert it.
>
>> We could keep it as a compile option.
>
> Perhaps.
>
>> > In the cases where a GPIO is available, I think we should be able to be
>> > less pessimistic. Is a GPIO available in your case?
>>
>> We don't have the battery_detect pin available. Incidentally, a bit of
>> lag reading out the battery is not a problem for us.
>
> So now we're back at sqaure one. The hardware is likely identical in the
> your case and the care-about-lag case. Whether or not you care about lag
> is a property of the user rather than the HW, so I don't think that
> belongs in the dt.
>
> It would be interesting to know what the lag was adversely affecting.
> Perhaps there's another way around this.

Exactly. I can imagine this really being a problem if the i2c bus
already has a lot of priority traffic.

Frans

Re: [RFC PATCH 1/2] sbs-battery: add forced instantiation from device tree

[Frans Klaver]

Olof, Anton,

Care to pitch in?

Thanks,
Frans

On Thu, Sep 25, 2014 at 11:32 AM, Frans Klaver <fransklaver@gmail.com> wrote:
> On Thu, Sep 25, 2014 at 11:27 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Wed, Sep 24, 2014 at 07:59:34PM +0100, Frans Klaver wrote:
>>> On Wed, Sep 24, 2014 at 04:34:32PM +0100, Mark Rutland wrote:
>>> > On Wed, Sep 24, 2014 at 04:14:48PM +0100, Frans Klaver wrote:
>>> > > On Wed, Sep 24, 2014 at 03:38:49PM +0100, Mark Rutland wrote:
>>> > > > On Wed, Sep 24, 2014 at 03:22:22PM +0100, Frans Klaver wrote:
>>> > > > You mention that there's a GPIO that can be used to detect the battery
>>> > > > presence. Why can't the driver always probe and then on check for the
>>> > > > presence of the battery dynamically using that GPIO? That should cover
>>> > > > both cases.
>>> > >
>>> > > I would say that this was the case before [1] was done. The GPIO is
>>> > > optional and if not configured, the presence or absence of the battery
>>> > > is detected by checking a status register much like probe() currently
>>> > > does. It seems all cases were covered before that patch. If you worry
>>> > > about speed, you should use the GPIO. I wonder if we might be able to
>>> > > revert [1] without doing much harm.
>>> >
>>> > But reverting that would re-introduce the lag on some systems, no? Given
>>> > the wording of the original commit I would guess that the GPIO wasn't
>>> > available. Perhaps Olof or Anton can enlighten us?
>>>
>>> It probably would yes. The battery_detect gpio was last touched in 2011, the
>>> probe check was added somewhere in 2012.
>>
>> We can't revert it unless we know doing so won't reintroduce the
>> problem. From the above it sounds like we can't revert it.
>>
>>> We could keep it as a compile option.
>>
>> Perhaps.
>>
>>> > In the cases where a GPIO is available, I think we should be able to be
>>> > less pessimistic. Is a GPIO available in your case?
>>>
>>> We don't have the battery_detect pin available. Incidentally, a bit of
>>> lag reading out the battery is not a problem for us.
>>
>> So now we're back at sqaure one. The hardware is likely identical in the
>> your case and the care-about-lag case. Whether or not you care about lag
>> is a property of the user rather than the HW, so I don't think that
>> belongs in the dt.
>>
>> It would be interesting to know what the lag was adversely affecting.
>> Perhaps there's another way around this.
>
> Exactly. I can imagine this really being a problem if the i2c bus
> already has a lot of priority traffic.
>
> Frans