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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).