| Message ID | 20240906061421.9392-4-Delphine_CC_Chiu@wiwynn.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (isl28022) new driver for ISL28022 power monitor | expand |
On 06/09/2024 08:14, Delphine CC Chiu wrote: > Added support reading shunt voltage in mV and revise code > for Renesas ISL28022. > > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com> > --- > drivers/hwmon/isl28022.c | 93 ++++++++++++++++++++++++++++------------ > 1 file changed, 66 insertions(+), 27 deletions(-) > > diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c > index f0494c3bd483..01220fad813d 100644 > --- a/drivers/hwmon/isl28022.c > +++ b/drivers/hwmon/isl28022.c > @@ -85,8 +85,6 @@ struct isl28022_data { > u32 shunt; > u32 gain; > u32 average; > - u16 config; > - u16 calib; > }; > > static int isl28022_read(struct device *dev, enum hwmon_sensor_types type, > @@ -95,20 +93,61 @@ static int isl28022_read(struct device *dev, enum hwmon_sensor_types type, > struct isl28022_data *data = dev_get_drvdata(dev); > unsigned int regval; > int err; > + u16 sign_bit; > > switch (type) { > case hwmon_in: > - switch (attr) { > - case hwmon_in_input: > - err = regmap_read(data->regmap, > - ISL28022_REG_BUS, ®val); > - if (err < 0) > - return err; > - /* driver supports only 60V mode (BRNG 11) */ > - *val = (long)(((u16)regval) & 0xFFFC); > + switch (channel) { > + case 0: > + switch (attr) { > + case hwmon_in_input: > + err = regmap_read(data->regmap, > + ISL28022_REG_BUS, ®val); > + if (err < 0) > + return err; > + /* driver supports only 60V mode (BRNG 11) */ > + *val = (long)(((u16)regval) & 0xFFFC); > + break; > + default: > + return -EOPNOTSUPP; > + } > + break; > + case 1: > + switch (attr) { > + case hwmon_in_input: > + err = regmap_read(data->regmap, > + ISL28022_REG_SHUNT, ®val); > + if (err < 0) > + return err; > + switch (data->gain) { > + case 8: > + sign_bit = (regval >> 15) & 0x01; > + *val = (long)((((u16)regval) & 0x7FFF) - > + (sign_bit * 32768)) / 100; > + break; > + case 4: > + sign_bit = (regval >> 14) & 0x01; > + *val = (long)((((u16)regval) & 0x3FFF) - > + (sign_bit * 16384)) / 100; > + break; > + case 2: > + sign_bit = (regval >> 13) & 0x01; > + *val = (long)((((u16)regval) & 0x1FFF) - > + (sign_bit * 8192)) / 100; > + break; > + case 1: > + sign_bit = (regval >> 12) & 0x01; > + *val = (long)((((u16)regval) & 0x0FFF) - > + (sign_bit * 4096)) / 100; > + break; > + } > + break; > + default: > + return -EOPNOTSUPP; > + } > break; > default: > - return -EINVAL; > + return -EOPNOTSUPP; > } > break; > case hwmon_curr: > @@ -122,7 +161,7 @@ static int isl28022_read(struct device *dev, enum hwmon_sensor_types type, > (long)data->shunt; > break; > default: > - return -EINVAL; > + return -EOPNOTSUPP; > } > break; > case hwmon_power: > @@ -136,11 +175,11 @@ static int isl28022_read(struct device *dev, enum hwmon_sensor_types type, > (long)data->shunt) * (long)regval; > break; > default: > - return -EINVAL; > + return -EOPNOTSUPP; > } > break; > default: > - return -EINVAL; > + return -EOPNOTSUPP; > } > > return 0; > @@ -182,7 +221,8 @@ static umode_t isl28022_is_visible(const void *data, enum hwmon_sensor_types typ > > static const struct hwmon_channel_info *isl28022_info[] = { > HWMON_CHANNEL_INFO(in, > - HWMON_I_INPUT), /* channel 0: bus voltage (mV) */ > + HWMON_I_INPUT, /* channel 0: bus voltage (mV) */ > + HWMON_I_INPUT), /* channel 1: shunt voltage (mV) */ > HWMON_CHANNEL_INFO(curr, > HWMON_C_INPUT), /* channel 1: current (mA) */ > HWMON_CHANNEL_INFO(power, > @@ -368,24 +408,22 @@ static int isl28022_read_properties(struct device *dev, struct isl28022_data *da > static int isl28022_config(struct isl28022_data *data) > { > int err; > + u16 config; > + u16 calib; > > - data->config = (ISL28022_MODE_CONT_SB << ISL28022_MODE_SHIFT) | > + config = (ISL28022_MODE_CONT_SB << ISL28022_MODE_SHIFT) | > (ISL28022_BRNG_60 << ISL28022_BRNG_SHIFT) | > (__ffs(data->gain) << ISL28022_PG_SHIFT) | > ((ISL28022_ADC_15_1 + __ffs(data->average)) << ISL28022_SADC_SHIFT) | > ((ISL28022_ADC_15_1 + __ffs(data->average)) << ISL28022_BADC_SHIFT); > > - data->calib = data->shunt ? 0x8000 / data->gain : 0; > - > - err = regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config); > - if (err < 0) > - return err; > + calib = data->shunt ? 0x8000 / data->gain : 0; > > - err = regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib); > + err = regmap_write(data->regmap, ISL28022_REG_CONFIG, config); > if (err < 0) > return err; > > - return 0; > + return regmap_write(data->regmap, ISL28022_REG_CALIB, calib); > } > > static int isl28022_probe(struct i2c_client *client) > @@ -396,8 +434,8 @@ static int isl28022_probe(struct i2c_client *client) > int err; > > if (!i2c_check_functionality(client->adapter, > - I2C_FUNC_SMBUS_BYTE_DATA | > - I2C_FUNC_SMBUS_WORD_DATA)) > + I2C_FUNC_SMBUS_BYTE_DATA | > + I2C_FUNC_SMBUS_WORD_DATA)) > return -ENODEV; > > data = devm_kzalloc(dev, sizeof(struct isl28022_data), GFP_KERNEL); > @@ -418,7 +456,7 @@ static int isl28022_probe(struct i2c_client *client) > > isl28022_debugfs_init(client, data); > > - hwmon_dev = devm_hwmon_device_register_with_info(dev, "isl28022_hwmon", > + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, > data, &isl28022_chip_info, NULL); > if (IS_ERR(hwmon_dev)) > return PTR_ERR(hwmon_dev); > @@ -437,8 +475,9 @@ static struct i2c_driver isl28022_driver = { > .class = I2C_CLASS_HWMON, > .driver = { > .name = "isl28022", > + .of_match_table = of_match_ptr(isl28022_of_match), This is both unrelated and wrong. Your of_match_ptr causes here warnings. Organize your patchset in logical chunks and be sure EACH has no warnings. Your patch #1 now has such warnings. Please run standard kernel tools for static analysis, like coccinelle, smatch and sparse, and fix reported warnings. Also please check for warnings when building with W=1. Most of these commands (checks or W=1 build) can build specific targets, like some directory, to narrow the scope to only your code. The code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. Best regards, Krzysztof
On 9/5/24 23:14, Delphine CC Chiu wrote: > Added support reading shunt voltage in mV and revise code > for Renesas ISL28022. > > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com> > --- > drivers/hwmon/isl28022.c | 93 ++++++++++++++++++++++++++++------------ > 1 file changed, 66 insertions(+), 27 deletions(-) > > diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c > index f0494c3bd483..01220fad813d 100644 > --- a/drivers/hwmon/isl28022.c > +++ b/drivers/hwmon/isl28022.c > @@ -85,8 +85,6 @@ struct isl28022_data { > u32 shunt; > u32 gain; > u32 average; > - u16 config; > - u16 calib; I don't see the point of separating this part of the driver from the first patch in the series. It makes me review code only to be replaced later. I am not going to do that. > }; > > static int isl28022_read(struct device *dev, enum hwmon_sensor_types type, > @@ -95,20 +93,61 @@ static int isl28022_read(struct device *dev, enum hwmon_sensor_types type, > struct isl28022_data *data = dev_get_drvdata(dev); > unsigned int regval; > int err; > + u16 sign_bit; > > switch (type) { > case hwmon_in: > - switch (attr) { > - case hwmon_in_input: > - err = regmap_read(data->regmap, > - ISL28022_REG_BUS, ®val); > - if (err < 0) > - return err; > - /* driver supports only 60V mode (BRNG 11) */ > - *val = (long)(((u16)regval) & 0xFFFC); > + switch (channel) { > + case 0: > + switch (attr) { > + case hwmon_in_input: > + err = regmap_read(data->regmap, > + ISL28022_REG_BUS, ®val); > + if (err < 0) > + return err; > + /* driver supports only 60V mode (BRNG 11) */ > + *val = (long)(((u16)regval) & 0xFFFC); > + break; > + default: > + return -EOPNOTSUPP; > + } > + break; > + case 1: > + switch (attr) { > + case hwmon_in_input: > + err = regmap_read(data->regmap, > + ISL28022_REG_SHUNT, ®val); > + if (err < 0) > + return err; > + switch (data->gain) { > + case 8: > + sign_bit = (regval >> 15) & 0x01; > + *val = (long)((((u16)regval) & 0x7FFF) - > + (sign_bit * 32768)) / 100; > + break; > + case 4: > + sign_bit = (regval >> 14) & 0x01; > + *val = (long)((((u16)regval) & 0x3FFF) - > + (sign_bit * 16384)) / 100; > + break; > + case 2: > + sign_bit = (regval >> 13) & 0x01; > + *val = (long)((((u16)regval) & 0x1FFF) - > + (sign_bit * 8192)) / 100; > + break; > + case 1: > + sign_bit = (regval >> 12) & 0x01; > + *val = (long)((((u16)regval) & 0x0FFF) - > + (sign_bit * 4096)) / 100; > + break; > + } > + break; > + default: > + return -EOPNOTSUPP; > + } Separate into its own voltage read function, and also provide separate functions for current and power. Guenter
Hi Delphine,
kernel test robot noticed the following build warnings:
[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.11-rc7 next-20240909]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Delphine-CC-Chiu/hwmon-isl28022-new-driver-for-ISL28022-power-monitor/20240906-141717
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/20240906061421.9392-4-Delphine_CC_Chiu%40wiwynn.com
patch subject: [PATCH v6 3/3] hwmon: (isl28022) support shunt voltage for ISL28022 power monitor
config: microblaze-randconfig-r073-20240909 (https://download.01.org/0day-ci/archive/20240910/202409101229.6mTYs5Rf-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 14.1.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409101229.6mTYs5Rf-lkp@intel.com/
smatch warnings:
drivers/hwmon/isl28022.c:122 isl28022_read() warn: inconsistent indenting
vim +122 drivers/hwmon/isl28022.c
89
90 static int isl28022_read(struct device *dev, enum hwmon_sensor_types type,
91 u32 attr, int channel, long *val)
92 {
93 struct isl28022_data *data = dev_get_drvdata(dev);
94 unsigned int regval;
95 int err;
96 u16 sign_bit;
97
98 switch (type) {
99 case hwmon_in:
100 switch (channel) {
101 case 0:
102 switch (attr) {
103 case hwmon_in_input:
104 err = regmap_read(data->regmap,
105 ISL28022_REG_BUS, ®val);
106 if (err < 0)
107 return err;
108 /* driver supports only 60V mode (BRNG 11) */
109 *val = (long)(((u16)regval) & 0xFFFC);
110 break;
111 default:
112 return -EOPNOTSUPP;
113 }
114 break;
115 case 1:
116 switch (attr) {
117 case hwmon_in_input:
118 err = regmap_read(data->regmap,
119 ISL28022_REG_SHUNT, ®val);
120 if (err < 0)
121 return err;
> 122 switch (data->gain) {
123 case 8:
124 sign_bit = (regval >> 15) & 0x01;
125 *val = (long)((((u16)regval) & 0x7FFF) -
126 (sign_bit * 32768)) / 100;
127 break;
128 case 4:
129 sign_bit = (regval >> 14) & 0x01;
130 *val = (long)((((u16)regval) & 0x3FFF) -
131 (sign_bit * 16384)) / 100;
132 break;
133 case 2:
134 sign_bit = (regval >> 13) & 0x01;
135 *val = (long)((((u16)regval) & 0x1FFF) -
136 (sign_bit * 8192)) / 100;
137 break;
138 case 1:
139 sign_bit = (regval >> 12) & 0x01;
140 *val = (long)((((u16)regval) & 0x0FFF) -
141 (sign_bit * 4096)) / 100;
142 break;
143 }
144 break;
145 default:
146 return -EOPNOTSUPP;
147 }
148 break;
149 default:
150 return -EOPNOTSUPP;
151 }
152 break;
153 case hwmon_curr:
154 switch (attr) {
155 case hwmon_curr_input:
156 err = regmap_read(data->regmap,
157 ISL28022_REG_CURRENT, ®val);
158 if (err < 0)
159 return err;
160 *val = ((long)regval * 1250L * (long)data->gain) /
161 (long)data->shunt;
162 break;
163 default:
164 return -EOPNOTSUPP;
165 }
166 break;
167 case hwmon_power:
168 switch (attr) {
169 case hwmon_power_input:
170 err = regmap_read(data->regmap,
171 ISL28022_REG_POWER, ®val);
172 if (err < 0)
173 return err;
174 *val = ((51200000L * ((long)data->gain)) /
175 (long)data->shunt) * (long)regval;
176 break;
177 default:
178 return -EOPNOTSUPP;
179 }
180 break;
181 default:
182 return -EOPNOTSUPP;
183 }
184
185 return 0;
186 }
187
diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c index f0494c3bd483..01220fad813d 100644 --- a/drivers/hwmon/isl28022.c +++ b/drivers/hwmon/isl28022.c @@ -85,8 +85,6 @@ struct isl28022_data { u32 shunt; u32 gain; u32 average; - u16 config; - u16 calib; }; static int isl28022_read(struct device *dev, enum hwmon_sensor_types type, @@ -95,20 +93,61 @@ static int isl28022_read(struct device *dev, enum hwmon_sensor_types type, struct isl28022_data *data = dev_get_drvdata(dev); unsigned int regval; int err; + u16 sign_bit; switch (type) { case hwmon_in: - switch (attr) { - case hwmon_in_input: - err = regmap_read(data->regmap, - ISL28022_REG_BUS, ®val); - if (err < 0) - return err; - /* driver supports only 60V mode (BRNG 11) */ - *val = (long)(((u16)regval) & 0xFFFC); + switch (channel) { + case 0: + switch (attr) { + case hwmon_in_input: + err = regmap_read(data->regmap, + ISL28022_REG_BUS, ®val); + if (err < 0) + return err; + /* driver supports only 60V mode (BRNG 11) */ + *val = (long)(((u16)regval) & 0xFFFC); + break; + default: + return -EOPNOTSUPP; + } + break; + case 1: + switch (attr) { + case hwmon_in_input: + err = regmap_read(data->regmap, + ISL28022_REG_SHUNT, ®val); + if (err < 0) + return err; + switch (data->gain) { + case 8: + sign_bit = (regval >> 15) & 0x01; + *val = (long)((((u16)regval) & 0x7FFF) - + (sign_bit * 32768)) / 100; + break; + case 4: + sign_bit = (regval >> 14) & 0x01; + *val = (long)((((u16)regval) & 0x3FFF) - + (sign_bit * 16384)) / 100; + break; + case 2: + sign_bit = (regval >> 13) & 0x01; + *val = (long)((((u16)regval) & 0x1FFF) - + (sign_bit * 8192)) / 100; + break; + case 1: + sign_bit = (regval >> 12) & 0x01; + *val = (long)((((u16)regval) & 0x0FFF) - + (sign_bit * 4096)) / 100; + break; + } + break; + default: + return -EOPNOTSUPP; + } break; default: - return -EINVAL; + return -EOPNOTSUPP; } break; case hwmon_curr: @@ -122,7 +161,7 @@ static int isl28022_read(struct device *dev, enum hwmon_sensor_types type, (long)data->shunt; break; default: - return -EINVAL; + return -EOPNOTSUPP; } break; case hwmon_power: @@ -136,11 +175,11 @@ static int isl28022_read(struct device *dev, enum hwmon_sensor_types type, (long)data->shunt) * (long)regval; break; default: - return -EINVAL; + return -EOPNOTSUPP; } break; default: - return -EINVAL; + return -EOPNOTSUPP; } return 0; @@ -182,7 +221,8 @@ static umode_t isl28022_is_visible(const void *data, enum hwmon_sensor_types typ static const struct hwmon_channel_info *isl28022_info[] = { HWMON_CHANNEL_INFO(in, - HWMON_I_INPUT), /* channel 0: bus voltage (mV) */ + HWMON_I_INPUT, /* channel 0: bus voltage (mV) */ + HWMON_I_INPUT), /* channel 1: shunt voltage (mV) */ HWMON_CHANNEL_INFO(curr, HWMON_C_INPUT), /* channel 1: current (mA) */ HWMON_CHANNEL_INFO(power, @@ -368,24 +408,22 @@ static int isl28022_read_properties(struct device *dev, struct isl28022_data *da static int isl28022_config(struct isl28022_data *data) { int err; + u16 config; + u16 calib; - data->config = (ISL28022_MODE_CONT_SB << ISL28022_MODE_SHIFT) | + config = (ISL28022_MODE_CONT_SB << ISL28022_MODE_SHIFT) | (ISL28022_BRNG_60 << ISL28022_BRNG_SHIFT) | (__ffs(data->gain) << ISL28022_PG_SHIFT) | ((ISL28022_ADC_15_1 + __ffs(data->average)) << ISL28022_SADC_SHIFT) | ((ISL28022_ADC_15_1 + __ffs(data->average)) << ISL28022_BADC_SHIFT); - data->calib = data->shunt ? 0x8000 / data->gain : 0; - - err = regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config); - if (err < 0) - return err; + calib = data->shunt ? 0x8000 / data->gain : 0; - err = regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib); + err = regmap_write(data->regmap, ISL28022_REG_CONFIG, config); if (err < 0) return err; - return 0; + return regmap_write(data->regmap, ISL28022_REG_CALIB, calib); } static int isl28022_probe(struct i2c_client *client) @@ -396,8 +434,8 @@ static int isl28022_probe(struct i2c_client *client) int err; if (!i2c_check_functionality(client->adapter, - I2C_FUNC_SMBUS_BYTE_DATA | - I2C_FUNC_SMBUS_WORD_DATA)) + I2C_FUNC_SMBUS_BYTE_DATA | + I2C_FUNC_SMBUS_WORD_DATA)) return -ENODEV; data = devm_kzalloc(dev, sizeof(struct isl28022_data), GFP_KERNEL); @@ -418,7 +456,7 @@ static int isl28022_probe(struct i2c_client *client) isl28022_debugfs_init(client, data); - hwmon_dev = devm_hwmon_device_register_with_info(dev, "isl28022_hwmon", + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, &isl28022_chip_info, NULL); if (IS_ERR(hwmon_dev)) return PTR_ERR(hwmon_dev); @@ -437,8 +475,9 @@ static struct i2c_driver isl28022_driver = { .class = I2C_CLASS_HWMON, .driver = { .name = "isl28022", + .of_match_table = of_match_ptr(isl28022_of_match), }, - .probe_new = isl28022_probe, + .probe = isl28022_probe, .id_table = isl28022_ids, };
Added support reading shunt voltage in mV and revise code for Renesas ISL28022. Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com> --- drivers/hwmon/isl28022.c | 93 ++++++++++++++++++++++++++++------------ 1 file changed, 66 insertions(+), 27 deletions(-)