diff mbox series

staging: iio: frequency: rename macros

Message ID 20241001202430.15874-2-tudor.reda@gmail.com (mailing list archive)
State New
Headers show
Series staging: iio: frequency: rename macros | expand

Commit Message

Tudor Gheorghiu Oct. 1, 2024, 8:24 p.m. UTC 
The frequency iio drivers use custom defined macros (inside dds.h) in
order to define sysfs attributes more easily.

However, due to their naming choice and the fact that in some of them the
first and/or second arguments are decimal, checkpatch will throw errors
in the source files they are used in (ad9832.c and ad9834.c).
This is because it thinks the argument is _mode, therefore
it expects octal notation, even if the argument itself
does not represent file permissions. Example:

ERROR: Use 4 digit octal (0777) not decimal permissions
+static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL, ad9834_write, AD9834_PSEL);

By re-naming the custom macros, we will avoid these false positive
checkpatch errors.

Signed-off-by: Tudor Gheorghiu <tudor.reda@gmail.com>
---
 drivers/staging/iio/frequency/ad9832.c | 26 +++++++++++-----------
 drivers/staging/iio/frequency/ad9834.c | 30 +++++++++++++-------------
 drivers/staging/iio/frequency/dds.h    | 20 ++++++++---------
 3 files changed, 38 insertions(+), 38 deletions(-)

Comments

Nam Cao Oct. 1, 2024, 10:54 p.m. UTC | #1 
On Tue, Oct 01, 2024 at 11:24:30PM +0300, Tudor Gheorghiu wrote:
> The frequency iio drivers use custom defined macros (inside dds.h) in
> order to define sysfs attributes more easily.
> 
> However, due to their naming choice and the fact that in some of them the
> first and/or second arguments are decimal, checkpatch will throw errors
> in the source files they are used in (ad9832.c and ad9834.c).
> This is because it thinks the argument is _mode, therefore
> it expects octal notation, even if the argument itself
> does not represent file permissions. Example:
> 
> ERROR: Use 4 digit octal (0777) not decimal permissions
> +static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL, ad9834_write, AD9834_PSEL);

You probably want to elaborate what you mean by "their naming choice" (i.e.
how does the naming choice causes this false warning?)

I got curious and digged into checkpatch.pl. This script expects macros
whose names match "IIO_DEV_ATTR_[A-Z_]+" to have the first integer argument
to be octal. And this driver defines macros which "luckily" match that
pattern.

There is only IIO_DEV_ATTR_SAMP_FREQ which matches the pattern, and accepts
umode_t as its first argument.

Instead of changing code just to make checkpatch.pl happy, perhaps it's
better to fix the checkpatch script? Maybe something like the untested
patch below?

Or since checkpatch is wrong, maybe just ignore it.

