Re: [PATCH v2 3/3] power: bq27xxx: add support for NVRAM R/W access

[Sebastian Reichel <sre@kernel.org>]

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

Hi,

On Thu, Jan 05, 2017 at 07:23:48PM -0800, Matt Ranostay wrote:
> On Thu, Jan 5, 2017 at 6:34 PM, Matt Ranostay <matt@ranostay.consulting> wrote:
> > On Thu, Jan 5, 2017 at 5:53 PM, Sebastian Reichel <sre@kernel.org> wrote:
> >> On Wed, Jan 04, 2017 at 06:10:07PM -0800, Matt Ranostay wrote:
> >>> Initial support for access and modification of the non-volatile regions
> >>> of the bq27425 fuel gauge DesignEnergy, DesignCapacity, and
> >>> TerminateVoltage settings.
> >>>
> >>> This is intended for fine tuning the fuel gauge state machine for the
> >>> respective battery specifications.
> >>>
> >>> Cc: Sebastian Reichel <sre@kernel.org>
> >>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> >>> ---
> >>>  drivers/power/supply/bq27xxx_battery_i2c.c | 335 +++++++++++++++++++++++++++++
> >>>  include/linux/power/bq27xxx_battery.h      |   4 +
> >>>  2 files changed, 339 insertions(+)
> >>
> >> I only skipped over this one, as the changed DT binding requires
> >> quite some changes in this patch anyways. Here are a couple of
> >> comments how I would like to see this implemented:
> >>
> >> Add a patch for the power-supply core, which implements a
> >> structure for the battery info. Something like this:
> >>
> >> struct power_supply_battery_info {
> >>     uint32 energy; /* µWh */
> >>     uint32 power;  /* µAh */
> >>     uint32 nominal_voltage; /* µV */
> >>     /* ... */
> >> };
> >>
> >> And a function in the core framework, which gets the information
> >> from DT. The function itself should *not* be DT specific, so that
> >> ACPI/platformdata/whatever support can be added later without
> >> modifying every single driver.
> >>
> >> static struct power_supply_battery_info*
> >> power_supply_get_battery_info(struct power_supply *psy) {
> >>     if (psy->dt) {
> >>         /* get battery phandle or return -ENXIO */
> >>         /* fill and return struct */
> >>     }
> >>
> >>     return -ENOTSUP;
> >> }
> >>
> >> Then call power_supply_get_battery_info() during bq27xxx probe and
> >> use the struct to initialize the registers. Also I expect that
> >> functionality in bq27xxx_battery.c instead of bq27xxx_battery_i2c.c,
> >> since it's not I²C specific.
> >
> > They may not be i2c specific but they are only used by the i2c path of
> > the code currently. Do you think that platform data would ever have a
> > struct to pass with the respective data?

I think it is possible, that there will be some non-i2c bq27xxx
device in the future, that needs this feature.

> > Also it would have to be in bq27xxx_battery_setup() and not
> > bq27xxx_battery_platform_probe() since the former is only thing called
> > by both code paths.

bq27xxx_battery_setup is fine.

> Also there is no bq27xxx_battery_platform_write to even write the
> configuration data.

Just check if the callback exists.

The plan is to convert the 1wire bq27xxx to regmap at some point, so
that the bus-specific bq27xxx files can be dropped by just using
regmap in the core bq27xxx driver.

-- Sebastian

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