* [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 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
* 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
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