Message ID | 22639314543a98b4c24e55b7e5a803325ad9e568.1631021349.git.krzysztof.adamski@nokia.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | Add per channel properies support in tmp421 | expand |
On 9/7/21 6:43 AM, Krzysztof Adamski wrote: > tmp42x is a multichannel temperature sensor with several external > channels. Since those channels can be used to connect diodes placed > anywhere in the system, their meaning will vary depending on the > project. For this case, the hwmon framework has an idea of labels which > allows us to assign the meaning to each channel. > > The similar concept is already implemented in ina3221 - the label for > each channel can be defined via device tree. See commit a9e9dd9c6de5 > ("hwmon: (ina3221) Read channel input source info from DT") > > This patch adds support for similar feature to tmp421. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > --- > drivers/hwmon/tmp421.c | 51 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c > index 1068fe59df0b..a1dba1d405ee 100644 > --- a/drivers/hwmon/tmp421.c > +++ b/drivers/hwmon/tmp421.c > @@ -88,6 +88,7 @@ static const struct of_device_id __maybe_unused tmp421_of_match[] = { > MODULE_DEVICE_TABLE(of, tmp421_of_match); > > struct tmp421_channel { > + const char *label; > s16 temp; > }; > > @@ -177,6 +178,16 @@ static int tmp421_read(struct device *dev, enum hwmon_sensor_types type, > > } > > +static int tmp421_read_string(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, const char **str) > +{ > + struct tmp421_data *data = dev_get_drvdata(dev); > + > + *str = data->channel[channel].label; > + > + return 0; > +} > + > static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type, > u32 attr, int channel) > { > @@ -187,6 +198,8 @@ static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type, > return 0444; > case hwmon_temp_input: > return 0444; > + case hwmon_temp_label: > + return 0444; > default: > return 0; > } > @@ -279,9 +292,45 @@ static int tmp421_detect(struct i2c_client *client, > return 0; > } > > +void tmp421_probe_child_from_dt(struct i2c_client *client, > + struct device_node *child, > + struct tmp421_data *data) > + > +{ > + struct device *dev = &client->dev; > + u32 i; > + int err; > + > + err = of_property_read_u32(child, "reg", &i); > + if (err) { > + dev_err(dev, "missing reg property of %pOFn\n", child); > + return; Report to caller > + } else if (i > MAX_CHANNELS) { else after return is pointless. > + dev_err(dev, "invalid reg %d of %pOFn\n", i, child); > + return; Again report to caller. > + } > + > + of_property_read_string(child, "label", &data->channel[i].label); > + if (data->channel[i].label) > + data->temp_config[i] |= HWMON_T_LABEL; > + > +} > + > +void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data) > +{ > + struct device *dev = &client->dev; > + const struct device_node *np = dev->of_node; > + struct device_node *child; > + > + for_each_child_of_node(np, child) { > + tmp421_probe_child_from_dt(client, child, data); > + } > +} > + > static const struct hwmon_ops tmp421_ops = { > .is_visible = tmp421_is_visible, > .read = tmp421_read, > + .read_string = tmp421_read_string, > }; > > static int tmp421_probe(struct i2c_client *client) > @@ -310,6 +359,8 @@ static int tmp421_probe(struct i2c_client *client) > for (i = 0; i < data->channels; i++) > data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT; > > + tmp421_probe_from_dt(client, data); > + > data->chip.ops = &tmp421_ops; > data->chip.info = data->info; > >
Dnia Tue, Sep 07, 2021 at 08:46:22AM -0700, Guenter Roeck napisał(a): >>+void tmp421_probe_child_from_dt(struct i2c_client *client, >>+ struct device_node *child, >>+ struct tmp421_data *data) >>+ >>+{ >>+ struct device *dev = &client->dev; >>+ u32 i; >>+ int err; >>+ >>+ err = of_property_read_u32(child, "reg", &i); >>+ if (err) { >>+ dev_err(dev, "missing reg property of %pOFn\n", child); >>+ return; > >Report to caller > My idea was to make those errors in DT non-critical. I.e. if one of the child nodes is not well structured, I just skip it but continue the probing. Do you think it should be considered critical and I should abort the whole probe function as soon as I detect such DT errors, instead? Krzysztof
On Tue, Sep 07, 2021 at 07:49:36PM +0200, Krzysztof Adamski wrote: > Dnia Tue, Sep 07, 2021 at 08:46:22AM -0700, Guenter Roeck napisał(a): > > > +void tmp421_probe_child_from_dt(struct i2c_client *client, > > > + struct device_node *child, > > > + struct tmp421_data *data) > > > + > > > +{ > > > + struct device *dev = &client->dev; > > > + u32 i; > > > + int err; > > > + > > > + err = of_property_read_u32(child, "reg", &i); > > > + if (err) { > > > + dev_err(dev, "missing reg property of %pOFn\n", child); > > > + return; > > > > Report to caller > > > > My idea was to make those errors in DT non-critical. I.e. if one of the > child nodes is not well structured, I just skip it but continue the > probing. Do you think it should be considered critical and I should > abort the whole probe function as soon as I detect such DT errors, > instead? > Yes, I do think so. Otherwise people will just generate bad DT files and never fix them or even on purpose ignore the error messages. I don't want to see such messages in a production system. On a side note, I do not accept dev_err/pr_err if the error is ignored. Either it is an error, or it isn't. Thanks, Guenter
Dnia Tue, Sep 07, 2021 at 10:55:52AM -0700, Guenter Roeck napisał(a): >On Tue, Sep 07, 2021 at 07:49:36PM +0200, Krzysztof Adamski wrote: >> Dnia Tue, Sep 07, 2021 at 08:46:22AM -0700, Guenter Roeck napisał(a): >> > > +void tmp421_probe_child_from_dt(struct i2c_client *client, >> > > + struct device_node *child, >> > > + struct tmp421_data *data) >> > > + >> > > +{ >> > > + struct device *dev = &client->dev; >> > > + u32 i; >> > > + int err; >> > > + >> > > + err = of_property_read_u32(child, "reg", &i); >> > > + if (err) { >> > > + dev_err(dev, "missing reg property of %pOFn\n", child); >> > > + return; >> > >> > Report to caller >> > >> >> My idea was to make those errors in DT non-critical. I.e. if one of the >> child nodes is not well structured, I just skip it but continue the >> probing. Do you think it should be considered critical and I should >> abort the whole probe function as soon as I detect such DT errors, >> instead? >> >Yes, I do think so. Otherwise people will just generate bad DT files and >never fix them or even on purpose ignore the error messages. I don't want >to see such messages in a production system. > >On a side note, I do not accept dev_err/pr_err if the error is ignored. >Either it is an error, or it isn't. Makes sense - if it's an error, it should be reported as such, otherwise it's just a warning. I'll change the code to fail on DT errors in v2. Thank you, Krzysztof
Hi Krzysztof, I love your patch! Perhaps something to improve: [auto build test WARNING on hwmon/hwmon-next] [also build test WARNING on robh/for-next v5.14 next-20210907] [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] url: https://github.com/0day-ci/linux/commits/Krzysztof-Adamski/Add-per-channel-properies-support-in-tmp421/20210907-214724 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next config: mips-randconfig-c004-20210907 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 9c476172b93367d2cb88d7d3f4b1b5b456fa6020) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install mips cross compiling tool for clang build # apt-get install binutils-mips-linux-gnu # https://github.com/0day-ci/linux/commit/50e1eb6bf222a2fb0df304033f2aaed075fd76b2 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Krzysztof-Adamski/Add-per-channel-properies-support-in-tmp421/20210907-214724 git checkout 50e1eb6bf222a2fb0df304033f2aaed075fd76b2 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> drivers/hwmon/tmp421.c:295:6: warning: no previous prototype for function 'tmp421_probe_child_from_dt' [-Wmissing-prototypes] void tmp421_probe_child_from_dt(struct i2c_client *client, ^ drivers/hwmon/tmp421.c:295:1: note: declare 'static' if the function is not intended to be used outside of this translation unit void tmp421_probe_child_from_dt(struct i2c_client *client, ^ static >> drivers/hwmon/tmp421.c:319:6: warning: no previous prototype for function 'tmp421_probe_from_dt' [-Wmissing-prototypes] void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data) ^ drivers/hwmon/tmp421.c:319:1: note: declare 'static' if the function is not intended to be used outside of this translation unit void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data) ^ static 2 warnings generated. vim +/tmp421_probe_child_from_dt +295 drivers/hwmon/tmp421.c 294 > 295 void tmp421_probe_child_from_dt(struct i2c_client *client, 296 struct device_node *child, 297 struct tmp421_data *data) 298 299 { 300 struct device *dev = &client->dev; 301 u32 i; 302 int err; 303 304 err = of_property_read_u32(child, "reg", &i); 305 if (err) { 306 dev_err(dev, "missing reg property of %pOFn\n", child); 307 return; 308 } else if (i > MAX_CHANNELS) { 309 dev_err(dev, "invalid reg %d of %pOFn\n", i, child); 310 return; 311 } 312 313 of_property_read_string(child, "label", &data->channel[i].label); 314 if (data->channel[i].label) 315 data->temp_config[i] |= HWMON_T_LABEL; 316 317 } 318 > 319 void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data) 320 { 321 struct device *dev = &client->dev; 322 const struct device_node *np = dev->of_node; 323 struct device_node *child; 324 325 for_each_child_of_node(np, child) { 326 tmp421_probe_child_from_dt(client, child, data); 327 } 328 } 329 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Krzysztof, I love your patch! Perhaps something to improve: [auto build test WARNING on hwmon/hwmon-next] [also build test WARNING on robh/for-next v5.14 next-20210909] [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] url: https://github.com/0day-ci/linux/commits/Krzysztof-Adamski/Add-per-channel-properies-support-in-tmp421/20210907-214724 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next config: i386-randconfig-s001-20210908 (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.4-dirty # https://github.com/0day-ci/linux/commit/50e1eb6bf222a2fb0df304033f2aaed075fd76b2 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Krzysztof-Adamski/Add-per-channel-properies-support-in-tmp421/20210907-214724 git checkout 50e1eb6bf222a2fb0df304033f2aaed075fd76b2 # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hwmon/ If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> sparse warnings: (new ones prefixed by >>) >> drivers/hwmon/tmp421.c:295:6: sparse: sparse: symbol 'tmp421_probe_child_from_dt' was not declared. Should it be static? >> drivers/hwmon/tmp421.c:319:6: sparse: sparse: symbol 'tmp421_probe_from_dt' was not declared. Should it be static? Please review and possibly fold the followup patch. --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c index 1068fe59df0b..a1dba1d405ee 100644 --- a/drivers/hwmon/tmp421.c +++ b/drivers/hwmon/tmp421.c @@ -88,6 +88,7 @@ static const struct of_device_id __maybe_unused tmp421_of_match[] = { MODULE_DEVICE_TABLE(of, tmp421_of_match); struct tmp421_channel { + const char *label; s16 temp; }; @@ -177,6 +178,16 @@ static int tmp421_read(struct device *dev, enum hwmon_sensor_types type, } +static int tmp421_read_string(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, const char **str) +{ + struct tmp421_data *data = dev_get_drvdata(dev); + + *str = data->channel[channel].label; + + return 0; +} + static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr, int channel) { @@ -187,6 +198,8 @@ static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type, return 0444; case hwmon_temp_input: return 0444; + case hwmon_temp_label: + return 0444; default: return 0; } @@ -279,9 +292,45 @@ static int tmp421_detect(struct i2c_client *client, return 0; } +void tmp421_probe_child_from_dt(struct i2c_client *client, + struct device_node *child, + struct tmp421_data *data) + +{ + struct device *dev = &client->dev; + u32 i; + int err; + + err = of_property_read_u32(child, "reg", &i); + if (err) { + dev_err(dev, "missing reg property of %pOFn\n", child); + return; + } else if (i > MAX_CHANNELS) { + dev_err(dev, "invalid reg %d of %pOFn\n", i, child); + return; + } + + of_property_read_string(child, "label", &data->channel[i].label); + if (data->channel[i].label) + data->temp_config[i] |= HWMON_T_LABEL; + +} + +void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data) +{ + struct device *dev = &client->dev; + const struct device_node *np = dev->of_node; + struct device_node *child; + + for_each_child_of_node(np, child) { + tmp421_probe_child_from_dt(client, child, data); + } +} + static const struct hwmon_ops tmp421_ops = { .is_visible = tmp421_is_visible, .read = tmp421_read, + .read_string = tmp421_read_string, }; static int tmp421_probe(struct i2c_client *client) @@ -310,6 +359,8 @@ static int tmp421_probe(struct i2c_client *client) for (i = 0; i < data->channels; i++) data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT; + tmp421_probe_from_dt(client, data); + data->chip.ops = &tmp421_ops; data->chip.info = data->info;
tmp42x is a multichannel temperature sensor with several external channels. Since those channels can be used to connect diodes placed anywhere in the system, their meaning will vary depending on the project. For this case, the hwmon framework has an idea of labels which allows us to assign the meaning to each channel. The similar concept is already implemented in ina3221 - the label for each channel can be defined via device tree. See commit a9e9dd9c6de5 ("hwmon: (ina3221) Read channel input source info from DT") This patch adds support for similar feature to tmp421. Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> --- drivers/hwmon/tmp421.c | 51 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)