Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

["André Draszik" <andre.draszik@linaro.org>]

On Tue, 2024-12-03 at 10:08 +0100, Thomas Antoine wrote:
> On 12/3/24 07:47, André Draszik wrote:
> > Hi Thomas,
> > 
> > Thanks for looking into this!
> 
> Hi,
> 
> With pleasure! This is my first time trying to contribute to the kernel
> so sorry for any beginner mistakes I might do.

No worries :-)

>  
> > > From: Thomas Antoine <t.antoine@uclouvain.be>
> > > 
> > > The Maxim max77759 fuel gauge has the same interface as the Maxim max1720x
> > > except for the non-volatile memory slave address which is not available.
> > 
> > It is not fully compatible, and it also has a lot more registers.
> > 
> > For example, the voltage now is not in register 0xda as this driver assumes.
> > With these changes, POWER_SUPPLY_PROP_VOLTAGE_NOW just reads as 0. 0xda
> > doesn't exist in max77759
> > 
> > I haven't compared in depth yet, though.
> 
> Is the voltage necessary for the driver? If so, we could just not
> read the voltage. If it is necessary, I can try to kook into it and try
> to find in which register it is located (if there is one).

Downstream reports it in
https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max1720x_battery.c#2400

so upstream should do, too.

> > >  static const char *const max17205_model = "MAX17205";
> > > +static const char *const max77759_model = "MAX77759";
> > > 
> > >  struct max1720x_device_info {
> > >       struct regmap *regmap;
> > > @@ -54,6 +57,21 @@ struct max1720x_device_info {
> > >       int rsense;
> > >  };
> > > 
> > > +struct chip_data {
> > > +     u16 default_nrsense; /* in regs in 10^-5 */
> > > +     u8 has_nvmem;
> > > +};
> > > +
> > > +static const struct chip_data max1720x_data  = {
> > > +     .default_nrsense = 1000,
> > > +     .has_nvmem = 1,
> > > +};
> > > +
> > > +static const struct chip_data max77759_data = {
> > > +     .default_nrsense = 500,
> > > +     .has_nvmem = 0,
> > > +};
> > 
> > This should be made a required devicetree property instead, at least for
> > max77759, as it's completely board dependent, 'shunt-resistor-micro-ohms'
> > is widely used.
> > 
> > I also don't think there should be a default. The driver should just fail
> > to probe if not specified in DT (for max77759).
> 
> I hesitated to do this but I didn't know what would be better. Will change
> for v2.

Just to clarify, has_nvmem can stay here in the driver, just rsense should
go into DT is what I mean.

> > > +
> > >  /*
> > >   * Model Gauge M5 Algorithm output register
> > >   * Volatile data (must not be cached)
> > > @@ -369,6 +387,8 @@ static int max1720x_battery_get_property(struct
> > > power_supply *psy,
> > >                       val->strval = max17201_model;
> > >               else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
> > >                       val->strval = max17205_model;
> > > +             else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX77759)
> > > +                     val->strval = max77759_model;
> > >               else
> > 
> > This is a 16 bit register, and while yes, MAX172XX_DEV_NAME_TYPE_MASK only
> > cares about the bottom 4 bits, the register is described as 'Firmware
> > Version Information'.
> > 
> > But maybe it's ok to do it like that, at least for now.
> 
> I thought this method would be ok as long as there is no collision on
> values. I hesitated to change the model evaluation method based on chip
> model, where the max77759 would thus have an hard-coded value and the
> max1720x would still evaluate the register value. I did not do it because
> it led to a lot more changes for no difference.

Downstream uses the upper bits for max77759:
https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max_m5.h#135

I don't know what the original max17201/5 report in this register
for those bits, though. Given for max77759 this register returns
the firmware version, I assume the lower bits can change.

Cheers,
Andre'