Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string.

PATCH In review Himanshi Jain 9 participants 18 messages Sep 13, 2017 – Sep 21, 2017

Subsystems (5)

Staging IIO Driver core Analog Devices IIO drivers +1 more subsystems

lore

Share & tools

lore

Himanshi Jain

author

2017-09-13 08:42 UTC #0 lore raw

v2 cover Rewrite the IIO_DEVICE_ATTR_NAMED API to pass name as string.

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(-)

Signature · 1 line

1.9.1

Himanshi Jain

author

2017-09-13 08:44 UTC #1 lore raw

PATCH v2 1/2 include: linux: sysfs: Add __ATTR_NAMED macro

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 <[email protected]>

---
 include/linux/sysfs.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/linux/sysfs.h +7 -0

index aa02c32..20321cf 100644
@@ -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) },\

Signature · 1 line

1.9.1

Himanshi Jain

author

2017-09-13 08:44 UTC #2 lore raw

PATCH v2 2/2 iio: Change to __ATTR_NAMED()

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 <[email protected]>

---
 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(-)

drivers/iio/adc/ad7793.c +1 -1

index e6706a0..d74e324 100644
@@ -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[] = {

drivers/staging/iio/adc/ad7192.c +1 -1

index d11c6de..daff38c 100644
@@ -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,

drivers/staging/iio/adc/ad7280a.c +2 -2

index f85dde9..fd32e9a 100644
@@ -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,

include/linux/iio/sysfs.h +5 -1

index ce9426c..49c81a4 100644
@@ -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			\

Signature · 1 line

1.9.1

Jonathan Cameron

maintainer

2017-09-13 17:03 UTC #3 lore raw

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

In reply to Himanshi Jain

Quoted 14 lines · On Wed, 13 Sep 2017 14:14:07 +0530 Himanshi Jain <[email protected]>…

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 <[email protected]>
---
 include/linux/sysfs.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/linux/sysfs.h +0 -0
index aa02c32..20321cf 100644

include/linux/sysfs.h struct attribute_group { · line 107

Quoted 4 lines

include/linux/sysfs.h +1 -0

@@ -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

Quoted 9 lines

include/linux/sysfs.h +6 -0

+	.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) },\

Greg KH

maintainer

2017-09-13 18:58 UTC #4 lore raw

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

Quoted 20 lines · Jonathan Cameron

Quoted 17 lines · On Wed, 13 Sep 2017 14:14:07 +0530 Himanshi Jain <[email protected]>…
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 <[email protected]>
---
 include/linux/sysfs.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/linux/sysfs.h +0 -0
index aa02c32..20321cf 100644
@@ -104,6 +104,13 @@ struct attribute_group {
 	.store	= _store,						\
 }
 

Quoted 8 lines

