From: walter harms <wharms@bfs.de>
To: kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] staging: pi433: fix bugs in register abstraction of rf69 chip
Date: Thu, 20 Jul 2017 12:22:56 +0000 [thread overview]
Message-ID: <5970A0A0.8020507@bfs.de> (raw)
In-Reply-To: <95873f94b04fbc67769c3fc0051d3304-EhVcX1pHQwdXWkQFBhENSgEKLlwACzJXX19HAVhEWENbS1kLMF52CEtUX1pBSEwcXlJRL1lQWAhbV34CV1E=-webmailer1@server02.webmailer.webmailer.hosteurope.de>
Am 19.07.2017 20:18, schrieb Wolf Entwicklungen:
> Bugfixes for rf69_set_modulation, rf69_set_deviation, rf69_set_lna_gain and rf69_get_lna_gain
> The fixes are cross-checked with the datasheet of the rfm69cw
>
> Fixes: 874bcba65f9a ("staging: pi433: New driver")
> Signed-off-by: Marcus Wolf <linux@wolf-entwicklungen.de>
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -101,7 +101,7 @@ int rf69_set_modulation(struct spi_device *spi, enum modulation modulation)
>
> currentValue = READ_REG(REG_DATAMODUL);
>
> - switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE >> 3) // TODO improvement: change 3 to define
> + switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE)
> {
> case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
> case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
> @@ -203,7 +203,7 @@ int rf69_set_deviation(struct spi_device *spi, u32 deviation)
> lsb = (f_reg&0xff);
>
> // check msb
> - if (msb & !FDEVMASB_MASK)
> + if (msb & ~FDEVMASB_MASK)
> {
> dev_dbg(&spi->dev, "set_deviation: err in calc of msb");
> INVALID_PARAM;
> @@ -366,13 +366,13 @@ int rf69_set_lna_gain(struct spi_device *spi, enum lnaGain lnaGain)
> #endif
>
> switch(lnaGain) {
> - case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_AUTO) );
> - case max: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_MAX) );
> - case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_6) );
> - case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_12) );
> - case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_24) );
> - case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_36) );
> - case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_48) );
> + case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_AUTO) );
> + case max: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_MAX) );
> + case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_6) );
> + case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_12) );
> + case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_24) );
> + case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_36) );
> + case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_48) );
> default: INVALID_PARAM;
> }
> }
looks resonable,
some nitpicking: perhaps you can can improve readability by using a helper like:
static int setstbit(int arg)
{
int get=READ_REG(REG_LNA) & ~MASK_LNA_GAIN;
return WRITE_REG( get| arg);
}
so this switch would be reduced to ...
case maxMinus6: return setstbit(LNA_GAIN_MAX_MINUS_6);
re,
wh
> @@ -387,7 +387,7 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
>
> currentValue = READ_REG(REG_LNA);
>
> - switch (currentValue & MASK_LNA_CURRENT_GAIN >> 3) // improvement: change 3 to define
> + switch (currentValue & MASK_LNA_GAIN)
> {
> case LNA_GAIN_AUTO: return automatic;
> case LNA_GAIN_MAX: return max;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
next prev parent reply other threads:[~2017-07-20 12:22 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-19 18:18 [PATCH] staging: pi433: fix bugs in register abstraction of rf69 chip Wolf Entwicklungen
2017-07-20 12:22 ` walter harms [this message]
2017-07-20 16:08 ` Marcus Wolf
2017-07-20 16:30 ` walter harms
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5970A0A0.8020507@bfs.de \
--to=wharms@bfs.de \
--cc=kernel-janitors@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.