Hi Jonathan and Denis,
Thanks for your review. I'll fix the patch and send back to you the V2 version.
Regards,
Giuseppe.
2015-07-20 5:47 GMT+02:00 Denis Ciocca <denis.ciocca@xxxxxx>:
> Hi Giuseppe, Jonathan,
>
> my comments inline.
>
>
> On 07/19/2015 07:03 PM, Jonathan Cameron wrote:
>>
>> On 16/07/15 10:17, Giuseppe Barba wrote:
>>>
>>> This patch permits to configure the WhoAmI register address
>>> because some device could doesn't have a standard address
>>> for this register.
>>>
>>> Signed-off-by: Giuseppe Barba <giuseppe.barba@xxxxxx>
>>
>> Hmm. I'd be tempted to rename the DEFAULT WAIT ADDRESS as it's clearly
>> only the default in the sense it was there first. Still probably not
>> worth the hassle..
>>
>> Looks good to me, but will leave this (and the rest of the series)
>> on the list for a few days for others to comment.
>>
>> cc'd Denis for comments. Ideally patches should also cc the reviewers
>> listed in MAINTAINERS (I don't care if you cc me as I have filters
>> set to squash stuff that comes to me and the list into one).
>>
>>> drivers/iio/accel/st_accel_core.c | 4 +++
>>> drivers/iio/common/st_sensors/st_sensors_core.c | 44
>>> +++++++++++++++----------
>>> drivers/iio/gyro/st_gyro_core.c | 3 ++
>>> drivers/iio/magnetometer/st_magn_core.c | 2 ++
>>> include/linux/iio/common/st_sensors.h | 2 ++
>>> 5 files changed, 37 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/st_accel_core.c
>>> b/drivers/iio/accel/st_accel_core.c
>>> index 4002e64..7ce027b 100644
>>> --- a/drivers/iio/accel/st_accel_core.c
>>> +++ b/drivers/iio/accel/st_accel_core.c
>>> @@ -226,6 +226,7 @@ static const struct iio_chan_spec
>>> st_accel_16bit_channels[] = {
>>> static const struct st_sensor_settings st_accel_sensors_settings[] = {
>>> {
>>> .wai = ST_ACCEL_1_WAI_EXP,
>>> + .wai_addr = ST_SENSORS_DEFAULT_WAI_ADDRESS,
>>> .sensors_supported = {
>>> [0] = LIS3DH_ACCEL_DEV_NAME,
>>> [1] = LSM303DLHC_ACCEL_DEV_NAME,
>>> @@ -297,6 +298,7 @@ static const struct st_sensor_settings
>>> st_accel_sensors_settings[] = {
>>> },
>>> {
>>> .wai = ST_ACCEL_2_WAI_EXP,
>>> + .wai_addr = ST_SENSORS_DEFAULT_WAI_ADDRESS,
>>> .sensors_supported = {
>>> [0] = LIS331DLH_ACCEL_DEV_NAME,
>>> [1] = LSM303DL_ACCEL_DEV_NAME,
>>> @@ -359,6 +361,7 @@ static const struct st_sensor_settings
>>> st_accel_sensors_settings[] = {
>>> },
>>> {
>>> .wai = ST_ACCEL_3_WAI_EXP,
>>> + .wai_addr = ST_SENSORS_DEFAULT_WAI_ADDRESS,
>>> .sensors_supported = {
>>> [0] = LSM330_ACCEL_DEV_NAME,
>>> },
>>> @@ -437,6 +440,7 @@ static const struct st_sensor_settings
>>> st_accel_sensors_settings[] = {
>>> },
>>> {
>>> .wai = ST_ACCEL_4_WAI_EXP,
>>> + .wai_addr = ST_SENSORS_DEFAULT_WAI_ADDRESS,
>>> .sensors_supported = {
>>> [0] = LIS3LV02DL_ACCEL_DEV_NAME,
>>> },
>>> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c
>>> b/drivers/iio/common/st_sensors/st_sensors_core.c
>>> index 8086cbc..c0a611e 100644
>>> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
>>> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
>>> @@ -479,35 +479,43 @@ int st_sensors_check_device_support(struct iio_dev
>>> *indio_dev,
>>> int num_sensors_list,
>>> const struct st_sensor_settings *sensor_settings)
>>> {
>>> - u8 wai;
>>> int i, n, err;
>>> + u8 wai, wai_addr, found_it;
>>> struct st_sensor_data *sdata = iio_priv(indio_dev);
>>> + found_it = 0;
>>> + for (i = 0; i < num_sensors_list; i++) {
>>> + for (n = 0; n < ST_SENSORS_MAX_4WAI; n++) {
>>> + if (strcmp(indio_dev->name,
>>> + sensor_settings[i].sensors_supported[n])
>>> == 0) {
>>> + found_it = 1;
>>> + break;
>>> + }
>>> + }
>>> + if (found_it)
>>> + break;
>
> found_it is very ugly :). No need to use the variable, if (n !=
> ST_SENSORS_MAX_4WAI) it means sensor found.
>
>>> + }
>>> + if (!found_it) {
>
> here just if (i == num_sensors_list)
>
>>> + dev_err(&indio_dev->dev, "device name %s not
>>> recognized.\n",
>>> + indio_dev->name);
>>> + goto sensor_name_mismatch;
>>> + }
>>> +
>>> + if (sensor_settings[i].wai_addr != 0)
>>> + wai_addr = sensor_settings[i].wai_addr;
>>> + else
>>> + wai_addr = ST_SENSORS_DEFAULT_WAI_ADDRESS;
>
> No need. All sensors must be filled with wai_addr in the static array. You
> forget to fill pressure sensors...
>
>
>>> +
>>> err = sdata->tf->read_byte(&sdata->tb, sdata->dev,
>>> - ST_SENSORS_DEFAULT_WAI_ADDRESS,
>>> &wai);
>>> + wai_addr, &wai);
>>> if (err < 0) {
>>> dev_err(&indio_dev->dev, "failed to read Who-Am-I
>>> register.\n");
>>> goto read_wai_error;
>>> }
>>> - for (i = 0; i < num_sensors_list; i++) {
>>> - if (sensor_settings[i].wai == wai)
>>> - break;
>>> - }
>>> - if (i == num_sensors_list)
>>> + if (sensor_settings[i].wai != wai)
>>> goto device_not_supported;
>>> - for (n = 0; n < ARRAY_SIZE(sensor_settings[i].sensors_supported);
>>> n++) {
>>> - if (strcmp(indio_dev->name,
>>> -
>>> &sensor_settings[i].sensors_supported[n][0]) == 0)
>>> - break;
>>> - }
>>> - if (n == ARRAY_SIZE(sensor_settings[i].sensors_supported)) {
>>> - dev_err(&indio_dev->dev, "device name \"%s\" and WhoAmI
>>> (0x%02x) mismatch",
>>> - indio_dev->name, wai);
>>> - goto sensor_name_mismatch;
>>> - }
>>> -
>>> sdata->sensor_settings =
>>> (struct st_sensor_settings *)&sensor_settings[i];
>>> diff --git a/drivers/iio/gyro/st_gyro_core.c
>>> b/drivers/iio/gyro/st_gyro_core.c
>>> index ffe9664..4b993a5 100644
>>> --- a/drivers/iio/gyro/st_gyro_core.c
>>> +++ b/drivers/iio/gyro/st_gyro_core.c
>>> @@ -131,6 +131,7 @@ static const struct iio_chan_spec
>>> st_gyro_16bit_channels[] = {
>>> static const struct st_sensor_settings st_gyro_sensors_settings[] = {
>>> {
>>> .wai = ST_GYRO_1_WAI_EXP,
>>> + .wai_addr = ST_SENSORS_DEFAULT_WAI_ADDRESS,
>>> .sensors_supported = {
>>> [0] = L3G4200D_GYRO_DEV_NAME,
>>> [1] = LSM330DL_GYRO_DEV_NAME,
>>> @@ -190,6 +191,7 @@ static const struct st_sensor_settings
>>> st_gyro_sensors_settings[] = {
>>> },
>>> {
>>> .wai = ST_GYRO_2_WAI_EXP,
>>> + .wai_addr = ST_SENSORS_DEFAULT_WAI_ADDRESS,
>>> .sensors_supported = {
>>> [0] = L3GD20_GYRO_DEV_NAME,
>>> [1] = LSM330D_GYRO_DEV_NAME,
>>> @@ -252,6 +254,7 @@ static const struct st_sensor_settings
>>> st_gyro_sensors_settings[] = {
>>> },
>>> {
>>> .wai = ST_GYRO_3_WAI_EXP,
>>> + .wai_addr = ST_SENSORS_DEFAULT_WAI_ADDRESS,
>>> .sensors_supported = {
>>> [0] = L3GD20_GYRO_DEV_NAME,
>>> },
>>> diff --git a/drivers/iio/magnetometer/st_magn_core.c
>>> b/drivers/iio/magnetometer/st_magn_core.c
>>> index b4bcfb7..63da293 100644
>>> --- a/drivers/iio/magnetometer/st_magn_core.c
>>> +++ b/drivers/iio/magnetometer/st_magn_core.c
>>> @@ -268,6 +268,7 @@ static const struct st_sensor_settings
>>> st_magn_sensors_settings[] = {
>>> },
>>> {
>>> .wai = ST_MAGN_1_WAI_EXP,
>>> + .wai_addr = ST_SENSORS_DEFAULT_WAI_ADDRESS,
>>> .sensors_supported = {
>>> [0] = LSM303DLHC_MAGN_DEV_NAME,
>>> [1] = LSM303DLM_MAGN_DEV_NAME,
>>> @@ -346,6 +347,7 @@ static const struct st_sensor_settings
>>> st_magn_sensors_settings[] = {
>>> },
>>> {
>>> .wai = ST_MAGN_2_WAI_EXP,
>>> + .wai_addr = ST_SENSORS_DEFAULT_WAI_ADDRESS,
>>> .sensors_supported = {
>>> [0] = LIS3MDL_MAGN_DEV_NAME,
>>> },
>>> diff --git a/include/linux/iio/common/st_sensors.h
>>> b/include/linux/iio/common/st_sensors.h
>>> index 2c476ac..f7c77b4 100644
>>> --- a/include/linux/iio/common/st_sensors.h
>>> +++ b/include/linux/iio/common/st_sensors.h
>>> @@ -166,6 +166,7 @@ struct st_sensor_transfer_function {
>>> /**
>>> * struct st_sensor_settings - ST specific sensor settings
>>> * @wai: Contents of WhoAmI register.
>>> + * @wai_addr: The address of WhoAmI register
>
> Forget . at the end.
>
>>> * @sensors_supported: List of supported sensors by struct itself.
>>> * @ch: IIO channels for the sensor.
>>> * @odr: Output data rate register and ODR list available.
>>> @@ -179,6 +180,7 @@ struct st_sensor_transfer_function {
>>> */
>>> struct st_sensor_settings {
>>> u8 wai;
>>> + u8 wai_addr;
>>> char sensors_supported[ST_SENSORS_MAX_4WAI][ST_SENSORS_MAX_NAME];
>>> struct iio_chan_spec *ch;
>>> int num_ch;
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
