[PATCH v3 0/3] thermal: qcom: tsens: small fixup for debugfs

[PATCH v3 0/3] thermal: qcom: tsens: small fixup for debugfs

[Christian Marangi]

This is a small series to fixup some bug in how tsens init debufs.

The first patch just handle situation where tsens fails to register and
debugfs are getting registred anyway. When tsens is tried to reprobe
debugs will print a warning as the directory are already there.

The second patch is a fixup for wrong version when the ancient VER_0 is
used.

The third patch is a rework of debugfs structure moving version in the
tsens istance instead of ignoring any other tsens istance if it will
ever be the case in the future of having multiple tsens instance with
different version. It's just futureproof on it's own and also removed
one additional check.

changes v3:
- remove extra space from patch 1
- split patch 2 to 2 different patch
- patch 3 rework wrong debugfs structure
changes v2:
- Changed sob name to new one.

(the resend was actually v2 but i totally forgot that I sent it as v2 with
the sob name fixed... but everything should be good now...)

Christian Marangi (3):
  thermal: qcom: tsens: init debugfs only with successful probe
  thermal: qcom: tsens: fix wrong version id dbg_version_show
  thermal: qcom: tsens: rework debugfs file structure

 drivers/thermal/qcom/tsens.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

-- 
2.37.2

[PATCH v3 1/3] thermal: qcom: tsens: init debugfs only with successful probe

[Christian Marangi]

calibrate and tsens_register can fail or PROBE_DEFER. This will cause a
double or a wrong init of the debugfs information. Init debugfs only
with successful probe fixing warning about directory already present.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Acked-by: Thara Gopinath <thara.gopinath@linaro.org>
---
 drivers/thermal/qcom/tsens.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index b1b10005fb28..af688ce4d14d 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -918,8 +918,6 @@ int __init init_common(struct tsens_priv *priv)
 	if (tsens_version(priv) >= VER_0_1)
 		tsens_enable_irq(priv);
 
-	tsens_debug_init(op);
-
 err_put_device:
 	put_device(&op->dev);
 	return ret;
@@ -1153,7 +1151,11 @@ static int tsens_probe(struct platform_device *pdev)
 		}
 	}
 
-	return tsens_register(priv);
+	ret = tsens_register(priv);
+	if (!ret)
+		tsens_debug_init(pdev);
+
+	return ret;
 }
 
 static int tsens_remove(struct platform_device *pdev)
-- 
2.37.2

[PATCH v3 2/3] thermal: qcom: tsens: fix wrong version id dbg_version_show

[Christian Marangi]

For VER_0 the version was incorrectly reported as 0.1.0.

Fix that and correctly report the major version for this old tsens
revision.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/thermal/qcom/tsens.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index af688ce4d14d..467585c45d34 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -692,7 +692,7 @@ static int dbg_version_show(struct seq_file *s, void *data)
 			return ret;
 		seq_printf(s, "%d.%d.%d\n", maj_ver, min_ver, step_ver);
 	} else {
-		seq_puts(s, "0.1.0\n");
+		seq_printf(s, "0.%d.0\n", priv->feat->ver_major);
 	}
 
 	return 0;
-- 
2.37.2

[PATCH v3 3/3] thermal: qcom: tsens: rework debugfs file structure

[Christian Marangi]

The current tsens debugfs structure is composed by:
- a tsens dir in debugfs with a version file
- a directory for each tsens istance with sensors file to dump all the
  sensors value.

This works on the assumption that we have the same version for each
istance but this assumption seems fragile and with more than one tsens
istance results in the version file not tracking each of them.

A better approach is to just create a subdirectory for each tsens
istance and put there version and sensors debugfs file.

Using this new implementation results in less code since debugfs entry
are created only on successful tsens probe.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/thermal/qcom/tsens.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 467585c45d34..fc12d7c07de4 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -704,21 +704,14 @@ 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_lookup("tsens", NULL);
+	if (!priv->debug_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("version", 0444, priv->debug, pdev, &dbg_version_fops);
 	debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops);
 }
 #else
-- 
2.37.2

Re: [PATCH v3 3/3] thermal: qcom: tsens: rework debugfs file structure

[Daniel Lezcano]

On 22/10/2022 14:56, Christian Marangi wrote:
> The current tsens debugfs structure is composed by:
> - a tsens dir in debugfs with a version file
> - a directory for each tsens istance with sensors file to dump all the
>    sensors value.

s/istance/instance/

The patch looks good to me, no need to resend, I'll fix the typos

> This works on the assumption that we have the same version for each
> istance but this assumption seems fragile and with more than one tsens
> istance results in the version file not tracking each of them.
> 
> A better approach is to just create a subdirectory for each tsens
> istance and put there version and sensors debugfs file.
> 
> Using this new implementation results in less code since debugfs entry
> are created only on successful tsens probe.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>   drivers/thermal/qcom/tsens.c | 13 +++----------
>   1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 467585c45d34..fc12d7c07de4 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -704,21 +704,14 @@ 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_lookup("tsens", NULL);
> +	if (!priv->debug_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("version", 0444, priv->debug, pdev, &dbg_version_fops);
>   	debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops);
>   }
>   #else


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3 3/3] thermal: qcom: tsens: rework debugfs file structure

