LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
To: Brian Norris <briannorris@chromium.org>
Cc: linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
	Rhyland Klein <rklein@nvidia.com>,
	Alexandru Stan <amstan@chromium.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Doug Anderson <dianders@chromium.org>,
	Phil Reid <preid@electromag.com.au>
Subject: Re: [PATCH 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats
Date: Fri, 6 Jul 2018 13:28:54 +0200	[thread overview]
Message-ID: <20180706112854.nj762elcad625idr@earth.universe> (raw)
In-Reply-To: <20180612202042.107603-1-briannorris@chromium.org>

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

Hi,

On Tue, Jun 12, 2018 at 01:20:41PM -0700, Brian Norris wrote:
> This driver was originally submitted for the TI BQ20Z75 battery IC
> (commit a7640bfa10c5 ("power_supply: Add driver for TI BQ20Z75 gas gauge
> IC")) and later renamed to express generic SBS support. While it's
> mostly true that this driver implemented a standard SBS command set, it
> takes liberties with the REG_MANUFACTURER_DATA register. This register
> is specified in the SBS spec, but it doesn't make any mention of what
> its actual contents are.
> 
> We've sort of noticed this optionality previously, with commit
> 17c6d3979e5b ("sbs-battery: make writes to ManufacturerAccess
> optional"), where we found that some batteries NAK writes to this
> register.
> 
> What this really means is that so far, we've just been lucky that most
> batteries have either been compatible with the TI chip, or else at least
> haven't reported highly-unexpected values.
> 
> For instance, one battery I have here seems to report either 0x0000 or
> 0x0100 to the MANUFACTURER_ACCESS_STATUS command -- while this seems to
> match either Wake Up (bits[11:8] = 0000b) or Normal Discharge
> (bits[11:8] = 0001b) status for the TI part [1], they don't seem to
> actually correspond to real states (for instance, I never see 0101b =
> Charge, even when charging).
> 
> On other batteries, I'm getting apparently random data in return, which
> means that occasionally, we interpret this as "battery not present" or
> "battery is not healthy".
> 
> All in all, it seems to be a really bad idea to make assumptions about
> REG_MANUFACTURER_DATA, unless we already know what battery we're using.
> Therefore, this patch reimplements the "present" and "health" checks to
> the following on most SBS batteries:
> 
> 1. HEALTH: report "unknown" -- I couldn't find a standard SBS command
>    that gives us much useful here
> 2. PRESENT: just send a REG_STATUS command; if it succeeds, then the
>    battery is present
> 
> Also, we stop sending MANUFACTURER_ACCESS_SLEEP to non-TI parts. I have
> no proof that this is useful and supported.
> 
> If someone explicitly provided a 'ti,bq20z75' compatible property, then
> we continue to use the existing TI command behaviors, and we effectively
> revert commit 17c6d3979e5b ("sbs-battery: make writes to
> ManufacturerAccess optional") to again make these commands required.
> 
> [1] http://www.ti.com/lit/er/sluu265a/sluu265a.pdf
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Acked-by: Rhyland Klein <rklein@nvidia.com>
> ---

Thanks, queued to power-supply-next.

-- Sebastian

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

  parent reply	other threads:[~2018-07-06 11:29 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-12 20:20 Brian Norris
2018-06-12 20:20 ` [PATCH 2/2] dt-bindings: power: sbs-battery: re-document "ti,bq20z75" Brian Norris
2018-07-06 11:31   ` Sebastian Reichel
2018-06-12 20:23 ` [PATCH 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats Brian Norris
2018-06-13 10:46   ` Sebastian Reichel
2018-07-06 11:28 ` Sebastian Reichel [this message]
  -- strict thread matches above, loose matches on Subject: below --
2018-06-01  0:32 Brian Norris
2018-06-01  1:20 ` Guenter Roeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180706112854.nj762elcad625idr@earth.universe \
    --to=sebastian.reichel@collabora.co.uk \
    --cc=amstan@chromium.org \
    --cc=briannorris@chromium.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=preid@electromag.com.au \
    --cc=rklein@nvidia.com \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH 1/2] power: supply: sbs-battery: don'\''t assume MANUFACTURER_DATA formats' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).