| Message ID | 20220830192212.28570-19-farbere@amazon.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Variety of fixes and new features for mr75203 driver | expand |
On Tue, Aug 30, 2022 at 07:22:11PM +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 ... > + pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); > + if (!pvt->dbgfs_dir) { > + dev_err(dev, "Failed to create dbgfs_dir\n"); > + return -EINVAL; > + } No, just don't check the return value of debugfs API calls. > + debugfs_create_file("ts_coeff_h", 0644, pvt->dbgfs_dir, pvt, > + &pvt_ts_coeff_h_fops); > + debugfs_create_file("ts_coeff_g", 0644, pvt->dbgfs_dir, pvt, > + &pvt_ts_coeff_g_fops); > + debugfs_create_file("ts_coeff_j", 0644, pvt->dbgfs_dir, pvt, > + &pvt_ts_coeff_j_fops); > + debugfs_create_file("ts_coeff_cal5", 0644, pvt->dbgfs_dir, pvt, > + &pvt_ts_coeff_cal5_fops); debugfs has helpers for POD types, use them and shrink your code by ~80%. ... > + ret = devm_add_action_or_reset(dev, devm_pvt_ts_dbgfs_remove, pvt); > + if (ret) { > + dev_err(dev, "failed to add action to remove pvt dbgfs (%d)\n", > + ret); > + return ret; > + } > + > + return 0; return devm_add_...
On 8/31/2022 3:14 PM, Andy Shevchenko wrote: > On Tue, Aug 30, 2022 at 07:22:11PM +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 > > ... > >> + pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); > >> + if (!pvt->dbgfs_dir) { >> + dev_err(dev, "Failed to create dbgfs_dir\n"); >> + return -EINVAL; >> + } > > No, just don't check the return value of debugfs API calls. > Do you mean that I should just do: debugfs_create_dir(dev_name(dev), NULL); Can you please explain why it's OK to ignore the return value? >> + debugfs_create_file("ts_coeff_h", 0644, pvt->dbgfs_dir, pvt, >> + &pvt_ts_coeff_h_fops); >> + debugfs_create_file("ts_coeff_g", 0644, pvt->dbgfs_dir, pvt, >> + &pvt_ts_coeff_g_fops); >> + debugfs_create_file("ts_coeff_j", 0644, pvt->dbgfs_dir, pvt, >> + &pvt_ts_coeff_j_fops); >> + debugfs_create_file("ts_coeff_cal5", 0644, pvt->dbgfs_dir, pvt, >> + &pvt_ts_coeff_cal5_fops); > > debugfs has helpers for POD types, use them and shrink your code by ~80%. > Fixed for v4. I used debugfs_create_u32() for ts_coeff_h, ts_coeff_g and ts_coeff_j. For ts_coeff_cal5 I still use debugfs_create_file() because I must make sure it is not set to 0. >> + ret = devm_add_action_or_reset(dev, devm_pvt_ts_dbgfs_remove, >> pvt); >> + if (ret) { >> + dev_err(dev, "failed to add action to remove pvt dbgfs >> (%d)\n", >> + ret); >> + return ret; >> + } >> + >> + return 0; > > return devm_add_... Fixed for v4. -- Thanks, Eliav
On Thu, Sep 01, 2022 at 09:54:21AM +0300, Farber, Eliav wrote: > On 8/31/2022 3:14 PM, Andy Shevchenko wrote: > > On Tue, Aug 30, 2022 at 07:22:11PM +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 > > > > ... > > > > > + pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); > > > > > + if (!pvt->dbgfs_dir) { > > > + dev_err(dev, "Failed to create dbgfs_dir\n"); > > > + return -EINVAL; > > > + } > > > > No, just don't check the return value of debugfs API calls. > > > Do you mean that I should just do: > debugfs_create_dir(dev_name(dev), NULL); > Can you please explain why it's OK to ignore the return value? > Because that is how debugfs code is supposed to be implemented. Other debugfs code checks if the dir parameter passed to it is NULL and do nothing if it is. This reduces code size overall by avoiding unnecessary error checks. Guenter
On Thu, Sep 01, 2022 at 09:54:21AM +0300, Farber, Eliav wrote: > On 8/31/2022 3:14 PM, Andy Shevchenko wrote: > > On Tue, Aug 30, 2022 at 07:22:11PM +0000, Eliav Farber wrote: ... > > > + pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); > > > > > + if (!pvt->dbgfs_dir) { > > > + dev_err(dev, "Failed to create dbgfs_dir\n"); > > > + return -EINVAL; > > > + } > > > > No, just don't check the return value of debugfs API calls. > > > Do you mean that I should just do: > debugfs_create_dir(dev_name(dev), NULL); > Can you please explain why it's OK to ignore the return value? Author of the debugfs is speaking: https://patchwork.kernel.org/project/linux-wireless/patch/20190122152151.16139-38-gregkh@linuxfoundation.org/
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c index d9fc5d225868..8c30f0521452 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> @@ -153,6 +154,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; @@ -162,6 +164,198 @@ struct pvt_device { u32 ip_freq; }; +static ssize_t pvt_ts_coeff_h_read(struct file *file, + char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct pvt_device *pvt = file->private_data; + char buf[16]; + unsigned int len; + + len = sprintf(buf, "%u\n", pvt->ts_coeff.h); + + return simple_read_from_buffer(user_buf, count, ppos, buf, len); +} + +static ssize_t pvt_ts_coeff_h_write(struct file *file, + const char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct pvt_device *pvt = file->private_data; + int ret; + u32 coeff; + + ret = kstrtou32_from_user(user_buf, count, 0, &coeff); + if (ret) + return ret; + + pvt->ts_coeff.h = coeff; + + return count; +} + +static const struct file_operations pvt_ts_coeff_h_fops = { + .read = pvt_ts_coeff_h_read, + .write = pvt_ts_coeff_h_write, + .open = simple_open, + .owner = THIS_MODULE, + .llseek = default_llseek, +}; + +static ssize_t pvt_ts_coeff_g_read(struct file *file, + char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct pvt_device *pvt = file->private_data; + char buf[16]; + unsigned int len; + + len = sprintf(buf, "%u\n", pvt->ts_coeff.g); + + return simple_read_from_buffer(user_buf, count, ppos, buf, len); +} + +static ssize_t pvt_ts_coeff_g_write(struct file *file, + const char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct pvt_device *pvt = file->private_data; + int ret; + u32 coeff; + + ret = kstrtou32_from_user(user_buf, count, 0, &coeff); + if (ret) + return ret; + + pvt->ts_coeff.g = coeff; + + return count; +} + +static const struct file_operations pvt_ts_coeff_g_fops = { + .read = pvt_ts_coeff_g_read, + .write = pvt_ts_coeff_g_write, + .open = simple_open, + .owner = THIS_MODULE, + .llseek = default_llseek, +}; + +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; + char buf[16]; + unsigned int len; + + len = sprintf(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; + s32 coeff; + + ret = kstrtos32_from_user(user_buf, count, 0, &coeff); + if (ret) + return ret; + + pvt->ts_coeff.j = coeff; + + 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 ssize_t pvt_ts_coeff_cal5_read(struct file *file, + char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct pvt_device *pvt = file->private_data; + char buf[16]; + unsigned int len; + + len = sprintf(buf, "%u\n", pvt->ts_coeff.cal5); + + return simple_read_from_buffer(user_buf, count, ppos, buf, len); +} + +static ssize_t pvt_ts_coeff_cal5_write(struct file *file, + const char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct pvt_device *pvt = file->private_data; + int ret; + u32 coeff; + + ret = kstrtou32_from_user(user_buf, count, 0, &coeff); + if (ret) + return ret; + + if (coeff == 0) + return -EINVAL; + + pvt->ts_coeff.cal5 = coeff; + + return count; +} + +static const struct file_operations pvt_ts_coeff_cal5_fops = { + .read = pvt_ts_coeff_cal5_read, + .write = pvt_ts_coeff_cal5_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) +{ + int ret; + + pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL); + if (!pvt->dbgfs_dir) { + dev_err(dev, "Failed to create dbgfs_dir\n"); + return -EINVAL; + } + + debugfs_create_file("ts_coeff_h", 0644, pvt->dbgfs_dir, pvt, + &pvt_ts_coeff_h_fops); + debugfs_create_file("ts_coeff_g", 0644, pvt->dbgfs_dir, pvt, + &pvt_ts_coeff_g_fops); + debugfs_create_file("ts_coeff_j", 0644, pvt->dbgfs_dir, pvt, + &pvt_ts_coeff_j_fops); + debugfs_create_file("ts_coeff_cal5", 0644, pvt->dbgfs_dir, pvt, + &pvt_ts_coeff_cal5_fops); + + ret = devm_add_action_or_reset(dev, devm_pvt_ts_dbgfs_remove, pvt); + if (ret) { + dev_err(dev, "failed to add action to remove pvt dbgfs (%d)\n", + ret); + return ret; + } + + return 0; +} + static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr, int channel) { @@ -814,6 +1008,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) {
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 <farbere@amazon.com> --- drivers/hwmon/mr75203.c | 196 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 196 insertions(+)