| Message ID | 20210419012930.7727-1-ansuelsmth@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [thermal-next,1/2] thermal: qcom: tsens: init debugfs only with successful probe | expand |
Hi, Please include a cover letter next time describing the patch series. On 4/18/21 9:29 PM, Ansuel Smith wrote: > Simplify debugfs init function. > - Drop useless variables > - 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: Ansuel Smith <ansuelsmth@gmail.com> > --- > drivers/thermal/qcom/tsens.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c > index f9d50a67e..b086d1496 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,19 @@ 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); Unconditionally creating priv->debug here is correct. The below if (!priv->debug) will never be true because as per your patch 1, we call tsens_debug_init once per instance of tsens. > + priv->debug = debugfs_lookup(dev_name(&pdev->dev), priv->debug_root); > + if (!priv->debug) > + priv->debug = debugfs_create_dir(dev_name(&pdev->dev), priv->debug_root); > debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops); > } > #else >
On Wed, Apr 28, 2021 at 12:47:30PM -0400, Thara Gopinath wrote: > Hi, > > Please include a cover letter next time describing the patch series. > Yes sorry, I tought that for a small series (2 patch) it wasn't needed. > On 4/18/21 9:29 PM, Ansuel Smith wrote: > > Simplify debugfs init function. > > - Drop useless variables > > - 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: Ansuel Smith <ansuelsmth@gmail.com> > > --- > > drivers/thermal/qcom/tsens.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c > > index f9d50a67e..b086d1496 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,19 @@ 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); > > Unconditionally creating priv->debug here is correct. The below if > (!priv->debug) will never be true because as per your patch 1, we call > tsens_debug_init once per instance of tsens. > You are right, will send a v2 if everything else is good. What do you think? > > + priv->debug = debugfs_lookup(dev_name(&pdev->dev), priv->debug_root); > > + if (!priv->debug) > > + priv->debug = debugfs_create_dir(dev_name(&pdev->dev), priv->debug_root); > > debugfs_create_file("sensors", 0444, priv->debug, pdev, &dbg_sensors_fops); > > } > > #else > > > > -- > Warm Regards > Thara
On 4/28/21 2:48 PM, Ansuel Smith wrote: > On Wed, Apr 28, 2021 at 12:47:30PM -0400, Thara Gopinath wrote: >> Hi, >> >> Please include a cover letter next time describing the patch series. >> > > Yes sorry, I tought that for a small series (2 patch) it wasn't needed. > >> On 4/18/21 9:29 PM, Ansuel Smith wrote: >>> Simplify debugfs init function. >>> - Drop useless variables >>> - 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: Ansuel Smith <ansuelsmth@gmail.com> >>> --- >>> drivers/thermal/qcom/tsens.c | 16 +++++++--------- >>> 1 file changed, 7 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c >>> index f9d50a67e..b086d1496 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,19 @@ 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); >> >> Unconditionally creating priv->debug here is correct. The below if >> (!priv->debug) will never be true because as per your patch 1, we call >> tsens_debug_init once per instance of tsens. >> > > You are right, will send a v2 if everything else is good. What do you > think? I have not tested this yet. The clean up itself looks okay to me. My question is have you tried this with 8960 tsens ? That is the only version of tsens that does not use init_common and hence looks to me that a debug interface is not created. I don't think this should be a problem though. So if you can fix the above, it is a go ahead from me.
On Wed, Apr 28, 2021 at 03:14:31PM -0400, Thara Gopinath wrote: > > > On 4/28/21 2:48 PM, Ansuel Smith wrote: > > On Wed, Apr 28, 2021 at 12:47:30PM -0400, Thara Gopinath wrote: > > > Hi, > > > > > > Please include a cover letter next time describing the patch series. > > > > > > > Yes sorry, I tought that for a small series (2 patch) it wasn't needed. > > > > > On 4/18/21 9:29 PM, Ansuel Smith wrote: > > > > Simplify debugfs init function. > > > > - Drop useless variables > > > > - 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: Ansuel Smith <ansuelsmth@gmail.com> > > > > --- > > > > drivers/thermal/qcom/tsens.c | 16 +++++++--------- > > > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c > > > > index f9d50a67e..b086d1496 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,19 @@ 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); > > > > > > Unconditionally creating priv->debug here is correct. The below if > > > (!priv->debug) will never be true because as per your patch 1, we call > > > tsens_debug_init once per instance of tsens. > > > > > > > You are right, will send a v2 if everything else is good. What do you > > think? > > I have not tested this yet. The clean up itself looks okay to me. > My question is have you tried this with 8960 tsens ? That is the only > version of tsens that does not use init_common and hence looks to me that a > debug interface is not created. I don't think this should be a problem > though. So if you can fix the above, it is a go ahead from me. > Recent commits should have switched 8960 to init_common. Actually I pushed this cause while testing 8960 I notice the warning about double debugfs. Anyway thx for the review. Will send v2 ASAP. > > -- > Warm Regards > Thara
On 4/28/21 3:19 PM, Ansuel Smith wrote: > On Wed, Apr 28, 2021 at 03:14:31PM -0400, Thara Gopinath wrote: >> >> >> On 4/28/21 2:48 PM, Ansuel Smith wrote: >>> On Wed, Apr 28, 2021 at 12:47:30PM -0400, Thara Gopinath wrote: >>>> Hi, >>>> >>>> Please include a cover letter next time describing the patch series. >>>> >>> >>> Yes sorry, I tought that for a small series (2 patch) it wasn't needed. >>> >>>> On 4/18/21 9:29 PM, Ansuel Smith wrote: >>>>> Simplify debugfs init function. >>>>> - Drop useless variables >>>>> - 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: Ansuel Smith <ansuelsmth@gmail.com> >>>>> --- >>>>> drivers/thermal/qcom/tsens.c | 16 +++++++--------- >>>>> 1 file changed, 7 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c >>>>> index f9d50a67e..b086d1496 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,19 @@ 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); >>>> >>>> Unconditionally creating priv->debug here is correct. The below if >>>> (!priv->debug) will never be true because as per your patch 1, we call >>>> tsens_debug_init once per instance of tsens. >>>> >>> >>> You are right, will send a v2 if everything else is good. What do you >>> think? >> >> I have not tested this yet. The clean up itself looks okay to me. >> My question is have you tried this with 8960 tsens ? That is the only >> version of tsens that does not use init_common and hence looks to me that a >> debug interface is not created. I don't think this should be a problem >> though. So if you can fix the above, it is a go ahead from me. >> > > Recent commits should have switched 8960 to init_common. Actually I > pushed this cause while testing 8960 I notice the warning about > double debugfs. Anyway thx for the review. Will send v2 ASAP. Sounds good. Thanks. > >> >> -- >> Warm Regards >> Thara
On 28/04/2021 18:47, Thara Gopinath wrote: > Hi, > > Please include a cover letter next time describing the patch series. Yes, a cover letter helps for the understanding of a patch series but in this case the changes are simple enough to get rid of it. > On 4/18/21 9:29 PM, Ansuel Smith wrote: >> Simplify debugfs init function. >> - Drop useless variables >> - 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: Ansuel Smith <ansuelsmth@gmail.com> >> --- >> drivers/thermal/qcom/tsens.c | 16 +++++++--------- >> 1 file changed, 7 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c >> index f9d50a67e..b086d1496 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,19 @@ 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); > > Unconditionally creating priv->debug here is correct. The below if > (!priv->debug) will never be true because as per your patch 1, we call > tsens_debug_init once per instance of tsens. > >> + priv->debug = debugfs_lookup(dev_name(&pdev->dev), >> priv->debug_root); >> + if (!priv->debug) >> + priv->debug = debugfs_create_dir(dev_name(&pdev->dev), >> priv->debug_root); >> debugfs_create_file("sensors", 0444, priv->debug, pdev, >> &dbg_sensors_fops); >> } >> #else >> >
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c index 4c7ebd1d3..f9d50a67e 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; @@ -1158,7 +1156,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)
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: Ansuel Smith <ansuelsmth@gmail.com> --- drivers/thermal/qcom/tsens.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)