diff mbox series

[RESEND,2/2] thermal: qcom: tsens: simplify debugfs init function

Message ID 20221021181906.16647-2-ansuelsmth@gmail.com (mailing list archive)
State Not Applicable
Headers show
Series [RESEND,1/2] thermal: qcom: tsens: init debugfs only with successful probe | expand

Commit Message

Christian Marangi Oct. 21, 2022, 6:19 p.m. UTC 
Simplify debugfs init function.
- Add check for existing dev directory.
- Fix wrong version in dbg_version_show (with version 0.0.0, 0.1.0 was
  incorrectly reported)

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

Comments

Daniel Lezcano Oct. 21, 2022, 6:33 p.m. UTC | #1 
On 21/10/2022 20:19, Christian Marangi wrote:
> Simplify debugfs init function.
> - Add check for existing dev directory.
> - Fix wrong version in dbg_version_show (with version 0.0.0, 0.1.0 was
>    incorrectly reported)
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Reviewed-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>   drivers/thermal/qcom/tsens.c | 14 +++++---------
>   1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index cc2965b8d409..ff82626cecef 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;
> @@ -704,21 +704,17 @@ 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)
> +	if (!debugfs_lookup("version", priv->debug_root))
>   		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);
> +	priv->debug = debugfs_lookup(dev_name(&pdev->dev), priv->debug_root);

Why the directory creation is replaced by the lookup ?

>   	debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops);
>   }
>   #else
Christian Marangi Oct. 21, 2022, 6:56 p.m. UTC | #2 
On Fri, Oct 21, 2022 at 08:33:41PM +0200, Daniel Lezcano wrote:
> On 21/10/2022 20:19, Christian Marangi wrote:
> > Simplify debugfs init function.
> > - Add check for existing dev directory.
> > - Fix wrong version in dbg_version_show (with version 0.0.0, 0.1.0 was
> >    incorrectly reported)
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > Reviewed-by: Thara Gopinath <thara.gopinath@linaro.org>
> > ---
> >   drivers/thermal/qcom/tsens.c | 14 +++++---------
> >   1 file changed, 5 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > index cc2965b8d409..ff82626cecef 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;
> > @@ -704,21 +704,17 @@ 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)
> > +	if (!debugfs_lookup("version", priv->debug_root))
> >   		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);
> > +	priv->debug = debugfs_lookup(dev_name(&pdev->dev), priv->debug_root);
> 
> Why the directory creation is replaced by the lookup ?
>

Hi,
thanks for the review! I have to be honest and do not create some fake
excuse for this. This patch is a bit old and was pending from a long
time so out of despair i just tried to RESEND this hoping someone would
pick it up. And it seems it have worked... Sooo sorry for making you
asking this.

On rechecking the change here, the entire debug_init logic seems
wrong... I don't know if it's possible but what if in the system there
are multiple version of tsens istance? Looks wrong to overwrite the
version with the last one...

I think the original idea of this was to create a directory for each
istance and put in there version and sensors debugfs.

I will propose this in the next version if it's ok for you and you agree
with this logic. Also I think I will split this in 2 different patch one
for the version fixup and one for this new logic.

Waiting for your feedback and again sorry for the noise.

> >   	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
Daniel Lezcano Oct. 21, 2022, 7:10 p.m. UTC | #3 
On 21/10/2022 20:56, Christian Marangi wrote:

[ ... ]

> Hi,
> thanks for the review! I have to be honest and do not create some fake
> excuse for this. This patch is a bit old and was pending from a long
> time so out of despair i just tried to RESEND this hoping someone would
> pick it up. And it seems it have worked... Sooo sorry for making you
> asking this.
> 
> On rechecking the change here, the entire debug_init logic seems
> wrong... I don't know if it's possible but what if in the system there
> are multiple version of tsens istance? Looks wrong to overwrite the
> version with the last one...

It sounds not logical to have different versions, a quick look at the DT 
seems to confirm this. However, it is an assumption and it may be safer 
to assume the opposite can happen

> I think the original idea of this was to create a directory for each
> istance and put in there version and sensors debugfs.
> 
> I will propose this in the next version if it's ok for you and you agree
> with this logic. Also I think I will split this in 2 different patch one
> for the version fixup and one for this new logic.

I don't have a strong opinion on that but it seems reasonable

> Waiting for your feedback and again sorry for the noise.

No worries ;)

>>>    	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
>
diff mbox series

Patch

diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index cc2965b8d409..ff82626cecef 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;
@@ -704,21 +704,17 @@  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)
+	if (!debugfs_lookup("version", priv->debug_root))
 		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);
+	priv->debug = debugfs_lookup(dev_name(&pdev->dev), priv->debug_root);
 	debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops);
 }
 #else