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

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

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;
> 				}
> 
> 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.

Thanks,
Srinivas
> 
>