include/linux/sysfs.h struct attribute_group { · line 107
Quoted 1 line
include/linux/sysfs.h +1 -0
+#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.

Quoted 3 lines

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

Lars-Peter Clausen

2017-09-13 19:23 UTC #5 lore raw

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

Quoted 39 lines · Greg KH

Quoted 20 lines · Jonathan Cameron
Quoted 17 lines · On Wed, 13 Sep 2017 14:14:07 +0530 Himanshi Jain <[email protected]>…
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 <[email protected]>
---
 include/linux/sysfs.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/linux/sysfs.h +0 -0
index aa02c32..20321cf 100644
@@ -104,6 +104,13 @@ struct attribute_group {
 	.store	= _store,						\
 }
 
Quoted 8 lines
include/linux/sysfs.h struct attribute_group { · line 107
Quoted 1 line
include/linux/sysfs.h +1 -0
+#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.
Quoted 3 lines
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...

Quoted 7 lines

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.

Greg KH

maintainer

2017-09-13 21:29 UTC #6 lore raw

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

Quoted 45 lines · Lars-Peter Clausen

Quoted 44 lines · Greg KH
Quoted 20 lines · Jonathan Cameron
Quoted 17 lines · On Wed, 13 Sep 2017 14:14:07 +0530 Himanshi Jain <[email protected]>…
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 <[email protected]>
---
 include/linux/sysfs.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/linux/sysfs.h +0 -0
index aa02c32..20321cf 100644
@@ -104,6 +104,13 @@ struct attribute_group {
 	.store	= _store,						\
 }
 
Quoted 8 lines
include/linux/sysfs.h struct attribute_group { · line 107
Quoted 1 line
include/linux/sysfs.h +1 -0
+#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.
Quoted 3 lines
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...

Quoted 6 lines

Quoted 2 lines
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 :)

Dan Carpenter

2017-09-13 21:44 UTC #7 lore raw

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

Quoted 46 lines · Greg KH

Quoted 45 lines · Lars-Peter Clausen
Quoted 44 lines · Greg KH
Quoted 20 lines · Jonathan Cameron
Quoted 17 lines · On Wed, 13 Sep 2017 14:14:07 +0530 Himanshi Jain <[email protected]>…
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 <[email protected]>
---
 include/linux/sysfs.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/linux/sysfs.h +0 -0
index aa02c32..20321cf 100644
@@ -104,6 +104,13 @@ struct attribute_group {
 	.store	= _store,						\
 }
 
Quoted 8 lines
include/linux/sysfs.h struct attribute_group { · line 107
Quoted 1 line
include/linux/sysfs.h +1 -0
+#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.
Quoted 3 lines
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...

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
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

Joe Perches

2017-09-13 21:58 UTC #8 lore raw

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

Quoted 1 line · Dan Carpenter

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.

Quoted 2 lines

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.

Quoted 1 line

The sysfs file has a hyphen in the middle.

Another option would be to use an underscore instead.

Jonathan Cameron

2017-09-13 23:16 UTC #9 lore raw

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

On 13 September 2017 14:58:23 GMT-07:00, Joe Perches <joe@perches=2Ecom> w= rote:

Quoted 12 lines · Dan Carpenter

Quoted 1 line
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
Quoted 2 lines
It's just checkpatch=2Epl complaining about adding spaces around the - operator=2E

Quoted 6 lines

checkpatch is a brain-damaged by design static checker=2E regexes can only be sensitive to patterns, not compiled code=2E
Quoted 1 line
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

Julia Lawall

2017-09-14 05:10 UTC #10 lore raw

Re: [Outreachy kernel] Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

In reply to Joe Perches

Quoted 12 lines · Joe Perches

Quoted 1 line · Dan Carpenter
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.
Quoted 2 lines
It's just checkpatch.pl complaining about adding spaces around the - operator.

Quoted 6 lines

checkpatch is a brain-damaged by design static checker. regexes can only be sensitive to patterns, not compiled code.
Quoted 1 line
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

Quoted 8 lines

--
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 [email protected]. To post to this group, send email to [email protected].
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.

Lars-Peter Clausen

2017-09-19 12:13 UTC #11 lore raw

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

In reply to Greg KH

Quoted 46 lines · Greg KH

Quoted 45 lines · Lars-Peter Clausen
Quoted 44 lines · Greg KH
Quoted 20 lines · Jonathan Cameron
Quoted 17 lines · On Wed, 13 Sep 2017 14:14:07 +0530 Himanshi Jain <[email protected]>…
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 <[email protected]>
---
 include/linux/sysfs.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/linux/sysfs.h +0 -0
index aa02c32..20321cf 100644
@@ -104,6 +104,13 @@ struct attribute_group {
 	.store	= _store,						\
 }
 
Quoted 8 lines
include/linux/sysfs.h struct attribute_group { · line 107
Quoted 1 line
include/linux/sysfs.h +1 -0
+#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.
Quoted 3 lines
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...

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
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.

Jonathan Cameron

2017-09-13 23:13 UTC #12 lore raw

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

In reply to Lars-Peter Clausen

Quoted 47 lines · Lars-Peter Clausen

Quoted 26 lines · Greg KH
Quoted 25 lines · Jonathan Cameron
Quoted 14 lines · On Wed, 13 Sep 2017 14:14:07 +0530 Himanshi Jain <[email protected]>…
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 <[email protected]>
---
 include/linux/sysfs.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/linux/sysfs.h +0 -0
index aa02c32..20321cf 100644
include/linux/sysfs.h struct attribute_group { · line 107
Quoted 4 lines
include/linux/sysfs.h +1 -0
@@ -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
Quoted 9 lines
Quoted 3 lines
__ATTR_STRING_NAME ?
Greg what do you think?
ick ick ick.
Quoted 3 lines
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.
Quoted 6 lines
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
Quoted 3 lines
another new one that people will start to use that I later have to change...

Quoted 8 lines

Quoted 1 line
So no, I don't like this, how about just changing your macros
instead?
Quoted 1 line
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

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Himanshi Jain

author

2017-09-18 10:49 UTC #13 lore raw

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

Quoted 59 lines · Jonathan Cameron

Quoted 47 lines · Lars-Peter Clausen
Quoted 26 lines · Greg KH
Quoted 25 lines · Jonathan Cameron
Quoted 14 lines · On Wed, 13 Sep 2017 14:14:07 +0530 Himanshi Jain <[email protected]>…
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 <[email protected]>
---
 include/linux/sysfs.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/linux/sysfs.h +0 -0
index aa02c32..20321cf 100644
include/linux/sysfs.h struct attribute_group { · line 107
Quoted 4 lines
include/linux/sysfs.h +1 -0
@@ -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
Quoted 9 lines
Quoted 3 lines
__ATTR_STRING_NAME ?
Greg what do you think?
ick ick ick.
Quoted 3 lines
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.
Quoted 6 lines
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
Quoted 3 lines
another new one that people will start to use that I later have to change...
Quoted 8 lines
Quoted 1 line
So no, I don't like this, how about just changing your macros
instead?
Quoted 1 line
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.

Quoted 4 lines

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?

Quoted 12 lines

If I am feeling really keen I might write this missing docs I promised a while back on that stuff. Jet lag dependant...
Jonathan
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Jonathan Cameron

2017-09-21 13:54 UTC #14 lore raw

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

Quoted 61 lines · On Mon, 18 Sep 2017 16:19:07 +0530 Himanshi Jain <[email protected]>…

Quoted 59 lines · Jonathan Cameron
Quoted 55 lines · Lars-Peter Clausen
Quoted 26 lines · Greg KH
Quoted 25 lines · Jonathan Cameron
Quoted 14 lines · On Wed, 13 Sep 2017 14:14:07 +0530 Himanshi Jain <[email protected]>…
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 <[email protected]>
---
 include/linux/sysfs.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/linux/sysfs.h +0 -0
index aa02c32..20321cf 100644
include/linux/sysfs.h struct attribute_group { · line 107
Quoted 4 lines
include/linux/sysfs.h +1 -0
@@ -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
Quoted 9 lines
Quoted 3 lines
__ATTR_STRING_NAME ?
Greg what do you think?
ick ick ick.
Quoted 3 lines
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.
Quoted 6 lines
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
Quoted 4 lines
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?
Quoted 1 line
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.

Quoted 8 lines

Quoted 4 lines
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

Quoted 20 lines

Or do you want me to put comments to not to fix this checkpatch warning as you suggested earlier?
Quoted 5 lines
If I am feeling really keen I might write this missing docs I promised a while back on that stuff. Jet lag dependant...
Jonathan
Quoted 1 line
Quoted 7 lines
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Julia Lawall

2017-09-21 14:07 UTC #15 lore raw

Re: [Outreachy kernel] Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

Quoted 110 lines · Jonathan Cameron

Quoted 61 lines · On Mon, 18 Sep 2017 16:19:07 +0530 Himanshi Jain <[email protected]>…
Quoted 59 lines · Jonathan Cameron
Quoted 55 lines · Lars-Peter Clausen
Quoted 26 lines · Greg KH
Quoted 25 lines · Jonathan Cameron
Quoted 14 lines · On Wed, 13 Sep 2017 14:14:07 +0530 Himanshi Jain <[email protected]>…
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 <[email protected]>
---
 include/linux/sysfs.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/linux/sysfs.h +0 -0
index aa02c32..20321cf 100644
include/linux/sysfs.h struct attribute_group { · line 107
Quoted 4 lines
include/linux/sysfs.h +1 -0
@@ -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
Quoted 9 lines
Quoted 3 lines
__ATTR_STRING_NAME ?
Greg what do you think?
ick ick ick.
Quoted 3 lines
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.
Quoted 6 lines
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
Quoted 4 lines
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?
Quoted 1 line
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.
Quoted 8 lines
Quoted 4 lines
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!

Quoted 5 lines

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,

Quoted 1 line

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

Quoted 33 lines

Thanks,
Jonathan
Quoted 14 lines
Or do you want me to put comments to not to fix this checkpatch warning as you suggested earlier?
Quoted 10 lines
If I am feeling really keen I might write this missing docs I promised a while back on that stuff. Jet lag dependant...
Jonathan
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 6 lines
Quoted 2 lines
--
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 [email protected]
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 [email protected]. To post to this group, send email to [email protected].
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.

Julia Lawall

2017-09-13 08:46 UTC #16 lore raw

In reply to Himanshi Jain

Quoted 2 lines · Himanshi Jain

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

Quoted 22 lines

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 [email protected]. To post to this group, send email to [email protected].
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.

Jonathan Cameron

maintainer

2017-09-13 17:04 UTC #17 lore raw

Quoted 9 lines · On Wed, 13 Sep 2017 10:46:59 +0200 (CEST) Julia Lawall <[email protected]>…

Quoted 2 lines · Himanshi Jain
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

Quoted 24 lines

Quoted 24 lines
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 [email protected]. To post to this group, send email to [email protected].
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.