Lee, I got your point. For me is ok...
Denis
> On Thu, 05 Sep 2013, Denis CIOCCA wrote:
>
>>>>> Due to the MACRO used, the task of reading, understanding and maintaining
>>>>> the LPS331AP's channel descriptor is substantially difficult. This patch
>>>>> is based on the view that it's better to have easy to read, maintainable
>>>>> code than to save a few lines here and there. For that reason we're
>>>>> expanding the array so initialisation is completed in full.
>>>> Also for this one, the channel names are general and can be shared
>>>> between different sensors. For the channel definition it's not a problem
>>>> for me, but I think it's not necessary adds all that code...
>>> I'm not sure what you mean by this. Would you be kind enough to
>>> explain it in a different way please?
>> The channel name (in this case st_press_channels) is not only specific
>> for one sensor but can be shared. Ok in this driver now is used only for
>> the lps331ap but for example in accelerometer driver is used by several
>> sensors. It's possible in the future for new pressure sensors use the
>> same channels definition.
> Ah yes I see what you mean. Well as you say, for the moment, as
> they're separated, this naming convention seems the most
> appropriate. If we add anymore devices which share the definition, we
> can pick the best naming convention for the situation I think. For
> instance, I like that you've split the channels up into the number of
> bits they support in the gyro and accel cases, so something of that
> nature could be utilised if other device support is added.
>
>> The channel definition is intended the switch by macro
>> ST_SENSORS_LSM_CHANNELS to the full definition, for me is not a problem
>> but I think it's not necessary.
> If you are familiar with the macro I guess you could get used to
> working with it, but coming from in as a first time reader, adding a
> new device was pretty difficult. I had to look up the macro in the
> header file, then have the original struct open too and cross
> reference in 3 different places. It's made even more difficult by the
> macro being in a different order to the original struct.
>
> Now I've had time to work with it, I could probably work with it as
> well. I was just thinking about helping out any new person that comes
> along and tries to add support for a new sensor.
>
>>>>> Signed-off-by: Lee Jones <lee.jo...@linaro.org>
>>>>> ---
>>>>> drivers/iio/pressure/st_pressure_core.c | 45
>>>>> +++++++++++++++++++++++++--------
>>>>> 1 file changed, 34 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/pressure/st_pressure_core.c
>>>>> b/drivers/iio/pressure/st_pressure_core.c
>>>>> index becfb25..7ba9299 100644
>>>>> --- a/drivers/iio/pressure/st_pressure_core.c
>>>>> +++ b/drivers/iio/pressure/st_pressure_core.c
>>>>> @@ -58,16 +58,39 @@
>>>>> #define ST_PRESS_LPS331AP_OUT_XL_ADDR 0x28
>>>>> #define ST_TEMP_LPS331AP_OUT_L_ADDR 0x2b
>>>>>
>>>>> -static const struct iio_chan_spec st_press_channels[] = {
>>>>> - ST_SENSORS_LSM_CHANNELS(IIO_PRESSURE,
>>>>> +static const struct iio_chan_spec st_press_lsp331ap_channels[] = {
>>>>> + {
>>>>> + .type = IIO_PRESSURE,
>>>>> + .channel2 = IIO_NO_MOD,
>>>>> + .address = ST_PRESS_LPS331AP_OUT_XL_ADDR,
>>>>> + .scan_index = ST_SENSORS_SCAN_X,
>>>>> + .scan_type = {
>>>>> + .sign = 'u',
>>>>> + .realbits = 24,
>>>>> + .storagebits = 24,
>>>>> + .endianness = IIO_LE,
>>>>> + },
>>>>> + .info_mask_separate =
>>>>> BIT(IIO_CHAN_INFO_RAW) |
>>>>> BIT(IIO_CHAN_INFO_SCALE),
>>>>> - ST_SENSORS_SCAN_X, 0, IIO_NO_MOD, 'u', IIO_LE, 24, 24,
>>>>> - ST_PRESS_LPS331AP_OUT_XL_ADDR),
>>>>> - ST_SENSORS_LSM_CHANNELS(IIO_TEMP,
>>>>> - BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) |
>>>>> - BIT(IIO_CHAN_INFO_OFFSET),
>>>>> - -1, 0, IIO_NO_MOD, 's', IIO_LE, 16, 16,
>>>>> - ST_TEMP_LPS331AP_OUT_L_ADDR),
>>>>> + .modified = 0,
>>>>> + },
>>>>> + {
>>>>> + .type = IIO_TEMP,
>>>>> + .channel2 = IIO_NO_MOD,
>>>>> + .address = ST_TEMP_LPS331AP_OUT_L_ADDR,
>>>>> + .scan_index = -1,
>>>>> + .scan_type = {
>>>>> + .sign = 'u',
>>>>> + .realbits = 16,
>>>>> + .storagebits = 16,
>>>>> + .endianness = IIO_LE,
>>>>> + },
>>>>> + .info_mask_separate =
>>>>> + BIT(IIO_CHAN_INFO_RAW) |
>>>>> + BIT(IIO_CHAN_INFO_SCALE) |
>>>>> + BIT(IIO_CHAN_INFO_OFFSET),
>>>>> + .modified = 0,
>>>>> + },
>>>>> IIO_CHAN_SOFT_TIMESTAMP(1)
>>>>> };
>>>>>
>>>>> @@ -77,7 +100,7 @@ static const struct st_sensors st_press_sensors[] = {
>>>>> .sensors_supported = {
>>>>> [0] = LPS331AP_PRESS_DEV_NAME,
>>>>> },
>>>>> - .ch = (struct iio_chan_spec *)st_press_channels,
>>>>> + .ch = (struct iio_chan_spec *)st_press_lsp331ap_channels,
>>>>> .odr = {
>>>>> .addr = ST_PRESS_LPS331AP_ODR_ADDR,
>>>>> .mask = ST_PRESS_LPS331AP_ODR_MASK,
>>>>> @@ -214,7 +237,7 @@ int st_press_common_probe(struct iio_dev *indio_dev)
>>>>> pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
>>>>> pdata->multiread_bit = pdata->sensor->multi_read_bit;
>>>>> indio_dev->channels = pdata->sensor->ch;
>>>>> - indio_dev->num_channels = ARRAY_SIZE(st_press_channels);
>>>>> + indio_dev->num_channels = ARRAY_SIZE(st_press_lsp331ap_channels);
>>>>>
>>>>> pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
>>>>>
>>>>> &pdata->sensor->fs.fs_avl[0];
