This change adds debugfs to read and write temperature sensor coefficients
- g, h, j and cal5.
The coefficients can vary between product and product, so it can be very
useful to be able to modify them on the fly during the calibration
process.
e.g.:
cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_cal5
4096
echo 83000 > sys/kernel/debug/940f23d0000.pvt/ts_coeff_g
Signed-off-by: Eliav Farber <[email protected]>
---
V5 -> V4:
- Return j coefficient to use debugfs_create_file() instead of
debugfs_create_u32() because j is signed.
V4 -> V3:
- Remove check of the debugfs_create_dir() return value.
- Use debugfs_create_u32() instead of debugfs_create_file().
- Return devm_add_action_or_reset() without checking return value and printing
an error message on failure.
drivers/hwmon/mr75203.c | 62 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 07668545c3ae..e6b49f810307 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -9,6 +9,7 @@
*/
#include <linux/bits.h>
#include <linux/clk.h>
+#include <linux/debugfs.h>
#include <linux/hwmon.h>
#include <linux/module.h>
#include <linux/mod_devicetable.h>
@@ -170,6 +171,7 @@ struct pvt_device {
struct regmap *v_map;
struct clk *clk;
struct reset_control *rst;
+ struct dentry *dbgfs_dir;
struct voltage_device *vd;
struct voltage_channels vm_channels;
struct temp_coeff ts_coeff;
@@ -179,6 +181,64 @@ struct pvt_device {
u32 ip_freq;
};
+static ssize_t pvt_ts_coeff_j_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct pvt_device *pvt = file->private_data;
+ unsigned int len;
+ char buf[13];
+
+ len = scnprintf(buf, sizeof(buf), "%d\n", pvt->ts_coeff.j);
+
+ return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t pvt_ts_coeff_j_write(struct file *file,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct pvt_device *pvt = file->private_data;
+ int ret;
+
+ ret = kstrtos32_from_user(user_buf, count, 0, &pvt->ts_coeff.j);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static const struct file_operations pvt_ts_coeff_j_fops = {
+ .read = pvt_ts_coeff_j_read,
+ .write = pvt_ts_coeff_j_write,
+ .open = simple_open,
+ .owner = THIS_MODULE,
+ .llseek = default_llseek,
+};
+
+static void devm_pvt_ts_dbgfs_remove(void *data)
+{
+ struct pvt_device *pvt = (struct pvt_device *)data;
+
+ debugfs_remove_recursive(pvt->dbgfs_dir);
+ pvt->dbgfs_dir = NULL;
+}
+
+static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
+{
+ pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
+
+ debugfs_create_u32("ts_coeff_h", 0644, pvt->dbgfs_dir,
+ &pvt->ts_coeff.h);
+ debugfs_create_u32("ts_coeff_g", 0644, pvt->dbgfs_dir,
+ &pvt->ts_coeff.g);
+ debugfs_create_u32("ts_coeff_cal5", 0644, pvt->dbgfs_dir,
+ &pvt->ts_coeff.cal5);
+ debugfs_create_file("ts_coeff_j", 0644, pvt->dbgfs_dir, pvt,
+ &pvt_ts_coeff_j_fops);
+
+ return devm_add_action_or_reset(dev, devm_pvt_ts_dbgfs_remove, pvt);
+}
+
static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
u32 attr, int channel)
{
@@ -813,6 +873,8 @@ static int mr75203_probe(struct platform_device *pdev)
memset32(temp_config, HWMON_T_INPUT, ts_num);
pvt_temp.config = temp_config;
pvt_info[index++] = &pvt_temp;
+
+ pvt_ts_dbgfs_create(pvt, dev);
}
if (pd_num) {
--
2.37.1
On Thu, Sep 08, 2022 at 03:24:48PM +0000, Eliav Farber wrote:
> This change adds debugfs to read and write temperature sensor coefficients
> - g, h, j and cal5.
>
> The coefficients can vary between product and product, so it can be very
> useful to be able to modify them on the fly during the calibration
> process.
>
> e.g.:
>
> cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_cal5
> 4096
>
> echo 83000 > sys/kernel/debug/940f23d0000.pvt/ts_coeff_g
...
> - Return j coefficient to use debugfs_create_file() instead of
> debugfs_create_u32() because j is signed.
You can use
DEFINE_DEBUGFS_ATTRIBUTE(ts_coeff_j, ts_coeff_j_get, ts_coeff_j_set, "%lld\n");
which still makes code compact.
--
With Best Regards,
Andy Shevchenko
On 9/8/2022 9:11 PM, Andy Shevchenko wrote:
> On Thu, Sep 08, 2022 at 03:24:48PM +0000, Eliav Farber wrote:
>> This change adds debugfs to read and write temperature sensor
>> coefficients
>> - g, h, j and cal5.
>>
>> The coefficients can vary between product and product, so it can be very
>> useful to be able to modify them on the fly during the calibration
>> process.
>>
>> e.g.:
>>
>> cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_cal5
>> 4096
>>
>> echo 83000 > sys/kernel/debug/940f23d0000.pvt/ts_coeff_g
>
> ...
>
>> - Return j coefficient to use debugfs_create_file() instead of
>> debugfs_create_u32() because j is signed.
>
> You can use
>
> DEFINE_DEBUGFS_ATTRIBUTE(ts_coeff_j, ts_coeff_j_get, ts_coeff_j_set,
> "%lld\n");
>
> which still makes code compact.
I tried your suggestion to use DEFINE_DEBUGFS_ATTRIBUTE but I can't set
j to be a negative value:
root@alpine:~# cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_j
0
root@alpine:~# echo 100 > /sys/kernel/debug/940f23d0000.pvt/ts_coeff_j
root@alpine:~# cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_j
100
root@alpine:~# echo -100 > /sys/kernel/debug/940f23d0000.pvt/ts_coeff_j
sh: write error: Invalid argument
This is the code I added:
static int ts_coeff_j_set(void *data, u64 val)
{
struct pvt_device *pvt = data;
pvt->ts_coeff.j = val;
return 0;
}
static int ts_coeff_j_get(void *data, u64 *val)
{
struct pvt_device *pvt = data;
*val = pvt->ts_coeff.j;
return 0;
}
DEFINE_DEBUGFS_ATTRIBUTE(ts_coeff_j_fops, ts_coeff_j_get,
ts_coeff_j_set, "%lld\n");
static void devm_pvt_ts_dbgfs_remove(void *data)
{
struct pvt_device *pvt = (struct pvt_device *)data;
debugfs_remove_recursive(pvt->dbgfs_dir);
pvt->dbgfs_dir = NULL;
}
static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
{
...
debugfs_create_file("ts_coeff_j", 0644, pvt->dbgfs_dir, pvt,
&ts_coeff_j_fops);
...
I'm using kernel 5.10.112.
Can you please see if I'm did anything wrong?
--
Thanks, Eliav
On 9/13/2022 4:06 PM, Farber, Eliav wrote:
> On 9/8/2022 9:11 PM, Andy Shevchenko wrote:
>> On Thu, Sep 08, 2022 at 03:24:48PM +0000, Eliav Farber wrote:
>>> This change adds debugfs to read and write temperature sensor
>>> coefficients
>>> - g, h, j and cal5.
>>>
>>> The coefficients can vary between product and product, so it can be
>>> very
>>> useful to be able to modify them on the fly during the calibration
>>> process.
>>>
>>> e.g.:
>>>
>>> cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_cal5
>>> 4096
>>>
>>> echo 83000 > sys/kernel/debug/940f23d0000.pvt/ts_coeff_g
>>
>> ...
>>
>>> - Return j coefficient to use debugfs_create_file() instead of
>>> debugfs_create_u32() because j is signed.
>>
>> You can use
>>
>> DEFINE_DEBUGFS_ATTRIBUTE(ts_coeff_j, ts_coeff_j_get, ts_coeff_j_set,
>> "%lld\n");
>>
>> which still makes code compact.
>
>
> I tried your suggestion to use DEFINE_DEBUGFS_ATTRIBUTE but I can't set
> j to be a negative value:
>
> root@alpine:~# cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_j
> 0
> root@alpine:~# echo 100 > /sys/kernel/debug/940f23d0000.pvt/ts_coeff_j
> root@alpine:~# cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_j
> 100
> root@alpine:~# echo -100 > /sys/kernel/debug/940f23d0000.pvt/ts_coeff_j
> sh: write error: Invalid argument
>
> This is the code I added:
>
> static int ts_coeff_j_set(void *data, u64 val)
> {
> struct pvt_device *pvt = data;
>
> pvt->ts_coeff.j = val;
> return 0;
> }
>
> static int ts_coeff_j_get(void *data, u64 *val)
> {
> struct pvt_device *pvt = data;
>
> *val = pvt->ts_coeff.j;
> return 0;
> }
>
> DEFINE_DEBUGFS_ATTRIBUTE(ts_coeff_j_fops, ts_coeff_j_get,
> ts_coeff_j_set, "%lld\n");
>
> static void devm_pvt_ts_dbgfs_remove(void *data)
> {
> struct pvt_device *pvt = (struct pvt_device *)data;
>
> debugfs_remove_recursive(pvt->dbgfs_dir);
> pvt->dbgfs_dir = NULL;
> }
>
> static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device
> *dev)
> {
> ...
> debugfs_create_file("ts_coeff_j", 0644, pvt->dbgfs_dir, pvt,
> &ts_coeff_j_fops);
> ...
>
> I'm using kernel 5.10.112.
> Can you please see if I'm did anything wrong?
It seems like debugfs_attr_write() calls simple_attr_write() and it uses
kstrtoull(), which is why it fails when setting a negative value.
This is the same also in v6.0-rc5.
debugfs_attr_read() on the other hand does show the correct value also
when j is negative.
--
Regards, Eliav
On Tue, Sep 13, 2022 at 05:40:16PM +0300, Farber, Eliav wrote:
> On 9/13/2022 4:06 PM, Farber, Eliav wrote:
...
> It seems like debugfs_attr_write() calls simple_attr_write() and it uses
> kstrtoull(), which is why it fails when setting a negative value.
> This is the same also in v6.0-rc5.
>
> debugfs_attr_read() on the other hand does show the correct value also
> when j is negative.
Which puzzles me since there is a few drivers that use %lld.
Yeah, changing it to
ret = sscanf(attr->set_buf, attr->fmt, &val);
if (ret != 1)
ret = -EINVAL;
probably can fix that. Dunno if debugfs maintainer is okay with this.
P.S. This needs revisiting all format strings to see if there are no additional
characters, otherwise that needs to be addressed first, if feasible.
--
With Best Regards,
Andy Shevchenko
On 9/13/2022 8:01 PM, Andy Shevchenko wrote:
> On Tue, Sep 13, 2022 at 05:40:16PM +0300, Farber, Eliav wrote:
>> On 9/13/2022 4:06 PM, Farber, Eliav wrote:
>
> ...
>
>> It seems like debugfs_attr_write() calls simple_attr_write() and it uses
>> kstrtoull(), which is why it fails when setting a negative value.
>> This is the same also in v6.0-rc5.
>>
>> debugfs_attr_read() on the other hand does show the correct value also
>> when j is negative.
>
> Which puzzles me since there is a few drivers that use %lld.
> Yeah, changing it to
>
> ret = sscanf(attr->set_buf, attr->fmt, &val);
> if (ret != 1)
> ret = -EINVAL;
>
> probably can fix that. Dunno if debugfs maintainer is okay with this.
>
> P.S. This needs revisiting all format strings to see if there are no
> additional
> characters, otherwise that needs to be addressed first, if feasible.
I was thinking of making such a correction:
- ret = kstrtoull(attr->set_buf, 0, &val);
+ if (attr->set_buf[0] == '-')
+ ret = kstrtoll(attr->set_buf, 0, &val);
+ else
+ ret = kstrtoull(attr->set_buf, 0, &val);
and when I tested the change it worked, but then I noticed this commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/libfs.c?h=v6.0-rc5&id=488dac0c9237647e9b8f788b6a342595bfa40bda
According to this, it previously used simple_strtoll() which supports
negative values, but was changed to use kstrtoull() to deliberately
return '-EINVAL' if it gets a negative value.
So I’m not sure debugfs maintainers will be okay with a fix that
basically reverts the commit I mentioned.
Hence, what do you suggest to do with my commit?
Is it ok to leave it as it is today?
--
Thanks, Eliav
On Wed, Sep 14, 2022 at 07:26:36AM +0300, Farber, Eliav wrote:
> On 9/13/2022 8:01 PM, Andy Shevchenko wrote:
> > On Tue, Sep 13, 2022 at 05:40:16PM +0300, Farber, Eliav wrote:
> > > On 9/13/2022 4:06 PM, Farber, Eliav wrote:
...
> > > It seems like debugfs_attr_write() calls simple_attr_write() and it uses
> > > kstrtoull(), which is why it fails when setting a negative value.
> > > This is the same also in v6.0-rc5.
> > >
> > > debugfs_attr_read() on the other hand does show the correct value also
> > > when j is negative.
> >
> > Which puzzles me since there is a few drivers that use %lld.
> > Yeah, changing it to
> >
> > ret = sscanf(attr->set_buf, attr->fmt, &val);
> > if (ret != 1)
> > ret = -EINVAL;
> >
> > probably can fix that. Dunno if debugfs maintainer is okay with this.
> >
> > P.S. This needs revisiting all format strings to see if there are no
> > additional
> > characters, otherwise that needs to be addressed first, if feasible.
>
> I was thinking of making such a correction:
>
> - ret = kstrtoull(attr->set_buf, 0, &val);
> + if (attr->set_buf[0] == '-')
> + ret = kstrtoll(attr->set_buf, 0, &val);
> + else
> + ret = kstrtoull(attr->set_buf, 0, &val);
>
> and when I tested the change it worked, but then I noticed this commit:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/libfs.c?h=v6.0-rc5&id=488dac0c9237647e9b8f788b6a342595bfa40bda
>
> According to this, it previously used simple_strtoll() which supports
> negative values, but was changed to use kstrtoull() to deliberately
> return '-EINVAL' if it gets a negative value.
>
> So I’m not sure debugfs maintainers will be okay with a fix that
> basically reverts the commit I mentioned.
> Hence, what do you suggest to do with my commit?
> Is it ok to leave it as it is today?
Meanwhile asking is not a problem, at least we will know for sure.
And yes, leave it as is, but point to the thread where you asking
the clarification.
--
With Best Regards,
Andy Shevchenko
On Wed, Sep 14, 2022 at 05:03:09PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 14, 2022 at 12:32:47PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 14, 2022 at 07:26:36AM +0300, Farber, Eliav wrote:
> > > On 9/13/2022 8:01 PM, Andy Shevchenko wrote:
> > > > On Tue, Sep 13, 2022 at 05:40:16PM +0300, Farber, Eliav wrote:
> > > > > On 9/13/2022 4:06 PM, Farber, Eliav wrote:
...
> > > > > It seems like debugfs_attr_write() calls simple_attr_write() and it uses
> > > > > kstrtoull(), which is why it fails when setting a negative value.
> > > > > This is the same also in v6.0-rc5.
> > > > >
> > > > > debugfs_attr_read() on the other hand does show the correct value also
> > > > > when j is negative.
> > > >
> > > > Which puzzles me since there is a few drivers that use %lld.
> > > > Yeah, changing it to
> > > >
> > > > ret = sscanf(attr->set_buf, attr->fmt, &val);
> > > > if (ret != 1)
> > > > ret = -EINVAL;
> > > >
> > > > probably can fix that. Dunno if debugfs maintainer is okay with this.
> > > >
> > > > P.S. This needs revisiting all format strings to see if there are no
> > > > additional
> > > > characters, otherwise that needs to be addressed first, if feasible.
> > >
> > > I was thinking of making such a correction:
> > >
> > > - ret = kstrtoull(attr->set_buf, 0, &val);
> > > + if (attr->set_buf[0] == '-')
> > > + ret = kstrtoll(attr->set_buf, 0, &val);
> > > + else
> > > + ret = kstrtoull(attr->set_buf, 0, &val);
> > >
> > > and when I tested the change it worked, but then I noticed this commit:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/libfs.c?h=v6.0-rc5&id=488dac0c9237647e9b8f788b6a342595bfa40bda
> > >
> > > According to this, it previously used simple_strtoll() which supports
> > > negative values, but was changed to use kstrtoull() to deliberately
> > > return '-EINVAL' if it gets a negative value.
> > >
> > > So I’m not sure debugfs maintainers will be okay with a fix that
> > > basically reverts the commit I mentioned.
> > > Hence, what do you suggest to do with my commit?
> > > Is it ok to leave it as it is today?
> >
> > Meanwhile asking is not a problem, at least we will know for sure.
> > And yes, leave it as is, but point to the thread where you asking
> > the clarification.
>
> For the record:
>
> $ git grep -n -A1 -w DEFINE_DEBUGFS_ATTRIBUTE | grep ');' | sed 's,.*\(".*%.*"\).*,\1,' | sort | uniq -c
> 1 "%08llx\n"
> 5 "0x%016llx\n"
> 5 "0x%02llx\n"
> 5 "0x%04llx\n"
> 13 "0x%08llx\n"
> 1 "0x%4.4llx\n"
> 3 "0x%.4llx\n"
> 4 "0x%llx\n"
> 1 "%1lld\n"
> 40 "%lld\n"
> 2 "%lli\n"
> 129 "%llu\n"
> 1 "%#llx\n"
> 2 "%llx\n"
>
> means that sscanf() should work and fix the issue. You may even propose a patch
> as a starter for a discussion.
Reading the commit 488dac0c9237 ("libfs: fix error cast of negative value in
simple_attr_write()") I think it should be either reverted or fixed, because
u64 is not an issue for negative numbers. The %lld and %llu in any case are
for 64-bit value, representing it as unsigned simplifies the generic code,
but it doesn't mean we can't keep there signed value if we know that (by
supplying proper format string). We have 43 current users of signed integer
and I'm in doubt this patch doesn't break none of them.
--
With Best Regards,
Andy Shevchenko
On Wed, Sep 14, 2022 at 12:32:47PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 14, 2022 at 07:26:36AM +0300, Farber, Eliav wrote:
> > On 9/13/2022 8:01 PM, Andy Shevchenko wrote:
> > > On Tue, Sep 13, 2022 at 05:40:16PM +0300, Farber, Eliav wrote:
> > > > On 9/13/2022 4:06 PM, Farber, Eliav wrote:
...
> > > > It seems like debugfs_attr_write() calls simple_attr_write() and it uses
> > > > kstrtoull(), which is why it fails when setting a negative value.
> > > > This is the same also in v6.0-rc5.
> > > >
> > > > debugfs_attr_read() on the other hand does show the correct value also
> > > > when j is negative.
> > >
> > > Which puzzles me since there is a few drivers that use %lld.
> > > Yeah, changing it to
> > >
> > > ret = sscanf(attr->set_buf, attr->fmt, &val);
> > > if (ret != 1)
> > > ret = -EINVAL;
> > >
> > > probably can fix that. Dunno if debugfs maintainer is okay with this.
> > >
> > > P.S. This needs revisiting all format strings to see if there are no
> > > additional
> > > characters, otherwise that needs to be addressed first, if feasible.
> >
> > I was thinking of making such a correction:
> >
> > - ret = kstrtoull(attr->set_buf, 0, &val);
> > + if (attr->set_buf[0] == '-')
> > + ret = kstrtoll(attr->set_buf, 0, &val);
> > + else
> > + ret = kstrtoull(attr->set_buf, 0, &val);
> >
> > and when I tested the change it worked, but then I noticed this commit:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/libfs.c?h=v6.0-rc5&id=488dac0c9237647e9b8f788b6a342595bfa40bda
> >
> > According to this, it previously used simple_strtoll() which supports
> > negative values, but was changed to use kstrtoull() to deliberately
> > return '-EINVAL' if it gets a negative value.
> >
> > So I’m not sure debugfs maintainers will be okay with a fix that
> > basically reverts the commit I mentioned.
> > Hence, what do you suggest to do with my commit?
> > Is it ok to leave it as it is today?
>
> Meanwhile asking is not a problem, at least we will know for sure.
> And yes, leave it as is, but point to the thread where you asking
> the clarification.
For the record:
$ git grep -n -A1 -w DEFINE_DEBUGFS_ATTRIBUTE | grep ');' | sed 's,.*\(".*%.*"\).*,\1,' | sort | uniq -c
1 "%08llx\n"
5 "0x%016llx\n"
5 "0x%02llx\n"
5 "0x%04llx\n"
13 "0x%08llx\n"
1 "0x%4.4llx\n"
3 "0x%.4llx\n"
4 "0x%llx\n"
1 "%1lld\n"
40 "%lld\n"
2 "%lli\n"
129 "%llu\n"
1 "%#llx\n"
2 "%llx\n"
means that sscanf() should work and fix the issue. You may even propose a patch
as a starter for a discussion.
--
With Best Regards,
Andy Shevchenko
On 9/14/2022 5:08 PM, Andy Shevchenko wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
>
>
>
> On Wed, Sep 14, 2022 at 05:03:09PM +0300, Andy Shevchenko wrote:
>> On Wed, Sep 14, 2022 at 12:32:47PM +0300, Andy Shevchenko wrote:
>> > On Wed, Sep 14, 2022 at 07:26:36AM +0300, Farber, Eliav wrote:
>> > > On 9/13/2022 8:01 PM, Andy Shevchenko wrote:
>> > > > On Tue, Sep 13, 2022 at 05:40:16PM +0300, Farber, Eliav wrote:
>> > > > > On 9/13/2022 4:06 PM, Farber, Eliav wrote:
>
> ...
>
>> > > > > It seems like debugfs_attr_write() calls simple_attr_write()
>> and it uses
>> > > > > kstrtoull(), which is why it fails when setting a negative
>> value.
>> > > > > This is the same also in v6.0-rc5.
>> > > > >
>> > > > > debugfs_attr_read() on the other hand does show the correct
>> value also
>> > > > > when j is negative.
>> > > >
>> > > > Which puzzles me since there is a few drivers that use %lld.
>> > > > Yeah, changing it to
>> > > >
>> > > > ret = sscanf(attr->set_buf, attr->fmt, &val);
>> > > > if (ret != 1)
>> > > > ret = -EINVAL;
>> > > >
>> > > > probably can fix that. Dunno if debugfs maintainer is okay with
>> this.
>> > > >
>> > > > P.S. This needs revisiting all format strings to see if there
>> are no
>> > > > additional
>> > > > characters, otherwise that needs to be addressed first, if
>> feasible.
>> > >
>> > > I was thinking of making such a correction:
>> > >
>> > > - ret = kstrtoull(attr->set_buf, 0, &val);
>> > > + if (attr->set_buf[0] == '-')
>> > > + ret = kstrtoll(attr->set_buf, 0, &val);
>> > > + else
>> > > + ret = kstrtoull(attr->set_buf, 0, &val);
>> > >
>> > > and when I tested the change it worked, but then I noticed this
>> commit:
>> > >
>> > >
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/libfs.c?h=v6.0-rc5&id=488dac0c9237647e9b8f788b6a342595bfa40bda
>> > >
>> > > According to this, it previously used simple_strtoll() which
>> supports
>> > > negative values, but was changed to use kstrtoull() to deliberately
>> > > return '-EINVAL' if it gets a negative value.
>> > >
>> > > So I’m not sure debugfs maintainers will be okay with a fix that
>> > > basically reverts the commit I mentioned.
>> > > Hence, what do you suggest to do with my commit?
>> > > Is it ok to leave it as it is today?
>> >
>> > Meanwhile asking is not a problem, at least we will know for sure.
>> > And yes, leave it as is, but point to the thread where you asking
>> > the clarification.
>>
>> For the record:
>>
>> $ git grep -n -A1 -w DEFINE_DEBUGFS_ATTRIBUTE | grep ');' | sed
>> 's,.*\(".*%.*"\).*,\1,' | sort | uniq -c
>> 1 "%08llx\n"
>> 5 "0x%016llx\n"
>> 5 "0x%02llx\n"
>> 5 "0x%04llx\n"
>> 13 "0x%08llx\n"
>> 1 "0x%4.4llx\n"
>> 3 "0x%.4llx\n"
>> 4 "0x%llx\n"
>> 1 "%1lld\n"
>> 40 "%lld\n"
>> 2 "%lli\n"
>> 129 "%llu\n"
>> 1 "%#llx\n"
>> 2 "%llx\n"
>>
>> means that sscanf() should work and fix the issue. You may even
>> propose a patch
>> as a starter for a discussion.
I proposed a patch with the fix you suggested.
--
Thanks, Eliav
On Thu, Sep 08, 2022 at 03:24:48PM +0000, Eliav Farber wrote:
> This change adds debugfs to read and write temperature sensor coefficients
> - g, h, j and cal5.
>
> The coefficients can vary between product and product, so it can be very
> useful to be able to modify them on the fly during the calibration
> process.
>
> e.g.:
>
> cat /sys/kernel/debug/940f23d0000.pvt/ts_coeff_cal5
> 4096
>
> echo 83000 > sys/kernel/debug/940f23d0000.pvt/ts_coeff_g
>
> Signed-off-by: Eliav Farber <[email protected]>
Applied to hwmon-next.
Thanks,
Guenter