* [PATCH 0/3] hwmon:(ina238)Add support for SQ52206 @ 2024-12-24 6:35 Wenliang Yan 2024-12-24 6:35 ` [PATCH 1/3] " Wenliang Yan 0 siblings, 1 reply; 6+ messages in thread From: Wenliang Yan @ 2024-12-24 6:35 UTC (permalink / raw) To: linux, jdelvare Cc: Wenliang, robh, krzk+dt, conor+dt, corbet, linux-hwmon, linux-kernel From: Wenliang <wenliang202407@163.com> Add support for Silergy i2c power monitor SQ52206 to the ina238 driver as those two are similar. Signed-off-by: Wenliang <wenliang202407@163.com> --- The manual content of the chip has not been fully disclosed yet. Can the manual be provided in the form of a cloud storage link for reviewing the driver code? If not, how can it be provided? It can be provided in V2. This version patch is displayed through text, and both chips are power monitors with a 16 bit delta sigma ADC specifically designed for current sensing applications Operates from a 2.7-V to 5.5-V supply. Same pin definition.In addition, SQ52206 add detection for power peak, energy and charge. The main differences between the two chips are as follows: ___________________________________ | INA238 | SQ52206 | VBUS_LSB | 3.125mV | 3.75mV | DIETEMP_LSB | 125 mC | 7.8125 mC | ____________|__________|___________| ________________________________________________ reg address | INA238 | SQ52206 | ____________|_______________|___________________| 0h | CONFIG | CONFIG | 1h | ADC_CONFIG | ADC_CONFIG | 2h | SHUNT_CAL | SHUNT_CAL | 3h | / |SHUNT_TEMPCO(16bit)| 4h | VSHUNT | VSHUNT | 5h | VBUS | VBUS | 6h | DIETEMP | DIETEMP | 7h | CURRENT | CURRENT | 8h | POWER | POWER | 9h | / | ENERGY(40bit) | Ah | / | CHARGE(40bit) | Bh | DIAG_ALERT | DIAG_ALERT | Ch | SOVL | SOVL | Dh | SUVL | SUVL | Eh | BOVL | BOVL | Fh | BUVL | BUVL | 10h | TEMP_LIMIT | TEMP_LIMIT | 11h | PWR_LIMIT | PWR_LIMIT | 20h | | PWR_PEAK(24bit) | 3Eh |MANUFACTURER_ID| / | 3Fh | DEVICE_ID | / | ____________|_______________|___________________| The ENERGY and CHARGE registers are 40 bit read-only registers used to display the values of energy and charge. PWR_PEAK register is 24 bit read-only registers.The CONFIG register has some differences: __________________________________________________________________________ BIT | INA238 | SQ52206 | ____|____________________________|________________________________________| 14 | Reserved. Always reads 0 | reset ENERGY and CHARGE registers to 0 | ____|____________________________|________________________________________| 4 |ADCRANGE 0h=163.84 1h=81.92 | (BIT4~3)ADCRANGE 2h/3h = 40.96, | ____|____________________________| 0h=163.84 1h=81.92 | 3 | Reserved. Always reads 0 | | ____|____________________________|________________________________________| Wenliang (3): hwmon:(ina238)Add support for SQ52206 hwmon:(ina238)Add new features for SQ52206 dt-bindings:Add SQ52206 to ina2xx devicetree bindings .../devicetree/bindings/hwmon/ti,ina2xx.yaml | 1 + Documentation/hwmon/ina238.rst | 16 + drivers/hwmon/ina238.c | 286 ++++++++++++++---- 3 files changed, 240 insertions(+), 63 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] hwmon:(ina238)Add support for SQ52206 2024-12-24 6:35 [PATCH 0/3] hwmon:(ina238)Add support for SQ52206 Wenliang Yan @ 2024-12-24 6:35 ` Wenliang Yan 2025-01-06 23:22 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Wenliang Yan @ 2024-12-24 6:35 UTC (permalink / raw) To: linux, Jean Delvare Cc: Wenliang, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-hwmon, linux-kernel From: Wenliang <wenliang202407@163.com> Add support for SQ52206 to the Ina238 driver. Add registers, add calculation formulas, increase compatibility, adjust struct and chip initialization. Signed-off-by: Wenliang <wenliang202407@163.com> --- Incorporate four additional registers to the original register set of SQ52206 beyond INA238. The ADC measurement range of SQ52206 is divided into 1/2/4, so change the configuration of INA238_ADC_CONFIG. SQ52206's calculation of power read values is different from INA238.Add new value of BUS_VOLTAGE_LSB and DIE-TEMP_LSB for SQ52206. As a result of these changes, modify both the power and temperature read and write operations. Add kind and struct ina238_config *config in struct ina238_data to save the chip type and different configurations for each chip type, promoting program reusability. Due to the temperature reading of SQ52206 being a signed 16 bit value, while INA238 is a 12 bit value. So we changed the temperature reading function. Extract the chip initialization process into a separate function named ina238_init to facilitate adjustments for various chip types. Add a corresponding compatible to the driver. drivers/hwmon/ina238.c | 209 ++++++++++++++++++++++++++++------------- 1 file changed, 144 insertions(+), 65 deletions(-) diff --git a/drivers/hwmon/ina238.c b/drivers/hwmon/ina238.c index 2d9f12f68d50..131f5faefdb3 100644 --- a/drivers/hwmon/ina238.c +++ b/drivers/hwmon/ina238.c @@ -21,11 +21,14 @@ #define INA238_CONFIG 0x0 #define INA238_ADC_CONFIG 0x1 #define INA238_SHUNT_CALIBRATION 0x2 +#define SQ52206_SHUNT_TEMPCO 0x3 #define INA238_SHUNT_VOLTAGE 0x4 #define INA238_BUS_VOLTAGE 0x5 #define INA238_DIE_TEMP 0x6 #define INA238_CURRENT 0x7 #define INA238_POWER 0x8 +#define SQ52206_ENERGY 0x9 +#define SQ52206_CHARGE 0xa #define INA238_DIAG_ALERT 0xb #define INA238_SHUNT_OVER_VOLTAGE 0xc #define INA238_SHUNT_UNDER_VOLTAGE 0xd @@ -33,9 +36,12 @@ #define INA238_BUS_UNDER_VOLTAGE 0xf #define INA238_TEMP_LIMIT 0x10 #define INA238_POWER_LIMIT 0x11 +#define SQ52206_POWER_PEAK 0x20 #define INA238_DEVICE_ID 0x3f /* not available on INA237 */ #define INA238_CONFIG_ADCRANGE BIT(4) +#define SQ52206_CONFIG_ADCRANGE GENMASK(4, 3) +#define SQ52206_READ_ADCRANGE(reg) FIELD_GET(SQ52206_CONFIG_ADCRANGE, reg) #define INA238_DIAG_ALERT_TMPOL BIT(7) #define INA238_DIAG_ALERT_SHNTOL BIT(6) @@ -44,7 +50,7 @@ #define INA238_DIAG_ALERT_BUSUL BIT(3) #define INA238_DIAG_ALERT_POL BIT(2) -#define INA238_REGISTERS 0x11 +#define INA238_MAX_REGISTERS 0x20 #define INA238_RSHUNT_DEFAULT 10000 /* uOhm */ @@ -87,27 +93,63 @@ * shunt = 0x4000 / (819.2 * 10^6) / 0.001 = 20000 uOhms (with 1mA/lsb) * * Current (mA) = register value * 20000 / rshunt / 4 * gain - * Power (W) = 0.2 * register value * 20000 / rshunt / 4 * gain + * Power (mW) = 0.2 * register value * 20000 / rshunt / 4 * gain + * (Specific for SQ52206) + * Power (mW) = 0.24 * register value * 20000 / rshunt / 4 * gain */ #define INA238_CALIBRATION_VALUE 16384 -#define INA238_FIXED_SHUNT 20000 +#define INA238_FIXED_SHUNT 20000 #define INA238_SHUNT_VOLTAGE_LSB 5 /* 5 uV/lsb */ #define INA238_BUS_VOLTAGE_LSB 3125 /* 3.125 mV/lsb */ -#define INA238_DIE_TEMP_LSB 125 /* 125 mC/lsb */ +#define INA238_DIE_TEMP_LSB 1250000 /* 125 mC/lsb */ +#define SQ52206_BUS_VOLTAGE_LSB 3750 /* 3.75 mV/lsb */ +#define SQ52206_DIE_TEMP_LSB 78125 /* 7.8125 mC/lsb */ static const struct regmap_config ina238_regmap_config = { - .max_register = INA238_REGISTERS, + .max_register = INA238_MAX_REGISTERS, .reg_bits = 8, .val_bits = 16, }; +enum ina238_ids { ina238, ina237, sq52206 }; + +struct ina238_config { + u16 config_default; + int calibration_value; + int bus_voltage_lsb; /* uV */ + int temp_lsb; /* mC */ +}; + struct ina238_data { + const struct ina238_config *config; struct i2c_client *client; struct mutex config_lock; struct regmap *regmap; u32 rshunt; int gain; + int kind; +}; + +static const struct ina238_config ina238_config[] = { + [ina238] = { + .config_default = INA238_CONFIG_DEFAULT, + .calibration_value = INA238_CALIBRATION_VALUE, + .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB, + .temp_lsb = INA238_DIE_TEMP_LSB, + }, + [ina237] = { + .config_default = INA238_CONFIG_DEFAULT, + .calibration_value = INA238_CALIBRATION_VALUE, + .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB, + .temp_lsb = INA238_DIE_TEMP_LSB, + }, + [sq52206] = { + .config_default = INA238_CONFIG_DEFAULT, + .calibration_value = INA238_CALIBRATION_VALUE, + .bus_voltage_lsb = SQ52206_BUS_VOLTAGE_LSB, + .temp_lsb = SQ52206_DIE_TEMP_LSB, + }, }; static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val) @@ -197,10 +239,10 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel, regval = (s16)regval; if (channel == 0) /* gain of 1 -> LSB / 4 */ - *val = (regval * INA238_SHUNT_VOLTAGE_LSB) / - (1000 * (4 - data->gain + 1)); + *val = (regval * INA238_SHUNT_VOLTAGE_LSB) * + data->gain / (1000 * 4); else - *val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000; + *val = (regval * data->config->bus_voltage_lsb) / 1000; break; case hwmon_in_max_alarm: case hwmon_in_min_alarm: @@ -225,8 +267,8 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel, case 0: /* signed value, clamp to max range +/-163 mV */ regval = clamp_val(val, -163, 163); - regval = (regval * 1000 * (4 - data->gain + 1)) / - INA238_SHUNT_VOLTAGE_LSB; + regval = (regval * 1000 * 4) / + INA238_SHUNT_VOLTAGE_LSB * data->gain; regval = clamp_val(regval, S16_MIN, S16_MAX); switch (attr) { @@ -242,7 +284,7 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel, case 1: /* signed value, positive values only. Clamp to max 102.396 V */ regval = clamp_val(val, 0, 102396); - regval = (regval * 1000) / INA238_BUS_VOLTAGE_LSB; + regval = (regval * 1000) / data->config->bus_voltage_lsb; regval = clamp_val(regval, 0, S16_MAX); switch (attr) { @@ -295,10 +337,14 @@ static int ina238_read_power(struct device *dev, u32 attr, long *val) err = ina238_read_reg24(data->client, INA238_POWER, ®val); if (err) return err; - - /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */ - power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT * - data->gain, 20 * data->rshunt); + if (data->kind == sq52206) + /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */ + power = div_u64(regval * 1200ULL * INA238_FIXED_SHUNT * + data->gain, 20 * data->rshunt); + else + /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */ + power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT * + data->gain, 20 * data->rshunt); /* Clamp value to maximum value of long */ *val = clamp_val(power, 0, LONG_MAX); break; @@ -311,8 +357,12 @@ static int ina238_read_power(struct device *dev, u32 attr, long *val) * Truncated 24-bit compare register, lower 8-bits are * truncated. Same conversion to/from uW as POWER register. */ - power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT * - data->gain, 20 * data->rshunt); + if (data->kind == sq52206) + power = div_u64((regval << 8) * 1200ULL * INA238_FIXED_SHUNT * + data->gain, 20 * data->rshunt); + else + power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT * + data->gain, 20 * data->rshunt); /* Clamp value to maximum value of long */ *val = clamp_val(power, 0, LONG_MAX); break; @@ -344,8 +394,12 @@ static int ina238_write_power(struct device *dev, u32 attr, long val) * register. */ regval = clamp_val(val, 0, LONG_MAX); - regval = div_u64(val * 20ULL * data->rshunt, - 1000ULL * INA238_FIXED_SHUNT * data->gain); + if (data->kind == sq52206) + regval = div_u64(val * 20ULL * data->rshunt, + 1000ULL * INA238_FIXED_SHUNT * data->gain); + else + regval = div_u64(val * 24ULL * data->rshunt, + 1000ULL * INA238_FIXED_SHUNT * data->gain); regval = clamp_val(regval >> 8, 0, U16_MAX); return regmap_write(data->regmap, INA238_POWER_LIMIT, regval); @@ -362,17 +416,29 @@ static int ina238_read_temp(struct device *dev, u32 attr, long *val) err = regmap_read(data->regmap, INA238_DIE_TEMP, ®val); if (err) return err; - + if (data->kind == sq52206) + /* Signed, bits 15-0 of register, result in mC */ + regval = div_s64((s16)regval * data->config->temp_lsb, + 10000); + else /* Signed, bits 15-4 of register, result in mC */ - *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB; + *val = div_s64(((s16)regval >> 4) * + data->config->temp_lsb, 10000); + *val = clamp_val(regval, S16_MIN, S16_MAX); break; case hwmon_temp_max: err = regmap_read(data->regmap, INA238_TEMP_LIMIT, ®val); if (err) return err; - - /* Signed, bits 15-4 of register, result in mC */ - *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB; + if (data->kind == sq52206) + /* Signed, bits 15-0 of register, result in mC */ + regval = div_s64((s16)regval * data->config->temp_lsb, + 10000); + else + /* Signed, bits 15-4 of register, result in mC */ + *val = div_s64(((s16)regval >> 4) * + data->config->temp_lsb, 10000); + *val = clamp_val(regval, S16_MIN, S16_MAX); break; case hwmon_temp_max_alarm: err = regmap_read(data->regmap, INA238_DIAG_ALERT, ®val); @@ -396,9 +462,14 @@ static int ina238_write_temp(struct device *dev, u32 attr, long val) if (attr != hwmon_temp_max) return -EOPNOTSUPP; - /* Signed, bits 15-4 of register */ - regval = (val / INA238_DIE_TEMP_LSB) << 4; - regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0; + if (data->kind == sq52206) { + regval = div_u64(val*10000, data->config->temp_lsb); + regval = clamp_val(regval, S16_MIN, S16_MAX); + } else { + /* Signed, bits 15-4 of register */ + regval = div_u64(val*10000, data->config->temp_lsb) << 4; + regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0; + } return regmap_write(data->regmap, INA238_TEMP_LIMIT, regval); } @@ -530,20 +601,58 @@ static const struct hwmon_chip_info ina238_chip_info = { .info = ina238_info, }; +/* + * Initialize the configuration and calibration registers. + */ +static int ina238_init(struct ina238_data *data) +{ + int ret = 0; + int config; + /* Setup CONFIG register */ + config = data->config->config_default; + if (data->gain == 1) + config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */ + ret = regmap_write(data->regmap, INA238_CONFIG, config); + if (ret < 0) + return ret; + /* Setup ADC_CONFIG register */ + ret = regmap_write(data->regmap, INA238_ADC_CONFIG, + INA238_ADC_CONFIG_DEFAULT); + if (ret < 0) + return ret; + /* Setup SHUNT_CALIBRATION register with fixed value */ + ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION, + data->config->calibration_value); + if (ret < 0) + return ret; + /* Setup alert/alarm configuration */ + ret = regmap_write(data->regmap, INA238_DIAG_ALERT, + INA238_DIAG_ALERT_DEFAULT); + if (ret < 0) + return ret; + return 0; +} +static const struct i2c_device_id ina238_id[]; static int ina238_probe(struct i2c_client *client) { struct ina2xx_platform_data *pdata = dev_get_platdata(&client->dev); struct device *dev = &client->dev; struct device *hwmon_dev; struct ina238_data *data; - int config; + enum ina238_ids chip; int ret; + chip = (uintptr_t)i2c_get_match_data(client); + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); if (!data) return -ENOMEM; + data->kind = chip; data->client = client; + /* set the device type */ + data->config = &ina238_config[data->kind]; + mutex_init(&data->config_lock); data->regmap = devm_regmap_init_i2c(client, &ina238_regmap_config); @@ -564,48 +673,15 @@ static int ina238_probe(struct i2c_client *client) /* load shunt gain value */ if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0) data->gain = 4; /* Default of ADCRANGE = 0 */ - if (data->gain != 1 && data->gain != 4) { + if (data->gain != 1 && data->gain != 2 && data->gain != 4) { dev_err(dev, "invalid shunt gain value %u\n", data->gain); return -EINVAL; } - - /* Setup CONFIG register */ - config = INA238_CONFIG_DEFAULT; - if (data->gain == 1) - config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */ - ret = regmap_write(data->regmap, INA238_CONFIG, config); - if (ret < 0) { - dev_err(dev, "error configuring the device: %d\n", ret); - return -ENODEV; - } - - /* Setup ADC_CONFIG register */ - ret = regmap_write(data->regmap, INA238_ADC_CONFIG, - INA238_ADC_CONFIG_DEFAULT); - if (ret < 0) { - dev_err(dev, "error configuring the device: %d\n", ret); - return -ENODEV; - } - - /* Setup SHUNT_CALIBRATION register with fixed value */ - ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION, - INA238_CALIBRATION_VALUE); - if (ret < 0) { - dev_err(dev, "error configuring the device: %d\n", ret); - return -ENODEV; - } - - /* Setup alert/alarm configuration */ - ret = regmap_write(data->regmap, INA238_DIAG_ALERT, - INA238_DIAG_ALERT_DEFAULT); - if (ret < 0) { - dev_err(dev, "error configuring the device: %d\n", ret); - return -ENODEV; - } + ret = ina238_init(data); hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, &ina238_chip_info, - NULL); + ina238_groups); if (IS_ERR(hwmon_dev)) return PTR_ERR(hwmon_dev); @@ -616,7 +692,9 @@ static int ina238_probe(struct i2c_client *client) } static const struct i2c_device_id ina238_id[] = { - { "ina238" }, + { "ina238", ina238 }, + { "ina237", ina237 }, + { "sq52206", sq52206 }, { } }; MODULE_DEVICE_TABLE(i2c, ina238_id); @@ -624,6 +702,7 @@ MODULE_DEVICE_TABLE(i2c, ina238_id); static const struct of_device_id __maybe_unused ina238_of_match[] = { { .compatible = "ti,ina237" }, { .compatible = "ti,ina238" }, + { .compatible = "silergy,sq52206" }, { }, }; MODULE_DEVICE_TABLE(of, ina238_of_match); -- 2.43.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] hwmon:(ina238)Add support for SQ52206 2024-12-24 6:35 ` [PATCH 1/3] " Wenliang Yan @ 2025-01-06 23:22 ` Guenter Roeck 2025-01-07 12:43 ` Wenliang Yan 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2025-01-06 23:22 UTC (permalink / raw) To: Wenliang Yan, Jean Delvare Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-hwmon, linux-kernel On 12/23/24 22:35, Wenliang Yan wrote: > From: Wenliang <wenliang202407@163.com> > > Add support for SQ52206 to the Ina238 driver. Add registers, > add calculation formulas, increase compatibility, adjust > struct and chip initialization. > > Signed-off-by: Wenliang <wenliang202407@163.com> > --- > > Incorporate four additional registers to the original register > set of SQ52206 beyond INA238. > The ADC measurement range of SQ52206 is divided into 1/2/4, > so change the configuration of INA238_ADC_CONFIG. > SQ52206's calculation of power read values is different from > INA238.Add new value of BUS_VOLTAGE_LSB and DIE-TEMP_LSB for > SQ52206. As a result of these changes, modify both the power > and temperature read and write operations. > Add kind and struct ina238_config *config in struct ina238_data > to save the chip type and different configurations for each > chip type, promoting program reusability. > Due to the temperature reading of SQ52206 being a signed 16 bit > value, while INA238 is a 12 bit value. So we changed the > temperature reading function. > Extract the chip initialization process into a separate function > named ina238_init to facilitate adjustments for various chip types. > Add a corresponding compatible to the driver. > > drivers/hwmon/ina238.c | 209 ++++++++++++++++++++++++++++------------- > 1 file changed, 144 insertions(+), 65 deletions(-) > > diff --git a/drivers/hwmon/ina238.c b/drivers/hwmon/ina238.c > index 2d9f12f68d50..131f5faefdb3 100644 > --- a/drivers/hwmon/ina238.c > +++ b/drivers/hwmon/ina238.c > @@ -21,11 +21,14 @@ > #define INA238_CONFIG 0x0 > #define INA238_ADC_CONFIG 0x1 > #define INA238_SHUNT_CALIBRATION 0x2 > +#define SQ52206_SHUNT_TEMPCO 0x3 > #define INA238_SHUNT_VOLTAGE 0x4 > #define INA238_BUS_VOLTAGE 0x5 > #define INA238_DIE_TEMP 0x6 > #define INA238_CURRENT 0x7 > #define INA238_POWER 0x8 > +#define SQ52206_ENERGY 0x9 > +#define SQ52206_CHARGE 0xa > #define INA238_DIAG_ALERT 0xb > #define INA238_SHUNT_OVER_VOLTAGE 0xc > #define INA238_SHUNT_UNDER_VOLTAGE 0xd > @@ -33,9 +36,12 @@ > #define INA238_BUS_UNDER_VOLTAGE 0xf > #define INA238_TEMP_LIMIT 0x10 > #define INA238_POWER_LIMIT 0x11 > +#define SQ52206_POWER_PEAK 0x20 > #define INA238_DEVICE_ID 0x3f /* not available on INA237 */ > > #define INA238_CONFIG_ADCRANGE BIT(4) > +#define SQ52206_CONFIG_ADCRANGE GENMASK(4, 3) > +#define SQ52206_READ_ADCRANGE(reg) FIELD_GET(SQ52206_CONFIG_ADCRANGE, reg) > > #define INA238_DIAG_ALERT_TMPOL BIT(7) > #define INA238_DIAG_ALERT_SHNTOL BIT(6) > @@ -44,7 +50,7 @@ > #define INA238_DIAG_ALERT_BUSUL BIT(3) > #define INA238_DIAG_ALERT_POL BIT(2) > > -#define INA238_REGISTERS 0x11 > +#define INA238_MAX_REGISTERS 0x20 Why this change ? > > #define INA238_RSHUNT_DEFAULT 10000 /* uOhm */ > > @@ -87,27 +93,63 @@ > * shunt = 0x4000 / (819.2 * 10^6) / 0.001 = 20000 uOhms (with 1mA/lsb) > * > * Current (mA) = register value * 20000 / rshunt / 4 * gain > - * Power (W) = 0.2 * register value * 20000 / rshunt / 4 * gain > + * Power (mW) = 0.2 * register value * 20000 / rshunt / 4 * gain > + * (Specific for SQ52206) > + * Power (mW) = 0.24 * register value * 20000 / rshunt / 4 * gain > */ > #define INA238_CALIBRATION_VALUE 16384 > -#define INA238_FIXED_SHUNT 20000 > +#define INA238_FIXED_SHUNT 20000 Why this change ? > > #define INA238_SHUNT_VOLTAGE_LSB 5 /* 5 uV/lsb */ > #define INA238_BUS_VOLTAGE_LSB 3125 /* 3.125 mV/lsb */ > -#define INA238_DIE_TEMP_LSB 125 /* 125 mC/lsb */ > +#define INA238_DIE_TEMP_LSB 1250000 /* 125 mC/lsb */ > +#define SQ52206_BUS_VOLTAGE_LSB 3750 /* 3.75 mV/lsb */ > +#define SQ52206_DIE_TEMP_LSB 78125 /* 7.8125 mC/lsb */ > > static const struct regmap_config ina238_regmap_config = { > - .max_register = INA238_REGISTERS, > + .max_register = INA238_MAX_REGISTERS, > .reg_bits = 8, > .val_bits = 16, > }; > > +enum ina238_ids { ina238, ina237, sq52206 }; > + > +struct ina238_config { > + u16 config_default; > + int calibration_value; .config_default and .calibration_value is the same for all chips. I don't see the point of not just using the respective defines. > + int bus_voltage_lsb; /* uV */ > + int temp_lsb; /* mC */ > +}; > + > struct ina238_data { > + const struct ina238_config *config; > struct i2c_client *client; > struct mutex config_lock; > struct regmap *regmap; > u32 rshunt; > int gain; > + int kind; > +}; > + > +static const struct ina238_config ina238_config[] = { > + [ina238] = { > + .config_default = INA238_CONFIG_DEFAULT, > + .calibration_value = INA238_CALIBRATION_VALUE, > + .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB, > + .temp_lsb = INA238_DIE_TEMP_LSB, > + }, > + [ina237] = { > + .config_default = INA238_CONFIG_DEFAULT, > + .calibration_value = INA238_CALIBRATION_VALUE, > + .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB, > + .temp_lsb = INA238_DIE_TEMP_LSB, > + }, > + [sq52206] = { > + .config_default = INA238_CONFIG_DEFAULT, > + .calibration_value = INA238_CALIBRATION_VALUE, > + .bus_voltage_lsb = SQ52206_BUS_VOLTAGE_LSB, > + .temp_lsb = SQ52206_DIE_TEMP_LSB, > + }, > }; > > static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val) > @@ -197,10 +239,10 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel, > regval = (s16)regval; > if (channel == 0) > /* gain of 1 -> LSB / 4 */ > - *val = (regval * INA238_SHUNT_VOLTAGE_LSB) / > - (1000 * (4 - data->gain + 1)); > + *val = (regval * INA238_SHUNT_VOLTAGE_LSB) * > + data->gain / (1000 * 4); Why this change ? > else > - *val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000; > + *val = (regval * data->config->bus_voltage_lsb) / 1000; > break; > case hwmon_in_max_alarm: > case hwmon_in_min_alarm: > @@ -225,8 +267,8 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel, > case 0: > /* signed value, clamp to max range +/-163 mV */ > regval = clamp_val(val, -163, 163); > - regval = (regval * 1000 * (4 - data->gain + 1)) / > - INA238_SHUNT_VOLTAGE_LSB; > + regval = (regval * 1000 * 4) / > + INA238_SHUNT_VOLTAGE_LSB * data->gain; Why this change ? > regval = clamp_val(regval, S16_MIN, S16_MAX); > > switch (attr) { > @@ -242,7 +284,7 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel, > case 1: > /* signed value, positive values only. Clamp to max 102.396 V */ > regval = clamp_val(val, 0, 102396); > - regval = (regval * 1000) / INA238_BUS_VOLTAGE_LSB; > + regval = (regval * 1000) / data->config->bus_voltage_lsb; > regval = clamp_val(regval, 0, S16_MAX); > > switch (attr) { > @@ -295,10 +337,14 @@ static int ina238_read_power(struct device *dev, u32 attr, long *val) > err = ina238_read_reg24(data->client, INA238_POWER, ®val); > if (err) > return err; > - > - /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */ > - power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT * > - data->gain, 20 * data->rshunt); > + if (data->kind == sq52206) > + /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */ > + power = div_u64(regval * 1200ULL * INA238_FIXED_SHUNT * > + data->gain, 20 * data->rshunt); > + else > + /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */ > + power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT * > + data->gain, 20 * data->rshunt); > /* Clamp value to maximum value of long */ > *val = clamp_val(power, 0, LONG_MAX); > break; > @@ -311,8 +357,12 @@ static int ina238_read_power(struct device *dev, u32 attr, long *val) > * Truncated 24-bit compare register, lower 8-bits are > * truncated. Same conversion to/from uW as POWER register. > */ > - power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT * > - data->gain, 20 * data->rshunt); > + if (data->kind == sq52206) > + power = div_u64((regval << 8) * 1200ULL * INA238_FIXED_SHUNT * > + data->gain, 20 * data->rshunt); > + else > + power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT * > + data->gain, 20 * data->rshunt); > /* Clamp value to maximum value of long */ > *val = clamp_val(power, 0, LONG_MAX); > break; > @@ -344,8 +394,12 @@ static int ina238_write_power(struct device *dev, u32 attr, long val) > * register. > */ > regval = clamp_val(val, 0, LONG_MAX); > - regval = div_u64(val * 20ULL * data->rshunt, > - 1000ULL * INA238_FIXED_SHUNT * data->gain); > + if (data->kind == sq52206) > + regval = div_u64(val * 20ULL * data->rshunt, > + 1000ULL * INA238_FIXED_SHUNT * data->gain); > + else > + regval = div_u64(val * 24ULL * data->rshunt, > + 1000ULL * INA238_FIXED_SHUNT * data->gain); There is a constant factor against data->gain for the different chips. Why not incorporate that into data->gain, or use a chip configuration value for it ? > regval = clamp_val(regval >> 8, 0, U16_MAX); > > return regmap_write(data->regmap, INA238_POWER_LIMIT, regval); > @@ -362,17 +416,29 @@ static int ina238_read_temp(struct device *dev, u32 attr, long *val) > err = regmap_read(data->regmap, INA238_DIE_TEMP, ®val); > if (err) > return err; > - > + if (data->kind == sq52206) > + /* Signed, bits 15-0 of register, result in mC */ > + regval = div_s64((s16)regval * data->config->temp_lsb, > + 10000); > + else > /* Signed, bits 15-4 of register, result in mC */ > - *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB; > + *val = div_s64(((s16)regval >> 4) * > + data->config->temp_lsb, 10000); > + *val = clamp_val(regval, S16_MIN, S16_MAX); Why is this clamp_val() now necessary ? It might make sense to add the shift count to struct ina238_config instead of using if/else here. In general, using if/else for some chip specifics and struct ina238_config for others is confusing. I'd suggest to move all chip specific information into struct ina238_config and avoid if/else in runtime code as much as possible. > break; > case hwmon_temp_max: > err = regmap_read(data->regmap, INA238_TEMP_LIMIT, ®val); > if (err) > return err; > - > - /* Signed, bits 15-4 of register, result in mC */ > - *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB; > + if (data->kind == sq52206) > + /* Signed, bits 15-0 of register, result in mC */ > + regval = div_s64((s16)regval * data->config->temp_lsb, > + 10000); > + else > + /* Signed, bits 15-4 of register, result in mC */ > + *val = div_s64(((s16)regval >> 4) * > + data->config->temp_lsb, 10000); > + *val = clamp_val(regval, S16_MIN, S16_MAX); > break; > case hwmon_temp_max_alarm: > err = regmap_read(data->regmap, INA238_DIAG_ALERT, ®val); > @@ -396,9 +462,14 @@ static int ina238_write_temp(struct device *dev, u32 attr, long val) > if (attr != hwmon_temp_max) > return -EOPNOTSUPP; > > - /* Signed, bits 15-4 of register */ > - regval = (val / INA238_DIE_TEMP_LSB) << 4; > - regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0; > + if (data->kind == sq52206) { > + regval = div_u64(val*10000, data->config->temp_lsb); > + regval = clamp_val(regval, S16_MIN, S16_MAX); > + } else { > + /* Signed, bits 15-4 of register */ > + regval = div_u64(val*10000, data->config->temp_lsb) << 4; > + regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0; > + } > > return regmap_write(data->regmap, INA238_TEMP_LIMIT, regval); > } > @@ -530,20 +601,58 @@ static const struct hwmon_chip_info ina238_chip_info = { > .info = ina238_info, > }; > > +/* > + * Initialize the configuration and calibration registers. > + */ > +static int ina238_init(struct ina238_data *data) > +{ > + int ret = 0; > + int config; > + /* Setup CONFIG register */ > + config = data->config->config_default; > + if (data->gain == 1) > + config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */ > + ret = regmap_write(data->regmap, INA238_CONFIG, config); > + if (ret < 0) > + return ret; > + /* Setup ADC_CONFIG register */ > + ret = regmap_write(data->regmap, INA238_ADC_CONFIG, > + INA238_ADC_CONFIG_DEFAULT); > + if (ret < 0) > + return ret; > + /* Setup SHUNT_CALIBRATION register with fixed value */ > + ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION, > + data->config->calibration_value); > + if (ret < 0) > + return ret; > + /* Setup alert/alarm configuration */ > + ret = regmap_write(data->regmap, INA238_DIAG_ALERT, > + INA238_DIAG_ALERT_DEFAULT); > + if (ret < 0) > + return ret; > + return 0; > +} > +static const struct i2c_device_id ina238_id[]; Please avoid forward declarations. > static int ina238_probe(struct i2c_client *client) > { > struct ina2xx_platform_data *pdata = dev_get_platdata(&client->dev); > struct device *dev = &client->dev; > struct device *hwmon_dev; > struct ina238_data *data; > - int config; > + enum ina238_ids chip; > int ret; > > + chip = (uintptr_t)i2c_get_match_data(client); > + > data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > if (!data) > return -ENOMEM; > > + data->kind = chip; > data->client = client; > + /* set the device type */ > + data->config = &ina238_config[data->kind]; > + > mutex_init(&data->config_lock); > > data->regmap = devm_regmap_init_i2c(client, &ina238_regmap_config); > @@ -564,48 +673,15 @@ static int ina238_probe(struct i2c_client *client) > /* load shunt gain value */ > if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0) > data->gain = 4; /* Default of ADCRANGE = 0 */ > - if (data->gain != 1 && data->gain != 4) { > + if (data->gain != 1 && data->gain != 2 && data->gain != 4) { Chip independent changes should be in separate patch(es). > dev_err(dev, "invalid shunt gain value %u\n", data->gain); > return -EINVAL; > } > - > - /* Setup CONFIG register */ > - config = INA238_CONFIG_DEFAULT; > - if (data->gain == 1) > - config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */ > - ret = regmap_write(data->regmap, INA238_CONFIG, config); > - if (ret < 0) { > - dev_err(dev, "error configuring the device: %d\n", ret); > - return -ENODEV; > - } > - > - /* Setup ADC_CONFIG register */ > - ret = regmap_write(data->regmap, INA238_ADC_CONFIG, > - INA238_ADC_CONFIG_DEFAULT); > - if (ret < 0) { > - dev_err(dev, "error configuring the device: %d\n", ret); > - return -ENODEV; > - } > - > - /* Setup SHUNT_CALIBRATION register with fixed value */ > - ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION, > - INA238_CALIBRATION_VALUE); > - if (ret < 0) { > - dev_err(dev, "error configuring the device: %d\n", ret); > - return -ENODEV; > - } > - > - /* Setup alert/alarm configuration */ > - ret = regmap_write(data->regmap, INA238_DIAG_ALERT, > - INA238_DIAG_ALERT_DEFAULT); > - if (ret < 0) { > - dev_err(dev, "error configuring the device: %d\n", ret); > - return -ENODEV; > - } > + ret = ina238_init(data); > Errors must not be ignored. > hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, > &ina238_chip_info, > - NULL); > + ina238_groups); > if (IS_ERR(hwmon_dev)) > return PTR_ERR(hwmon_dev); > > @@ -616,7 +692,9 @@ static int ina238_probe(struct i2c_client *client) > } > > static const struct i2c_device_id ina238_id[] = { > - { "ina238" }, > + { "ina238", ina238 }, > + { "ina237", ina237 }, Unrelated change, and out of order (ina237 should come first). > + { "sq52206", sq52206 }, > { } > }; > MODULE_DEVICE_TABLE(i2c, ina238_id); > @@ -624,6 +702,7 @@ MODULE_DEVICE_TABLE(i2c, ina238_id); > static const struct of_device_id __maybe_unused ina238_of_match[] = { > { .compatible = "ti,ina237" }, > { .compatible = "ti,ina238" }, > + { .compatible = "silergy,sq52206" }, .data is missing. > { }, > }; > MODULE_DEVICE_TABLE(of, ina238_of_match); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] hwmon:(ina238)Add support for SQ52206 2025-01-06 23:22 ` Guenter Roeck @ 2025-01-07 12:43 ` Wenliang Yan 2025-01-07 15:02 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Wenliang Yan @ 2025-01-07 12:43 UTC (permalink / raw) To: linux Cc: conor+dt, corbet, jdelvare, krzk+dt, linux-hwmon, linux-kernel, robh, wenliang202407 Add support for SQ52206, the datasheet depends on https://us1.silergy.com/cloud/index/uniqid/676b659b4a503 The password is fx6NEe. At 2025-01-07 07:22:01, "Guenter Roeck" <linux@roeck-us.net> wrote: >On 12/23/24 22:35, Wenliang Yan wrote: >> From: Wenliang <wenliang202407@163.com> >> >> Add support for SQ52206 to the Ina238 driver. Add registers, >> add calculation formulas, increase compatibility, adjust >> struct and chip initialization. >> >> Signed-off-by: Wenliang <wenliang202407@163.com> >> --- >> >> Incorporate four additional registers to the original register >> set of SQ52206 beyond INA238. >> The ADC measurement range of SQ52206 is divided into 1/2/4, >> so change the configuration of INA238_ADC_CONFIG. >> SQ52206's calculation of power read values is different from >> INA238.Add new value of BUS_VOLTAGE_LSB and DIE-TEMP_LSB for >> SQ52206. As a result of these changes, modify both the power >> and temperature read and write operations. >> Add kind and struct ina238_config *config in struct ina238_data >> to save the chip type and different configurations for each >> chip type, promoting program reusability. >> Due to the temperature reading of SQ52206 being a signed 16 bit >> value, while INA238 is a 12 bit value. So we changed the >> temperature reading function. >> Extract the chip initialization process into a separate function >> named ina238_init to facilitate adjustments for various chip types. >> Add a corresponding compatible to the driver. >> >> drivers/hwmon/ina238.c | 209 ++++++++++++++++++++++++++++------------- >> 1 file changed, 144 insertions(+), 65 deletions(-) >> >> diff --git a/drivers/hwmon/ina238.c b/drivers/hwmon/ina238.c >> index 2d9f12f68d50..131f5faefdb3 100644 >> --- a/drivers/hwmon/ina238.c >> +++ b/drivers/hwmon/ina238.c >> @@ -21,11 +21,14 @@ >> #define INA238_CONFIG 0x0 >> #define INA238_ADC_CONFIG 0x1 >> #define INA238_SHUNT_CALIBRATION 0x2 >> +#define SQ52206_SHUNT_TEMPCO 0x3 >> #define INA238_SHUNT_VOLTAGE 0x4 >> #define INA238_BUS_VOLTAGE 0x5 >> #define INA238_DIE_TEMP 0x6 >> #define INA238_CURRENT 0x7 >> #define INA238_POWER 0x8 >> +#define SQ52206_ENERGY 0x9 >> +#define SQ52206_CHARGE 0xa >> #define INA238_DIAG_ALERT 0xb >> #define INA238_SHUNT_OVER_VOLTAGE 0xc >> #define INA238_SHUNT_UNDER_VOLTAGE 0xd >> @@ -33,9 +36,12 @@ >> #define INA238_BUS_UNDER_VOLTAGE 0xf >> #define INA238_TEMP_LIMIT 0x10 >> #define INA238_POWER_LIMIT 0x11 >> +#define SQ52206_POWER_PEAK 0x20 >> #define INA238_DEVICE_ID 0x3f /* not available on INA237 */ >> >> #define INA238_CONFIG_ADCRANGE BIT(4) >> +#define SQ52206_CONFIG_ADCRANGE GENMASK(4, 3) >> +#define SQ52206_READ_ADCRANGE(reg) FIELD_GET(SQ52206_CONFIG_ADCRANGE, reg) >> >> #define INA238_DIAG_ALERT_TMPOL BIT(7) >> #define INA238_DIAG_ALERT_SHNTOL BIT(6) >> @@ -44,7 +50,7 @@ >> #define INA238_DIAG_ALERT_BUSUL BIT(3) >> #define INA238_DIAG_ALERT_POL BIT(2) >> >> -#define INA238_REGISTERS 0x11 >> +#define INA238_MAX_REGISTERS 0x20 > >Why this change ? > The maximum register address for SQ52206 is 0x20. >> >> #define INA238_RSHUNT_DEFAULT 10000 /* uOhm */ >> >> @@ -87,27 +93,63 @@ >> * shunt = 0x4000 / (819.2 * 10^6) / 0.001 = 20000 uOhms (with 1mA/lsb) >> * >> * Current (mA) = register value * 20000 / rshunt / 4 * gain >> - * Power (W) = 0.2 * register value * 20000 / rshunt / 4 * gain >> + * Power (mW) = 0.2 * register value * 20000 / rshunt / 4 * gain >> + * (Specific for SQ52206) >> + * Power (mW) = 0.24 * register value * 20000 / rshunt / 4 * gain >> */ >> #define INA238_CALIBRATION_VALUE 16384 >> -#define INA238_FIXED_SHUNT 20000 >> +#define INA238_FIXED_SHUNT 20000 > >Why this change ? > Also refer to the chip manual provided in the website above. >> >> #define INA238_SHUNT_VOLTAGE_LSB 5 /* 5 uV/lsb */ >> #define INA238_BUS_VOLTAGE_LSB 3125 /* 3.125 mV/lsb */ >> -#define INA238_DIE_TEMP_LSB 125 /* 125 mC/lsb */ >> +#define INA238_DIE_TEMP_LSB 1250000 /* 125 mC/lsb */ >> +#define SQ52206_BUS_VOLTAGE_LSB 3750 /* 3.75 mV/lsb */ >> +#define SQ52206_DIE_TEMP_LSB 78125 /* 7.8125 mC/lsb */ >> >> static const struct regmap_config ina238_regmap_config = { >> - .max_register = INA238_REGISTERS, >> + .max_register = INA238_MAX_REGISTERS, >> .reg_bits = 8, >> .val_bits = 16, >> }; >> >> +enum ina238_ids { ina238, ina237, sq52206 }; >> + >> +struct ina238_config { >> + u16 config_default; >> + int calibration_value; > >.config_default and .calibration_value is the same for all chips. >I don't see the point of not just using the respective defines. > Okay, I will change the definition here >> + int bus_voltage_lsb; /* uV */ >> + int temp_lsb; /* mC */ >> +}; >> + >> struct ina238_data { >> + const struct ina238_config *config; >> struct i2c_client *client; >> struct mutex config_lock; >> struct regmap *regmap; >> u32 rshunt; >> int gain; >> + int kind; >> +}; >> + >> +static const struct ina238_config ina238_config[] = { >> + [ina238] = { >> + .config_default = INA238_CONFIG_DEFAULT, >> + .calibration_value = INA238_CALIBRATION_VALUE, >> + .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB, >> + .temp_lsb = INA238_DIE_TEMP_LSB, >> + }, >> + [ina237] = { >> + .config_default = INA238_CONFIG_DEFAULT, >> + .calibration_value = INA238_CALIBRATION_VALUE, >> + .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB, >> + .temp_lsb = INA238_DIE_TEMP_LSB, >> + }, >> + [sq52206] = { >> + .config_default = INA238_CONFIG_DEFAULT, >> + .calibration_value = INA238_CALIBRATION_VALUE, >> + .bus_voltage_lsb = SQ52206_BUS_VOLTAGE_LSB, >> + .temp_lsb = SQ52206_DIE_TEMP_LSB, >> + }, >> }; >> >> static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val) >> @@ -197,10 +239,10 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel, >> regval = (s16)regval; >> if (channel == 0) >> /* gain of 1 -> LSB / 4 */ >> - *val = (regval * INA238_SHUNT_VOLTAGE_LSB) / >> - (1000 * (4 - data->gain + 1)); >> + *val = (regval * INA238_SHUNT_VOLTAGE_LSB) * >> + data->gain / (1000 * 4); > >Why this change ? > Because INA238 only has two gains of 1 and 4, the previous formula can be used, but SQ52206 has a gain of 2, so I changed the formula >> else >> - *val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000; >> + *val = (regval * data->config->bus_voltage_lsb) / 1000; >> break; >> case hwmon_in_max_alarm: >> case hwmon_in_min_alarm: >> @@ -225,8 +267,8 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel, >> case 0: >> /* signed value, clamp to max range +/-163 mV */ >> regval = clamp_val(val, -163, 163); >> - regval = (regval * 1000 * (4 - data->gain + 1)) / >> - INA238_SHUNT_VOLTAGE_LSB; >> + regval = (regval * 1000 * 4) / >> + INA238_SHUNT_VOLTAGE_LSB * data->gain; > >Why this change ? > Consistent with the reason described in the previous article. >> regval = clamp_val(regval, S16_MIN, S16_MAX); >> >> switch (attr) { >> @@ -242,7 +284,7 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel, >> case 1: >> /* signed value, positive values only. Clamp to max 102.396 V */ >> regval = clamp_val(val, 0, 102396); >> - regval = (regval * 1000) / INA238_BUS_VOLTAGE_LSB; >> + regval = (regval * 1000) / data->config->bus_voltage_lsb; >> regval = clamp_val(regval, 0, S16_MAX); >> >> switch (attr) { >> @@ -295,10 +337,14 @@ static int ina238_read_power(struct device *dev, u32 attr, long *val) >> err = ina238_read_reg24(data->client, INA238_POWER, ®val); >> if (err) >> return err; >> - >> - /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */ >> - power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT * >> - data->gain, 20 * data->rshunt); >> + if (data->kind == sq52206) >> + /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */ >> + power = div_u64(regval * 1200ULL * INA238_FIXED_SHUNT * >> + data->gain, 20 * data->rshunt); >> + else >> + /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */ >> + power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT * >> + data->gain, 20 * data->rshunt); > > >> /* Clamp value to maximum value of long */ >> *val = clamp_val(power, 0, LONG_MAX); >> break; >> @@ -311,8 +357,12 @@ static int ina238_read_power(struct device *dev, u32 attr, long *val) >> * Truncated 24-bit compare register, lower 8-bits are >> * truncated. Same conversion to/from uW as POWER register. >> */ >> - power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT * >> - data->gain, 20 * data->rshunt); >> + if (data->kind == sq52206) >> + power = div_u64((regval << 8) * 1200ULL * INA238_FIXED_SHUNT * >> + data->gain, 20 * data->rshunt); >> + else >> + power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT * >> + data->gain, 20 * data->rshunt); >> /* Clamp value to maximum value of long */ >> *val = clamp_val(power, 0, LONG_MAX); >> break; >> @@ -344,8 +394,12 @@ static int ina238_write_power(struct device *dev, u32 attr, long val) >> * register. >> */ >> regval = clamp_val(val, 0, LONG_MAX); >> - regval = div_u64(val * 20ULL * data->rshunt, >> - 1000ULL * INA238_FIXED_SHUNT * data->gain); >> + if (data->kind == sq52206) >> + regval = div_u64(val * 20ULL * data->rshunt, >> + 1000ULL * INA238_FIXED_SHUNT * data->gain); >> + else >> + regval = div_u64(val * 24ULL * data->rshunt, >> + 1000ULL * INA238_FIXED_SHUNT * data->gain); > >There is a constant factor against data->gain for the different chips. >Why not incorporate that into data->gain, or use a chip configuration >value for it ? > I will make adjustments in V2. >> regval = clamp_val(regval >> 8, 0, U16_MAX); >> >> return regmap_write(data->regmap, INA238_POWER_LIMIT, regval); >> @@ -362,17 +416,29 @@ static int ina238_read_temp(struct device *dev, u32 attr, long *val) >> err = regmap_read(data->regmap, INA238_DIE_TEMP, ®val); >> if (err) >> return err; >> - >> + if (data->kind == sq52206) >> + /* Signed, bits 15-0 of register, result in mC */ >> + regval = div_s64((s16)regval * data->config->temp_lsb, >> + 10000); >> + else >> /* Signed, bits 15-4 of register, result in mC */ >> - *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB; >> + *val = div_s64(((s16)regval >> 4) * >> + data->config->temp_lsb, 10000); >> + *val = clamp_val(regval, S16_MIN, S16_MAX); > >Why is this clamp_val() now necessary ? > Yes, it's not necessary. I will make the necessary modifications. >It might make sense to add the shift count to struct ina238_config >instead of using if/else here. > >In general, using if/else for some chip specifics and struct ina238_config >for others is confusing. I'd suggest to move all chip specific information >into struct ina238_config and avoid if/else in runtime code as much as possible. > I will move all chip specific information into struct ina238_config. >> break; >> case hwmon_temp_max: >> err = regmap_read(data->regmap, INA238_TEMP_LIMIT, ®val); >> if (err) >> return err; >> - >> - /* Signed, bits 15-4 of register, result in mC */ >> - *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB; >> + if (data->kind == sq52206) >> + /* Signed, bits 15-0 of register, result in mC */ >> + regval = div_s64((s16)regval * data->config->temp_lsb, >> + 10000); >> + else >> + /* Signed, bits 15-4 of register, result in mC */ >> + *val = div_s64(((s16)regval >> 4) * >> + data->config->temp_lsb, 10000); >> + *val = clamp_val(regval, S16_MIN, S16_MAX); >> break; >> case hwmon_temp_max_alarm: >> err = regmap_read(data->regmap, INA238_DIAG_ALERT, ®val); >> @@ -396,9 +462,14 @@ static int ina238_write_temp(struct device *dev, u32 attr, long val) >> if (attr != hwmon_temp_max) >> return -EOPNOTSUPP; >> >> - /* Signed, bits 15-4 of register */ >> - regval = (val / INA238_DIE_TEMP_LSB) << 4; >> - regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0; >> + if (data->kind == sq52206) { >> + regval = div_u64(val*10000, data->config->temp_lsb); >> + regval = clamp_val(regval, S16_MIN, S16_MAX); >> + } else { >> + /* Signed, bits 15-4 of register */ >> + regval = div_u64(val*10000, data->config->temp_lsb) << 4; >> + regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0; >> + } >> >> return regmap_write(data->regmap, INA238_TEMP_LIMIT, regval); >> } >> @@ -530,20 +601,58 @@ static const struct hwmon_chip_info ina238_chip_info = { >> .info = ina238_info, >> }; >> >> +/* >> + * Initialize the configuration and calibration registers. >> + */ >> +static int ina238_init(struct ina238_data *data) >> +{ >> + int ret = 0; >> + int config; >> + /* Setup CONFIG register */ >> + config = data->config->config_default; >> + if (data->gain == 1) >> + config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */ >> + ret = regmap_write(data->regmap, INA238_CONFIG, config); >> + if (ret < 0) >> + return ret; >> + /* Setup ADC_CONFIG register */ >> + ret = regmap_write(data->regmap, INA238_ADC_CONFIG, >> + INA238_ADC_CONFIG_DEFAULT); >> + if (ret < 0) >> + return ret; >> + /* Setup SHUNT_CALIBRATION register with fixed value */ >> + ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION, >> + data->config->calibration_value); >> + if (ret < 0) >> + return ret; >> + /* Setup alert/alarm configuration */ >> + ret = regmap_write(data->regmap, INA238_DIAG_ALERT, >> + INA238_DIAG_ALERT_DEFAULT); >> + if (ret < 0) >> + return ret; >> + return 0; >> +} >> +static const struct i2c_device_id ina238_id[]; > >Please avoid forward declarations. > I will make adjustments in V2. >> static int ina238_probe(struct i2c_client *client) >> { >> struct ina2xx_platform_data *pdata = dev_get_platdata(&client->dev); >> struct device *dev = &client->dev; >> struct device *hwmon_dev; >> struct ina238_data *data; >> - int config; >> + enum ina238_ids chip; >> int ret; >> >> + chip = (uintptr_t)i2c_get_match_data(client); >> + >> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); >> if (!data) >> return -ENOMEM; >> >> + data->kind = chip; >> data->client = client; >> + /* set the device type */ >> + data->config = &ina238_config[data->kind]; >> + >> mutex_init(&data->config_lock); >> >> data->regmap = devm_regmap_init_i2c(client, &ina238_regmap_config); >> @@ -564,48 +673,15 @@ static int ina238_probe(struct i2c_client *client) >> /* load shunt gain value */ >> if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0) >> data->gain = 4; /* Default of ADCRANGE = 0 */ >> - if (data->gain != 1 && data->gain != 4) { >> + if (data->gain != 1 && data->gain != 2 && data->gain != 4) { > >Chip independent changes should be in separate patch(es). > SQ52206 has a gain of 2. >> dev_err(dev, "invalid shunt gain value %u\n", data->gain); >> return -EINVAL; >> } >> - >> - /* Setup CONFIG register */ >> - config = INA238_CONFIG_DEFAULT; >> - if (data->gain == 1) >> - config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */ >> - ret = regmap_write(data->regmap, INA238_CONFIG, config); >> - if (ret < 0) { >> - dev_err(dev, "error configuring the device: %d\n", ret); >> - return -ENODEV; >> - } >> - >> - /* Setup ADC_CONFIG register */ >> - ret = regmap_write(data->regmap, INA238_ADC_CONFIG, >> - INA238_ADC_CONFIG_DEFAULT); >> - if (ret < 0) { >> - dev_err(dev, "error configuring the device: %d\n", ret); >> - return -ENODEV; >> - } >> - >> - /* Setup SHUNT_CALIBRATION register with fixed value */ >> - ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION, >> - INA238_CALIBRATION_VALUE); >> - if (ret < 0) { >> - dev_err(dev, "error configuring the device: %d\n", ret); >> - return -ENODEV; >> - } >> - >> - /* Setup alert/alarm configuration */ >> - ret = regmap_write(data->regmap, INA238_DIAG_ALERT, >> - INA238_DIAG_ALERT_DEFAULT); >> - if (ret < 0) { >> - dev_err(dev, "error configuring the device: %d\n", ret); >> - return -ENODEV; >> - } >> + ret = ina238_init(data); >> > >Errors must not be ignored. > I will make adjustments in V2. >> hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, >> &ina238_chip_info, >> - NULL); >> + ina238_groups); >> if (IS_ERR(hwmon_dev)) >> return PTR_ERR(hwmon_dev); >> >> @@ -616,7 +692,9 @@ static int ina238_probe(struct i2c_client *client) >> } >> >> static const struct i2c_device_id ina238_id[] = { >> - { "ina238" }, >> + { "ina238", ina238 }, >> + { "ina237", ina237 }, > >Unrelated change, and out of order (ina237 should come first). > I will make adjustments in V2. >> + { "sq52206", sq52206 }, >> { } >> }; >> MODULE_DEVICE_TABLE(i2c, ina238_id); >> @@ -624,6 +702,7 @@ MODULE_DEVICE_TABLE(i2c, ina238_id); >> static const struct of_device_id __maybe_unused ina238_of_match[] = { >> { .compatible = "ti,ina237" }, >> { .compatible = "ti,ina238" }, >> + { .compatible = "silergy,sq52206" }, > >.data is missing. > I will make adjustments in V2. >> { }, >> }; >> MODULE_DEVICE_TABLE(of, ina238_of_match); Thanks, Guenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] hwmon:(ina238)Add support for SQ52206 2025-01-07 12:43 ` Wenliang Yan @ 2025-01-07 15:02 ` Guenter Roeck 2025-01-08 2:53 ` Wenliang Yan 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2025-01-07 15:02 UTC (permalink / raw) To: Wenliang Yan Cc: conor+dt, corbet, jdelvare, krzk+dt, linux-hwmon, linux-kernel, robh On 1/7/25 04:43, Wenliang Yan wrote: ... >>> -#define INA238_REGISTERS 0x11 >>> +#define INA238_MAX_REGISTERS 0x20 >> >> Why this change ? >> > > The maximum register address for SQ52206 is 0x20. > That isn't what I referred to. Th value change is ok. The question was why change INA238_REGISTERS to INA238_MAX_REGISTERS. That is a personal preference change. I strongly discourage such changes because if I accept them someone else may come tomorrow and change the name again to match their preference. >>> -#define INA238_FIXED_SHUNT 20000 >>> +#define INA238_FIXED_SHUNT 20000 >> >> Why this change ? >> > > Also refer to the chip manual provided in the website above. > The value didn't change. The indentation changed without reason. >>> >>> static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val) >>> @@ -197,10 +239,10 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel, >>> regval = (s16)regval; >>> if (channel == 0) >>> /* gain of 1 -> LSB / 4 */ >>> - *val = (regval * INA238_SHUNT_VOLTAGE_LSB) / >>> - (1000 * (4 - data->gain + 1)); >>> + *val = (regval * INA238_SHUNT_VOLTAGE_LSB) * >>> + data->gain / (1000 * 4); >> >> Why this change ? >> > > Because INA238 only has two gains of 1 and 4, the previous formula can > be used, but SQ52206 has a gain of 2, so I changed the formula > Ok. >>> else >>> - *val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000; >>> + *val = (regval * data->config->bus_voltage_lsb) / 1000; >>> break; >>> case hwmon_in_max_alarm: >>> case hwmon_in_min_alarm: >>> @@ -225,8 +267,8 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel, >>> case 0: >>> /* signed value, clamp to max range +/-163 mV */ >>> regval = clamp_val(val, -163, 163); >>> - regval = (regval * 1000 * (4 - data->gain + 1)) / >>> - INA238_SHUNT_VOLTAGE_LSB; >>> + regval = (regval * 1000 * 4) / >>> + INA238_SHUNT_VOLTAGE_LSB * data->gain; >> >> Why this change ? >> > > Consistent with the reason described in the previous article. > Ok. >>> data->regmap = devm_regmap_init_i2c(client, &ina238_regmap_config); >>> @@ -564,48 +673,15 @@ static int ina238_probe(struct i2c_client *client) >>> /* load shunt gain value */ >>> if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0) >>> data->gain = 4; /* Default of ADCRANGE = 0 */ >>> - if (data->gain != 1 && data->gain != 4) { >>> + if (data->gain != 1 && data->gain != 2 && data->gain != 4) { >> >> Chip independent changes should be in separate patch(es). >> > > SQ52206 has a gain of 2. > Ok. Thanks, Guenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] hwmon:(ina238)Add support for SQ52206 2025-01-07 15:02 ` Guenter Roeck @ 2025-01-08 2:53 ` Wenliang Yan 0 siblings, 0 replies; 6+ messages in thread From: Wenliang Yan @ 2025-01-08 2:53 UTC (permalink / raw) To: linux Cc: conor+dt, corbet, jdelvare, krzk+dt, linux-hwmon, linux-kernel, robh, wenliang202407 At 2025-01-07 23:02:09, "Guenter Roeck" <linux@roeck-us.net> wrote: >On 1/7/25 04:43, Wenliang Yan wrote: >... >>>> -#define INA238_REGISTERS 0x11 >>>> +#define INA238_MAX_REGISTERS 0x20 >>> >>> Why this change ? >>> >> >> The maximum register address for SQ52206 is 0x20. >> > >That isn't what I referred to. Th value change is ok. >The question was why change INA238_REGISTERS to INA238_MAX_REGISTERS. >That is a personal preference change. I strongly discourage such >changes because if I accept them someone else may come tomorrow >and change the name again to match their preference. > I understand, I will modify. >>>> -#define INA238_FIXED_SHUNT 20000 >>>> +#define INA238_FIXED_SHUNT 20000 >>> >>> Why this change ? >>> >> >> Also refer to the chip manual provided in the website above. >> > >The value didn't change. The indentation changed without reason. > This is due to my format alignment, I will adjust it back. >>>> >>>> static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val) >>>> @@ -197,10 +239,10 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel, >>>> regval = (s16)regval; >>>> if (channel == 0) >>>> /* gain of 1 -> LSB / 4 */ >>>> - *val = (regval * INA238_SHUNT_VOLTAGE_LSB) / >>>> - (1000 * (4 - data->gain + 1)); >>>> + *val = (regval * INA238_SHUNT_VOLTAGE_LSB) * >>>> + data->gain / (1000 * 4); >>> >>> Why this change ? >>> >> >> Because INA238 only has two gains of 1 and 4, the previous formula can >> be used, but SQ52206 has a gain of 2, so I changed the formula >> >Ok. > >>>> else >>>> - *val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000; >>>> + *val = (regval * data->config->bus_voltage_lsb) / 1000; >>>> break; >>>> case hwmon_in_max_alarm: >>>> case hwmon_in_min_alarm: >>>> @@ -225,8 +267,8 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel, >>>> case 0: >>>> /* signed value, clamp to max range +/-163 mV */ >>>> regval = clamp_val(val, -163, 163); >>>> - regval = (regval * 1000 * (4 - data->gain + 1)) / >>>> - INA238_SHUNT_VOLTAGE_LSB; >>>> + regval = (regval * 1000 * 4) / >>>> + INA238_SHUNT_VOLTAGE_LSB * data->gain; >>> >>> Why this change ? >>> >> >> Consistent with the reason described in the previous article. >> >Ok. > >>>> data->regmap = devm_regmap_init_i2c(client, &ina238_regmap_config); >>>> @@ -564,48 +673,15 @@ static int ina238_probe(struct i2c_client *client) >>>> /* load shunt gain value */ >>>> if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0) >>>> data->gain = 4; /* Default of ADCRANGE = 0 */ >>>> - if (data->gain != 1 && data->gain != 4) { >>>> + if (data->gain != 1 && data->gain != 2 && data->gain != 4) { >>> >>> Chip independent changes should be in separate patch(es). >>> >> >> SQ52206 has a gain of 2. >> >Ok. > > >Thanks, >Guenter Thanks, Wenliang Yan ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-01-08 2:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-12-24 6:35 [PATCH 0/3] hwmon:(ina238)Add support for SQ52206 Wenliang Yan 2024-12-24 6:35 ` [PATCH 1/3] " Wenliang Yan 2025-01-06 23:22 ` Guenter Roeck 2025-01-07 12:43 ` Wenliang Yan 2025-01-07 15:02 ` Guenter Roeck 2025-01-08 2:53 ` Wenliang Yan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®