* [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute
@ 2023-06-27 18:40 Eddie James
2023-06-27 22:19 ` Guenter Roeck
2023-06-29 6:53 ` Dan Carpenter
0 siblings, 2 replies; 8+ messages in thread
From: Eddie James @ 2023-06-27 18:40 UTC (permalink / raw)
To: linux-hwmon; +Cc: linux-kernel, linux, jdelvare, lakshmiy, Eddie James
Like the IBM CFFPS driver, export the PSU's firmware version to a
debugfs attribute as reported in the manufacturer register.
Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
drivers/hwmon/pmbus/acbel-fsg032.c | 48 ++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/drivers/hwmon/pmbus/acbel-fsg032.c b/drivers/hwmon/pmbus/acbel-fsg032.c
index 0a0ef4ce3493..4b97f108cfe3 100644
--- a/drivers/hwmon/pmbus/acbel-fsg032.c
+++ b/drivers/hwmon/pmbus/acbel-fsg032.c
@@ -3,6 +3,7 @@
* Copyright 2023 IBM Corp.
*/
+#include <linux/debugfs.h>
#include <linux/device.h>
#include <linux/fs.h>
#include <linux/i2c.h>
@@ -11,6 +12,52 @@
#include <linux/hwmon-sysfs.h>
#include "pmbus.h"
+#define ACBEL_MFR_FW_REVISION 0xd9
+
+static ssize_t acbel_fsg032_debugfs_read(struct file *file, char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ struct i2c_client *client = file->private_data;
+ char data[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
+ char out[8];
+ int rc;
+ int i;
+
+ rc = pmbus_lock_interruptible(client);
+ if (rc)
+ return rc;
+
+ rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data);
+ pmbus_unlock(client);
+ if (rc < 0)
+ return rc;
+
+ for (i = 0; i < rc && i < 3; ++i)
+ snprintf(&out[i * 2], 3, "%02X", data[i]);
+
+ rc = i * 2;
+ out[rc++] = '\n';
+ out[rc++] = 0;
+ return simple_read_from_buffer(buf, count, ppos, out, rc);
+}
+
+static const struct file_operations acbel_debugfs_ops = {
+ .llseek = noop_llseek,
+ .read = acbel_fsg032_debugfs_read,
+ .write = NULL,
+ .open = simple_open,
+};
+
+static void acbel_fsg032_init_debugfs(struct i2c_client *client)
+{
+ struct dentry *debugfs = pmbus_get_debugfs_dir(client);
+
+ if (!debugfs)
+ return;
+
+ debugfs_create_file("fw_version", 0444, debugfs, client, &acbel_debugfs_ops);
+}
+
static const struct i2c_device_id acbel_fsg032_id[] = {
{ "acbel_fsg032" },
{}
@@ -59,6 +106,7 @@ static int acbel_fsg032_probe(struct i2c_client *client)
if (rc)
return rc;
+ acbel_fsg032_init_debugfs(client);
return 0;
}
--
2.39.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute
2023-06-27 18:40 [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute Eddie James
@ 2023-06-27 22:19 ` Guenter Roeck
2023-06-28 14:50 ` Eddie James
2023-06-29 6:53 ` Dan Carpenter
1 sibling, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2023-06-27 22:19 UTC (permalink / raw)
To: Eddie James, linux-hwmon; +Cc: linux-kernel, jdelvare, lakshmiy
On 6/27/23 11:40, Eddie James wrote:
> Like the IBM CFFPS driver, export the PSU's firmware version to a
> debugfs attribute as reported in the manufacturer register.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
> drivers/hwmon/pmbus/acbel-fsg032.c | 48 ++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/acbel-fsg032.c b/drivers/hwmon/pmbus/acbel-fsg032.c
> index 0a0ef4ce3493..4b97f108cfe3 100644
> --- a/drivers/hwmon/pmbus/acbel-fsg032.c
> +++ b/drivers/hwmon/pmbus/acbel-fsg032.c
> @@ -3,6 +3,7 @@
> * Copyright 2023 IBM Corp.
> */
>
> +#include <linux/debugfs.h>
> #include <linux/device.h>
> #include <linux/fs.h>
> #include <linux/i2c.h>
> @@ -11,6 +12,52 @@
> #include <linux/hwmon-sysfs.h>
> #include "pmbus.h"
>
> +#define ACBEL_MFR_FW_REVISION 0xd9
> +
> +static ssize_t acbel_fsg032_debugfs_read(struct file *file, char __user *buf, size_t count,
> + loff_t *ppos)
> +{
> + struct i2c_client *client = file->private_data;
> + char data[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
> + char out[8];
> + int rc;
> + int i;
> +
> + rc = pmbus_lock_interruptible(client);
Is that needed here ?
> + if (rc)
> + return rc;
> +
> + rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data);
> + pmbus_unlock(client);
> + if (rc < 0)
> + return rc;
> +
> + for (i = 0; i < rc && i < 3; ++i)
> + snprintf(&out[i * 2], 3, "%02X", data[i]);
> +
> + rc = i * 2;
> + out[rc++] = '\n';
> + out[rc++] = 0;
Any reason for not using one of the %*ph variants ?
Thanks,
Guenter
> + return simple_read_from_buffer(buf, count, ppos, out, rc);
> +}
> +
> +static const struct file_operations acbel_debugfs_ops = {
> + .llseek = noop_llseek,
> + .read = acbel_fsg032_debugfs_read,
> + .write = NULL,
> + .open = simple_open,
> +};
> +
> +static void acbel_fsg032_init_debugfs(struct i2c_client *client)
> +{
> + struct dentry *debugfs = pmbus_get_debugfs_dir(client);
> +
> + if (!debugfs)
> + return;
> +
> + debugfs_create_file("fw_version", 0444, debugfs, client, &acbel_debugfs_ops);
> +}
> +
> static const struct i2c_device_id acbel_fsg032_id[] = {
> { "acbel_fsg032" },
> {}
> @@ -59,6 +106,7 @@ static int acbel_fsg032_probe(struct i2c_client *client)
> if (rc)
> return rc;
>
> + acbel_fsg032_init_debugfs(client);
> return 0;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute
2023-06-27 22:19 ` Guenter Roeck
@ 2023-06-28 14:50 ` Eddie James
0 siblings, 0 replies; 8+ messages in thread
From: Eddie James @ 2023-06-28 14:50 UTC (permalink / raw)
To: Guenter Roeck, linux-hwmon; +Cc: linux-kernel, jdelvare, lakshmiy
On 6/27/23 17:19, Guenter Roeck wrote:
> On 6/27/23 11:40, Eddie James wrote:
>> Like the IBM CFFPS driver, export the PSU's firmware version to a
>> debugfs attribute as reported in the manufacturer register.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>> drivers/hwmon/pmbus/acbel-fsg032.c | 48 ++++++++++++++++++++++++++++++
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/hwmon/pmbus/acbel-fsg032.c
>> b/drivers/hwmon/pmbus/acbel-fsg032.c
>> index 0a0ef4ce3493..4b97f108cfe3 100644
>> --- a/drivers/hwmon/pmbus/acbel-fsg032.c
>> +++ b/drivers/hwmon/pmbus/acbel-fsg032.c
>> @@ -3,6 +3,7 @@
>> * Copyright 2023 IBM Corp.
>> */
>> +#include <linux/debugfs.h>
>> #include <linux/device.h>
>> #include <linux/fs.h>
>> #include <linux/i2c.h>
>> @@ -11,6 +12,52 @@
>> #include <linux/hwmon-sysfs.h>
>> #include "pmbus.h"
>> +#define ACBEL_MFR_FW_REVISION 0xd9
>> +
>> +static ssize_t acbel_fsg032_debugfs_read(struct file *file, char
>> __user *buf, size_t count,
>> + loff_t *ppos)
>> +{
>> + struct i2c_client *client = file->private_data;
>> + char data[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
>> + char out[8];
>> + int rc;
>> + int i;
>> +
>> + rc = pmbus_lock_interruptible(client);
>
> Is that needed here ?
Good point, it's not.
>
>> + if (rc)
>> + return rc;
>> +
>> + rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION,
>> data);
>> + pmbus_unlock(client);
>> + if (rc < 0)
>> + return rc;
>> +
>> + for (i = 0; i < rc && i < 3; ++i)
>> + snprintf(&out[i * 2], 3, "%02X", data[i]);
>> +
>> + rc = i * 2;
>> + out[rc++] = '\n';
>> + out[rc++] = 0;
>
> Any reason for not using one of the %*ph variants ?
Nope, that will work great, thanks.
Eddie
>
> Thanks,
> Guenter
>
>> + return simple_read_from_buffer(buf, count, ppos, out, rc);
>> +}
>> +
>> +static const struct file_operations acbel_debugfs_ops = {
>> + .llseek = noop_llseek,
>> + .read = acbel_fsg032_debugfs_read,
>> + .write = NULL,
>> + .open = simple_open,
>> +};
>> +
>> +static void acbel_fsg032_init_debugfs(struct i2c_client *client)
>> +{
>> + struct dentry *debugfs = pmbus_get_debugfs_dir(client);
>> +
>> + if (!debugfs)
>> + return;
>> +
>> + debugfs_create_file("fw_version", 0444, debugfs, client,
>> &acbel_debugfs_ops);
>> +}
>> +
>> static const struct i2c_device_id acbel_fsg032_id[] = {
>> { "acbel_fsg032" },
>> {}
>> @@ -59,6 +106,7 @@ static int acbel_fsg032_probe(struct i2c_client
>> *client)
>> if (rc)
>> return rc;
>> + acbel_fsg032_init_debugfs(client);
>> return 0;
>> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute
2023-06-27 18:40 [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute Eddie James
2023-06-27 22:19 ` Guenter Roeck
@ 2023-06-29 6:53 ` Dan Carpenter
2023-06-29 18:05 ` Guenter Roeck
2023-06-29 18:59 ` Dan Carpenter
1 sibling, 2 replies; 8+ messages in thread
From: Dan Carpenter @ 2023-06-29 6:53 UTC (permalink / raw)
To: oe-kbuild, Eddie James, linux-hwmon
Cc: lkp, oe-kbuild-all, linux-kernel, linux, jdelvare, lakshmiy,
Eddie James
Hi Eddie,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Eddie-James/hwmon-pmbus-acbel-fsg032-Add-firmware-version-debugfs-attribute/20230628-030615
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/20230627184027.16343-1-eajames%40linux.ibm.com
patch subject: [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute
config: x86_64-randconfig-m001-20230627 (https://download.01.org/0day-ci/archive/20230628/202306281205.NFXmu7xb-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230628/202306281205.NFXmu7xb-lkp@intel.com/reproduce)
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>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202306281205.NFXmu7xb-lkp@intel.com/
smatch warnings:
drivers/hwmon/pmbus/acbel-fsg032.c:36 acbel_fsg032_debugfs_read() warn: argument 4 to %02X specifier has type 'char'
vim +/char +36 drivers/hwmon/pmbus/acbel-fsg032.c
d2c6444389b625 Eddie James 2023-06-27 17 static ssize_t acbel_fsg032_debugfs_read(struct file *file, char __user *buf, size_t count,
d2c6444389b625 Eddie James 2023-06-27 18 loff_t *ppos)
d2c6444389b625 Eddie James 2023-06-27 19 {
d2c6444389b625 Eddie James 2023-06-27 20 struct i2c_client *client = file->private_data;
d2c6444389b625 Eddie James 2023-06-27 21 char data[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
data should probably be a u8.
d2c6444389b625 Eddie James 2023-06-27 22 char out[8];
d2c6444389b625 Eddie James 2023-06-27 23 int rc;
d2c6444389b625 Eddie James 2023-06-27 24 int i;
d2c6444389b625 Eddie James 2023-06-27 25
d2c6444389b625 Eddie James 2023-06-27 26 rc = pmbus_lock_interruptible(client);
d2c6444389b625 Eddie James 2023-06-27 27 if (rc)
d2c6444389b625 Eddie James 2023-06-27 28 return rc;
d2c6444389b625 Eddie James 2023-06-27 29
d2c6444389b625 Eddie James 2023-06-27 30 rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data);
d2c6444389b625 Eddie James 2023-06-27 31 pmbus_unlock(client);
d2c6444389b625 Eddie James 2023-06-27 32 if (rc < 0)
d2c6444389b625 Eddie James 2023-06-27 33 return rc;
d2c6444389b625 Eddie James 2023-06-27 34
d2c6444389b625 Eddie James 2023-06-27 35 for (i = 0; i < rc && i < 3; ++i)
d2c6444389b625 Eddie James 2023-06-27 @36 snprintf(&out[i * 2], 3, "%02X", data[i]);
If data[i] is negative this will print FFFFFFF1 etc. (This is an x86
config... Did we ever merge that patch to make char signed by default?)
d2c6444389b625 Eddie James 2023-06-27 37
d2c6444389b625 Eddie James 2023-06-27 38 rc = i * 2;
d2c6444389b625 Eddie James 2023-06-27 39 out[rc++] = '\n';
d2c6444389b625 Eddie James 2023-06-27 40 out[rc++] = 0;
d2c6444389b625 Eddie James 2023-06-27 41 return simple_read_from_buffer(buf, count, ppos, out, rc);
d2c6444389b625 Eddie James 2023-06-27 42 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute
2023-06-29 6:53 ` Dan Carpenter
@ 2023-06-29 18:05 ` Guenter Roeck
2023-06-29 18:59 ` Dan Carpenter
1 sibling, 0 replies; 8+ messages in thread
From: Guenter Roeck @ 2023-06-29 18:05 UTC (permalink / raw)
To: Dan Carpenter, oe-kbuild, Eddie James, linux-hwmon
Cc: lkp, oe-kbuild-all, linux-kernel, jdelvare, lakshmiy
On 6/28/23 23:53, Dan Carpenter wrote:
> Hi Eddie,
>
> kernel test robot noticed the following build warnings:
>
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Eddie-James/hwmon-pmbus-acbel-fsg032-Add-firmware-version-debugfs-attribute/20230628-030615
> base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
> patch link: https://lore.kernel.org/r/20230627184027.16343-1-eajames%40linux.ibm.com
> patch subject: [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute
> config: x86_64-randconfig-m001-20230627 (https://download.01.org/0day-ci/archive/20230628/202306281205.NFXmu7xb-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20230628/202306281205.NFXmu7xb-lkp@intel.com/reproduce)
>
> 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>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202306281205.NFXmu7xb-lkp@intel.com/
>
> smatch warnings:
> drivers/hwmon/pmbus/acbel-fsg032.c:36 acbel_fsg032_debugfs_read() warn: argument 4 to %02X specifier has type 'char'
>
> vim +/char +36 drivers/hwmon/pmbus/acbel-fsg032.c
>
> d2c6444389b625 Eddie James 2023-06-27 17 static ssize_t acbel_fsg032_debugfs_read(struct file *file, char __user *buf, size_t count,
> d2c6444389b625 Eddie James 2023-06-27 18 loff_t *ppos)
> d2c6444389b625 Eddie James 2023-06-27 19 {
> d2c6444389b625 Eddie James 2023-06-27 20 struct i2c_client *client = file->private_data;
> d2c6444389b625 Eddie James 2023-06-27 21 char data[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
>
> data should probably be a u8.
>
> d2c6444389b625 Eddie James 2023-06-27 22 char out[8];
> d2c6444389b625 Eddie James 2023-06-27 23 int rc;
> d2c6444389b625 Eddie James 2023-06-27 24 int i;
> d2c6444389b625 Eddie James 2023-06-27 25
> d2c6444389b625 Eddie James 2023-06-27 26 rc = pmbus_lock_interruptible(client);
> d2c6444389b625 Eddie James 2023-06-27 27 if (rc)
> d2c6444389b625 Eddie James 2023-06-27 28 return rc;
> d2c6444389b625 Eddie James 2023-06-27 29
> d2c6444389b625 Eddie James 2023-06-27 30 rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data);
> d2c6444389b625 Eddie James 2023-06-27 31 pmbus_unlock(client);
> d2c6444389b625 Eddie James 2023-06-27 32 if (rc < 0)
> d2c6444389b625 Eddie James 2023-06-27 33 return rc;
> d2c6444389b625 Eddie James 2023-06-27 34
> d2c6444389b625 Eddie James 2023-06-27 35 for (i = 0; i < rc && i < 3; ++i)
> d2c6444389b625 Eddie James 2023-06-27 @36 snprintf(&out[i * 2], 3, "%02X", data[i]);
>
> If data[i] is negative this will print FFFFFFF1 etc. (This is an x86
> config... Did we ever merge that patch to make char signed by default?)
>
That makes me wonder what "%*phN\n" in the updated patch does, as it only passes
'data' as pointer argument.
Either case, no need to resend. I fixed this up when applying it by
declaring data[] as u8. I also dropped the unused variable.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute
2023-06-29 6:53 ` Dan Carpenter
2023-06-29 18:05 ` Guenter Roeck
@ 2023-06-29 18:59 ` Dan Carpenter
2023-06-29 19:09 ` Guenter Roeck
1 sibling, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2023-06-29 18:59 UTC (permalink / raw)
To: oe-kbuild, Eddie James, linux-hwmon
Cc: lkp, oe-kbuild-all, linux-kernel, linux, jdelvare, lakshmiy
On Thu, Jun 29, 2023 at 09:53:16AM +0300, Dan Carpenter wrote:
> d2c6444389b625 Eddie James 2023-06-27 22 char out[8];
> d2c6444389b625 Eddie James 2023-06-27 23 int rc;
> d2c6444389b625 Eddie James 2023-06-27 24 int i;
> d2c6444389b625 Eddie James 2023-06-27 25
> d2c6444389b625 Eddie James 2023-06-27 26 rc = pmbus_lock_interruptible(client);
> d2c6444389b625 Eddie James 2023-06-27 27 if (rc)
> d2c6444389b625 Eddie James 2023-06-27 28 return rc;
> d2c6444389b625 Eddie James 2023-06-27 29
> d2c6444389b625 Eddie James 2023-06-27 30 rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data);
> d2c6444389b625 Eddie James 2023-06-27 31 pmbus_unlock(client);
> d2c6444389b625 Eddie James 2023-06-27 32 if (rc < 0)
> d2c6444389b625 Eddie James 2023-06-27 33 return rc;
> d2c6444389b625 Eddie James 2023-06-27 34
> d2c6444389b625 Eddie James 2023-06-27 35 for (i = 0; i < rc && i < 3; ++i)
> d2c6444389b625 Eddie James 2023-06-27 @36 snprintf(&out[i * 2], 3, "%02X", data[i]);
>
> If data[i] is negative this will print FFFFFFF1 etc. (This is an x86
> config... Did we ever merge that patch to make char signed by default?)
I meant unsigned not signed. But actually we debated both ways...
Signed by default would annoy PowerPC devs since they try to really
lean into the fact that char is unsigned on that arch. :P
https://lwn.net/Articles/911914/
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute
2023-06-29 18:59 ` Dan Carpenter
@ 2023-06-29 19:09 ` Guenter Roeck
2023-06-30 6:18 ` Dan Carpenter
0 siblings, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2023-06-29 19:09 UTC (permalink / raw)
To: Dan Carpenter, oe-kbuild, Eddie James, linux-hwmon
Cc: lkp, oe-kbuild-all, linux-kernel, jdelvare, lakshmiy
On 6/29/23 11:59, Dan Carpenter wrote:
> On Thu, Jun 29, 2023 at 09:53:16AM +0300, Dan Carpenter wrote:
>> d2c6444389b625 Eddie James 2023-06-27 22 char out[8];
>> d2c6444389b625 Eddie James 2023-06-27 23 int rc;
>> d2c6444389b625 Eddie James 2023-06-27 24 int i;
>> d2c6444389b625 Eddie James 2023-06-27 25
>> d2c6444389b625 Eddie James 2023-06-27 26 rc = pmbus_lock_interruptible(client);
>> d2c6444389b625 Eddie James 2023-06-27 27 if (rc)
>> d2c6444389b625 Eddie James 2023-06-27 28 return rc;
>> d2c6444389b625 Eddie James 2023-06-27 29
>> d2c6444389b625 Eddie James 2023-06-27 30 rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data);
>> d2c6444389b625 Eddie James 2023-06-27 31 pmbus_unlock(client);
>> d2c6444389b625 Eddie James 2023-06-27 32 if (rc < 0)
>> d2c6444389b625 Eddie James 2023-06-27 33 return rc;
>> d2c6444389b625 Eddie James 2023-06-27 34
>> d2c6444389b625 Eddie James 2023-06-27 35 for (i = 0; i < rc && i < 3; ++i)
>> d2c6444389b625 Eddie James 2023-06-27 @36 snprintf(&out[i * 2], 3, "%02X", data[i]);
>>
>> If data[i] is negative this will print FFFFFFF1 etc. (This is an x86
>> config... Did we ever merge that patch to make char signed by default?)
>
> I meant unsigned not signed. But actually we debated both ways...
> Signed by default would annoy PowerPC devs since they try to really
> lean into the fact that char is unsigned on that arch. :P
>
> https://lwn.net/Articles/911914/
>
As if anything would be easy nowadays ;-). Anyway, in this case, the array
should be explicitly unsigned, so changing the type to u8 was the right
thing to do. Also, the driver should be usable on non-Intel systems,
which is another reason to make the type sign-specific (even more so in
the context of the above discussion).
Thanks,
Guenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute
2023-06-29 19:09 ` Guenter Roeck
@ 2023-06-30 6:18 ` Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2023-06-30 6:18 UTC (permalink / raw)
To: Guenter Roeck
Cc: oe-kbuild, Eddie James, linux-hwmon, lkp, oe-kbuild-all,
linux-kernel, jdelvare, lakshmiy
On Thu, Jun 29, 2023 at 12:09:17PM -0700, Guenter Roeck wrote:
> On 6/29/23 11:59, Dan Carpenter wrote:
> > On Thu, Jun 29, 2023 at 09:53:16AM +0300, Dan Carpenter wrote:
> > > d2c6444389b625 Eddie James 2023-06-27 22 char out[8];
> > > d2c6444389b625 Eddie James 2023-06-27 23 int rc;
> > > d2c6444389b625 Eddie James 2023-06-27 24 int i;
> > > d2c6444389b625 Eddie James 2023-06-27 25
> > > d2c6444389b625 Eddie James 2023-06-27 26 rc = pmbus_lock_interruptible(client);
> > > d2c6444389b625 Eddie James 2023-06-27 27 if (rc)
> > > d2c6444389b625 Eddie James 2023-06-27 28 return rc;
> > > d2c6444389b625 Eddie James 2023-06-27 29
> > > d2c6444389b625 Eddie James 2023-06-27 30 rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data);
> > > d2c6444389b625 Eddie James 2023-06-27 31 pmbus_unlock(client);
> > > d2c6444389b625 Eddie James 2023-06-27 32 if (rc < 0)
> > > d2c6444389b625 Eddie James 2023-06-27 33 return rc;
> > > d2c6444389b625 Eddie James 2023-06-27 34
> > > d2c6444389b625 Eddie James 2023-06-27 35 for (i = 0; i < rc && i < 3; ++i)
> > > d2c6444389b625 Eddie James 2023-06-27 @36 snprintf(&out[i * 2], 3, "%02X", data[i]);
> > >
> > > If data[i] is negative this will print FFFFFFF1 etc. (This is an x86
> > > config... Did we ever merge that patch to make char signed by default?)
> >
> > I meant unsigned not signed. But actually we debated both ways...
> > Signed by default would annoy PowerPC devs since they try to really
> > lean into the fact that char is unsigned on that arch. :P
> >
> > https://lwn.net/Articles/911914/
> >
>
> As if anything would be easy nowadays ;-). Anyway, in this case, the array
> should be explicitly unsigned, so changing the type to u8 was the right
> thing to do. Also, the driver should be usable on non-Intel systems,
> which is another reason to make the type sign-specific (even more so in
> the context of the above discussion).
Actually we did make char unsigned. I don't know if I'm super
comfortable with code that assumes char is unsigned. It's makes
backporting trickier. But this is definitely a false positive so I have
silenced the warning in Smatch.
regards,
dan carpenter
--- a/check_kernel_printf.c
+++ b/check_kernel_printf.c
@@ -827,7 +827,7 @@ hexbyte(const char *fmt, int fmt_len, struct expression *arg, int vaidx, struct
sm_warning("could not determine type of argument %d", vaidx);
return;
}
- if (type == &char_ctype || type == &schar_ctype)
+ if (type_signed(type) && (type == &char_ctype || type == &schar_ctype))
sm_warning("argument %d to %.*s specifier has type '%s'",
vaidx, fmt_len, fmt, type_to_str(type));
}
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-06-30 6:19 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-27 18:40 [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute Eddie James
2023-06-27 22:19 ` Guenter Roeck
2023-06-28 14:50 ` Eddie James
2023-06-29 6:53 ` Dan Carpenter
2023-06-29 18:05 ` Guenter Roeck
2023-06-29 18:59 ` Dan Carpenter
2023-06-29 19:09 ` Guenter Roeck
2023-06-30 6:18 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox