* [PATCH v2 0/2] Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string. @ 2017-09-13 8:55 ` Himanshi Jain 0 siblings, 0 replies; 23+ messages in thread From: Himanshi Jain @ 2017-09-13 8:42 UTC (permalink / raw) To: outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, nick.desaulniers, daniel.baluta This patchset is to rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string. Himanshi Jain (2): include: linux: sysfs: Add __ATTR_NAMED macro iio: Change to __ATTR_NAMED() drivers/iio/adc/ad7793.c | 2 +- drivers/staging/iio/adc/ad7192.c | 2 +- drivers/staging/iio/adc/ad7280a.c | 4 ++-- include/linux/iio/sysfs.h | 6 +++++- include/linux/sysfs.h | 7 +++++++ 5 files changed, 16 insertions(+), 5 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 0/2] Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string. @ 2017-09-13 8:55 ` Himanshi Jain 0 siblings, 0 replies; 23+ messages in thread From: Himanshi Jain @ 2017-09-13 8:55 UTC (permalink / raw) To: outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, nick.desaulniers, daniel.baluta This patchset is to rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string. Change in v2: Split patch into two based on subsytems affected. Himanshi Jain (2): include: linux: sysfs: Add __ATTR_NAMED macro iio: Change to __ATTR_NAMED() drivers/iio/adc/ad7793.c | 2 +- drivers/staging/iio/adc/ad7192.c | 2 +- drivers/staging/iio/adc/ad7280a.c | 4 ++-- include/linux/iio/sysfs.h | 6 +++++- include/linux/sysfs.h | 7 +++++++ 5 files changed, 16 insertions(+), 5 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-13 8:55 ` Himanshi Jain (?) @ 2017-09-13 8:44 ` Himanshi Jain 2017-09-13 8:44 ` [PATCH v2 2/2] iio: Change to __ATTR_NAMED() Himanshi Jain 2017-09-13 17:03 ` [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro Jonathan Cameron -1 siblings, 2 replies; 23+ messages in thread From: Himanshi Jain @ 2017-09-13 8:44 UTC (permalink / raw) To: outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, nick.desaulniers, daniel.baluta Add __ATTR_NAMED macro similar to __ATTR but taking name as a string instead of implicit conversion of argument to string using the macro _stringify(_name). Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> --- include/linux/sysfs.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index aa02c32..20321cf 100644 --- a/include/linux/sysfs.h +++ b/include/linux/sysfs.h @@ -104,6 +104,13 @@ struct attribute_group { .store = _store, \ } +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ + .attr = {.name = _name, \ + .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \ + .show = _show, \ + .store = _store, \ +} + #define __ATTR_PREALLOC(_name, _mode, _show, _store) { \ .attr = {.name = __stringify(_name), \ .mode = SYSFS_PREALLOC | VERIFY_OCTAL_PERMISSIONS(_mode) },\ -- 1.9.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 2/2] iio: Change to __ATTR_NAMED() 2017-09-13 8:44 ` [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro Himanshi Jain @ 2017-09-13 8:44 ` Himanshi Jain 2017-09-13 17:03 ` [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro Jonathan Cameron 1 sibling, 0 replies; 23+ messages in thread From: Himanshi Jain @ 2017-09-13 8:44 UTC (permalink / raw) To: outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, nick.desaulniers, daniel.baluta Add IIO_ATTR_NAMED macro to use __ATTR_NAMED to allow passing name as string to IIO_DEVICE_ATTR_NAMED. Change current usage of IIO_DEVICE_ATTR_NAMED to pass name as string. Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> --- drivers/iio/adc/ad7793.c | 2 +- drivers/staging/iio/adc/ad7192.c | 2 +- drivers/staging/iio/adc/ad7280a.c | 4 ++-- include/linux/iio/sysfs.h | 6 +++++- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c index e6706a0..d74e324 100644 --- a/drivers/iio/adc/ad7793.c +++ b/drivers/iio/adc/ad7793.c @@ -420,7 +420,7 @@ static ssize_t ad7793_show_scale_available(struct device *dev, } static IIO_DEVICE_ATTR_NAMED(in_m_in_scale_available, - in_voltage-voltage_scale_available, S_IRUGO, + "in_voltage-voltage_scale_available", S_IRUGO, ad7793_show_scale_available, NULL, 0); static struct attribute *ad7793_attributes[] = { diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c index d11c6de..daff38c 100644 --- a/drivers/staging/iio/adc/ad7192.c +++ b/drivers/staging/iio/adc/ad7192.c @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st, } static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available, - in_voltage-voltage_scale_available, + "in_voltage-voltage_scale_available", 0444, ad7192_show_scale_available, NULL, 0); static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444, diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c index f85dde9..fd32e9a 100644 --- a/drivers/staging/iio/adc/ad7280a.c +++ b/drivers/staging/iio/adc/ad7280a.c @@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void *private) } static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value, - in_voltage-voltage_thresh_low_value, + "in_voltage-voltage_thresh_low_value", 0644, ad7280_read_channel_config, ad7280_write_channel_config, AD7280A_CELL_UNDERVOLTAGE); static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value, - in_voltage-voltage_thresh_high_value, + "in_voltage-voltage_thresh_high_value", 0644, ad7280_read_channel_config, ad7280_write_channel_config, diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h index ce9426c..49c81a4 100644 --- a/include/linux/iio/sysfs.h +++ b/include/linux/iio/sysfs.h @@ -55,6 +55,10 @@ struct iio_const_attr { { .dev_attr = __ATTR(_name, _mode, _show, _store), \ .address = _addr } +#define IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr) \ + { .dev_attr = __ATTR_NAMED(_name, _mode, _show, _store), \ + .address = _addr } + #define IIO_ATTR_RO(_name, _addr) \ { .dev_attr = __ATTR_RO(_name), \ .address = _addr } @@ -85,7 +89,7 @@ struct iio_const_attr { #define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \ struct iio_dev_attr iio_dev_attr_##_vname \ - = IIO_ATTR(_name, _mode, _show, _store, _addr) + = IIO_ATTR_NAMED(_name, _mode, _show, _store, _addr) #define IIO_CONST_ATTR(_name, _string) \ struct iio_const_attr iio_const_attr_##_name \ -- 1.9.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-13 8:44 ` [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro Himanshi Jain 2017-09-13 8:44 ` [PATCH v2 2/2] iio: Change to __ATTR_NAMED() Himanshi Jain @ 2017-09-13 17:03 ` Jonathan Cameron 2017-09-13 18:58 ` Greg KH 1 sibling, 1 reply; 23+ messages in thread From: Jonathan Cameron @ 2017-09-13 17:03 UTC (permalink / raw) To: Himanshi Jain Cc: outreachy-kernel, lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, nick.desaulniers, daniel.baluta On Wed, 13 Sep 2017 14:14:07 +0530 Himanshi Jain <himshijain.hj@gmail.com> wrote: > Add __ATTR_NAMED macro similar to __ATTR but taking name as a > string instead of implicit conversion of argument to string using > the macro _stringify(_name). > > Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> > --- > include/linux/sysfs.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > index aa02c32..20321cf 100644 > --- a/include/linux/sysfs.h > +++ b/include/linux/sysfs.h > @@ -104,6 +104,13 @@ struct attribute_group { > .store = _store, \ > } > > +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ I'm not sure about the naming here. The normal __ATTR macro is also 'named'. Maybe something as awful as __ATTR_STRING_NAME ? Greg what do you think? This is all to allow us to have names with operators in them without checkpatch complaining about them... A worthwhile aim just to stop more people wasting time trying to 'fix' those cases by adding spaces. Jonathan > + .attr = {.name = _name, \ > + .mode = VERIFY_OCTAL_PERMISSIONS(_mode) }, \ > + .show = _show, \ > + .store = _store, \ > +} > + > #define __ATTR_PREALLOC(_name, _mode, _show, _store) { \ > .attr = {.name = __stringify(_name), \ > .mode = SYSFS_PREALLOC | VERIFY_OCTAL_PERMISSIONS(_mode) },\ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-13 17:03 ` [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro Jonathan Cameron @ 2017-09-13 18:58 ` Greg KH 2017-09-13 19:23 ` Lars-Peter Clausen 0 siblings, 1 reply; 23+ messages in thread From: Greg KH @ 2017-09-13 18:58 UTC (permalink / raw) To: Jonathan Cameron Cc: Himanshi Jain, devel, daniel.baluta, lars, Michael.Hennerich, linux-iio, linux-kernel, nick.desaulniers, outreachy-kernel, pmeerw, knaack.h On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote: > On Wed, 13 Sep 2017 14:14:07 +0530 > Himanshi Jain <himshijain.hj@gmail.com> wrote: > > > Add __ATTR_NAMED macro similar to __ATTR but taking name as a > > string instead of implicit conversion of argument to string using > > the macro _stringify(_name). > > > > Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> > > --- > > include/linux/sysfs.h | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > > index aa02c32..20321cf 100644 > > --- a/include/linux/sysfs.h > > +++ b/include/linux/sysfs.h > > @@ -104,6 +104,13 @@ struct attribute_group { > > .store = _store, \ > > } > > > > +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ > > I'm not sure about the naming here. The normal __ATTR macro is also > 'named'. Maybe something as awful as > > __ATTR_STRING_NAME ? > > Greg what do you think? ick ick ick. > This is all to allow us to have names with operators in them without > checkpatch complaining about them... A worthwhile aim just to stop > more people wasting time trying to 'fix' those cases by adding spaces. Yeah, but this really seems "heavy" for just a crazy sysfs name in a macro. Adding a whole new "core" define for that is a hard sell... I also want to get rid of the "generic" __ATTR type macros, and force people to use the proper _RW and friends instead. I don't want to add another new one that people will start to use that I later have to change... So no, I don't like this, how about just changing your macros instead? No one else has this problem :) thanks, greg k-h ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-13 18:58 ` Greg KH @ 2017-09-13 19:23 ` Lars-Peter Clausen 2017-09-13 20:50 ` Jonathan Cameron 2017-09-13 21:29 ` Greg KH 0 siblings, 2 replies; 23+ messages in thread From: Lars-Peter Clausen @ 2017-09-13 19:23 UTC (permalink / raw) To: Greg KH, Jonathan Cameron Cc: Himanshi Jain, devel, daniel.baluta, Michael.Hennerich, linux-iio, linux-kernel, nick.desaulniers, outreachy-kernel, pmeerw, knaack.h On 09/13/2017 08:58 PM, Greg KH wrote: > On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote: >> On Wed, 13 Sep 2017 14:14:07 +0530 >> Himanshi Jain <himshijain.hj@gmail.com> wrote: >> >>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a >>> string instead of implicit conversion of argument to string using >>> the macro _stringify(_name). >>> >>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> >>> --- >>> include/linux/sysfs.h | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h >>> index aa02c32..20321cf 100644 >>> --- a/include/linux/sysfs.h >>> +++ b/include/linux/sysfs.h >>> @@ -104,6 +104,13 @@ struct attribute_group { >>> .store = _store, \ >>> } >>> >>> +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ >> >> I'm not sure about the naming here. The normal __ATTR macro is also >> 'named'. Maybe something as awful as >> >> __ATTR_STRING_NAME ? >> >> Greg what do you think? > > ick ick ick. > >> This is all to allow us to have names with operators in them without >> checkpatch complaining about them... A worthwhile aim just to stop >> more people wasting time trying to 'fix' those cases by adding spaces. > > Yeah, but this really seems "heavy" for just a crazy sysfs name in a > macro. Adding a whole new "core" define for that is a hard sell... > > I also want to get rid of the "generic" __ATTR type macros, and force > people to use the proper _RW and friends instead. I don't want to add > another new one that people will start to use that I later have to > change... > > So no, I don't like this, how about just changing your macros instead? > No one else has this problem :) Nobody else realized they have this problem yet. E.g. there are a few users of __ATTR in block/genhd.c that have the same issue and are likely to generate the same false positives from static checkers. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-13 19:23 ` Lars-Peter Clausen @ 2017-09-13 20:50 ` Jonathan Cameron 2017-09-13 21:29 ` Greg KH 1 sibling, 0 replies; 23+ messages in thread From: Jonathan Cameron @ 2017-09-13 20:50 UTC (permalink / raw) To: Lars-Peter Clausen, Greg KH, Jonathan Cameron Cc: Himanshi Jain, devel, daniel.baluta, Michael.Hennerich, linux-iio, linux-kernel, nick.desaulniers, outreachy-kernel, pmeerw, knaack.h On 13 September 2017 12:23:31 GMT-07:00, Lars-Peter Clausen <lars@metafoo.de> wrote: >On 09/13/2017 08:58 PM, Greg KH wrote: >> On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote: >>> On Wed, 13 Sep 2017 14:14:07 +0530 >>> Himanshi Jain <himshijain.hj@gmail.com> wrote: >>> >>>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a >>>> string instead of implicit conversion of argument to string using >>>> the macro _stringify(_name). >>>> >>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> >>>> --- >>>> include/linux/sysfs.h | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h >>>> index aa02c32..20321cf 100644 >>>> --- a/include/linux/sysfs.h >>>> +++ b/include/linux/sysfs.h >>>> @@ -104,6 +104,13 @@ struct attribute_group { >>>> .store = _store, \ >>>> } >>>> >>>> +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ >>> >>> I'm not sure about the naming here. The normal __ATTR macro is also >>> 'named'. Maybe something as awful as >>> >>> __ATTR_STRING_NAME ? >>> >>> Greg what do you think? >> >> ick ick ick. >> >>> This is all to allow us to have names with operators in them without >>> checkpatch complaining about them... A worthwhile aim just to stop >>> more people wasting time trying to 'fix' those cases by adding >spaces. >> >> Yeah, but this really seems "heavy" for just a crazy sysfs name in a >> macro. Adding a whole new "core" define for that is a hard sell... >> >> I also want to get rid of the "generic" __ATTR type macros, and force >> people to use the proper _RW and friends instead. I don't want to >add >> another new one that people will start to use that I later have to >> change... >> >> So no, I don't like this, how about just changing your macros >instead? >> No one else has this problem :) > >Nobody else realized they have this problem yet. E.g. there are a few >users >of __ATTR in block/genhd.c that have the same issue and are likely to >generate the same false positives from static checkers. For IIO there is the option of moving these over to the core generated available callbacks, but that won't work in every case and is a more major change. I need to shift a few more drivers over to the available callbacks and see how well it works out. Might find time to do one in a gap between interesting talks this afternoon... If I am feeling really keen I might write this missing docs I promised a while back on that stuff. Jet lag dependant... Jonathan > >-- >To unsubscribe from this list: send the line "unsubscribe linux-iio" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro @ 2017-09-13 20:50 ` Jonathan Cameron 0 siblings, 0 replies; 23+ messages in thread From: Jonathan Cameron @ 2017-09-13 20:50 UTC (permalink / raw) To: Lars-Peter Clausen, Greg KH, Jonathan Cameron Cc: Himanshi Jain, devel, daniel.baluta, Michael.Hennerich, linux-iio, linux-kernel, nick.desaulniers, outreachy-kernel, pmeerw, knaack.h On 13 September 2017 12:23:31 GMT-07:00, Lars-Peter Clausen <lars@metafoo.de> wrote: >On 09/13/2017 08:58 PM, Greg KH wrote: >> On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote: >>> On Wed, 13 Sep 2017 14:14:07 +0530 >>> Himanshi Jain <himshijain.hj@gmail.com> wrote: >>> >>>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a >>>> string instead of implicit conversion of argument to string using >>>> the macro _stringify(_name). >>>> >>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> >>>> --- >>>> include/linux/sysfs.h | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h >>>> index aa02c32..20321cf 100644 >>>> --- a/include/linux/sysfs.h >>>> +++ b/include/linux/sysfs.h >>>> @@ -104,6 +104,13 @@ struct attribute_group { >>>> .store = _store, \ >>>> } >>>> >>>> +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ >>> >>> I'm not sure about the naming here. The normal __ATTR macro is also >>> 'named'. Maybe something as awful as >>> >>> __ATTR_STRING_NAME ? >>> >>> Greg what do you think? >> >> ick ick ick. >> >>> This is all to allow us to have names with operators in them without >>> checkpatch complaining about them... A worthwhile aim just to stop >>> more people wasting time trying to 'fix' those cases by adding >spaces. >> >> Yeah, but this really seems "heavy" for just a crazy sysfs name in a >> macro. Adding a whole new "core" define for that is a hard sell... >> >> I also want to get rid of the "generic" __ATTR type macros, and force >> people to use the proper _RW and friends instead. I don't want to >add >> another new one that people will start to use that I later have to >> change... >> >> So no, I don't like this, how about just changing your macros >instead? >> No one else has this problem :) > >Nobody else realized they have this problem yet. E.g. there are a few >users >of __ATTR in block/genhd.c that have the same issue and are likely to >generate the same false positives from static checkers. For IIO there is the option of moving these over to the core generated available callbacks, but that won't work in every case and is a more major change. I need to shift a few more drivers over to the available callbacks and see how well it works out. Might find time to do one in a gap between interesting talks this afternoon... If I am feeling really keen I might write this missing docs I promised a while back on that stuff. Jet lag dependant... Jonathan > >-- >To unsubscribe from this list: send the line "unsubscribe linux-iio" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-13 20:50 ` Jonathan Cameron (?) @ 2017-09-18 10:49 ` Himanshi Jain 2017-09-21 13:54 ` Jonathan Cameron -1 siblings, 1 reply; 23+ messages in thread From: Himanshi Jain @ 2017-09-18 10:49 UTC (permalink / raw) To: Jonathan Cameron Cc: Lars-Peter Clausen, Greg KH, Jonathan Cameron, driverdev, Daniel Baluta, Hennerich, Michael, linux-iio, Linux Kernel Mailing List, Nick Desaulniers, outreachy-kernel, Peter Meerwald, Hartmut Knaack On Thu, Sep 14, 2017 at 2:20 AM, Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote: > > > On 13 September 2017 12:23:31 GMT-07:00, Lars-Peter Clausen <lars@metafoo.de> wrote: >>On 09/13/2017 08:58 PM, Greg KH wrote: >>> On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote: >>>> On Wed, 13 Sep 2017 14:14:07 +0530 >>>> Himanshi Jain <himshijain.hj@gmail.com> wrote: >>>> >>>>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a >>>>> string instead of implicit conversion of argument to string using >>>>> the macro _stringify(_name). >>>>> >>>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> >>>>> --- >>>>> include/linux/sysfs.h | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h >>>>> index aa02c32..20321cf 100644 >>>>> --- a/include/linux/sysfs.h >>>>> +++ b/include/linux/sysfs.h >>>>> @@ -104,6 +104,13 @@ struct attribute_group { >>>>> .store = _store, \ >>>>> } >>>>> >>>>> +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ >>>> >>>> I'm not sure about the naming here. The normal __ATTR macro is also >>>> 'named'. Maybe something as awful as >>>> >>>> __ATTR_STRING_NAME ? >>>> >>>> Greg what do you think? >>> >>> ick ick ick. >>> >>>> This is all to allow us to have names with operators in them without >>>> checkpatch complaining about them... A worthwhile aim just to stop >>>> more people wasting time trying to 'fix' those cases by adding >>spaces. >>> >>> Yeah, but this really seems "heavy" for just a crazy sysfs name in a >>> macro. Adding a whole new "core" define for that is a hard sell... >>> >>> I also want to get rid of the "generic" __ATTR type macros, and force >>> people to use the proper _RW and friends instead. I don't want to >>add >>> another new one that people will start to use that I later have to >>> change... >>> >>> So no, I don't like this, how about just changing your macros >>instead? >>> No one else has this problem :) >> >>Nobody else realized they have this problem yet. E.g. there are a few >>users >>of __ATTR in block/genhd.c that have the same issue and are likely to >>generate the same false positives from static checkers. > > For IIO there is the option of moving these over to the core generated available callbacks, but > that won't work in every case and is a more major change. I need to shift a few more drivers > over to the available callbacks and see how well it works out. Might find time to do one in a > gap between interesting talks this afternoon... Can I help you in this? It is about exploring options as far as I can make out, although can't really understand what options are those for now. Or do you want me to put comments to not to fix this checkpatch warning as you suggested earlier? > > If I am feeling really keen I might write this missing docs I promised a while back on that stuff. Jet lag dependant... > > Jonathan >> >>-- >>To unsubscribe from this list: send the line "unsubscribe linux-iio" in >>the body of a message to majordomo@vger.kernel.org >>More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-18 10:49 ` Himanshi Jain @ 2017-09-21 13:54 ` Jonathan Cameron 2017-09-21 14:07 ` Julia Lawall 0 siblings, 1 reply; 23+ messages in thread From: Jonathan Cameron @ 2017-09-21 13:54 UTC (permalink / raw) To: Himanshi Jain, Peter Meerwald Cc: Jonathan Cameron, Lars-Peter Clausen, Greg KH, Jonathan Cameron, driverdev, Daniel Baluta, Hennerich, Michael, linux-iio, Linux Kernel Mailing List, Nick Desaulniers, outreachy-kernel, Hartmut Knaack On Mon, 18 Sep 2017 16:19:07 +0530 Himanshi Jain <himshijain.hj@gmail.com> wrote: > On Thu, Sep 14, 2017 at 2:20 AM, Jonathan Cameron > <jic23@jic23.retrosnub.co.uk> wrote: > > > > > > On 13 September 2017 12:23:31 GMT-07:00, Lars-Peter Clausen <lars@metafoo.de> wrote: > >>On 09/13/2017 08:58 PM, Greg KH wrote: > >>> On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote: > >>>> On Wed, 13 Sep 2017 14:14:07 +0530 > >>>> Himanshi Jain <himshijain.hj@gmail.com> wrote: > >>>> > >>>>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a > >>>>> string instead of implicit conversion of argument to string using > >>>>> the macro _stringify(_name). > >>>>> > >>>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> > >>>>> --- > >>>>> include/linux/sysfs.h | 7 +++++++ > >>>>> 1 file changed, 7 insertions(+) > >>>>> > >>>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > >>>>> index aa02c32..20321cf 100644 > >>>>> --- a/include/linux/sysfs.h > >>>>> +++ b/include/linux/sysfs.h > >>>>> @@ -104,6 +104,13 @@ struct attribute_group { > >>>>> .store = _store, \ > >>>>> } > >>>>> > >>>>> +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ > >>>> > >>>> I'm not sure about the naming here. The normal __ATTR macro is also > >>>> 'named'. Maybe something as awful as > >>>> > >>>> __ATTR_STRING_NAME ? > >>>> > >>>> Greg what do you think? > >>> > >>> ick ick ick. > >>> > >>>> This is all to allow us to have names with operators in them without > >>>> checkpatch complaining about them... A worthwhile aim just to stop > >>>> more people wasting time trying to 'fix' those cases by adding > >>spaces. > >>> > >>> Yeah, but this really seems "heavy" for just a crazy sysfs name in a > >>> macro. Adding a whole new "core" define for that is a hard sell... > >>> > >>> I also want to get rid of the "generic" __ATTR type macros, and force > >>> people to use the proper _RW and friends instead. I don't want to > >>add > >>> another new one that people will start to use that I later have to > >>> change... > >>> > >>> So no, I don't like this, how about just changing your macros > >>instead? > >>> No one else has this problem :) > >> > >>Nobody else realized they have this problem yet. E.g. there are a few > >>users > >>of __ATTR in block/genhd.c that have the same issue and are likely to > >>generate the same false positives from static checkers. > > > > For IIO there is the option of moving these over to the core generated available callbacks, but > > that won't work in every case and is a more major change. I need to shift a few more drivers > > over to the available callbacks and see how well it works out. Might find time to do one in a > > gap between interesting talks this afternoon... > > Can I help you in this? It is about exploring options as far as I can > make out, although can't really understand what options are those for > now. You are welcome to have a go. However the relevant code isn't all that well tested so I'd want to actually get enough of the setup to run to see if it works (which is trickier). I doubt anyone has ready access to this hardware. Anyhow, to see what I mean see : https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/drivers/iio/dac/dpot-dac.c In particular dpot_dac_read_avail() and static const struct iio_chan_spec dpot_dac_iio_channel = { .type = IIO_VOLTAGE, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), .info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW), .output = 1, .indexed = 1, }; What this does is allow the core to create *_available attributes (sometimes this is required by in kernel consumers of the channels as they need to know the range of the channel for example). The intent ultimately is to move drivers over to this interface and hopefully get rid of the majority of remaining hand specified attrs. It's always been well down the todo list though as it is easy to get wrong in a given driver and we need to get more emulation in place to allow testing of the various drivers. There are a couple of ways of doing such emulation (i2c devices can use the i2c-stub framework, or we can do full blown qemu emulation) - both are interesting diversions. Also I promised to write the ABI docs (see the original patches introducing it for some docs) but haven't actually done so yet which makes it tricky to tell people to start using this stuff even in new drivers! Anyhow, whilst you are welcome to look at this it might be a high risk choice for where to get started during the outreachy applications period. Still nothing wrong with being brave, but perhaps discuss with Alison, Daniel or Julia. Thanks, Jonathan > > Or do you want me to put comments to not to fix this checkpatch > warning as you suggested earlier? > > > > > If I am feeling really keen I might write this missing docs I promised a while back on that stuff. Jet lag dependant... > > > > Jonathan > >> > >>-- > >>To unsubscribe from this list: send the line "unsubscribe linux-iio" in > >>the body of a message to majordomo@vger.kernel.org > >>More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > > Sent from my Android device with K-9 Mail. Please excuse my brevity. > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-21 13:54 ` Jonathan Cameron @ 2017-09-21 14:07 ` Julia Lawall 0 siblings, 0 replies; 23+ messages in thread From: Julia Lawall @ 2017-09-21 14:07 UTC (permalink / raw) To: Jonathan Cameron Cc: Himanshi Jain, Peter Meerwald, Jonathan Cameron, Lars-Peter Clausen, Greg KH, Jonathan Cameron, driverdev, Daniel Baluta, Hennerich, Michael, linux-iio, Linux Kernel Mailing List, Nick Desaulniers, outreachy-kernel, Hartmut Knaack On Thu, 21 Sep 2017, Jonathan Cameron wrote: > On Mon, 18 Sep 2017 16:19:07 +0530 > Himanshi Jain <himshijain.hj@gmail.com> wrote: > > > On Thu, Sep 14, 2017 at 2:20 AM, Jonathan Cameron > > <jic23@jic23.retrosnub.co.uk> wrote: > > > > > > > > > On 13 September 2017 12:23:31 GMT-07:00, Lars-Peter Clausen <lars@metafoo.de> wrote: > > >>On 09/13/2017 08:58 PM, Greg KH wrote: > > >>> On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote: > > >>>> On Wed, 13 Sep 2017 14:14:07 +0530 > > >>>> Himanshi Jain <himshijain.hj@gmail.com> wrote: > > >>>> > > >>>>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a > > >>>>> string instead of implicit conversion of argument to string using > > >>>>> the macro _stringify(_name). > > >>>>> > > >>>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> > > >>>>> --- > > >>>>> include/linux/sysfs.h | 7 +++++++ > > >>>>> 1 file changed, 7 insertions(+) > > >>>>> > > >>>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > > >>>>> index aa02c32..20321cf 100644 > > >>>>> --- a/include/linux/sysfs.h > > >>>>> +++ b/include/linux/sysfs.h > > >>>>> @@ -104,6 +104,13 @@ struct attribute_group { > > >>>>> .store = _store, \ > > >>>>> } > > >>>>> > > >>>>> +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ > > >>>> > > >>>> I'm not sure about the naming here. The normal __ATTR macro is also > > >>>> 'named'. Maybe something as awful as > > >>>> > > >>>> __ATTR_STRING_NAME ? > > >>>> > > >>>> Greg what do you think? > > >>> > > >>> ick ick ick. > > >>> > > >>>> This is all to allow us to have names with operators in them without > > >>>> checkpatch complaining about them... A worthwhile aim just to stop > > >>>> more people wasting time trying to 'fix' those cases by adding > > >>spaces. > > >>> > > >>> Yeah, but this really seems "heavy" for just a crazy sysfs name in a > > >>> macro. Adding a whole new "core" define for that is a hard sell... > > >>> > > >>> I also want to get rid of the "generic" __ATTR type macros, and force > > >>> people to use the proper _RW and friends instead. I don't want to > > >>add > > >>> another new one that people will start to use that I later have to > > >>> change... > > >>> > > >>> So no, I don't like this, how about just changing your macros > > >>instead? > > >>> No one else has this problem :) > > >> > > >>Nobody else realized they have this problem yet. E.g. there are a few > > >>users > > >>of __ATTR in block/genhd.c that have the same issue and are likely to > > >>generate the same false positives from static checkers. > > > > > > For IIO there is the option of moving these over to the core generated available callbacks, but > > > that won't work in every case and is a more major change. I need to shift a few more drivers > > > over to the available callbacks and see how well it works out. Might find time to do one in a > > > gap between interesting talks this afternoon... > > > > Can I help you in this? It is about exploring options as far as I can > > make out, although can't really understand what options are those for > > now. > > You are welcome to have a go. However the relevant code isn't all that > well tested so I'd want to actually get enough of the setup to run to see > if it works (which is trickier). I doubt anyone has ready access > to this hardware. > > Anyhow, to see what I mean see : > > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/drivers/iio/dac/dpot-dac.c > > In particular dpot_dac_read_avail() and > > static const struct iio_chan_spec dpot_dac_iio_channel = { > .type = IIO_VOLTAGE, > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) > | BIT(IIO_CHAN_INFO_SCALE), > .info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW), > .output = 1, > .indexed = 1, > }; > > What this does is allow the core to create *_available attributes > (sometimes this is required by in kernel consumers of the channels as they > need to know the range of the channel for example). > > The intent ultimately is to move drivers over to this interface and hopefully > get rid of the majority of remaining hand specified attrs. > > It's always been well down the todo list though as it is easy to get wrong > in a given driver and we need to get more emulation in place to allow > testing of the various drivers. There are a couple of ways of doing such > emulation (i2c devices can use the i2c-stub framework, or we can do full blown > qemu emulation) - both are interesting diversions. > > Also I promised to write the ABI docs (see the original patches introducing > it for some docs) but haven't actually done so yet which makes it tricky > to tell people to start using this stuff even in new drivers! > > Anyhow, whilst you are welcome to look at this it might be a high risk choice > for where to get started during the outreachy applications period. > > Still nothing wrong with being brave, but perhaps discuss with Alison, Daniel > or Julia. Himanshi, >From a practical point of view, being brave would definitely be taken into account. But be sure that you have some accepted patches during the application period as well. If you spend all your time on this and it doesn't work out, that would be too bad. julia > > Thanks, > > Jonathan > > > > > Or do you want me to put comments to not to fix this checkpatch > > warning as you suggested earlier? > > > > > > > > If I am feeling really keen I might write this missing docs I promised a while back on that stuff. Jet lag dependant... > > > > > > Jonathan > > >> > > >>-- > > >>To unsubscribe from this list: send the line "unsubscribe linux-iio" in > > >>the body of a message to majordomo@vger.kernel.org > > >>More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > -- > > > Sent from my Android device with K-9 Mail. Please excuse my brevity. > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170921145442.0000160b%40huawei.com. > For more options, visit https://groups.google.com/d/optout. > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro @ 2017-09-21 14:07 ` Julia Lawall 0 siblings, 0 replies; 23+ messages in thread From: Julia Lawall @ 2017-09-21 14:07 UTC (permalink / raw) To: Jonathan Cameron Cc: Himanshi Jain, Peter Meerwald, Jonathan Cameron, Lars-Peter Clausen, Greg KH, Jonathan Cameron, driverdev, Daniel Baluta, Hennerich, Michael, linux-iio, Linux Kernel Mailing List, Nick Desaulniers, outreachy-kernel, Hartmut Knaack On Thu, 21 Sep 2017, Jonathan Cameron wrote: > On Mon, 18 Sep 2017 16:19:07 +0530 > Himanshi Jain <himshijain.hj@gmail.com> wrote: > > > On Thu, Sep 14, 2017 at 2:20 AM, Jonathan Cameron > > <jic23@jic23.retrosnub.co.uk> wrote: > > > > > > > > > On 13 September 2017 12:23:31 GMT-07:00, Lars-Peter Clausen <lars@metafoo.de> wrote: > > >>On 09/13/2017 08:58 PM, Greg KH wrote: > > >>> On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote: > > >>>> On Wed, 13 Sep 2017 14:14:07 +0530 > > >>>> Himanshi Jain <himshijain.hj@gmail.com> wrote: > > >>>> > > >>>>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a > > >>>>> string instead of implicit conversion of argument to string using > > >>>>> the macro _stringify(_name). > > >>>>> > > >>>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> > > >>>>> --- > > >>>>> include/linux/sysfs.h | 7 +++++++ > > >>>>> 1 file changed, 7 insertions(+) > > >>>>> > > >>>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > > >>>>> index aa02c32..20321cf 100644 > > >>>>> --- a/include/linux/sysfs.h > > >>>>> +++ b/include/linux/sysfs.h > > >>>>> @@ -104,6 +104,13 @@ struct attribute_group { > > >>>>> .store = _store, \ > > >>>>> } > > >>>>> > > >>>>> +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ > > >>>> > > >>>> I'm not sure about the naming here. The normal __ATTR macro is also > > >>>> 'named'. Maybe something as awful as > > >>>> > > >>>> __ATTR_STRING_NAME ? > > >>>> > > >>>> Greg what do you think? > > >>> > > >>> ick ick ick. > > >>> > > >>>> This is all to allow us to have names with operators in them without > > >>>> checkpatch complaining about them... A worthwhile aim just to stop > > >>>> more people wasting time trying to 'fix' those cases by adding > > >>spaces. > > >>> > > >>> Yeah, but this really seems "heavy" for just a crazy sysfs name in a > > >>> macro. Adding a whole new "core" define for that is a hard sell... > > >>> > > >>> I also want to get rid of the "generic" __ATTR type macros, and force > > >>> people to use the proper _RW and friends instead. I don't want to > > >>add > > >>> another new one that people will start to use that I later have to > > >>> change... > > >>> > > >>> So no, I don't like this, how about just changing your macros > > >>instead? > > >>> No one else has this problem :) > > >> > > >>Nobody else realized they have this problem yet. E.g. there are a few > > >>users > > >>of __ATTR in block/genhd.c that have the same issue and are likely to > > >>generate the same false positives from static checkers. > > > > > > For IIO there is the option of moving these over to the core generated available callbacks, but > > > that won't work in every case and is a more major change. I need to shift a few more drivers > > > over to the available callbacks and see how well it works out. Might find time to do one in a > > > gap between interesting talks this afternoon... > > > > Can I help you in this? It is about exploring options as far as I can > > make out, although can't really understand what options are those for > > now. > > You are welcome to have a go. However the relevant code isn't all that > well tested so I'd want to actually get enough of the setup to run to see > if it works (which is trickier). I doubt anyone has ready access > to this hardware. > > Anyhow, to see what I mean see : > > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/drivers/iio/dac/dpot-dac.c > > In particular dpot_dac_read_avail() and > > static const struct iio_chan_spec dpot_dac_iio_channel = { > .type = IIO_VOLTAGE, > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) > | BIT(IIO_CHAN_INFO_SCALE), > .info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW), > .output = 1, > .indexed = 1, > }; > > What this does is allow the core to create *_available attributes > (sometimes this is required by in kernel consumers of the channels as they > need to know the range of the channel for example). > > The intent ultimately is to move drivers over to this interface and hopefully > get rid of the majority of remaining hand specified attrs. > > It's always been well down the todo list though as it is easy to get wrong > in a given driver and we need to get more emulation in place to allow > testing of the various drivers. There are a couple of ways of doing such > emulation (i2c devices can use the i2c-stub framework, or we can do full blown > qemu emulation) - both are interesting diversions. > > Also I promised to write the ABI docs (see the original patches introducing > it for some docs) but haven't actually done so yet which makes it tricky > to tell people to start using this stuff even in new drivers! > > Anyhow, whilst you are welcome to look at this it might be a high risk choice > for where to get started during the outreachy applications period. > > Still nothing wrong with being brave, but perhaps discuss with Alison, Daniel > or Julia. Himanshi, From a practical point of view, being brave would definitely be taken into account. But be sure that you have some accepted patches during the application period as well. If you spend all your time on this and it doesn't work out, that would be too bad. julia > > Thanks, > > Jonathan > > > > > Or do you want me to put comments to not to fix this checkpatch > > warning as you suggested earlier? > > > > > > > > If I am feeling really keen I might write this missing docs I promised a while back on that stuff. Jet lag dependant... > > > > > > Jonathan > > >> > > >>-- > > >>To unsubscribe from this list: send the line "unsubscribe linux-iio" in > > >>the body of a message to majordomo@vger.kernel.org > > >>More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > -- > > > Sent from my Android device with K-9 Mail. Please excuse my brevity. > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170921145442.0000160b%40huawei.com. > For more options, visit https://groups.google.com/d/optout. > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-13 19:23 ` Lars-Peter Clausen 2017-09-13 20:50 ` Jonathan Cameron @ 2017-09-13 21:29 ` Greg KH 2017-09-13 21:43 ` Dan Carpenter 2017-09-19 12:13 ` Lars-Peter Clausen 1 sibling, 2 replies; 23+ messages in thread From: Greg KH @ 2017-09-13 21:29 UTC (permalink / raw) To: Lars-Peter Clausen Cc: Jonathan Cameron, devel, daniel.baluta, pmeerw, Michael.Hennerich, linux-iio, linux-kernel, nick.desaulniers, outreachy-kernel, Himanshi Jain, knaack.h On Wed, Sep 13, 2017 at 09:23:31PM +0200, Lars-Peter Clausen wrote: > On 09/13/2017 08:58 PM, Greg KH wrote: > > On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote: > >> On Wed, 13 Sep 2017 14:14:07 +0530 > >> Himanshi Jain <himshijain.hj@gmail.com> wrote: > >> > >>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a > >>> string instead of implicit conversion of argument to string using > >>> the macro _stringify(_name). > >>> > >>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> > >>> --- > >>> include/linux/sysfs.h | 7 +++++++ > >>> 1 file changed, 7 insertions(+) > >>> > >>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > >>> index aa02c32..20321cf 100644 > >>> --- a/include/linux/sysfs.h > >>> +++ b/include/linux/sysfs.h > >>> @@ -104,6 +104,13 @@ struct attribute_group { > >>> .store = _store, \ > >>> } > >>> > >>> +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ > >> > >> I'm not sure about the naming here. The normal __ATTR macro is also > >> 'named'. Maybe something as awful as > >> > >> __ATTR_STRING_NAME ? > >> > >> Greg what do you think? > > > > ick ick ick. > > > >> This is all to allow us to have names with operators in them without > >> checkpatch complaining about them... A worthwhile aim just to stop > >> more people wasting time trying to 'fix' those cases by adding spaces. > > > > Yeah, but this really seems "heavy" for just a crazy sysfs name in a > > macro. Adding a whole new "core" define for that is a hard sell... > > > > I also want to get rid of the "generic" __ATTR type macros, and force > > people to use the proper _RW and friends instead. I don't want to add > > another new one that people will start to use that I later have to > > change... > > > > So no, I don't like this, how about just changing your macros instead? > > No one else has this problem :) > > Nobody else realized they have this problem yet. E.g. there are a few users > of __ATTR in block/genhd.c that have the same issue and are likely to > generate the same false positives from static checkers. Then fix the broken static checkers :) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-13 21:29 ` Greg KH @ 2017-09-13 21:43 ` Dan Carpenter 2017-09-13 21:58 ` Joe Perches 2017-09-19 12:13 ` Lars-Peter Clausen 1 sibling, 1 reply; 23+ messages in thread From: Dan Carpenter @ 2017-09-13 21:43 UTC (permalink / raw) To: Greg KH Cc: Lars-Peter Clausen, devel, daniel.baluta, Himanshi Jain, Michael.Hennerich, linux-iio, linux-kernel, nick.desaulniers, outreachy-kernel, pmeerw, knaack.h, Jonathan Cameron On Wed, Sep 13, 2017 at 02:29:52PM -0700, Greg KH wrote: > On Wed, Sep 13, 2017 at 09:23:31PM +0200, Lars-Peter Clausen wrote: > > On 09/13/2017 08:58 PM, Greg KH wrote: > > > On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote: > > >> On Wed, 13 Sep 2017 14:14:07 +0530 > > >> Himanshi Jain <himshijain.hj@gmail.com> wrote: > > >> > > >>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a > > >>> string instead of implicit conversion of argument to string using > > >>> the macro _stringify(_name). > > >>> > > >>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> > > >>> --- > > >>> include/linux/sysfs.h | 7 +++++++ > > >>> 1 file changed, 7 insertions(+) > > >>> > > >>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > > >>> index aa02c32..20321cf 100644 > > >>> --- a/include/linux/sysfs.h > > >>> +++ b/include/linux/sysfs.h > > >>> @@ -104,6 +104,13 @@ struct attribute_group { > > >>> .store = _store, \ > > >>> } > > >>> > > >>> +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ > > >> > > >> I'm not sure about the naming here. The normal __ATTR macro is also > > >> 'named'. Maybe something as awful as > > >> > > >> __ATTR_STRING_NAME ? > > >> > > >> Greg what do you think? > > > > > > ick ick ick. > > > > > >> This is all to allow us to have names with operators in them without > > >> checkpatch complaining about them... A worthwhile aim just to stop > > >> more people wasting time trying to 'fix' those cases by adding spaces. > > > > > > Yeah, but this really seems "heavy" for just a crazy sysfs name in a > > > macro. Adding a whole new "core" define for that is a hard sell... > > > > > > I also want to get rid of the "generic" __ATTR type macros, and force > > > people to use the proper _RW and friends instead. I don't want to add > > > another new one that people will start to use that I later have to > > > change... > > > > > > So no, I don't like this, how about just changing your macros instead? > > > No one else has this problem :) > > > > Nobody else realized they have this problem yet. E.g. there are a few users > > of __ATTR in block/genhd.c that have the same issue and are likely to > > generate the same false positives from static checkers. > > Then fix the broken static checkers :) He was exagerating a bit to call it a "static checker" warning... It's just checkpatch.pl complaining about adding spaces around the - operator. The sysfs file has a hyphen in the middle. regards, dan carpenter ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-13 21:43 ` Dan Carpenter @ 2017-09-13 21:58 ` Joe Perches 2017-09-13 23:16 ` Jonathan Cameron 2017-09-14 5:10 ` [Outreachy kernel] " Julia Lawall 0 siblings, 2 replies; 23+ messages in thread From: Joe Perches @ 2017-09-13 21:58 UTC (permalink / raw) To: Dan Carpenter, Greg KH Cc: Lars-Peter Clausen, devel, daniel.baluta, Himanshi Jain, Michael.Hennerich, linux-iio, linux-kernel, nick.desaulniers, outreachy-kernel, pmeerw, knaack.h, Jonathan Cameron On Thu, 2017-09-14 at 00:43 +0300, Dan Carpenter wrote: > He was exagerating a bit to call it a "static checker" warning... Not really. False positives and false negatives exist in just about every static checker. > It's just checkpatch.pl complaining about adding spaces around the - > operator. checkpatch is a brain-damaged by design static checker. regexes can only be sensitive to patterns, not compiled code. > The sysfs file has a hyphen in the middle. Another option would be to use an underscore instead. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-13 21:58 ` Joe Perches 2017-09-13 23:16 ` Jonathan Cameron @ 2017-09-13 23:16 ` Jonathan Cameron 1 sibling, 0 replies; 23+ messages in thread From: Jonathan Cameron @ 2017-09-13 23:16 UTC (permalink / raw) To: Joe Perches, Dan Carpenter, Greg KH Cc: Lars-Peter Clausen, devel, daniel.baluta, Himanshi Jain, Michael.Hennerich, linux-iio, linux-kernel, nick.desaulniers, outreachy-kernel, pmeerw, knaack.h, Jonathan Cameron On 13 September 2017 14:58:23 GMT-07:00, Joe Perches <joe@perches.com> wrote: >On Thu, 2017-09-14 at 00:43 +0300, Dan Carpenter wrote: >> He was exagerating a bit to call it a "static checker" warning... > >Not really. > >False positives and false negatives exist in just about >every static >checker. > >> It's just checkpatch.pl complaining about adding spaces around the - >> operator. > >checkpatch is a brain-damaged by design static checker. >regexes can only be sensitive to patterns, not compiled code. > >> The sysfs file has a hyphen in the middle. > >Another option would be to use an underscore instead. Userspace ABI plus it really does mean subtract. Used in description of differential channels. Most such IIO ABI is generated by the IIO core, this just comes up with corner cases of the ABI. Jonathan -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro @ 2017-09-13 23:16 ` Jonathan Cameron 0 siblings, 0 replies; 23+ messages in thread From: Jonathan Cameron @ 2017-09-13 23:16 UTC (permalink / raw) To: Joe Perches, Dan Carpenter, Greg KH Cc: Lars-Peter Clausen, devel, daniel.baluta, Himanshi Jain, Michael.Hennerich, linux-iio, linux-kernel, nick.desaulniers, outreachy-kernel, pmeerw, knaack.h, Jonathan Cameron On 13 September 2017 14:58:23 GMT-07:00, Joe Perches <joe@perches.com> wrote: >On Thu, 2017-09-14 at 00:43 +0300, Dan Carpenter wrote: >> He was exagerating a bit to call it a "static checker" warning... > >Not really. > >False positives and false negatives exist in just about >every static >checker. > >> It's just checkpatch.pl complaining about adding spaces around the - >> operator. > >checkpatch is a brain-damaged by design static checker. >regexes can only be sensitive to patterns, not compiled code. > >> The sysfs file has a hyphen in the middle. > >Another option would be to use an underscore instead. Userspace ABI plus it really does mean subtract. Used in description of differential channels. Most such IIO ABI is generated by the IIO core, this just comes up with corner cases of the ABI. Jonathan -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro @ 2017-09-13 23:16 ` Jonathan Cameron 0 siblings, 0 replies; 23+ messages in thread From: Jonathan Cameron @ 2017-09-13 23:16 UTC (permalink / raw) To: Joe Perches, Dan Carpenter, Greg KH Cc: Lars-Peter Clausen, devel, daniel.baluta, Himanshi Jain, Michael.Hennerich, linux-iio, linux-kernel, nick.desaulniers, outreachy-kernel, pmeerw, knaack.h, Jonathan Cameron On 13 September 2017 14:58:23 GMT-07:00, Joe Perches <joe@perches=2Ecom> w= rote: >On Thu, 2017-09-14 at 00:43 +0300, Dan Carpenter wrote: >> He was exagerating a bit to call it a "static checker" warning=2E=2E=2E > >Not really=2E > >False positives and false negatives exist in just about >every static >checker=2E > >> It's just checkpatch=2Epl complaining about adding spaces around the - >> operator=2E > >checkpatch is a brain-damaged by design static checker=2E >regexes can only be sensitive to patterns, not compiled code=2E > >> The sysfs file has a hyphen in the middle=2E > >Another option would be to use an underscore instead=2E Userspace ABI plus it really does mean subtract=2E Used in description of= differential channels=2E Most such IIO ABI is generated by the IIO core, this just comes up with co= rner cases of the ABI=2E Jonathan --=20 Sent from my Android device with K-9 Mail=2E Please excuse my brevity=2E ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-13 21:58 ` Joe Perches 2017-09-13 23:16 ` Jonathan Cameron @ 2017-09-14 5:10 ` Julia Lawall 1 sibling, 0 replies; 23+ messages in thread From: Julia Lawall @ 2017-09-14 5:10 UTC (permalink / raw) To: Joe Perches Cc: Dan Carpenter, Greg KH, Lars-Peter Clausen, devel, daniel.baluta, Himanshi Jain, Michael.Hennerich, linux-iio, linux-kernel, nick.desaulniers, outreachy-kernel, pmeerw, knaack.h, Jonathan Cameron On Wed, 13 Sep 2017, Joe Perches wrote: > On Thu, 2017-09-14 at 00:43 +0300, Dan Carpenter wrote: > > He was exagerating a bit to call it a "static checker" warning... > > Not really. > > False positives and false negatives exist in just about > every static > checker. > > > It's just checkpatch.pl complaining about adding spaces around the - > > operator. > > checkpatch is a brain-damaged by design static checker. > regexes can only be sensitive to patterns, not compiled code. > > > The sysfs file has a hyphen in the middle. > > Another option would be to use an underscore instead. There are already underscores on either side. julia > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1505339903.8969.20.camel%40perches.com. > For more options, visit https://groups.google.com/d/optout. > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro 2017-09-13 21:29 ` Greg KH 2017-09-13 21:43 ` Dan Carpenter @ 2017-09-19 12:13 ` Lars-Peter Clausen 1 sibling, 0 replies; 23+ messages in thread From: Lars-Peter Clausen @ 2017-09-19 12:13 UTC (permalink / raw) To: Greg KH Cc: Jonathan Cameron, devel, daniel.baluta, pmeerw, Michael.Hennerich, linux-iio, linux-kernel, nick.desaulniers, outreachy-kernel, Himanshi Jain, knaack.h On 09/13/2017 11:29 PM, Greg KH wrote: > On Wed, Sep 13, 2017 at 09:23:31PM +0200, Lars-Peter Clausen wrote: >> On 09/13/2017 08:58 PM, Greg KH wrote: >>> On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote: >>>> On Wed, 13 Sep 2017 14:14:07 +0530 >>>> Himanshi Jain <himshijain.hj@gmail.com> wrote: >>>> >>>>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a >>>>> string instead of implicit conversion of argument to string using >>>>> the macro _stringify(_name). >>>>> >>>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com> >>>>> --- >>>>> include/linux/sysfs.h | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h >>>>> index aa02c32..20321cf 100644 >>>>> --- a/include/linux/sysfs.h >>>>> +++ b/include/linux/sysfs.h >>>>> @@ -104,6 +104,13 @@ struct attribute_group { >>>>> .store = _store, \ >>>>> } >>>>> >>>>> +#define __ATTR_NAMED(_name, _mode, _show, _store) { \ >>>> >>>> I'm not sure about the naming here. The normal __ATTR macro is also >>>> 'named'. Maybe something as awful as >>>> >>>> __ATTR_STRING_NAME ? >>>> >>>> Greg what do you think? >>> >>> ick ick ick. >>> >>>> This is all to allow us to have names with operators in them without >>>> checkpatch complaining about them... A worthwhile aim just to stop >>>> more people wasting time trying to 'fix' those cases by adding spaces. >>> >>> Yeah, but this really seems "heavy" for just a crazy sysfs name in a >>> macro. Adding a whole new "core" define for that is a hard sell... >>> >>> I also want to get rid of the "generic" __ATTR type macros, and force >>> people to use the proper _RW and friends instead. I don't want to add >>> another new one that people will start to use that I later have to >>> change... >>> >>> So no, I don't like this, how about just changing your macros instead? >>> No one else has this problem :) >> >> Nobody else realized they have this problem yet. E.g. there are a few users >> of __ATTR in block/genhd.c that have the same issue and are likely to >> generate the same false positives from static checkers. > > Then fix the broken static checkers :) The static checkers aren't broken, the macro is. It takes a string parameter, but instead of a string it is passed as an expression and then transformed to string using preprocessor magic in the macro. That's not very good semantics. And hence this gets detected as a false positive, because nobody expects such strange behavior. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] [PATCH v2 0/2] Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string. 2017-09-13 8:55 ` Himanshi Jain (?) (?) @ 2017-09-13 8:46 ` Julia Lawall 2017-09-13 17:04 ` Jonathan Cameron -1 siblings, 1 reply; 23+ messages in thread From: Julia Lawall @ 2017-09-13 8:46 UTC (permalink / raw) To: Himanshi Jain Cc: outreachy-kernel, lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, nick.desaulniers, daniel.baluta On Wed, 13 Sep 2017, Himanshi Jain wrote: > This patchset is to rewrite the IIO_DEVICE_ATTR_NAMED API to pass name > as string. You need to indicate what has changed in the v2, either here or in the individual patches. julia > > Himanshi Jain (2): > include: linux: sysfs: Add __ATTR_NAMED macro > iio: Change to __ATTR_NAMED() > > drivers/iio/adc/ad7793.c | 2 +- > drivers/staging/iio/adc/ad7192.c | 2 +- > drivers/staging/iio/adc/ad7280a.c | 4 ++-- > include/linux/iio/sysfs.h | 6 +++++- > include/linux/sysfs.h | 7 +++++++ > 5 files changed, 16 insertions(+), 5 deletions(-) > > -- > 1.9.1 > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/cover.1505291907.git.himshijain.hj%40gmail.com. > For more options, visit https://groups.google.com/d/optout. > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] [PATCH v2 0/2] Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string. 2017-09-13 8:46 ` [Outreachy kernel] [PATCH v2 0/2] Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string Julia Lawall @ 2017-09-13 17:04 ` Jonathan Cameron 0 siblings, 0 replies; 23+ messages in thread From: Jonathan Cameron @ 2017-09-13 17:04 UTC (permalink / raw) To: Julia Lawall Cc: Himanshi Jain, outreachy-kernel, lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, linux-iio, devel, linux-kernel, nick.desaulniers, daniel.baluta On Wed, 13 Sep 2017 10:46:59 +0200 (CEST) Julia Lawall <julia.lawall@lip6.fr> wrote: > On Wed, 13 Sep 2017, Himanshi Jain wrote: > > > This patchset is to rewrite the IIO_DEVICE_ATTR_NAMED API to pass name > > as string. > > You need to indicate what has changed in the v2, either here or in the > individual patches. > > julia Absolutely agree with Julia. However, I'd wait for a day or so to see if anything else comes up rather than immediately sending a v3 with a correct change log. Thanks, Jonathan > > > > > > Himanshi Jain (2): > > include: linux: sysfs: Add __ATTR_NAMED macro > > iio: Change to __ATTR_NAMED() > > > > drivers/iio/adc/ad7793.c | 2 +- > > drivers/staging/iio/adc/ad7192.c | 2 +- > > drivers/staging/iio/adc/ad7280a.c | 4 ++-- > > include/linux/iio/sysfs.h | 6 +++++- > > include/linux/sysfs.h | 7 +++++++ > > 5 files changed, 16 insertions(+), 5 deletions(-) > > > > -- > > 1.9.1 > > > > -- > > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > > To post to this group, send email to outreachy-kernel@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/cover.1505291907.git.himshijain.hj%40gmail.com. > > For more options, visit https://groups.google.com/d/optout. > > ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2017-09-21 14:07 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-13 8:42 [PATCH v2 0/2] Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string Himanshi Jain 2017-09-13 8:55 ` Himanshi Jain 2017-09-13 8:44 ` [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro Himanshi Jain 2017-09-13 8:44 ` [PATCH v2 2/2] iio: Change to __ATTR_NAMED() Himanshi Jain 2017-09-13 17:03 ` [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro Jonathan Cameron 2017-09-13 18:58 ` Greg KH 2017-09-13 19:23 ` Lars-Peter Clausen 2017-09-13 20:50 ` Jonathan Cameron 2017-09-13 20:50 ` Jonathan Cameron 2017-09-18 10:49 ` Himanshi Jain 2017-09-21 13:54 ` Jonathan Cameron 2017-09-21 14:07 ` [Outreachy kernel] " Julia Lawall 2017-09-21 14:07 ` Julia Lawall 2017-09-13 21:29 ` Greg KH 2017-09-13 21:43 ` Dan Carpenter 2017-09-13 21:58 ` Joe Perches 2017-09-13 23:16 ` Jonathan Cameron 2017-09-13 23:16 ` Jonathan Cameron 2017-09-13 23:16 ` Jonathan Cameron 2017-09-14 5:10 ` [Outreachy kernel] " Julia Lawall 2017-09-19 12:13 ` Lars-Peter Clausen 2017-09-13 8:46 ` [Outreachy kernel] [PATCH v2 0/2] Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string Julia Lawall 2017-09-13 17:04 ` Jonathan Cameron
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.