diff mbox series

[7/8] hwmon: (tmp421) really disable channels

Message ID d0a1be24701dcf19a2f7501a9bc7fddf2b739792.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:46 p.m. UTC 
Recent patch added possibility to disable selected channels. That would
only make sure that the ENODATA is returned for those channels but would
not configure the actual hardware.

With this patch, the config register is written to make sure the
channels are disabled also at hardware level.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/tmp421.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

Comments

Guenter Roeck Sept. 7, 2021, 3:37 p.m. UTC | #1 
On 9/7/21 6:46 AM, Krzysztof Adamski wrote:
> Recent patch added possibility to disable selected channels. That would
> only make sure that the ENODATA is returned for those channels but would
> not configure the actual hardware.
> 
> With this patch, the config register is written to make sure the
> channels are disabled also at hardware level.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
>   drivers/hwmon/tmp421.c | 35 ++++++++++++++++++++++++++++++++---
>   1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
> index 90c6b094785e..cec25fb1c771 100644
> --- a/drivers/hwmon/tmp421.c
> +++ b/drivers/hwmon/tmp421.c
> @@ -33,6 +33,8 @@ enum chips { tmp421, tmp422, tmp423, tmp441, tmp442 };
>   /* The TMP421 registers */
>   #define TMP421_STATUS_REG			0x08
>   #define TMP421_CONFIG_REG_1			0x09
> +#define TMP421_CONFIG_REG_2			0x0A
> +#define TMP421_CONFIG_REG_REN(x)		(BIT(3 + (x)))
>   #define TMP421_CONVERSION_RATE_REG		0x0B
>   #define TMP421_N_FACTOR_REG_1			0x21
>   #define TMP421_MANUFACTURER_ID_REG		0xFE
> @@ -351,6 +353,25 @@ void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
>   	}
>   }
>   
> +void tmp421_disable_channels(struct i2c_client *client, uint8_t mask)
> +{
> +	int err;
> +	int cfg = i2c_smbus_read_byte_data(client, TMP421_CONFIG_REG_2);
> +
> +	if (cfg < 0) {
> +		dev_err(&client->dev,
> +			"error reading register, can't disable channels\n");
> +		return;
> +	}
> +
> +	cfg &= ~mask;
> +
> +	err = i2c_smbus_write_byte_data(client, TMP421_CONFIG_REG_2, cfg);
> +	if (err < 0)
> +		dev_err(&client->dev,
> +			"error writing register, can't disable channels\n");
> +}
> +
>   static const struct hwmon_ops tmp421_ops = {
>   	.is_visible = tmp421_is_visible,
>   	.read = tmp421_read,
> @@ -363,6 +384,7 @@ static int tmp421_probe(struct i2c_client *client)
>   	struct device *hwmon_dev;
>   	struct tmp421_data *data;
>   	int i, err;
> +	u8 disable = 0;
>   
>   	data = devm_kzalloc(dev, sizeof(struct tmp421_data), GFP_KERNEL);
>   	if (!data)
> @@ -380,11 +402,18 @@ static int tmp421_probe(struct i2c_client *client)
>   	if (err)
>   		return err;
>   
> -	for (i = 0; i < data->channels; i++)
> -		data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
> -
>   	tmp421_probe_from_dt(client, data);
>   
> +	for (i = 0; i < data->channels; i++) {
> +		data->temp_config[i] |= HWMON_T_INPUT | HWMON_T_FAULT;
> +		if (data->channel[i].disabled)
> +			disable |= TMP421_CONFIG_REG_REN(i);
> +
> +	}
> +
> +	if (disable)
> +		tmp421_disable_channels(client, disable);
> +

This doesn't take into account that channels may already be disabled
by the BIOS/ROMMON. The code will have to set the channel status explicitly
for all channels if configured through dt, or read it from the chip
otherwise. Also, as mentioned, the sysfs attribute should be supported
as well (meaning it should also be possible to enable a channel).

Guenter

>   	data->chip.ops = &tmp421_ops;
>   	data->chip.info = data->info;
>   
>
kernel test robot Sept. 7, 2021, 7:52 p.m. UTC | #2 
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/56fe3c49ca67e9ea9c4f8a40438b5adc58c417f1
        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 56fe3c49ca67e9ea9c4f8a40438b5adc58c417f1
        # 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:301: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:301: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:345: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:345: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 
>> drivers/hwmon/tmp421.c:356:6: warning: no previous prototype for function 'tmp421_disable_channels' [-Wmissing-prototypes]
   void tmp421_disable_channels(struct i2c_client *client, uint8_t mask)
        ^
   drivers/hwmon/tmp421.c:356:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void tmp421_disable_channels(struct i2c_client *client, uint8_t mask)
   ^
   static 
   3 warnings generated.


vim +/tmp421_disable_channels +356 drivers/hwmon/tmp421.c

   355	
 > 356	void tmp421_disable_channels(struct i2c_client *client, uint8_t mask)
   357	{
   358		int err;
   359		int cfg = i2c_smbus_read_byte_data(client, TMP421_CONFIG_REG_2);
   360	
   361		if (cfg < 0) {
   362			dev_err(&client->dev,
   363				"error reading register, can't disable channels\n");
   364			return;
   365		}
   366	
   367		cfg &= ~mask;
   368	
   369		err = i2c_smbus_write_byte_data(client, TMP421_CONFIG_REG_2, cfg);
   370		if (err < 0)
   371			dev_err(&client->dev,
   372				"error writing register, can't disable channels\n");
   373	}
   374	

---
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, 8:40 p.m. UTC | #3 
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/56fe3c49ca67e9ea9c4f8a40438b5adc58c417f1
        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 56fe3c49ca67e9ea9c4f8a40438b5adc58c417f1
        # 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:301:6: sparse: sparse: symbol 'tmp421_probe_child_from_dt' was not declared. Should it be static?
   drivers/hwmon/tmp421.c:345:6: sparse: sparse: symbol 'tmp421_probe_from_dt' was not declared. Should it be static?
>> drivers/hwmon/tmp421.c:356:6: sparse: sparse: symbol 'tmp421_disable_channels' 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 90c6b094785e..cec25fb1c771 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -33,6 +33,8 @@  enum chips { tmp421, tmp422, tmp423, tmp441, tmp442 };
 /* The TMP421 registers */
 #define TMP421_STATUS_REG			0x08
 #define TMP421_CONFIG_REG_1			0x09
+#define TMP421_CONFIG_REG_2			0x0A
+#define TMP421_CONFIG_REG_REN(x)		(BIT(3 + (x)))
 #define TMP421_CONVERSION_RATE_REG		0x0B
 #define TMP421_N_FACTOR_REG_1			0x21
 #define TMP421_MANUFACTURER_ID_REG		0xFE
@@ -351,6 +353,25 @@  void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
 	}
 }
 
