From: Dan Robertson <dan@dlrobertson.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
linux-iio <linux-iio@vger.kernel.org>,
Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
devicetree <devicetree@vger.kernel.org>,
Hartmut Knaack <knaack.h@gmx.de>,
Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Randy Dunlap <rdunlap@infradead.org>
Subject: Re: [PATCH v4 2/2] iio: (bma400) add driver for the BMA400
Date: Sat, 19 Oct 2019 02:43:51 +0000 [thread overview]
Message-ID: <20191019024351.GB8593@nessie> (raw)
In-Reply-To: <CAHp75VfMW0fvmO9jGTnQumJ9Sm-SgNL0ohjSR4qRQY365aeMBw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5354 bytes --]
On Fri, Oct 18, 2019 at 10:23:38AM +0300, Andy Shevchenko wrote:
> On Fri, Oct 18, 2019 at 6:44 AM Dan Robertson <dan@dlrobertson.com> wrote:
> > + * bma400.h - Register constants and other forward declarations
> > + * needed by the bma400 sources.
>
> Including file name in the file is not the best practice. Imagine if
> by some reason we will need to rename it (to support more sensors, for
> example, and reflect it by replacing 00 -> 0x).
> So, please, remove here and everywhere else.
That makes sense.
> > +#define BMA400_TWO_BITS_MASK 0x03
> > +#define BMA400_LP_OSR_MASK 0x60
> > +#define BMA400_NP_OSR_MASK 0x30
> > +#define BMA400_ACC_ODR_MASK 0x0f
> > +#define BMA400_ACC_SCALE_MASK 0xc0
>
> GENMASK()
> (Don't forget to include bits.h for it)
Thanks.
> > +static const int bma400_scale_table[] = {
> > + 0, 38344,
> > + 0, 76590,
> > + 0, 153277,
>
> > + 0, 306457
>
> Better to leave comma here. It doesn't matter for this device, but
> make of use the better practices.
> > +};
>
> Also, I'm wondering why values are not exactly multiply by 2. Is in DS
> of the chip any explanation for this?
It would be a multiply by 2. I tried to follow the bma180 driver here, but I'm
starting to think that may be the wrong approach.
> > +static const int bma400_osr_table[] = { 0, 1, 3 };
>
> > +/* See the ACC_CONFIG1 section of the datasheet */
> > +static const int bma400_sample_freqs[] = {
> > + 12, 500000,
> > + 25, 0,
> > + 50, 0,
> > + 100, 0,
> > + 200, 0,
> > + 400, 0,
> > + 800, 0,
> > +};
>
> This can be replaced by a formula(s).
Yeah I think I can implement the get, set, and read functions for sample_freq
with a formula, but the scale and sample frequency tables are needed by the
implementation of read_avail. A implementation of read_avail with a range and
a step would be ideal, but I couldn't find any documentation on implementing
read_avail where the step value of the range is a multiple. Please correct
me if I've missed something.
Note that this applies to the scale table as well.
> > +struct bma400_sample_freq {
> > + int hz;
> > + int uhz;
> > +};
>
> I'm wondering why above table is not using this struct.
Originally it did, but I changed this in the second version when I added support
for iio_info read_avail to try to be a little closer to other implementations of
iio_read avail.
> > +const struct regmap_config bma400_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = BMA400_CMD_REG,
> > + .cache_type = REGCACHE_RBTREE,
> > + .writeable_reg = bma400_is_writable_reg,
> > + .volatile_reg = bma400_is_volatile_reg,
> > +};
>
> > +EXPORT_SYMBOL(bma400_regmap_config);
>
> Why? And why it's not _GPL?
This is used by the bma400_i2c module.
> > + int ret;
> > + int host_temp;
> > + unsigned int raw_temp;
>
> Better reversed xmas tree order.
Sounds good.
>
> > + if (idx + 1 >= ARRAY_SIZE(bma400_sample_freqs)) {
>
> Why do you need this churn with +1 and = ?
Since we've "flattened" the array of sample frequency we need to ensure that the
Hz (bma400_sample_freqs[idx]) and uHz (bma400_sample_freqs[idx + 1]) are both
valid. This will be negated in the next version as I'll switch to a formula.
Instead I'll ensure the returned ODR value is not above 0x0b.
> > + dev_err(data->dev, "sample freq index is too high");
> > + ret = -EINVAL;
> > + goto error;
> > + }
>
>
> > + for (i = 0; i + 1 < ARRAY_SIZE(bma400_sample_freqs); i += 2) {
>
> Using defined struct will guarantee you to have always 2x members in
> the array. So, drop this arithmetic churn.
I should be able to figure out how to use a formula here, but I see where you're
coming from and I agree.
> > + if (ret < 0) {
> > + dev_err(data->dev, "Failed to read chip id register: %x!", ret);
>
> %x for returned error code is too hackerish.
Makes sense. I'll change this in the update.
> > + return ret;
>
> > + } else if (val != BMA400_ID_REG_VAL) {
>
> Redundant 'else'
> > + dev_err(data->dev, "CHIP ID MISMATCH: %x!", ret);
>
> Hacker detected!
:)
> > + return -ENODEV;
> > + }
>
> > + /*
> > + * TODO: The datasheet waits 1500us here in the example, but
> > + * lists 2/ODR as the wakeup time.
> > + */
> > + usleep_range(1500, 20000);
>
> These range values are too sparse. Usually the second one is less than
> first one * 2.
> Fix it now.
Good to know. I'll fix this in the update.
> > +EXPORT_SYMBOL(bma400_probe);
>
> Why is not GPL?
Ah, saw in the docs "GPL" means GPL-2.0.
>
> > +EXPORT_SYMBOL(bma400_remove);
>
> Ditto.
This symbol is used in bma400_i2c.
>
> P.S. I probably missed some places with the same mistake as commented
> above. Please address all places in the code where my comments are
> applicable.
Noted. Thanks for the feedback!
Cheers,
- Dan
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2019-10-19 2:58 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-18 3:18 [PATCH v3 0/2] iio: add driver for Bosch BMA400 accelerometer Dan Robertson
2019-10-18 3:18 ` [PATCH v4 1/2] dt-bindings: iio: accel: bma400: add bindings Dan Robertson
2019-10-25 16:35 ` Rob Herring
2019-10-18 3:18 ` [PATCH v4 2/2] iio: (bma400) add driver for the BMA400 Dan Robertson
2019-10-18 4:25 ` Randy Dunlap
2019-10-19 1:35 ` Dan Robertson
2019-10-18 7:23 ` Andy Shevchenko
2019-10-19 2:43 ` Dan Robertson [this message]
2019-10-21 15:20 ` Jonathan Cameron
2019-11-18 0:25 ` Dan Robertson
2019-11-23 12:51 ` Jonathan Cameron
2019-11-24 22:37 ` Dan Robertson
2019-10-19 4:25 ` Joe Perches
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=20191019024351.GB8593@nessie \
--to=dan@dlrobertson.com \
--cc=andy.shevchenko@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=pmeerw@pmeerw.net \
--cc=rdunlap@infradead.org \
--cc=robh+dt@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).