Hi Dimitri,
On 4/22/25 20:48, Dimitri Fedrau wrote:
> Hi Thomas,
>
> On Mon, Apr 21, 2025 at 08:13:33PM +0200, Thomas Antoine via B4 Relay wrote:
>> From: Thomas Antoine <t.antoine@xxxxxxxxxxxx>
[...]
>> #define MAX172XX_REPCAP 0x05 /* Average capacity */
>> #define MAX172XX_REPSOC 0x06 /* Percentage of charge */
>> #define MAX172XX_TEMP 0x08 /* Temperature */
>> +#define MAX172XX_VCELL 0x09 /* Lowest cell voltage */
>> #define MAX172XX_CURRENT 0x0A /* Actual current */
>> #define MAX172XX_AVG_CURRENT 0x0B /* Average current */
>> #define MAX172XX_FULL_CAP 0x10 /* Calculated full capacity */
>> @@ -50,19 +51,32 @@
>> #define MAX172XX_DEV_NAME_TYPE_MASK GENMASK(3, 0)
>> #define MAX172XX_DEV_NAME_TYPE_MAX17201 BIT(0)
>> #define MAX172XX_DEV_NAME_TYPE_MAX17205 (BIT(0) | BIT(2))
>> +#define MAX77759_DEV_NAME_TYPE_MASK GENMASK(15, 9)
>> +#define MAX77759_DEV_NAME_TYPE_MAX77759 0x31
>> #define MAX172XX_QR_TABLE10 0x22
>> +#define MAX77759_TASKPERIOD 0x3C
>> +#define MAX77759_TASKPERIOD_175MS 0x1680
>> +#define MAX77759_TASKPERIOD_351MS 0x2D00
> I think it would be more readable if MAX77759_ defines are separated to
> the MAX172XX defines instead of mixing them up.
Will fix in v4.
>> #define MAX172XX_BATT 0xDA /* Battery voltage */
>> #define MAX172XX_ATAVCAP 0xDF
>>
>> static const char *const max1720x_manufacturer = "Maxim Integrated";
>> static const char *const max17201_model = "MAX17201";
>> static const char *const max17205_model = "MAX17205";
>> +static const char *const max77759_model = "MAX77759";
>> +
>> +enum chip_id {
>> + MAX1720X_ID,
>> + MAX77759_ID,
>> +};
>>
>> struct max1720x_device_info {
>> struct regmap *regmap;
>> struct regmap *regmap_nv;
>> struct i2c_client *ancillary;
>> int rsense;
>> + int charge_full_design;
> Don't see charge_full_design is used somewhere besides reading it from
> device-tree and it isn't part of the bindings. If not needed, remove it.
>
Its a leftover of a previous experimentation, will remove in next version.
>> + enum chip_id id;
>> };
>>
>>
> [...]
>
>> +static int max172xx_cell_voltage_to_ps(unsigned int reg)
>> +{
>> + return reg * 625 / 8; /* in uV */
>> +}
>> +
>> static int max172xx_capacity_to_ps(unsigned int reg,
>> - struct max1720x_device_info *info)
>> + struct max1720x_device_info *info,
>> + int *intval)
>> {
>> - return reg * (500000 / info->rsense); /* in uAh */
>> + int lsb = 1;
>> + int reg_val;
> The naming of reg_val is somehow confusing because of reg. Better rename
> it to something like reg_task_period or similar and reg_val should be of
> type unsigned int.
>
Will change in v4.
>> + int ret;
>> +
>> + if (info->id == MAX77759_ID) {
>> + ret = regmap_read(info->regmap, MAX77759_TASKPERIOD, ®_val);
>> + if (ret)
>> + return ret;
>> +
>> + switch (reg_val) {
>> + case MAX77759_TASKPERIOD_175MS:
>> + break;
>> + case MAX77759_TASKPERIOD_351MS:
>> + lsb = 2;
>> + break;
>> + default:
>> + return -ENODEV;
>> + }
>> + }
>> + *intval = reg * (500000 / info->rsense) * lsb; /* in uAh */
>> + return 0;
> nit: add newline before return.
>
Will fix in v4
>> }
>>
>> /*
>> @@ -306,6 +420,28 @@ static int max172xx_temperature_to_ps(unsigned int reg)
>> return val * 10 / 256; /* in tenths of deg. C */
>> }
>>
>> +static const char *max1720x_devname_to_model(unsigned int reg_val,
>> + union power_supply_propval *val,
>> + struct max1720x_device_info *info)
>> +{
>> + switch (info->id) {
>> + case MAX1720X_ID:
>> + reg_val = FIELD_GET(MAX172XX_DEV_NAME_TYPE_MASK, reg_val);
>> + if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17201)
>> + return max17201_model;
>> + else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
>> + return max17205_model;
>> + return NULL;
> nit: return NULL in else case.
>
>> + case MAX77759_ID:
>> + reg_val = FIELD_GET(MAX77759_DEV_NAME_TYPE_MASK, reg_val);
>> + if (reg_val == MAX77759_DEV_NAME_TYPE_MAX77759)
>> + return max77759_model;
>> + return NULL;
> nit: return NULL in else case.
>
Will fix both in v4.
>> + default:
>> + return NULL;
>> + }
>> +}
>> +
>> /*
>> * Calculating current registers resolution:
>> *
>> @@ -390,19 +526,31 @@ static int max1720x_battery_get_property(struct power_supply *psy,
>> val->intval = max172xx_percent_to_ps(reg_val);
>> break;
>> case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> - ret = regmap_read(info->regmap, MAX172XX_BATT, ®_val);
>> - val->intval = max172xx_voltage_to_ps(reg_val);
>> + if (info->id == MAX1720X_ID) {
>> + ret = regmap_read(info->regmap, MAX172XX_BATT, ®_val);
>> + val->intval = max172xx_voltage_to_ps(reg_val);
>> + } else if (info->id == MAX77759_ID) {
>> + ret = regmap_read(info->regmap, MAX172XX_VCELL, ®_val);
>> + val->intval = max172xx_cell_voltage_to_ps(reg_val);
>> + } else
>> + return -ENODEV;
>> break;
>> case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>> ret = regmap_read(info->regmap, MAX172XX_DESIGN_CAP, ®_val);
>> - val->intval = max172xx_capacity_to_ps(reg_val);
>> + if (ret)
>> + break;
> I would keep max172xx_capacity_to_ps as it was before and add the
> calculation for the MAX77759 after handling the MAX1720X case. Creating
> a function max77759_capacity_to_ps that further processes the value
> calculated by max172xx_capacity_to_ps or just inline this code.
> Otherwise the naming of the function is somehow confusing.
>
Will change for v4.
>> + ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
>> break;
>> case POWER_SUPPLY_PROP_CHARGE_AVG:
>> ret = regmap_read(info->regmap, MAX172XX_REPCAP, ®_val);
>> - val->intval = max172xx_capacity_to_ps(reg_val);
>> + if (ret)
>> + break;
>> +
> Same as above.
>
>> + ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
>> break;
>> case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
>> ret = regmap_read(info->regmap, MAX172XX_TTE, ®_val);
>> + pr_info("RAW TTE: %d", reg_val);
> Remove pr_info.
>
Once again debug I forgot, sorry for this.
>> val->intval = max172xx_time_to_ps(reg_val);
>> break;
>> case POWER_SUPPLY_PROP_TIME_TO_FULL_AVG:
>> @@ -423,17 +571,15 @@ static int max1720x_battery_get_property(struct power_supply *psy,
>> break;
>> case POWER_SUPPLY_PROP_CHARGE_FULL:
>> ret = regmap_read(info->regmap, MAX172XX_FULL_CAP, ®_val);
>> - val->intval = max172xx_capacity_to_ps(reg_val);
> ...
>
>> + if (ret)
>> + break;
>> + ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
>> break;
>> case POWER_SUPPLY_PROP_MODEL_NAME:
>> ret = regmap_read(info->regmap, MAX172XX_DEV_NAME, ®_val);
>> - reg_val = FIELD_GET(MAX172XX_DEV_NAME_TYPE_MASK, reg_val);
>> - if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17201)
>> - val->strval = max17201_model;
>> - else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
>> - val->strval = max17205_model;
>> - else
>> - return -ENODEV;
>> + val->strval = max1720x_devname_to_model(reg_val, val, info);
> Wouldn't it be better to just inline this function ?
>
I think my reason for this was that this case became quite long and indented
compared to all the others. If you think it is better, I will inline it for v4.
>> + if (!val->strval)
>> + ret = -ENODEV;
>> {
> [...]
>
>> struct power_supply_config psy_cfg = {};
>> struct device *dev = &client->dev;
>> struct max1720x_device_info *info;
>> struct power_supply *bat;
>> + const struct chip_data *data;
>> + const struct power_supply_desc *bat_desc;
>> int ret;
>>
>> info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>> if (!info)
>> return -ENOMEM;
>>
>> + data = device_get_match_data(dev);
>> + if (!data)
>> + return dev_err_probe(dev, -EINVAL, "Failed to get chip data\n");
>> +
>> psy_cfg.drv_data = info;
>> psy_cfg.fwnode = dev_fwnode(dev);
>> - psy_cfg.attr_grp = max1720x_groups;
>> + switch (data->id) {
>> + case MAX1720X_ID:
>> + psy_cfg.attr_grp = max1720x_groups;
>> + bat_desc = &max1720x_bat_desc;
>> + break;
>> + case MAX77759_ID:
>> + bat_desc = &max77759_bat_desc;
>> + break;
>> + default:
>> + return dev_err_probe(dev, -EINVAL, "Unsupported chip\n");
>> + }
> nit: add empty line
>
Will add in v4.
[...]
Best regards,
Thomas Antoine