+void tmp421_disable_channels(struct i2c_client *client, uint8_t mask)
+{
+	int err;
+	int cfg = i2c_smbus_read_byte_data(client, TMP421_CONFIG_REG_2);
+
+	if (cfg < 0) {
+		dev_err(&client->dev,
+			"error reading register, can't disable channels\n");
+		return;
+	}
+
+	cfg &= ~mask;
+
+	err = i2c_smbus_write_byte_data(client, TMP421_CONFIG_REG_2, cfg);
+	if (err < 0)
+		dev_err(&client->dev,
+			"error writing register, can't disable channels\n");
+}
+
 static const struct hwmon_ops tmp421_ops = {
 	.is_visible = tmp421_is_visible,
 	.read = tmp421_read,
@@ -363,6 +384,7 @@  static int tmp421_probe(struct i2c_client *client)
 	struct device *hwmon_dev;
 	struct tmp421_data *data;
 	int i, err;
+	u8 disable = 0;
 
 	data = devm_kzalloc(dev, sizeof(struct tmp421_data), GFP_KERNEL);
 	if (!data)
@@ -380,11 +402,18 @@  static int tmp421_probe(struct i2c_client *client)
 	if (err)
 		return err;
 
-	for (i = 0; i < data->channels; i++)
-		data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
-
 	tmp421_probe_from_dt(client, data);
 
+	for (i = 0; i < data->channels; i++) {
+		data->temp_config[i] |= HWMON_T_INPUT | HWMON_T_FAULT;
+		if (data->channel[i].disabled)
+			disable |= TMP421_CONFIG_REG_REN(i);
+
+	}
+
+	if (disable)
+		tmp421_disable_channels(client, disable);
+
 	data->chip.ops = &tmp421_ops;
 	data->chip.info = data->info;