Re: [PATCH v3 3/3] iio: accel: BMC150: add support for other Bosch chips

Laurentiu Palcu
2014-09-02T10:30:17Z

On Mon, Sep 01, 2014 at 08:51:36AM -0700, Srinivas Pandruvada wrote:
> On Mon, 2014-09-01 at 08:36 -0700, Joe Perches wrote:
> > On Mon, 2014-09-01 at 12:11 +0300, Laurentiu Palcu wrote:
> > > The following chips are either similar or have only the resolution
> > > different. Hence, change this driver to support these chips too:
> > >
> > > BMI055 - combo chip (accelerometer part is identical to BMC150's)
> > > BMA255 - identical to BMC150's accelerometer
> > > BMA222E - 8 bit resolution
> > > BMA250E - 10 bit resolution
> > > BMA280 - 14 bit resolution
> > >
> > > Additionally:
> > > * add bmc150_accel_match_acpi_device() function to check that the device
> > > has been enumerated through ACPI;
> > > * rename bmc150_accel_acpi_gpio_probe() to bmc150_accel_gpio_probe()
> > > since the ACPI matching has been moved to the new function. Also, this
> > > will allow for the GPIO matching to be done against a device tree too, not only
> > > ACPI tree;
> > []
> > > diff --git a/drivers/iio/accel/bmc150-accel.c b/drivers/iio/accel/bmc150-accel.c
> > []
> > > @@ -647,12 +659,13 @@ static int bmc150_accel_read_raw(struct iio_dev *indio_dev,
> > > {
> > > int i;
> > >
> > > - for (i = 0; i < ARRAY_SIZE(bmc150_accel_scale_table);
> > > - ++i) {
> > > - if (bmc150_accel_scale_table[i].range ==
> > > + for (i = 0;
> > > + i < ARRAY_SIZE(data->chip_info->scale_table);
> > > + ++i) {
> > > + if (data->chip_info->scale_table[i].range ==
> > > data->range) {
> > > *val2 =
> > > - bmc150_accel_scale_table[i].scale;
> > > + data->chip_info->scale_table[i].scale;
> > > return IIO_VAL_INT_PLUS_MICRO;
> > > }
> > > }
> >
> > This looks like it would read a lot better with
> > a temporary for data->chip_info->scale_table[i]
> >
> > so these could become:
> >
> > for (i = 0; i < etc; i++) {
> > type *temp = &data->chip_info->scale_table[i];
> > if (temp->range == data->range) {
> > *val2 = temp->scale;
> > return IIO_VAL_INT_PLUS_MICRO;
> > }
Rewrote this part in v5. Thanks for the suggestion.

> >
> > Maybe all the bmc150_ variable names could be removed.
> > The prefixes don't seem to serve a purpose other than
> > to make the code longer.
> >
> > The filename could be changed to be more generic.
> Then this will also require change in the CONFIG name to match. This
> will require all current users to change the config file once they
> update to new version of the driver, which they don't like to change
> once product config is finalized. Since the most of the chips will just
> differ by a number at the end and they may not be compatible to each
> other, finding a common name will be challenge.
> Instead the CONFIG description for this module should explicitly state
> the names of chips it is compatible to.
v5 contains the changed CONFIG description that includes the new added chips.

thanks,
laurentiu