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

[Laurentiu Palcu]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/