It is more clear to use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs file
operation rather than DEFINE_SIMPLE_ATTRIBUTE.
Signed-off-by: zhong jiang <[email protected]>
---
drivers/iio/imu/adis16460.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
index 6aed9e8..2e7a582 100644
--- a/drivers/iio/imu/adis16460.c
+++ b/drivers/iio/imu/adis16460.c
@@ -87,7 +87,7 @@ static int adis16460_show_serial_number(void *arg, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(adis16460_serial_number_fops,
+DEFINE_DEBUGFS_ATTRIBUTE(adis16460_serial_number_fops,
adis16460_show_serial_number, NULL, "0x%.4llx\n");
static int adis16460_show_product_id(void *arg, u64 *val)
@@ -105,7 +105,7 @@ static int adis16460_show_product_id(void *arg, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(adis16460_product_id_fops,
+DEFINE_DEBUGFS_ATTRIBUTE(adis16460_product_id_fops,
adis16460_show_product_id, NULL, "%llu\n");
static int adis16460_show_flash_count(void *arg, u64 *val)
@@ -123,7 +123,7 @@ static int adis16460_show_flash_count(void *arg, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(adis16460_flash_count_fops,
+DEFINE_DEBUGFS_ATTRIBUTE(adis16460_flash_count_fops,
adis16460_show_flash_count, NULL, "%lld\n");
static int adis16460_debugfs_init(struct iio_dev *indio_dev)
--
1.7.12.4
On Wed, 2019-10-30 at 16:19 +0800, zhong jiang wrote:
> [External]
>
> It is more clear to use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs file
> operation rather than DEFINE_SIMPLE_ATTRIBUTE.
Not sure if "more clear" is the word.
But it is more correct to use DEFINE_DEBUGFS_ATTRIBUTE(), since they are
debugfs attrs.
In any case, this is no big deal.
So:
Reviewed-by: Alexandru Ardelean <[email protected]>
>
> Signed-off-by: zhong jiang <[email protected]>
> ---
> drivers/iio/imu/adis16460.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
> index 6aed9e8..2e7a582 100644
> --- a/drivers/iio/imu/adis16460.c
> +++ b/drivers/iio/imu/adis16460.c
> @@ -87,7 +87,7 @@ static int adis16460_show_serial_number(void *arg, u64
> *val)
>
> return 0;
> }
> -DEFINE_SIMPLE_ATTRIBUTE(adis16460_serial_number_fops,
> +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_serial_number_fops,
> adis16460_show_serial_number, NULL, "0x%.4llx\n");
>
> static int adis16460_show_product_id(void *arg, u64 *val)
> @@ -105,7 +105,7 @@ static int adis16460_show_product_id(void *arg, u64
> *val)
>
> return 0;
> }
> -DEFINE_SIMPLE_ATTRIBUTE(adis16460_product_id_fops,
> +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_product_id_fops,
> adis16460_show_product_id, NULL, "%llu\n");
>
> static int adis16460_show_flash_count(void *arg, u64 *val)
> @@ -123,7 +123,7 @@ static int adis16460_show_flash_count(void *arg, u64
> *val)
>
> return 0;
> }
> -DEFINE_SIMPLE_ATTRIBUTE(adis16460_flash_count_fops,
> +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_flash_count_fops,
> adis16460_show_flash_count, NULL, "%lld\n");
>
> static int adis16460_debugfs_init(struct iio_dev *indio_dev)
On 2019/10/30 17:13, Ardelean, Alexandru wrote:
> On Wed, 2019-10-30 at 16:19 +0800, zhong jiang wrote:
>> [External]
>>
>> It is more clear to use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs file
>> operation rather than DEFINE_SIMPLE_ATTRIBUTE.
> Not sure if "more clear" is the word.
Should be more clearly. :-)
> But it is more correct to use DEFINE_DEBUGFS_ATTRIBUTE(), since they are
> debugfs attrs.
>
> In any case, this is no big deal.
> So:
>
> Reviewed-by: Alexandru Ardelean <[email protected]>
>
>> Signed-off-by: zhong jiang <[email protected]>
>> ---
>> drivers/iio/imu/adis16460.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
>> index 6aed9e8..2e7a582 100644
>> --- a/drivers/iio/imu/adis16460.c
>> +++ b/drivers/iio/imu/adis16460.c
>> @@ -87,7 +87,7 @@ static int adis16460_show_serial_number(void *arg, u64
>> *val)
>>
>> return 0;
>> }
>> -DEFINE_SIMPLE_ATTRIBUTE(adis16460_serial_number_fops,
>> +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_serial_number_fops,
>> adis16460_show_serial_number, NULL, "0x%.4llx\n");
>>
>> static int adis16460_show_product_id(void *arg, u64 *val)
>> @@ -105,7 +105,7 @@ static int adis16460_show_product_id(void *arg, u64
>> *val)
>>
>> return 0;
>> }
>> -DEFINE_SIMPLE_ATTRIBUTE(adis16460_product_id_fops,
>> +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_product_id_fops,
>> adis16460_show_product_id, NULL, "%llu\n");
>>
>> static int adis16460_show_flash_count(void *arg, u64 *val)
>> @@ -123,7 +123,7 @@ static int adis16460_show_flash_count(void *arg, u64
>> *val)
>>
>> return 0;
>> }
>> -DEFINE_SIMPLE_ATTRIBUTE(adis16460_flash_count_fops,
>> +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_flash_count_fops,
>> adis16460_show_flash_count, NULL, "%lld\n");
>>
>> static int adis16460_debugfs_init(struct iio_dev *indio_dev)
On Wed, 30 Oct 2019 17:33:14 +0800
zhong jiang <[email protected]> wrote:
> On 2019/10/30 17:13, Ardelean, Alexandru wrote:
> > On Wed, 2019-10-30 at 16:19 +0800, zhong jiang wrote:
> >> [External]
> >>
> >> It is more clear to use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs file
> >> operation rather than DEFINE_SIMPLE_ATTRIBUTE.
> > Not sure if "more clear" is the word.
> Should be more clearly. :-)
> > But it is more correct to use DEFINE_DEBUGFS_ATTRIBUTE(), since they are
> > debugfs attrs.
> >
> > In any case, this is no big deal.
> > So:
> >
> > Reviewed-by: Alexandru Ardelean <[email protected]>
> >
> >> Signed-off-by: zhong jiang <[email protected]>
I started looking into why this attributes were introduced.
There are potential issues, so Alex can you confirm you've tested this
series. Whilst it looks right, it seems some other patches making this
change have had to switch over to the unsafe registration functions.
https://patchwork.kernel.org/patch/11051725/
https://lkml.org/lkml/2019/10/30/144
The reference counting is subtly different between the two versions.
Seems you are getting some push back on similar patches.
Perhaps a v2 with reference to the other threads if those get resolved
to say it is sensible to make this change.
Thanks,
Jonathan
> >> ---
> >> drivers/iio/imu/adis16460.c | 6 +++---
> >> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
> >> index 6aed9e8..2e7a582 100644
> >> --- a/drivers/iio/imu/adis16460.c
> >> +++ b/drivers/iio/imu/adis16460.c
> >> @@ -87,7 +87,7 @@ static int adis16460_show_serial_number(void *arg, u64
> >> *val)
> >>
> >> return 0;
> >> }
> >> -DEFINE_SIMPLE_ATTRIBUTE(adis16460_serial_number_fops,
> >> +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_serial_number_fops,
> >> adis16460_show_serial_number, NULL, "0x%.4llx\n");
> >>
> >> static int adis16460_show_product_id(void *arg, u64 *val)
> >> @@ -105,7 +105,7 @@ static int adis16460_show_product_id(void *arg, u64
> >> *val)
> >>
> >> return 0;
> >> }
> >> -DEFINE_SIMPLE_ATTRIBUTE(adis16460_product_id_fops,
> >> +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_product_id_fops,
> >> adis16460_show_product_id, NULL, "%llu\n");
> >>
> >> static int adis16460_show_flash_count(void *arg, u64 *val)
> >> @@ -123,7 +123,7 @@ static int adis16460_show_flash_count(void *arg, u64
> >> *val)
> >>
> >> return 0;
> >> }
> >> -DEFINE_SIMPLE_ATTRIBUTE(adis16460_flash_count_fops,
> >> +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_flash_count_fops,
> >> adis16460_show_flash_count, NULL, "%lld\n");
> >>
> >> static int adis16460_debugfs_init(struct iio_dev *indio_dev)
>
>
On Sun, 2019-11-03 at 11:25 +0000, Jonathan Cameron wrote:
> On Wed, 30 Oct 2019 17:33:14 +0800
> zhong jiang <[email protected]> wrote:
>
> > On 2019/10/30 17:13, Ardelean, Alexandru wrote:
> > > On Wed, 2019-10-30 at 16:19 +0800, zhong jiang wrote:
> > > > [External]
> > > >
> > > > It is more clear to use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs
> > > > file
> > > > operation rather than DEFINE_SIMPLE_ATTRIBUTE.
> > > Not sure if "more clear" is the word.
> > Should be more clearly. :-)
> > > But it is more correct to use DEFINE_DEBUGFS_ATTRIBUTE(), since they
> > > are
> > > debugfs attrs.
> > >
> > > In any case, this is no big deal.
> > > So:
> > >
> > > Reviewed-by: Alexandru Ardelean <[email protected]>
> > >
> > > > Signed-off-by: zhong jiang <[email protected]>
> I started looking into why this attributes were introduced.
> There are potential issues, so Alex can you confirm you've tested this
> series. Whilst it looks right, it seems some other patches making this
> change have had to switch over to the unsafe registration functions.
I haven't tested this locally yet.
Will try to alloc some time for this.
>
> https://patchwork.kernel.org/patch/11051725/
> https://lkml.org/lkml/2019/10/30/144
>
> The reference counting is subtly different between the two versions.
> Seems you are getting some push back on similar patches.
>
> Perhaps a v2 with reference to the other threads if those get resolved
> to say it is sensible to make this change.
>
> Thanks,
>
> Jonathan
>
>
> > > > ---
> > > > drivers/iio/imu/adis16460.c | 6 +++---
> > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/imu/adis16460.c
> > > > b/drivers/iio/imu/adis16460.c
> > > > index 6aed9e8..2e7a582 100644
> > > > --- a/drivers/iio/imu/adis16460.c
> > > > +++ b/drivers/iio/imu/adis16460.c
> > > > @@ -87,7 +87,7 @@ static int adis16460_show_serial_number(void
> > > > *arg, u64
> > > > *val)
> > > >
> > > > return 0;
> > > > }
> > > > -DEFINE_SIMPLE_ATTRIBUTE(adis16460_serial_number_fops,
> > > > +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_serial_number_fops,
> > > > adis16460_show_serial_number, NULL, "0x%.4llx\n");
> > > >
> > > > static int adis16460_show_product_id(void *arg, u64 *val)
> > > > @@ -105,7 +105,7 @@ static int adis16460_show_product_id(void *arg,
> > > > u64
> > > > *val)
> > > >
> > > > return 0;
> > > > }
> > > > -DEFINE_SIMPLE_ATTRIBUTE(adis16460_product_id_fops,
> > > > +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_product_id_fops,
> > > > adis16460_show_product_id, NULL, "%llu\n");
> > > >
> > > > static int adis16460_show_flash_count(void *arg, u64 *val)
> > > > @@ -123,7 +123,7 @@ static int adis16460_show_flash_count(void
> > > > *arg, u64
> > > > *val)
> > > >
> > > > return 0;
> > > > }
> > > > -DEFINE_SIMPLE_ATTRIBUTE(adis16460_flash_count_fops,
> > > > +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_flash_count_fops,
> > > > adis16460_show_flash_count, NULL, "%lld\n");
> > > >
> > > > static int adis16460_debugfs_init(struct iio_dev *indio_dev)