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

[Jonathan Cameron <jic23@kernel.org>]

On 02/09/14 11:30, Laurentiu Palcu wrote:
> 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.
Agreed. Don't rename the driver.  Now, if companies went with naming based on
the interface it might be a good idea.  In reality it is a nightmare so much
better to keep to a name that covers one part and as you say, make it clear
in the Kconfig help what other parts are supported...

Jonathan
> 
> thanks,
> laurentiu
>