* [RESEND PATCH 1/2] thermal: qcom: tsens: init debugfs only with successful probe
@ 2022-10-21 18:19 Christian Marangi
2022-10-21 18:19 ` [RESEND PATCH 2/2] thermal: qcom: tsens: simplify debugfs init function Christian Marangi
2022-10-21 18:28 ` [RESEND PATCH 1/2] thermal: qcom: tsens: init debugfs only with successful probe Daniel Lezcano
0 siblings, 2 replies; 6+ messages in thread
From: Christian Marangi @ 2022-10-21 18:19 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Amit Kucheria,
Thara Gopinath, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
linux-arm-msm, linux-pm, linux-kernel
Cc: Christian Marangi, Thara Gopinath
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 | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index b1b10005fb28..cc2965b8d409 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,12 @@ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RESEND PATCH 2/2] thermal: qcom: tsens: simplify debugfs init function
2022-10-21 18:19 [RESEND PATCH 1/2] thermal: qcom: tsens: init debugfs only with successful probe Christian Marangi
@ 2022-10-21 18:19 ` Christian Marangi
2022-10-21 18:33 ` Daniel Lezcano
2022-10-21 18:28 ` [RESEND PATCH 1/2] thermal: qcom: tsens: init debugfs only with successful probe Daniel Lezcano
1 sibling, 1 reply; 6+ messages in thread
From: Christian Marangi @ 2022-10-21 18:19 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Amit Kucheria,
Thara Gopinath, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
linux-arm-msm, linux-pm, linux-kernel
Cc: Christian Marangi, Thara Gopinath
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);
debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops);
}
#else
--
2.37.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH 1/2] thermal: qcom: tsens: init debugfs only with successful probe
2022-10-21 18:19 [RESEND PATCH 1/2] thermal: qcom: tsens: init debugfs only with successful probe Christian Marangi
2022-10-21 18:19 ` [RESEND PATCH 2/2] thermal: qcom: tsens: simplify debugfs init function Christian Marangi
@ 2022-10-21 18:28 ` Daniel Lezcano
1 sibling, 0 replies; 6+ messages in thread
From: Daniel Lezcano @ 2022-10-21 18:28 UTC (permalink / raw)
To: Christian Marangi, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Amit Kucheria, Thara Gopinath, Rafael J. Wysocki, Zhang Rui,
linux-arm-msm, linux-pm, linux-kernel
Cc: Thara Gopinath
On 21/10/2022 20:19, Christian Marangi wrote:
> 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 | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index b1b10005fb28..cc2965b8d409 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,12 @@ static int tsens_probe(struct platform_device *pdev)
> }
> }
>
> - return tsens_register(priv);
> + ret = tsens_register(priv);
> +
extra line
> + if (!ret)
> + tsens_debug_init(pdev);
> +
> + return ret;
> }
>
> static int tsens_remove(struct platform_device *pdev)
--
<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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH 2/2] thermal: qcom: tsens: simplify debugfs init function
2022-10-21 18:19 ` [RESEND PATCH 2/2] thermal: qcom: tsens: simplify debugfs init function Christian Marangi
@ 2022-10-21 18:33 ` Daniel Lezcano
2022-10-21 18:56 ` Christian Marangi
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Lezcano @ 2022-10-21 18:33 UTC (permalink / raw)
To: Christian Marangi, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Amit Kucheria, Thara Gopinath, Rafael J. Wysocki, Zhang Rui,
linux-arm-msm, linux-pm, linux-kernel
Cc: Thara Gopinath
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
--
<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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH 2/2] thermal: qcom: tsens: simplify debugfs init function
2022-10-21 18:33 ` Daniel Lezcano
@ 2022-10-21 18:56 ` Christian Marangi
2022-10-21 19:10 ` Daniel Lezcano
0 siblings, 1 reply; 6+ messages in thread
From: Christian Marangi @ 2022-10-21 18:56 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Amit Kucheria,
Thara Gopinath, Rafael J. Wysocki, Zhang Rui, linux-arm-msm,
linux-pm, linux-kernel, Thara Gopinath
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
--
Ansuel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH 2/2] thermal: qcom: tsens: simplify debugfs init function
2022-10-21 18:56 ` Christian Marangi
@ 2022-10-21 19:10 ` Daniel Lezcano
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Lezcano @ 2022-10-21 19:10 UTC (permalink / raw)
To: Christian Marangi
Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Amit Kucheria,
Thara Gopinath, Rafael J. Wysocki, Zhang Rui, linux-arm-msm,
linux-pm, linux-kernel, Dmitry Baryshkov
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
>
--
<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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-21 19:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-21 18:19 [RESEND PATCH 1/2] thermal: qcom: tsens: init debugfs only with successful probe Christian Marangi
2022-10-21 18:19 ` [RESEND PATCH 2/2] thermal: qcom: tsens: simplify debugfs init function Christian Marangi
2022-10-21 18:33 ` Daniel Lezcano
2022-10-21 18:56 ` Christian Marangi
2022-10-21 19:10 ` Daniel Lezcano
2022-10-21 18:28 ` [RESEND PATCH 1/2] thermal: qcom: tsens: init debugfs only with successful probe Daniel Lezcano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox