Re: [PATCH] staging: pi433: Cleanup codestyle, indent statements after case labels

[Dan Carpenter]

On Tue, Jan 09, 2018 at 09:04:50PM +0100, Valentin Vidic wrote:
> On Sun, Dec 24, 2017 at 02:42:57PM +0100, Marcus Wolf wrote:
> > >  int rf69_set_dagc(struct spi_device *spi, enum dagc dagc)
> > >  {
> > >   switch (dagc) {
> > > - case normalMode: return rf69_write_reg(spi, 
> > > REG_TESTDAGC, DAGC_NORMAL);
> > > - case improve:return rf69_write_reg(spi, 
> > > REG_TESTDAGC, DAGC_IMPROVED_LOWBETA0);
> > > - case improve4LowModulationIndex: return rf69_write_reg(spi, 
> > > REG_TESTDAGC, DAGC_IMPROVED_LOWBETA1);
> > > + case normalMode:
> > > + return rf69_write_reg(spi, REG_TESTDAGC, DAGC_NORMAL);
> > > + case improve:
> > > + return rf69_write_reg(spi, REG_TESTDAGC, 
> > > DAGC_IMPROVED_LOWBETA0);
> > > + case improve4LowModulationIndex:
> > > + return rf69_write_reg(spi, REG_TESTDAGC, 
> > > DAGC_IMPROVED_LOWBETA1);
> > >   default:
> > >   dev_dbg(>dev, "set: illegal input param");
> > >   return -EINVAL;
> > > 
> > 
> > Hi Michael,
> > 
> > first of all thank you for your effort :-)
> > 
> > For me, the readability is reduced with this patch.
> > 
> > But that's just my opinion/favour...
> 
> Would something like this be any better for these simple switch
> statements?
> 

It's often better to avoid switch statements.

int rf69_set_dagc(struct spi_device *spi, enum dagc dagc)
{
static const int dagc_reg[] = {
[normalMode] = DAGC_NORMAL,
[improve] = DAGC_IMPROVED_LOWBETA0,
[improve4LowModulationIndex] = DAGC_IMPROVED_LOWBETA1,
};

if (dagc >= ARRAY_SIZE(dagc_reg))
return -EINVAL;

return rf69_write_reg(spi, REG_TESTDAGC, dagc_reg[dagc]);
}

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: pi433: Cleanup codestyle, indent statements after case labels

[Marcus Wolf]

Am 09.01.2018 um 21:04 schrieb Valentin Vidic:
> On Sun, Dec 24, 2017 at 02:42:57PM +0100, Marcus Wolf wrote:
>>>  int rf69_set_dagc(struct spi_device *spi, enum dagc dagc)
>>>  {
>>> switch (dagc) {
>>> -   case normalMode: return rf69_write_reg(spi, 
>>> REG_TESTDAGC, DAGC_NORMAL);
>>> -   case improve:return rf69_write_reg(spi, 
>>> REG_TESTDAGC, DAGC_IMPROVED_LOWBETA0);
>>> -   case improve4LowModulationIndex: return rf69_write_reg(spi, 
>>> REG_TESTDAGC, DAGC_IMPROVED_LOWBETA1);
>>> +   case normalMode:
>>> +   return rf69_write_reg(spi, REG_TESTDAGC, DAGC_NORMAL);
>>> +   case improve:
>>> +   return rf69_write_reg(spi, REG_TESTDAGC, 
>>> DAGC_IMPROVED_LOWBETA0);
>>> +   case improve4LowModulationIndex:
>>> +   return rf69_write_reg(spi, REG_TESTDAGC, 
>>> DAGC_IMPROVED_LOWBETA1);
>>> default:
>>> dev_dbg(>dev, "set: illegal input param");
>>> return -EINVAL;
>>>
>>
>> Hi Michael,
>>
>> first of all thank you for your effort :-)
>>
>> For me, the readability is reduced with this patch.
>>
>> But that's just my opinion/favour...
> 
> Would something like this be any better for these simple switch
> statements?
> 
> int rf69_set_dagc(struct spi_device *spi, enum dagc dagc)
> {
>int dagc_value;
> 
>switch (dagc) {
>case normalMode:   dagc_value = DAGC_NORMAL;
>case improve:  dagc_value = DAGC_IMPROVED_LOWBETA0;
>...
>default:
> dev_dbg(>dev, "set: illegal input param");
> return -EINVAL;
>}
> 
>return rf69_write_reg(spi, REG_TESTDAGC, dagc_value);
> }
> 

Hi Valentin,

I'd like such code very much. Marcin Ciupak already made such a proposal
but most probably the mainline changed to fast so he couldn't place his
patch...

If you would like to refactor the rf69.c in that way, I would be very happy.
Hope others will like this, too.

Thanks,

Marcus

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: pi433: Cleanup codestyle, indent statements after case labels

[Valentin Vidic]

On Sun, Dec 24, 2017 at 02:42:57PM +0100, Marcus Wolf wrote:
> >  int rf69_set_dagc(struct spi_device *spi, enum dagc dagc)
> >  {
> > switch (dagc) {
> > -   case normalMode: return rf69_write_reg(spi, 
> > REG_TESTDAGC, DAGC_NORMAL);
> > -   case improve:return rf69_write_reg(spi, 
> > REG_TESTDAGC, DAGC_IMPROVED_LOWBETA0);
> > -   case improve4LowModulationIndex: return rf69_write_reg(spi, 
> > REG_TESTDAGC, DAGC_IMPROVED_LOWBETA1);
> > +   case normalMode:
> > +   return rf69_write_reg(spi, REG_TESTDAGC, DAGC_NORMAL);
> > +   case improve:
> > +   return rf69_write_reg(spi, REG_TESTDAGC, 
> > DAGC_IMPROVED_LOWBETA0);
> > +   case improve4LowModulationIndex:
> > +   return rf69_write_reg(spi, REG_TESTDAGC, 
> > DAGC_IMPROVED_LOWBETA1);
> > default:
> > dev_dbg(>dev, "set: illegal input param");
> > return -EINVAL;
> > 
> 
> Hi Michael,
> 
> first of all thank you for your effort :-)
> 
> For me, the readability is reduced with this patch.
> 
> But that's just my opinion/favour...

Would something like this be any better for these simple switch
statements?

int rf69_set_dagc(struct spi_device *spi, enum dagc dagc)
{
   int dagc_value;

   switch (dagc) {
   case normalMode:   dagc_value = DAGC_NORMAL;
   case improve:  dagc_value = DAGC_IMPROVED_LOWBETA0;
   ...
   default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
   }

   return rf69_write_reg(spi, REG_TESTDAGC, dagc_value);
}

-- 
Valentin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: pi433: Cleanup codestyle, indent statements after case labels

[Greg KH]

On Sun, Dec 24, 2017 at 04:38:02AM +0100, Michael Panzlaff wrote:
> This patch changes the indentation of the statements after case labels.
> The linux coding guidelines do not explicitly mentiond this but pretty
> much all existing code doesn't put any statements into the same line of
> their belonging case labels. Therefore this adapts to the more usual style.
> 
> Please note that there is still a lot of > 80 character lines which will
> cause checkpatch warnings. This patch does not intent to fix this
> already existing issue.
> 
> Signed-off-by: Michael Panzlaff 
> Signed-off-by: Tillmann Zipperer 
> ---
>  drivers/staging/pi433/rf69.c | 246 
> ---
>  1 file changed, 164 insertions(+), 82 deletions(-)

Does not apply to my tree, please rebase and resend.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: pi433: Cleanup codestyle, indent statements after case labels

[Greg KH]

On Sun, Dec 24, 2017 at 04:38:02AM +0100, Michael Panzlaff wrote:
> To be applied on:
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> Branch:
> staging-next

That's not needed to be in the changelog text.
I'll hand-edit it for now, but please don't include it in future
patches.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: pi433: Cleanup codestyle, indent statements after case labels

[Marcus Wolf]

Am 24.12.2017 um 04:38 schrieb Michael Panzlaff:
> To be applied on:
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> Branch:
> staging-next
> 
> This patch changes the indentation of the statements after case labels.
> The linux coding guidelines do not explicitly mentiond this but pretty
> much all existing code doesn't put any statements into the same line of
> their belonging case labels. Therefore this adapts to the more usual style.
> 
> Please note that there is still a lot of > 80 character lines which will
> cause checkpatch warnings. This patch does not intent to fix this
> already existing issue.
> 
> Signed-off-by: Michael Panzlaff 
> Signed-off-by: Tillmann Zipperer 
> ---
>  drivers/staging/pi433/rf69.c | 246 
> ---
>  1 file changed, 164 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index 8bc09e1..1375b49 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -111,11 +111,16 @@ static inline int rf69_read_mod_write(struct spi_device 
> *spi, u8 reg, u8 mask, u
>  int rf69_set_mode(struct spi_device *spi, enum mode mode)
>  {
>   switch (mode) {
> - case transmit:return rf69_read_mod_write(spi, REG_OPMODE, 
> MASK_OPMODE_MODE, OPMODE_MODE_TRANSMIT);
> - case receive: return rf69_read_mod_write(spi, REG_OPMODE, 
> MASK_OPMODE_MODE, OPMODE_MODE_RECEIVE);
> - case synthesizer: return rf69_read_mod_write(spi, REG_OPMODE, 
> MASK_OPMODE_MODE, OPMODE_MODE_SYNTHESIZER);
> - case standby: return rf69_read_mod_write(spi, REG_OPMODE, 
> MASK_OPMODE_MODE, OPMODE_MODE_STANDBY);
> - case mode_sleep:  return rf69_read_mod_write(spi, REG_OPMODE, 
> MASK_OPMODE_MODE, OPMODE_MODE_SLEEP);
> + case transmit:
> + return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE, 
> OPMODE_MODE_TRANSMIT);
> + case receive:
> + return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE, 
> OPMODE_MODE_RECEIVE);
> + case synthesizer:
> + return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE, 
> OPMODE_MODE_SYNTHESIZER);
> + case standby:
> + return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE, 
> OPMODE_MODE_STANDBY);
> + case mode_sleep:
> + return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE, 
> OPMODE_MODE_SLEEP);
>   default:
>   dev_dbg(>dev, "set: illegal input param");
>   return -EINVAL;
> @@ -134,8 +139,10 @@ int rf69_set_data_mode(struct spi_device *spi, u8 
> data_mode)
>  int rf69_set_modulation(struct spi_device *spi, enum modulation modulation)
>  {
>   switch (modulation) {
> - case OOK: return rf69_read_mod_write(spi, REG_DATAMODUL, 
> MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
> - case FSK: return rf69_read_mod_write(spi, REG_DATAMODUL, 
> MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
> + case OOK:
> + return rf69_read_mod_write(spi, REG_DATAMODUL, 
> MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
> + case FSK:
> + return rf69_read_mod_write(spi, REG_DATAMODUL, 
> MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
>   default:
>   dev_dbg(>dev, "set: illegal input param");
>   return -EINVAL;
> @@ -149,9 +156,12 @@ static enum modulation rf69_get_modulation(struct 
> spi_device *spi)
>   currentValue = rf69_read_reg(spi, REG_DATAMODUL);
>  
>   switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE) {
> - case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
> - case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
> - default:return UNDEF;
> + case DATAMODUL_MODULATION_TYPE_OOK:
> + return OOK;
> + case DATAMODUL_MODULATION_TYPE_FSK:
> + return FSK;
> + default:
> + return UNDEF;
>   }
>  }
>  
> @@ -161,19 +171,26 @@ int rf69_set_modulation_shaping(struct spi_device *spi,
>   switch (rf69_get_modulation(spi)) {
>   case FSK:
>   switch (mod_shaping) {
> - case SHAPING_OFF: return rf69_read_mod_write(spi, 
> REG_DATAMODUL, MASK_DATAMODUL_MODULATION_SHAPE, 
> DATAMODUL_MODULATION_SHAPE_NONE);
> - case SHAPING_1_0: return rf69_read_mod_write(spi, 
> REG_DATAMODUL, MASK_DATAMODUL_MODULATION_SHAPE, 
> DATAMODUL_MODULATION_SHAPE_1_0);
> - case SHAPING_0_5: return rf69_read_mod_write(spi, 
> REG_DATAMODUL, MASK_DATAMODUL_MODULATION_SHAPE, 
> DATAMODUL_MODULATION_SHAPE_0_5);
> - case SHAPING_0_3: return rf69_read_mod_write(spi, 
> REG_DATAMODUL, MASK_DATAMODUL_MODULATION_SHAPE, 
> DATAMODUL_MODULATION_SHAPE_0_3);
> + case SHAPING_OFF:
> + return rf69_read_mod_write(spi, REG_DATAMODUL, 
> 

[PATCH] staging: pi433: Cleanup codestyle, indent statements after case labels

[Michael Panzlaff]

To be applied on:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
Branch:
staging-next

This patch changes the indentation of the statements after case labels.
The linux coding guidelines do not explicitly mentiond this but pretty
much all existing code doesn't put any statements into the same line of
their belonging case labels. Therefore this adapts to the more usual style.

Please note that there is still a lot of > 80 character lines which will
cause checkpatch warnings. This patch does not intent to fix this
already existing issue.

Signed-off-by: Michael Panzlaff 
Signed-off-by: Tillmann Zipperer 
---
 drivers/staging/pi433/rf69.c | 246 ---
 1 file changed, 164 insertions(+), 82 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 8bc09e1..1375b49 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -111,11 +111,16 @@ static inline int rf69_read_mod_write(struct spi_device 
*spi, u8 reg, u8 mask, u
 int rf69_set_mode(struct spi_device *spi, enum mode mode)
 {
switch (mode) {
-   case transmit:return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_TRANSMIT);
-   case receive: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_RECEIVE);
-   case synthesizer: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SYNTHESIZER);
-   case standby: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_STANDBY);
-   case mode_sleep:  return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SLEEP);
+   case transmit:
+   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE, 
OPMODE_MODE_TRANSMIT);
+   case receive:
+   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE, 
OPMODE_MODE_RECEIVE);
+   case synthesizer:
+   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE, 
OPMODE_MODE_SYNTHESIZER);
+   case standby:
+   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE, 
OPMODE_MODE_STANDBY);
+   case mode_sleep:
+   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE, 
OPMODE_MODE_SLEEP);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -134,8 +139,10 @@ int rf69_set_data_mode(struct spi_device *spi, u8 
data_mode)
 int rf69_set_modulation(struct spi_device *spi, enum modulation modulation)
 {
switch (modulation) {
-   case OOK: return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
-   case FSK: return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
+   case OOK:
+   return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
+   case FSK:
+   return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -149,9 +156,12 @@ static enum modulation rf69_get_modulation(struct 
spi_device *spi)
currentValue = rf69_read_reg(spi, REG_DATAMODUL);
 
switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE) {
-   case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
-   case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
-   default:return UNDEF;
+   case DATAMODUL_MODULATION_TYPE_OOK:
+   return OOK;
+   case DATAMODUL_MODULATION_TYPE_FSK:
+   return FSK;
+   default:
+   return UNDEF;
}
 }
 
@@ -161,19 +171,26 @@ int rf69_set_modulation_shaping(struct spi_device *spi,
switch (rf69_get_modulation(spi)) {
case FSK:
switch (mod_shaping) {
-   case SHAPING_OFF: return rf69_read_mod_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODULATION_SHAPE, 
DATAMODUL_MODULATION_SHAPE_NONE);
-   case SHAPING_1_0: return rf69_read_mod_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODULATION_SHAPE, DATAMODUL_MODULATION_SHAPE_1_0);
-   case SHAPING_0_5: return rf69_read_mod_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODULATION_SHAPE, DATAMODUL_MODULATION_SHAPE_0_5);
-   case SHAPING_0_3: return rf69_read_mod_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODULATION_SHAPE, DATAMODUL_MODULATION_SHAPE_0_3);
+   case SHAPING_OFF:
+   return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_SHAPE, DATAMODUL_MODULATION_SHAPE_NONE);
+   case SHAPING_1_0:
+   return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_SHAPE,