[Christian Marangi]

On Sat, Oct 22, 2022 at 03:08:46PM +0200, Daniel Lezcano wrote:
> On 22/10/2022 14:56, Christian Marangi wrote:
> > The current tsens debugfs structure is composed by:
> > - a tsens dir in debugfs with a version file
> > - a directory for each tsens istance with sensors file to dump all the
> >    sensors value.
> 
> s/istance/instance/
> 
> The patch looks good to me, no need to resend, I'll fix the typos
>

Thanks for picking this, np for fixing typos.

> > This works on the assumption that we have the same version for each
> > istance but this assumption seems fragile and with more than one tsens
> > istance results in the version file not tracking each of them.
> > 
> > A better approach is to just create a subdirectory for each tsens
> > istance and put there version and sensors debugfs file.
> > 
> > Using this new implementation results in less code since debugfs entry
> > are created only on successful tsens probe.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >   drivers/thermal/qcom/tsens.c | 13 +++----------
> >   1 file changed, 3 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > index 467585c45d34..fc12d7c07de4 100644
> > --- a/drivers/thermal/qcom/tsens.c
> > +++ b/drivers/thermal/qcom/tsens.c
> > @@ -704,21 +704,14 @@ 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_lookup("tsens", NULL);
> > +	if (!priv->debug_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("version", 0444, priv->debug, pdev, &dbg_version_fops);
> >   	debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops);
> >   }
> >   #else
> 
> 
> -- 
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

-- 
	Ansuel

Re: [PATCH v3 0/3] thermal: qcom: tsens: small fixup for debugfs

[Daniel Lezcano]

On 22/10/2022 14:56, Christian Marangi wrote:
> This is a small series to fixup some bug in how tsens init debufs.
> 
> The first patch just handle situation where tsens fails to register and
> debugfs are getting registred anyway. When tsens is tried to reprobe
> debugs will print a warning as the directory are already there.
> 
> The second patch is a fixup for wrong version when the ancient VER_0 is
> used.
> 
> The third patch is a rework of debugfs structure moving version in the
> tsens istance instead of ignoring any other tsens istance if it will
> ever be the case in the future of having multiple tsens instance with
> different version. It's just futureproof on it's own and also removed
> one additional check.
> 
> changes v3:
> - remove extra space from patch 1
> - split patch 2 to 2 different patch
> - patch 3 rework wrong debugfs structure
> changes v2:
> - Changed sob name to new one.
> 
> (the resend was actually v2 but i totally forgot that I sent it as v2 with
> the sob name fixed... but everything should be good now...)
> 
> Christian Marangi (3):
>    thermal: qcom: tsens: init debugfs only with successful probe
>    thermal: qcom: tsens: fix wrong version id dbg_version_show
>    thermal: qcom: tsens: rework debugfs file structure
> 
>   drivers/thermal/qcom/tsens.c | 23 +++++++++--------------
>   1 file changed, 9 insertions(+), 14 deletions(-)

Applied thanks


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v3 3/3] thermal: qcom: tsens: rework debugfs file structure

[Christian Marangi]

On Sat, Oct 22, 2022 at 03:08:46PM +0200, Daniel Lezcano wrote:
> On 22/10/2022 14:56, Christian Marangi wrote:
> > The current tsens debugfs structure is composed by:
> > - a tsens dir in debugfs with a version file
> > - a directory for each tsens istance with sensors file to dump all the
> >    sensors value.
> 
> s/istance/instance/
> 
> The patch looks good to me, no need to resend, I'll fix the typos
>

By checking linux-next it looks like the wrong revision was applied [0].
I think this was done by mistake while fixing the typo. Can this be
fixed? The applied revision conflicts with what we agreed was a good
solution.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/drivers/thermal/qcom/tsens.c

> > This works on the assumption that we have the same version for each
> > istance but this assumption seems fragile and with more than one tsens
> > istance results in the version file not tracking each of them.
> > 
> > A better approach is to just create a subdirectory for each tsens
> > istance and put there version and sensors debugfs file.
> > 
> > Using this new implementation results in less code since debugfs entry
> > are created only on successful tsens probe.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >   drivers/thermal/qcom/tsens.c | 13 +++----------
> >   1 file changed, 3 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > index 467585c45d34..fc12d7c07de4 100644
> > --- a/drivers/thermal/qcom/tsens.c
> > +++ b/drivers/thermal/qcom/tsens.c
> > @@ -704,21 +704,14 @@ 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_lookup("tsens", NULL);
> > +	if (!priv->debug_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("version", 0444, priv->debug, pdev, &dbg_version_fops);
> >   	debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops);
> >   }
> >   #else
> 
> 
> -- 
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

-- 
	Ansuel