diff mbox series

[4/8] hwmon: (tmp421) add support for defining labels from DT

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

Commit Message

Krzysztof Adamski Sept. 7, 2021, 1:43 p.m. UTC 
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(+)

Comments

Guenter Roeck Sept. 7, 2021, 3:46 p.m. UTC | #1 
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;
>   
>
Krzysztof Adamski Sept. 7, 2021, 5:49 p.m. UTC | #2 
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
Guenter Roeck Sept. 7, 2021, 5:55 p.m. UTC | #3 
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
Krzysztof Adamski Sept. 7, 2021, 6:08 p.m. UTC | #4 
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
kernel test robot Sept. 7, 2021, 6:28 p.m. UTC | #5 
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
kernel test robot Sept. 9, 2021, 5:29 p.m. UTC | #6 
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 mbox series

Patch

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;