Re: [PATCH] staging: pi433: Cleanup codestyle, indent statements after case labels
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(&spi->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
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(&spi->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(&spi->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
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(&spi->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(&spi->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
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
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
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(&spi->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(&spi->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);
>
