| Message ID | YkHOpCT2Gad1YaxN@hp-amd-paul (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | IIO: accel: fixed coding style issues | expand |
On Mon, 28 Mar 2022 10:05:08 -0500 Paul Lemmermann <thepaulodoom@thepaulodoom.com> wrote: > Fixed case statement issues and spacing issues. > > Signed-off-by: Paul Lemmermann <thepaulodoom@thepaulodoom.com> Hi Paul, Thanks for the patch. These are drivers written / maintained by different authors, so one patch per driver preferred. Particularly handy as people might not agree with all of them so separate patches would allow me to pick and choose which ones to pick up. Comments inline. Please state how you identified the changes btw. Script? Jonathan > --- > drivers/iio/accel/bmc150-accel-core.c | 15 +++++++++++---- > drivers/iio/accel/dmard09.c | 2 +- > drivers/iio/accel/kxsd9-spi.c | 4 ++-- > 3 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c > index d11f66801..f7dd7ec2c 100644 > --- a/drivers/iio/accel/bmc150-accel-core.c > +++ b/drivers/iio/accel/bmc150-accel-core.c > @@ -432,10 +432,17 @@ static bool bmc150_apply_bosc0200_acpi_orientation(struct device *dev, > > for (j = 0; j < 3; j++) { > switch (val[j]) { > - case -1: str = "-1"; break; > - case 0: str = "0"; break; > - case 1: str = "1"; break; > - default: goto unknown_format; > + case -1: > + str = "-1"; > + break; > + case 0: > + str = "0"; > + break; > + case 1: > + str = "1"; > + break; > + default: > + goto unknown_format; I'm not seeing any clear advantage to this change. > } > orientation->rotation[i * 3 + j] = str; > } > diff --git a/drivers/iio/accel/dmard09.c b/drivers/iio/accel/dmard09.c > index e6e28c964..87bc38d4d 100644 > --- a/drivers/iio/accel/dmard09.c > +++ b/drivers/iio/accel/dmard09.c > @@ -24,7 +24,7 @@ > #define DMARD09_AXIS_Y 1 > #define DMARD09_AXIS_Z 2 > #define DMARD09_AXIS_X_OFFSET ((DMARD09_AXIS_X + 1) * 2) > -#define DMARD09_AXIS_Y_OFFSET ((DMARD09_AXIS_Y + 1 )* 2) > +#define DMARD09_AXIS_Y_OFFSET ((DMARD09_AXIS_Y + 1) * 2) This is one is good. > #define DMARD09_AXIS_Z_OFFSET ((DMARD09_AXIS_Z + 1) * 2) > > struct dmard09_data { > diff --git a/drivers/iio/accel/kxsd9-spi.c b/drivers/iio/accel/kxsd9-spi.c > index 57c451cfb..989f53fb0 100644 > --- a/drivers/iio/accel/kxsd9-spi.c > +++ b/drivers/iio/accel/kxsd9-spi.c > @@ -44,8 +44,8 @@ static const struct spi_device_id kxsd9_spi_id[] = { > MODULE_DEVICE_TABLE(spi, kxsd9_spi_id); > > static const struct of_device_id kxsd9_of_match[] = { > - { .compatible = "kionix,kxsd9" }, > - { }, > + { .compatible = "kionix,kxsd9" }, Also good to clean up. Thanks, Jonathan > + { }, > }; > MODULE_DEVICE_TABLE(of, kxsd9_of_match); >
On Mon, Mar 28, 2022 at 06:06:23PM +0100, Jonathan Cameron wrote: > On Mon, 28 Mar 2022 10:05:08 -0500 > Paul Lemmermann <thepaulodoom@thepaulodoom.com> wrote: > > > Fixed case statement issues and spacing issues. > > > > Signed-off-by: Paul Lemmermann <thepaulodoom@thepaulodoom.com> > Hi Paul, > > Thanks for the patch. > > These are drivers written / maintained by different authors, so one > patch per driver preferred. Particularly handy as people might not > agree with all of them so separate patches would allow me to pick > and choose which ones to pick up. > > Comments inline. > > Please state how you identified the changes btw. Script? I used the checkpatch.pl script to identify these changes. > > Jonathan > > > --- > > drivers/iio/accel/bmc150-accel-core.c | 15 +++++++++++---- > > drivers/iio/accel/dmard09.c | 2 +- > > drivers/iio/accel/kxsd9-spi.c | 4 ++-- > > 3 files changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c > > index d11f66801..f7dd7ec2c 100644 > > --- a/drivers/iio/accel/bmc150-accel-core.c > > +++ b/drivers/iio/accel/bmc150-accel-core.c > > @@ -432,10 +432,17 @@ static bool bmc150_apply_bosc0200_acpi_orientation(struct device *dev, > > > > for (j = 0; j < 3; j++) { > > switch (val[j]) { > > - case -1: str = "-1"; break; > > - case 0: str = "0"; break; > > - case 1: str = "1"; break; > > - default: goto unknown_format; > > + case -1: > > + str = "-1"; > > + break; > > + case 0: > > + str = "0"; > > + break; > > + case 1: > > + str = "1"; > > + break; > > + default: > > + goto unknown_format; > > I'm not seeing any clear advantage to this change. > The reason for this change is that most, if not all, case statements in the kernel are multiple lines. Also, in the kernel docs (https://docs.kernel.org/process/coding-style.html) it says, to not put multiple statements on one line. Thanks, Paul > > > } > > orientation->rotation[i * 3 + j] = str; > > } > > diff --git a/drivers/iio/accel/dmard09.c b/drivers/iio/accel/dmard09.c > > index e6e28c964..87bc38d4d 100644 > > --- a/drivers/iio/accel/dmard09.c > > +++ b/drivers/iio/accel/dmard09.c > > @@ -24,7 +24,7 @@ > > #define DMARD09_AXIS_Y 1 > > #define DMARD09_AXIS_Z 2 > > #define DMARD09_AXIS_X_OFFSET ((DMARD09_AXIS_X + 1) * 2) > > -#define DMARD09_AXIS_Y_OFFSET ((DMARD09_AXIS_Y + 1 )* 2) > > +#define DMARD09_AXIS_Y_OFFSET ((DMARD09_AXIS_Y + 1) * 2) > > This is one is good. > > > #define DMARD09_AXIS_Z_OFFSET ((DMARD09_AXIS_Z + 1) * 2) > > > > struct dmard09_data { > > diff --git a/drivers/iio/accel/kxsd9-spi.c b/drivers/iio/accel/kxsd9-spi.c > > index 57c451cfb..989f53fb0 100644 > > --- a/drivers/iio/accel/kxsd9-spi.c > > +++ b/drivers/iio/accel/kxsd9-spi.c > > @@ -44,8 +44,8 @@ static const struct spi_device_id kxsd9_spi_id[] = { > > MODULE_DEVICE_TABLE(spi, kxsd9_spi_id); > > > > static const struct of_device_id kxsd9_of_match[] = { > > - { .compatible = "kionix,kxsd9" }, > > - { }, > > + { .compatible = "kionix,kxsd9" }, > > Also good to clean up. > > Thanks, > > Jonathan > > > > + { }, > > }; > > MODULE_DEVICE_TABLE(of, kxsd9_of_match); > > >
On Mon, Mar 28, 2022 at 7:45 PM Paul Lemmermann <thepaulodoom@thepaulodoom.com> wrote: > > Fixed case statement issues and spacing issues. ... > switch (val[j]) { > - case -1: str = "-1"; break; > - case 0: str = "0"; break; > - case 1: str = "1"; break; > - default: goto unknown_format; > + case -1: > + str = "-1"; > + break; > + case 0: > + str = "0"; > + break; > + case 1: > + str = "1"; > + break; > + default: > + goto unknown_format; > } What you cited from documentation mostly affects the new code, but this code is already in the kernel and modifying it, esp. taking into account 3x LOCs count, looks like an unneeded churn, even if documentation thinks otherwise. What I could acknowledge from your proposal is the default case. Otherwise just leave it to the point if we touch this code for something else in the future. ... > static const struct of_device_id kxsd9_of_match[] = { > - { .compatible = "kionix,kxsd9" }, > - { }, > + { .compatible = "kionix,kxsd9" }, > + { }, I dunno why you touched this line (likely TABs vs. spaces), but please remove the comma as well here. > };
On Mon, Mar 28, 2022 at 11:32:44PM +0300, Andy Shevchenko wrote: > On Mon, Mar 28, 2022 at 7:45 PM Paul Lemmermann > <thepaulodoom@thepaulodoom.com> wrote: > > > > Fixed case statement issues and spacing issues. > > ... > > > switch (val[j]) { > > - case -1: str = "-1"; break; > > - case 0: str = "0"; break; > > - case 1: str = "1"; break; > > - default: goto unknown_format; > > + case -1: > > + str = "-1"; > > + break; > > + case 0: > > + str = "0"; > > + break; > > + case 1: > > + str = "1"; > > + break; > > + default: > > + goto unknown_format; > > } > > What you cited from documentation mostly affects the new code, but > this code is already in the kernel and modifying it, esp. taking into > account 3x LOCs count, looks like an unneeded churn, even if > documentation thinks otherwise. > > What I could acknowledge from your proposal is the default case. > Otherwise just leave it to the point if we touch this code for > something else in the future. > > ... > > > static const struct of_device_id kxsd9_of_match[] = { > > - { .compatible = "kionix,kxsd9" }, > > - { }, > > + { .compatible = "kionix,kxsd9" }, > > > + { }, > > I dunno why you touched this line (likely TABs vs. spaces), but please > remove the comma as well here. Yes, that is exactly why this patch exists. Thank you, I will take these thoughts into account and submit a new patch. Thanks! Paul > > > }; > > -- > With Best Regards, > Andy Shevchenko
On Mon, 28 Mar 2022 14:17:51 -0500 Paul Lemmermann <thepaulodoom@thepaulodoom.com> wrote: > On Mon, Mar 28, 2022 at 06:06:23PM +0100, Jonathan Cameron wrote: > > On Mon, 28 Mar 2022 10:05:08 -0500 > > Paul Lemmermann <thepaulodoom@thepaulodoom.com> wrote: > > > > > Fixed case statement issues and spacing issues. > > > > > > Signed-off-by: Paul Lemmermann <thepaulodoom@thepaulodoom.com> > > Hi Paul, > > > > Thanks for the patch. > > > > These are drivers written / maintained by different authors, so one > > patch per driver preferred. Particularly handy as people might not > > agree with all of them so separate patches would allow me to pick > > and choose which ones to pick up. > > > > Comments inline. > > > > Please state how you identified the changes btw. Script? > > I used the checkpatch.pl script to identify these changes. Please mention that in patch descriptions in future. It's useful to anyone tracking this sort of information. I added it to the broken out patches whilst applying. Thanks, Jonathan > > > > > Jonathan > > > > > --- > > > drivers/iio/accel/bmc150-accel-core.c | 15 +++++++++++---- > > > drivers/iio/accel/dmard09.c | 2 +- > > > drivers/iio/accel/kxsd9-spi.c | 4 ++-- > > > 3 files changed, 14 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c > > > index d11f66801..f7dd7ec2c 100644 > > > --- a/drivers/iio/accel/bmc150-accel-core.c > > > +++ b/drivers/iio/accel/bmc150-accel-core.c > > > @@ -432,10 +432,17 @@ static bool bmc150_apply_bosc0200_acpi_orientation(struct device *dev, > > > > > > for (j = 0; j < 3; j++) { > > > switch (val[j]) { > > > - case -1: str = "-1"; break; > > > - case 0: str = "0"; break; > > > - case 1: str = "1"; break; > > > - default: goto unknown_format; > > > + case -1: > > > + str = "-1"; > > > + break; > > > + case 0: > > > + str = "0"; > > > + break; > > > + case 1: > > > + str = "1"; > > > + break; > > > + default: > > > + goto unknown_format; > > > > I'm not seeing any clear advantage to this change. > > > The reason for this change is that most, if not all, case statements in > the kernel are multiple lines. Also, in the kernel docs (https://docs.kernel.org/process/coding-style.html) > it says, to not put multiple statements on one line. > > Thanks, > Paul > > > > > } > > > orientation->rotation[i * 3 + j] = str; > > > } > > > diff --git a/drivers/iio/accel/dmard09.c b/drivers/iio/accel/dmard09.c > > > index e6e28c964..87bc38d4d 100644 > > > --- a/drivers/iio/accel/dmard09.c > > > +++ b/drivers/iio/accel/dmard09.c > > > @@ -24,7 +24,7 @@ > > > #define DMARD09_AXIS_Y 1 > > > #define DMARD09_AXIS_Z 2 > > > #define DMARD09_AXIS_X_OFFSET ((DMARD09_AXIS_X + 1) * 2) > > > -#define DMARD09_AXIS_Y_OFFSET ((DMARD09_AXIS_Y + 1 )* 2) > > > +#define DMARD09_AXIS_Y_OFFSET ((DMARD09_AXIS_Y + 1) * 2) > > > > This is one is good. > > > > > #define DMARD09_AXIS_Z_OFFSET ((DMARD09_AXIS_Z + 1) * 2) > > > > > > struct dmard09_data { > > > diff --git a/drivers/iio/accel/kxsd9-spi.c b/drivers/iio/accel/kxsd9-spi.c > > > index 57c451cfb..989f53fb0 100644 > > > --- a/drivers/iio/accel/kxsd9-spi.c > > > +++ b/drivers/iio/accel/kxsd9-spi.c > > > @@ -44,8 +44,8 @@ static const struct spi_device_id kxsd9_spi_id[] = { > > > MODULE_DEVICE_TABLE(spi, kxsd9_spi_id); > > > > > > static const struct of_device_id kxsd9_of_match[] = { > > > - { .compatible = "kionix,kxsd9" }, > > > - { }, > > > + { .compatible = "kionix,kxsd9" }, > > > > Also good to clean up. > > > > Thanks, > > > > Jonathan > > > > > > > + { }, > > > }; > > > MODULE_DEVICE_TABLE(of, kxsd9_of_match); > > > > >
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c index d11f66801..f7dd7ec2c 100644 --- a/drivers/iio/accel/bmc150-accel-core.c +++ b/drivers/iio/accel/bmc150-accel-core.c @@ -432,10 +432,17 @@ static bool bmc150_apply_bosc0200_acpi_orientation(struct device *dev, for (j = 0; j < 3; j++) { switch (val[j]) { - case -1: str = "-1"; break; - case 0: str = "0"; break; - case 1: str = "1"; break; - default: goto unknown_format; + case -1: + str = "-1"; + break; + case 0: + str = "0"; + break; + case 1: + str = "1"; + break; + default: + goto unknown_format; } orientation->rotation[i * 3 + j] = str; } diff --git a/drivers/iio/accel/dmard09.c b/drivers/iio/accel/dmard09.c index e6e28c964..87bc38d4d 100644 --- a/drivers/iio/accel/dmard09.c +++ b/drivers/iio/accel/dmard09.c @@ -24,7 +24,7 @@ #define DMARD09_AXIS_Y 1 #define DMARD09_AXIS_Z 2 #define DMARD09_AXIS_X_OFFSET ((DMARD09_AXIS_X + 1) * 2) -#define DMARD09_AXIS_Y_OFFSET ((DMARD09_AXIS_Y + 1 )* 2) +#define DMARD09_AXIS_Y_OFFSET ((DMARD09_AXIS_Y + 1) * 2) #define DMARD09_AXIS_Z_OFFSET ((DMARD09_AXIS_Z + 1) * 2) struct dmard09_data { diff --git a/drivers/iio/accel/kxsd9-spi.c b/drivers/iio/accel/kxsd9-spi.c index 57c451cfb..989f53fb0 100644 --- a/drivers/iio/accel/kxsd9-spi.c +++ b/drivers/iio/accel/kxsd9-spi.c @@ -44,8 +44,8 @@ static const struct spi_device_id kxsd9_spi_id[] = { MODULE_DEVICE_TABLE(spi, kxsd9_spi_id); static const struct of_device_id kxsd9_of_match[] = { - { .compatible = "kionix,kxsd9" }, - { }, + { .compatible = "kionix,kxsd9" }, + { }, }; MODULE_DEVICE_TABLE(of, kxsd9_of_match);
Fixed case statement issues and spacing issues. Signed-off-by: Paul Lemmermann <thepaulodoom@thepaulodoom.com> --- drivers/iio/accel/bmc150-accel-core.c | 15 +++++++++++---- drivers/iio/accel/dmard09.c | 2 +- drivers/iio/accel/kxsd9-spi.c | 4 ++-- 3 files changed, 14 insertions(+), 7 deletions(-)