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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>> 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:
signature.asc
Description: PGP signature
