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 <[email protected]>
---
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);
+ 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
--
2.30.2
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 <[email protected]>
> ---
> 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
>
--
Warm Regards
Thara
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 <[email protected]>
> > ---
> > 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 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 <[email protected]>
> > > > ---
> > > > 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 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 <[email protected]>
>> ---
>> 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
>>
>
--
<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
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 <[email protected]>
>>> ---
>>> 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.
--
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 <[email protected]>
>>>>> ---
>>>>> 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
--
Warm Regards
Thara