From: Denis CIOCCA <denis.ciocca@st.com>
To: Lee Jones <lee.jones@linaro.org>
Cc: "linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "jic23@cam.ac.uk" <jic23@cam.ac.uk>, "arnd@arndb.de" <arnd@arndb.de>,
        "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>
Date: Thu, 5 Sep 2013 10:35:35 +0200
Subject: Re: [PATCH 06/11] iio: pressure-core: st: Expand and rename
 LPS331AP's channel descriptor
Thread-Topic: [PATCH 06/11] iio: pressure-core: st: Expand and rename
 LPS331AP's channel descriptor
Thread-Index: Ac6qEuVgfjaz93WATZaVcnj3LNfIaA==
Message-ID: <52284257.5070802@st.com>
References: <1378287103-21765-1-git-send-email-lee.jones@linaro.org>
 <1378287103-21765-7-git-send-email-lee.jones@linaro.org>
 <522794F1.90301@st.com> <20130905072114.GD8980@lee--X1>
 <52283367.4070603@st.com> <20130905075953.GF8980@lee--X1>
In-Reply-To: <20130905075953.GF8980@lee--X1>
Accept-Language: it-IT, en-US
Content-Language: en-US
user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804
 Thunderbird/17.0.8
acceptlanguage: it-IT, en-US
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Transfer-Encoding: 8bit
Content-Length: 5572
Lines: 119

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.jones@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];
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?