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@xxxxxxxxxxxxxxx> 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:
signature.asc
Description: Digital signature
