| Message ID | 534b5017c2210ba8d541c206dace204d6617b4c9.1564091601.git.amit.kucheria@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | thermal: qcom: tsens: Add interrupt support | expand |
Quoting Amit Kucheria (2019-07-25 15:18:39) > Dump some basic version info and sensor details into debugfs > Maybe you can put some sample output in the commit text. > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > drivers/thermal/qcom/tsens-common.c | 85 +++++++++++++++++++++++++++++ > drivers/thermal/qcom/tsens.c | 2 + > drivers/thermal/qcom/tsens.h | 6 ++ > 3 files changed, 93 insertions(+) > > diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c > index 7437bfe196e5..7ab2e740a1da 100644 > --- a/drivers/thermal/qcom/tsens-common.c > +++ b/drivers/thermal/qcom/tsens-common.c > @@ -3,6 +3,7 @@ > * Copyright (c) 2015, The Linux Foundation. All rights reserved. > */ > > +#include <linux/debugfs.h> > #include <linux/err.h> > #include <linux/io.h> > #include <linux/nvmem-consumer.h> > @@ -139,6 +140,79 @@ int get_temp_common(struct tsens_sensor *s, int *temp) > return 0; > } > > +#ifdef CONFIG_DEBUG_FS > +static int dbg_sensors_show(struct seq_file *s, void *data) > +{ > + struct platform_device *pdev = s->private; > + struct tsens_priv *priv = platform_get_drvdata(pdev); > + int i; > + > + seq_printf(s, "max: %2d\nnum: %2d\n\n", > + priv->feat->max_sensors, priv->num_sensors); > + > + seq_puts(s, " id slope offset\n------------------------\n"); > + for (i = 0; i < priv->num_sensors; i++) { > + seq_printf(s, "%8d%8d%8d\n", priv->sensor[i].hw_id, Does this not have spaces between the digits on purpose? > + priv->sensor[i].slope, priv->sensor[i].offset); > + } > + > + return 0; > +} > + > +static int dbg_version_show(struct seq_file *s, void *data) > +{ > + struct platform_device *pdev = s->private; > + struct tsens_priv *priv = platform_get_drvdata(pdev); > + u32 maj_ver, min_ver, step_ver; > + int ret; > + > + if (tsens_ver(priv) > VER_0_1) { > + ret = regmap_field_read(priv->rf[VER_MAJOR], &maj_ver); > + if (ret) > + return ret; > + ret = regmap_field_read(priv->rf[VER_MINOR], &min_ver); > + if (ret) > + return ret; > + ret = regmap_field_read(priv->rf[VER_STEP], &step_ver); > + if (ret) > + return ret; > + seq_printf(s, "%d.%d.%d\n", maj_ver, min_ver, step_ver); > + } else { > + seq_puts(s, "0.1.0\n"); > + } > + > + return 0; > +} > + > +DEFINE_SHOW_ATTRIBUTE(dbg_version); > +DEFINE_SHOW_ATTRIBUTE(dbg_sensors); > + > +static void tsens_debug_init(struct platform_device *pdev) > +{ > + struct tsens_priv *priv = platform_get_drvdata(pdev); > + struct dentry *root, *file; > + > + root = debugfs_lookup("tsens", NULL); Does this get created many times? Why doesn't tsens have a pointer to the root saved away somewhere globally? > + if (!root) > + priv->debug_root = debugfs_create_dir("tsens", NULL); > + else > + priv->debug_root = root; > + > + file = debugfs_lookup("version", priv->debug_root); > + if (!file) > + debugfs_create_file("version", 0444, priv->debug_root, > + pdev, &dbg_version_fops); > + > + /* A directory for each instance of the TSENS IP */ > + priv->debug = debugfs_create_dir(dev_name(&pdev->dev), priv->debug_root); > + debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops); > + > + return; > +}
On Sat, Aug 17, 2019 at 9:37 AM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Amit Kucheria (2019-07-25 15:18:39) > > Dump some basic version info and sensor details into debugfs > > > > Maybe you can put some sample output in the commit text. Will do. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > --- > > drivers/thermal/qcom/tsens-common.c | 85 +++++++++++++++++++++++++++++ > > drivers/thermal/qcom/tsens.c | 2 + > > drivers/thermal/qcom/tsens.h | 6 ++ > > 3 files changed, 93 insertions(+) > > > > diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c > > index 7437bfe196e5..7ab2e740a1da 100644 > > --- a/drivers/thermal/qcom/tsens-common.c > > +++ b/drivers/thermal/qcom/tsens-common.c > > @@ -3,6 +3,7 @@ > > * Copyright (c) 2015, The Linux Foundation. All rights reserved. > > */ > > > > +#include <linux/debugfs.h> > > #include <linux/err.h> > > #include <linux/io.h> > > #include <linux/nvmem-consumer.h> > > @@ -139,6 +140,79 @@ int get_temp_common(struct tsens_sensor *s, int *temp) > > return 0; > > } > > > > +#ifdef CONFIG_DEBUG_FS > > +static int dbg_sensors_show(struct seq_file *s, void *data) > > +{ > > + struct platform_device *pdev = s->private; > > + struct tsens_priv *priv = platform_get_drvdata(pdev); > > + int i; > > + > > + seq_printf(s, "max: %2d\nnum: %2d\n\n", > > + priv->feat->max_sensors, priv->num_sensors); > > + > > + seq_puts(s, " id slope offset\n------------------------\n"); > > + for (i = 0; i < priv->num_sensors; i++) { > > + seq_printf(s, "%8d%8d%8d\n", priv->sensor[i].hw_id, > > Does this not have spaces between the digits on purpose? Nice catch. The 8-char width above hid the problem. Will add. > > + priv->sensor[i].slope, priv->sensor[i].offset); > > + } > > + > > + return 0; > > +} > > + > > +static int dbg_version_show(struct seq_file *s, void *data) > > +{ > > + struct platform_device *pdev = s->private; > > + struct tsens_priv *priv = platform_get_drvdata(pdev); > > + u32 maj_ver, min_ver, step_ver; > > + int ret; > > + > > + if (tsens_ver(priv) > VER_0_1) { > > + ret = regmap_field_read(priv->rf[VER_MAJOR], &maj_ver); > > + if (ret) > > + return ret; > > + ret = regmap_field_read(priv->rf[VER_MINOR], &min_ver); > > + if (ret) > > + return ret; > > + ret = regmap_field_read(priv->rf[VER_STEP], &step_ver); > > + if (ret) > > + return ret; > > + seq_printf(s, "%d.%d.%d\n", maj_ver, min_ver, step_ver); > > + } else { > > + seq_puts(s, "0.1.0\n"); > > + } > > + > > + return 0; > > +} > > + > > +DEFINE_SHOW_ATTRIBUTE(dbg_version); > > +DEFINE_SHOW_ATTRIBUTE(dbg_sensors); > > + > > +static void tsens_debug_init(struct platform_device *pdev) > > +{ > > + struct tsens_priv *priv = platform_get_drvdata(pdev); > > + struct dentry *root, *file; > > + > > + root = debugfs_lookup("tsens", NULL); > > Does this get created many times? Why doesn't tsens have a pointer to > the root saved away somewhere globally? > I guess we could call the statement below to create the root dir and save away the pointer. I was trying to avoid #ifdef CONFIG_DEBUG_FS in init_common() and instead have all of it in a single function that gets called once per instance of the tsens controller. > > + if (!root) > > + priv->debug_root = debugfs_create_dir("tsens", NULL); > > + else > > + priv->debug_root = root; > > + > > + file = debugfs_lookup("version", priv->debug_root); > > + if (!file) > > + debugfs_create_file("version", 0444, priv->debug_root, > > + pdev, &dbg_version_fops); > > + > > + /* A directory for each instance of the TSENS IP */ > > + priv->debug = debugfs_create_dir(dev_name(&pdev->dev), priv->debug_root); > > + debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops); > > + > > + return; > > +}
Quoting Amit Kucheria (2019-08-19 00:58:23) > On Sat, Aug 17, 2019 at 9:37 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > + > > > +static void tsens_debug_init(struct platform_device *pdev) > > > +{ > > > + struct tsens_priv *priv = platform_get_drvdata(pdev); > > > + struct dentry *root, *file; > > > + > > > + root = debugfs_lookup("tsens", NULL); > > > > Does this get created many times? Why doesn't tsens have a pointer to > > the root saved away somewhere globally? > > > > I guess we could call the statement below to create the root dir and > save away the pointer. I was trying to avoid #ifdef CONFIG_DEBUG_FS in > init_common() and instead have all of it in a single function that > gets called once per instance of the tsens controller. Or call this code many times and try to create the tsens node if !tsens_root exists where the variable is some global. > > > > + if (!root) > > > + priv->debug_root = debugfs_create_dir("tsens", NULL); > > > + else > > > + priv->debug_root = root; > > > +
On Mon, Aug 19, 2019 at 7:57 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Amit Kucheria (2019-08-19 00:58:23) > > On Sat, Aug 17, 2019 at 9:37 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > + > > > > +static void tsens_debug_init(struct platform_device *pdev) > > > > +{ > > > > + struct tsens_priv *priv = platform_get_drvdata(pdev); > > > > + struct dentry *root, *file; > > > > + > > > > + root = debugfs_lookup("tsens", NULL); > > > > > > Does this get created many times? Why doesn't tsens have a pointer to > > > the root saved away somewhere globally? > > > > > > > I guess we could call the statement below to create the root dir and > > save away the pointer. I was trying to avoid #ifdef CONFIG_DEBUG_FS in > > init_common() and instead have all of it in a single function that > > gets called once per instance of the tsens controller. > > Or call this code many times and try to create the tsens node if > !tsens_root exists where the variable is some global. So I didn't quite understand this statement. The change you're requesting is that the 'root' variable below should be a global? tsens_probe() will get called twice on platforms with two instances of the controller. So I will need to check some place if the 'tsens' root dir already exists in debugfs, no? That is what I'm doing below. > > > > > > + if (!root) > > > > + priv->debug_root = debugfs_create_dir("tsens", NULL); > > > > + else > > > > + priv->debug_root = root; > > > > +
Quoting Amit Kucheria (2019-08-21 05:55:39) > On Mon, Aug 19, 2019 at 7:57 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Amit Kucheria (2019-08-19 00:58:23) > > > On Sat, Aug 17, 2019 at 9:37 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > + > > > > > +static void tsens_debug_init(struct platform_device *pdev) > > > > > +{ > > > > > + struct tsens_priv *priv = platform_get_drvdata(pdev); > > > > > + struct dentry *root, *file; > > > > > + > > > > > + root = debugfs_lookup("tsens", NULL); > > > > > > > > Does this get created many times? Why doesn't tsens have a pointer to > > > > the root saved away somewhere globally? > > > > > > > > > > I guess we could call the statement below to create the root dir and > > > save away the pointer. I was trying to avoid #ifdef CONFIG_DEBUG_FS in > > > init_common() and instead have all of it in a single function that > > > gets called once per instance of the tsens controller. > > > > Or call this code many times and try to create the tsens node if > > !tsens_root exists where the variable is some global. > > So I didn't quite understand this statement. The change you're > requesting is that the 'root' variable below should be a global? > > tsens_probe() will get called twice on platforms with two instances of > the controller. So I will need to check some place if the 'tsens' root > dir already exists in debugfs, no? That is what I'm doing below. > Yeah. I was suggesting making a global instead of doing the lookup, but I guess the lookup is fine and avoids a global variable. It's all debugfs so it doesn't really matter. Sorry! Do whatever then.
diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c index 7437bfe196e5..7ab2e740a1da 100644 --- a/drivers/thermal/qcom/tsens-common.c +++ b/drivers/thermal/qcom/tsens-common.c @@ -3,6 +3,7 @@ * Copyright (c) 2015, The Linux Foundation. All rights reserved. */ +#include <linux/debugfs.h> #include <linux/err.h> #include <linux/io.h> #include <linux/nvmem-consumer.h> @@ -139,6 +140,79 @@ int get_temp_common(struct tsens_sensor *s, int *temp) return 0; } +#ifdef CONFIG_DEBUG_FS +static int dbg_sensors_show(struct seq_file *s, void *data) +{ + struct platform_device *pdev = s->private; + struct tsens_priv *priv = platform_get_drvdata(pdev); + int i; + + seq_printf(s, "max: %2d\nnum: %2d\n\n", + priv->feat->max_sensors, priv->num_sensors); + + seq_puts(s, " id slope offset\n------------------------\n"); + for (i = 0; i < priv->num_sensors; i++) { + seq_printf(s, "%8d%8d%8d\n", priv->sensor[i].hw_id, + priv->sensor[i].slope, priv->sensor[i].offset); + } + + return 0; +} + +static int dbg_version_show(struct seq_file *s, void *data) +{ + struct platform_device *pdev = s->private; + struct tsens_priv *priv = platform_get_drvdata(pdev); + u32 maj_ver, min_ver, step_ver; + int ret; + + if (tsens_ver(priv) > VER_0_1) { + ret = regmap_field_read(priv->rf[VER_MAJOR], &maj_ver); + if (ret) + return ret; + ret = regmap_field_read(priv->rf[VER_MINOR], &min_ver); + if (ret) + return ret; + ret = regmap_field_read(priv->rf[VER_STEP], &step_ver); + if (ret) + return ret; + seq_printf(s, "%d.%d.%d\n", maj_ver, min_ver, step_ver); + } else { + seq_puts(s, "0.1.0\n"); + } + + return 0; +} + +DEFINE_SHOW_ATTRIBUTE(dbg_version); +DEFINE_SHOW_ATTRIBUTE(dbg_sensors); + +static void tsens_debug_init(struct platform_device *pdev) +{ + struct tsens_priv *priv = platform_get_drvdata(pdev); + struct dentry *root, *file; + + root = debugfs_lookup("tsens", NULL); + if (!root) + priv->debug_root = debugfs_create_dir("tsens", NULL); + else + priv->debug_root = root; + + file = debugfs_lookup("version", priv->debug_root); + if (!file) + debugfs_create_file("version", 0444, priv->debug_root, + pdev, &dbg_version_fops); + + /* A directory for each instance of the TSENS IP */ + priv->debug = debugfs_create_dir(dev_name(&pdev->dev), priv->debug_root); + debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops); + + return; +} +#else +static inline void tsens_debug_init(struct platform_device *pdev) {} +#endif + static const struct regmap_config tsens_config = { .name = "tm", .reg_bits = 32, @@ -199,6 +273,15 @@ int __init init_common(struct tsens_priv *priv) goto err_put_device; } + if (tsens_ver(priv) > VER_0_1) { + for (i = VER_MAJOR; i <= VER_STEP; i++) { + priv->rf[i] = devm_regmap_field_alloc(dev, priv->srot_map, + priv->fields[i]); + if (IS_ERR(priv->rf[i])) + return PTR_ERR(priv->rf[i]); + } + } + priv->rf[TSENS_EN] = devm_regmap_field_alloc(dev, priv->srot_map, priv->fields[TSENS_EN]); if (IS_ERR(priv->rf[TSENS_EN])) { @@ -238,6 +321,8 @@ int __init init_common(struct tsens_priv *priv) } } + tsens_debug_init(op); + return 0; err_put_device: diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c index 06c6bbd69a1a..772aa76b50e1 100644 --- a/drivers/thermal/qcom/tsens.c +++ b/drivers/thermal/qcom/tsens.c @@ -3,6 +3,7 @@ * Copyright (c) 2015, The Linux Foundation. All rights reserved. */ +#include <linux/debugfs.h> #include <linux/err.h> #include <linux/module.h> #include <linux/of.h> @@ -176,6 +177,7 @@ static int tsens_remove(struct platform_device *pdev) { struct tsens_priv *priv = platform_get_drvdata(pdev); + debugfs_remove_recursive(priv->debug_root); if (priv->ops->disable) priv->ops->disable(priv); diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h index d022e726d074..e1d6af71b2b9 100644 --- a/drivers/thermal/qcom/tsens.h +++ b/drivers/thermal/qcom/tsens.h @@ -292,6 +292,8 @@ struct tsens_context { * @feat: features of the IP * @fields: bitfield locations * @ops: pointer to list of callbacks supported by this device + * @debug_root: pointer to debugfs dentry for all tsens + * @debug: pointer to debugfs dentry for tsens controller * @sensor: list of sensors attached to this device */ struct tsens_priv { @@ -305,6 +307,10 @@ struct tsens_priv { const struct tsens_features *feat; const struct reg_field *fields; const struct tsens_ops *ops; + + struct dentry *debug_root; + struct dentry *debug; + struct tsens_sensor sensor[0]; };
Dump some basic version info and sensor details into debugfs Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> --- drivers/thermal/qcom/tsens-common.c | 85 +++++++++++++++++++++++++++++ drivers/thermal/qcom/tsens.c | 2 + drivers/thermal/qcom/tsens.h | 6 ++ 3 files changed, 93 insertions(+)