diff mbox series

[04/15] drivers: thermal: tsens: Add debugfs support

Message ID 534b5017c2210ba8d541c206dace204d6617b4c9.1564091601.git.amit.kucheria@linaro.org (mailing list archive)
State Superseded
Headers show
Series thermal: qcom: tsens: Add interrupt support | expand

Commit Message

Amit Kucheria July 25, 2019, 10:18 p.m. UTC
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(+)

Comments

Stephen Boyd Aug. 17, 2019, 4:07 a.m. UTC | #1
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;
> +}
Amit Kucheria Aug. 19, 2019, 7:58 a.m. UTC | #2
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;
> > +}
Stephen Boyd Aug. 19, 2019, 2:27 p.m. UTC | #3
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;
> > > +
Amit Kucheria Aug. 21, 2019, 12:55 p.m. UTC | #4
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;
> > > > +
Stephen Boyd Aug. 26, 2019, 8:55 p.m. UTC | #5
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 mbox series

Patch

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];
 };