Best regards,
Nam

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4427572b2477..2fb4549fede2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -817,7 +817,7 @@ our @mode_permission_funcs = (
 	["debugfs_create_(?:file|u8|u16|u32|u64|x8|x16|x32|x64|size_t|atomic_t|bool|blob|regset32|u32_array)", 2],
 	["proc_create(?:_data|)", 2],
 	["(?:CLASS|DEVICE|SENSOR|SENSOR_DEVICE|IIO_DEVICE)_ATTR", 2],
-	["IIO_DEV_ATTR_[A-Z_]+", 1],
+	["IIO_DEV_ATTR_SAMP_FREQ", 1],
 	["SENSOR_(?:DEVICE_|)ATTR_2", 2],
 	["SENSOR_TEMPLATE(?:_2|)", 3],
 	["__ATTR", 2],
Tudor Gheorghiu Oct. 2, 2024, 2:49 a.m. UTC | #2 
On Wed, Oct 02, 2024 at 12:54:26AM +0200, Nam Cao wrote:
> 
> You probably want to elaborate what you mean by "their naming choice" (i.e.
> how does the naming choice causes this false warning?)
> 
> I got curious and digged into checkpatch.pl. This script expects macros
> whose names match "IIO_DEV_ATTR_[A-Z_]+" to have the first integer argument
> to be octal. And this driver defines macros which "luckily" match that
> pattern.
> 
> There is only IIO_DEV_ATTR_SAMP_FREQ which matches the pattern, and accepts
> umode_t as its first argument.
> 
> Instead of changing code just to make checkpatch.pl happy, perhaps it's
> better to fix the checkpatch script? Maybe something like the untested
> patch below?
> 
> Or since checkpatch is wrong, maybe just ignore it.
> 
> Best regards,
> Nam
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4427572b2477..2fb4549fede2 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -817,7 +817,7 @@ our @mode_permission_funcs = (
>  	["debugfs_create_(?:file|u8|u16|u32|u64|x8|x16|x32|x64|size_t|atomic_t|bool|blob|regset32|u32_array)", 2],
>  	["proc_create(?:_data|)", 2],
>  	["(?:CLASS|DEVICE|SENSOR|SENSOR_DEVICE|IIO_DEVICE)_ATTR", 2],
> -	["IIO_DEV_ATTR_[A-Z_]+", 1],
> +	["IIO_DEV_ATTR_SAMP_FREQ", 1],
>  	["SENSOR_(?:DEVICE_|)ATTR_2", 2],
>  	["SENSOR_TEMPLATE(?:_2|)", 3],
>  	["__ATTR", 2],

Hi!

Yes, this is exactly what I discovered while inspecting checkpatch
myself, however it did not occur to me this could be a problem with
checkpatch. But looking deeper, it seems like it is:

IIO_DEV_ATTR_SAMP_FREQ is defined in include/linux/iio/sysfs.h, along
with other helper macros:

> /**
>  * IIO_DEV_ATTR_SAMP_FREQ - sets any internal clock frequency
>  * @_mode: sysfs file mode/permissions
>  * @_show: output method for the attribute
>  * @_store: input method for the attribute
>  **/
> #define IIO_DEV_ATTR_SAMP_FREQ(_mode, _show, _store)			\
> 	IIO_DEVICE_ATTR(sampling_frequency, _mode, _show, _store, 0)
> 
> /**
>  * IIO_DEV_ATTR_SAMP_FREQ_AVAIL - list available sampling frequencies
>  * @_show: output method for the attribute
>  *
>  * May be mode dependent on some devices
>  **/
> #define IIO_DEV_ATTR_SAMP_FREQ_AVAIL(_show)				\
> 	IIO_DEVICE_ATTR(sampling_frequency_available, S_IRUGO, _show, NULL, 0)
> /**
>  * IIO_DEV_ATTR_INT_TIME_AVAIL - list available integration times
>  * @_show: output method for the attribute
>  **/
> #define IIO_DEV_ATTR_INT_TIME_AVAIL(_show)		\
> 	IIO_DEVICE_ATTR(integration_time_available, S_IRUGO, _show, NULL, 0)
> 
> #define IIO_DEV_ATTR_TEMP_RAW(_show)			\
> 	IIO_DEVICE_ATTR(in_temp_raw, S_IRUGO, _show, NULL, 0)

The checkpatch script will match all these macros, even if
IIO_DEV_ATTR_SAMP_FREQ is the only one where we need to check for octal
literal arguments. I grep'd through the entire sourcecode, and the only
false positives with literal decimal arguments which match "IIO_DEV_ATTR_[A-Z_]+"
are inside this driver.

I accidentally discovered this issue by running
checkpatch on the said driver files.

I will submit a patch to the checkpatch maintainers with this thread
linked, and if they agree this is a bug and accept the patch,
this driver patch will no longer be needed, since checkpatch will no longer flag
these macros as false positives.

Do I have your permission to add your name and email to Suggested-by?

Thanks!
Tudor
Nam Cao Oct. 2, 2024, 6:06 a.m. UTC | #3 
On Wed, Oct 02, 2024 at 05:49:15AM +0300, Tudor Gheorghiu wrote:
> I will submit a patch to the checkpatch maintainers with this thread
> linked, and if they agree this is a bug and accept the patch,
> this driver patch will no longer be needed, since checkpatch will no longer flag
> these macros as false positives.
> 
> Do I have your permission to add your name and email to Suggested-by?

Sure:
Suggested-by: Nam Cao <namcao@linutronix.de>
diff mbox series

Patch

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index 6c390c4eb26d..621543e50dc5 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -252,22 +252,22 @@  static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
  * see dds.h for further information
  */
 
-static IIO_DEV_ATTR_FREQ(0, 0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
-static IIO_DEV_ATTR_FREQ(0, 1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
-static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9832_write, AD9832_FREQ_SYM);
-static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
-
-static IIO_DEV_ATTR_PHASE(0, 0, 0200, NULL, ad9832_write, AD9832_PHASE0H);
-static IIO_DEV_ATTR_PHASE(0, 1, 0200, NULL, ad9832_write, AD9832_PHASE1H);
-static IIO_DEV_ATTR_PHASE(0, 2, 0200, NULL, ad9832_write, AD9832_PHASE2H);
-static IIO_DEV_ATTR_PHASE(0, 3, 0200, NULL, ad9832_write, AD9832_PHASE3H);
-static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL,
+static IIO_DEV_FREQ_ATTR(0, 0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
+static IIO_DEV_FREQ_ATTR(0, 1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
+static IIO_DEV_FREQSYMBOL_ATTR(0, 0200, NULL, ad9832_write, AD9832_FREQ_SYM);
+static IIO_CONST_FREQ_SCALE_ATTR(0, "1"); /* 1Hz */
+
+static IIO_DEV_PHASE_ATTR(0, 0, 0200, NULL, ad9832_write, AD9832_PHASE0H);
+static IIO_DEV_PHASE_ATTR(0, 1, 0200, NULL, ad9832_write, AD9832_PHASE1H);
+static IIO_DEV_PHASE_ATTR(0, 2, 0200, NULL, ad9832_write, AD9832_PHASE2H);
+static IIO_DEV_PHASE_ATTR(0, 3, 0200, NULL, ad9832_write, AD9832_PHASE3H);
+static IIO_DEV_PHASESYMBOL_ATTR(0, 0200, NULL,
 				ad9832_write, AD9832_PHASE_SYM);
-static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
+static IIO_CONST_PHASE_SCALE_ATTR(0, "0.0015339808"); /* 2PI/2^12 rad*/
 
-static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL,
+static IIO_DEV_PINCONTROL_EN_ATTR(0, 0200, NULL,
 				ad9832_write, AD9832_PINCTRL_EN);
-static IIO_DEV_ATTR_OUT_ENABLE(0, 0200, NULL,
+static IIO_DEV_OUT_ENABLE_ATTR(0, 0200, NULL,
 				ad9832_write, AD9832_OUTPUT_EN);
 
 static struct attribute *ad9832_attributes[] = {
diff --git a/drivers/staging/iio/frequency/ad9834.c b/drivers/staging/iio/frequency/ad9834.c
index 47e7d7e6d920..e1f7469d8011 100644
--- a/drivers/staging/iio/frequency/ad9834.c
+++ b/drivers/staging/iio/frequency/ad9834.c
@@ -321,21 +321,21 @@  static IIO_DEVICE_ATTR(out_altvoltage0_out1_wavetype_available, 0444,
  * see dds.h for further information
  */
 
-static IIO_DEV_ATTR_FREQ(0, 0, 0200, NULL, ad9834_write, AD9834_REG_FREQ0);
-static IIO_DEV_ATTR_FREQ(0, 1, 0200, NULL, ad9834_write, AD9834_REG_FREQ1);
-static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9834_write, AD9834_FSEL);
-static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
-
-static IIO_DEV_ATTR_PHASE(0, 0, 0200, NULL, ad9834_write, AD9834_REG_PHASE0);
-static IIO_DEV_ATTR_PHASE(0, 1, 0200, NULL, ad9834_write, AD9834_REG_PHASE1);
-static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL, ad9834_write, AD9834_PSEL);
-static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
-
-static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL, ad9834_write, AD9834_PIN_SW);
-static IIO_DEV_ATTR_OUT_ENABLE(0, 0200, NULL, ad9834_write, AD9834_RESET);
-static IIO_DEV_ATTR_OUTY_ENABLE(0, 1, 0200, NULL, ad9834_write, AD9834_OPBITEN);
-static IIO_DEV_ATTR_OUT_WAVETYPE(0, 0, ad9834_store_wavetype, 0);
-static IIO_DEV_ATTR_OUT_WAVETYPE(0, 1, ad9834_store_wavetype, 1);
+static IIO_DEV_FREQ_ATTR(0, 0, 0200, NULL, ad9834_write, AD9834_REG_FREQ0);
+static IIO_DEV_FREQ_ATTR(0, 1, 0200, NULL, ad9834_write, AD9834_REG_FREQ1);
+static IIO_DEV_FREQSYMBOL_ATTR(0, 0200, NULL, ad9834_write, AD9834_FSEL);
+static IIO_CONST_FREQ_SCALE_ATTR(0, "1"); /* 1Hz */
+
+static IIO_DEV_PHASE_ATTR(0, 0, 0200, NULL, ad9834_write, AD9834_REG_PHASE0);
+static IIO_DEV_PHASE_ATTR(0, 1, 0200, NULL, ad9834_write, AD9834_REG_PHASE1);
+static IIO_DEV_PHASESYMBOL_ATTR(0, 0200, NULL, ad9834_write, AD9834_PSEL);
+static IIO_CONST_PHASE_SCALE_ATTR(0, "0.0015339808"); /* 2PI/2^12 rad*/
+
+static IIO_DEV_PINCONTROL_EN_ATTR(0, 0200, NULL, ad9834_write, AD9834_PIN_SW);
+static IIO_DEV_OUT_ENABLE_ATTR(0, 0200, NULL, ad9834_write, AD9834_RESET);
+static IIO_DEV_OUTY_ENABLE_ATTR(0, 1, 0200, NULL, ad9834_write, AD9834_OPBITEN);
+static IIO_DEV_OUT_WAVETYPE_ATTR(0, 0, ad9834_store_wavetype, 0);
+static IIO_DEV_OUT_WAVETYPE_ATTR(0, 1, ad9834_store_wavetype, 1);
 
 static struct attribute *ad9834_attributes[] = {
 	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
diff --git a/drivers/staging/iio/frequency/dds.h b/drivers/staging/iio/frequency/dds.h
index 2ebe68eb7398..7f28eb437305 100644
--- a/drivers/staging/iio/frequency/dds.h
+++ b/drivers/staging/iio/frequency/dds.h
@@ -11,7 +11,7 @@ 
  * /sys/bus/iio/devices/.../out_altvoltageX_frequencyY
  */
 
-#define IIO_DEV_ATTR_FREQ(_channel, _num, _mode, _show, _store, _addr)	\
+#define IIO_DEV_FREQ_ATTR(_channel, _num, _mode, _show, _store, _addr)	\
 	IIO_DEVICE_ATTR(out_altvoltage##_channel##_frequency##_num,	\
 			_mode, _show, _store, _addr)
 
@@ -19,14 +19,14 @@ 
  * /sys/bus/iio/devices/.../out_altvoltageX_frequencyY_scale
  */
 
-#define IIO_CONST_ATTR_FREQ_SCALE(_channel, _string)			\
+#define IIO_CONST_FREQ_SCALE_ATTR(_channel, _string)			\
 	IIO_CONST_ATTR(out_altvoltage##_channel##_frequency_scale, _string)
 
 /**
  * /sys/bus/iio/devices/.../out_altvoltageX_frequencysymbol
  */
 
-#define IIO_DEV_ATTR_FREQSYMBOL(_channel, _mode, _show, _store, _addr)	\
+#define IIO_DEV_FREQSYMBOL_ATTR(_channel, _mode, _show, _store, _addr)	\
 	IIO_DEVICE_ATTR(out_altvoltage##_channel##_frequencysymbol,	\
 			_mode, _show, _store, _addr)
 
@@ -34,7 +34,7 @@ 
  * /sys/bus/iio/devices/.../out_altvoltageX_phaseY
  */
 
-#define IIO_DEV_ATTR_PHASE(_channel, _num, _mode, _show, _store, _addr)	\
+#define IIO_DEV_PHASE_ATTR(_channel, _num, _mode, _show, _store, _addr)	\
 	IIO_DEVICE_ATTR(out_altvoltage##_channel##_phase##_num,		\
 			_mode, _show, _store, _addr)
 
@@ -42,14 +42,14 @@ 
  * /sys/bus/iio/devices/.../out_altvoltageX_phaseY_scale
  */
 
-#define IIO_CONST_ATTR_PHASE_SCALE(_channel, _string)			\
+#define IIO_CONST_PHASE_SCALE_ATTR(_channel, _string)			\
 	IIO_CONST_ATTR(out_altvoltage##_channel##_phase_scale, _string)
 
 /**
  * /sys/bus/iio/devices/.../out_altvoltageX_phasesymbol
  */
 
-#define IIO_DEV_ATTR_PHASESYMBOL(_channel, _mode, _show, _store, _addr)	\
+#define IIO_DEV_PHASESYMBOL_ATTR(_channel, _mode, _show, _store, _addr)	\
 	IIO_DEVICE_ATTR(out_altvoltage##_channel##_phasesymbol,		\
 			_mode, _show, _store, _addr)
 
@@ -57,7 +57,7 @@ 
  * /sys/bus/iio/devices/.../out_altvoltageX_pincontrol_en
  */
 
-#define IIO_DEV_ATTR_PINCONTROL_EN(_channel, _mode, _show, _store, _addr)\
+#define IIO_DEV_PINCONTROL_EN_ATTR(_channel, _mode, _show, _store, _addr)\
 	IIO_DEVICE_ATTR(out_altvoltage##_channel##_pincontrol_en,	\
 			_mode, _show, _store, _addr)
 
@@ -81,7 +81,7 @@ 
  * /sys/bus/iio/devices/.../out_altvoltageX_out_enable
  */
 
-#define IIO_DEV_ATTR_OUT_ENABLE(_channel, _mode, _show, _store, _addr)	\
+#define IIO_DEV_OUT_ENABLE_ATTR(_channel, _mode, _show, _store, _addr)	\
 	IIO_DEVICE_ATTR(out_altvoltage##_channel##_out_enable,		\
 			_mode, _show, _store, _addr)
 
@@ -89,7 +89,7 @@ 
  * /sys/bus/iio/devices/.../out_altvoltageX_outY_enable
  */
 
-#define IIO_DEV_ATTR_OUTY_ENABLE(_channel, _output,			\
+#define IIO_DEV_OUTY_ENABLE_ATTR(_channel, _output,			\
 			_mode, _show, _store, _addr)			\
 	IIO_DEVICE_ATTR(out_altvoltage##_channel##_out##_output##_enable,\
 			_mode, _show, _store, _addr)
@@ -98,7 +98,7 @@ 
  * /sys/bus/iio/devices/.../out_altvoltageX_outY_wavetype
  */
 
-#define IIO_DEV_ATTR_OUT_WAVETYPE(_channel, _output, _store, _addr)	\
+#define IIO_DEV_OUT_WAVETYPE_ATTR(_channel, _output, _store, _addr)	\
 	IIO_DEVICE_ATTR(out_altvoltage##_channel##_out##_output##_wavetype,\
 			0200, NULL, _store, _addr)