Re: [PATCH v3 1/2] lis3: add generic DT matching code

[Daniel Mack <zonque@gmail.com>]

On 08.08.2012 07:19, AnilKumar, Chimata wrote:
> Hi Mack,

Call me Daniel :)

> On Wed, Aug 08, 2012 at 00:19:01, Daniel Mack wrote:
>> On 06.08.2012 12:45, AnilKumar, Chimata wrote:
>>> On Sun, Aug 05, 2012 at 21:48:42, Daniel Mack wrote:
>>>> On 30.07.2012 09:36, Daniel Mack wrote:
>>>>> This patch adds logic to parse lis3 properties from a device tree node
>>>>> and store them in a freshly allocated lis3lv02d_platform_data.
>>>>>
>>>>> Note that the actual match tables are left out here. This part should
>>>>> happen in the drivers that bind to the individual busses (SPI/I2C/PCI).
>>>>>
>>>>> Also adds some DT bindinds documentation.
>>>>>
>>>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>>>> ---
>>>>> Changes from v2:
>>>>>  - kzalloc braino
>>>>>
>>>>> Changes from v1:
>>>>>  - some typos in properties fixed
>>>>>
>>>>>
>>>>>  Documentation/devicetree/bindings/misc/lis302.txt |  74 ++++++++++++
>>>>>  drivers/misc/lis3lv02d/lis3lv02d.c                | 137 ++++++++++++++++++++++
>>>>>  drivers/misc/lis3lv02d/lis3lv02d.h                |   4 +
>>>>>  3 files changed, 215 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/misc/lis302.txt
>>>>>

[...]

>>>>> +Example for a SPI device node:
>>>>> +
>>>>> +	lis302@0 {
>>>>> +		compatible = "st,lis302dl-spi";
>>>>> +		reg = <0>;
>>>>> +		spi-max-frequency = <1000000>;
>>>>> +		interrupt-parent = <&gpio>;
>>>>> +		interrupts = <104 0>;
>>>>> +
>>>>> +		st,click-single-x;
>>>>> +		st,click-single-y;
>>>>> +		st,click-single-z;
>>>>> +		st,click-thresh-x = <10>;
>>>>> +		st,click-thresh-y = <10>;
>>>>> +		st,click-thresh-z = <10>;
>>>>> +		st,irq1-click;
>>>>> +		st,irq2-click;
>>>>> +		st,wakeup-x-lo;
>>>>> +		st,wakeup-x-hi;
>>>>> +		st,wakeup-y-lo;
>>>>> +		st,wakeup-y-hi;
>>>>> +		st,wakeup-z-lo;
>>>>> +		st,wakeup-z-hi;
>>>>> +	};
>>>
>>> Why can't we add these flags in driver itself like below?
>>> Is these parameters varies from SoC to SoC or accelerometer
>>> - to - accelerometer?
>>
>> I don't understand, sorry. The point here is that the driver that is
>> probed for device initialization are the PCI/I2C/SPI drivers. The
> 
> Look at the below example it has different drivers (SPI, I2C).
> Compatible name changes form acc-acc so that we can use different
> parameters corresponding to accelerometer, if it is acce specific.
> 
> Like
> .compatible = "st,lis302dl-spi",
> .compatible = "st,lis331dlh-i2c",
> .compatible = "st,xx-spi",
> .compatible = "st,xx-i2c",
> 
> If we do like this we can reduce lot of DT reads in driver.

No, we can't. Look, this chip has a huge number of registers that can be
set in order to accomplish a variety of different functions. It can be
used as a free-fall detector or as a full-fledged accelerometer, with
different settings per axis. None of them is specific to the bus this
chip is connected through.

Of course we want to leave it to the board vendor what functions to use,
and hence all of them are exposed via DT. The whole idea of DT is to
describe the hardware and the desired behaviour of each component as
exact as possible, so vendors don't have to hack along in the source
code but can boot a generic kernel.

[...]

>>> #ifdef CONFIG_OF
>>> static struct lis3lv02d_platform_data lis302dl_spi_pdata = {
>>>         .click_flags    = LIS3_CLICK_SINGLE_X |
>>>                           LIS3_CLICK_SINGLE_Y |
>>>                           LIS3_CLICK_SINGLE_Z,
>>>         .irq_cfg        = LIS3_IRQ1_CLICK | LIS3_IRQ2_CLICK,
>>>         .wakeup_flags   = LIS3_WAKEUP_X_LO | LIS3_WAKEUP_X_HI |
>>>                           LIS3_WAKEUP_Y_LO | LIS3_WAKEUP_Y_HI |
>>>                           LIS3_WAKEUP_Z_LO | LIS3_WAKEUP_Z_HI,
>>> };
>>>
>>> static struct lis3lv02d_platform_data lis331dlh_i2c_pdata = {
>>>         .click_flags    = LIS3_CLICK_SINGLE_X |
>>>                           LIS3_CLICK_SINGLE_Y |
>>>                           LIS3_CLICK_SINGLE_Z,
>>>         .irq_cfg        = LIS3_IRQ1_CLICK | LIS3_IRQ2_CLICK,
>>>         .wakeup_flags   = LIS3_WAKEUP_X_LO | LIS3_WAKEUP_X_HI |
>>>                           LIS3_WAKEUP_Y_LO | LIS3_WAKEUP_Y_HI |
>>>                           LIS3_WAKEUP_Z_LO | LIS3_WAKEUP_Z_HI,
>>> };
>>>
>>> static const struct of_device_id lis3_of_match[] = {
>>>        {
>>>                .compatible = "st,lis302dl-spi",
>>>                .data = &lis302dl_spi_pdata,
>>>        },
>>>        {
>>>                .compatible = "st,lis331dlh-i2c",
>>>                .data = &lis331dlh_i2c_pdata,
>>>        },
>>>        { },
>>> };
>>> MODULE_DEVICE_TABLE(of, lis3_of_match);
>>> #endif
>>>
>>> Ignore if parameters between SoC - SoC are different. In
>>> probe we can add these flags to pdata.
>>
>> No. We want to expose all hardware features to DT so users can configure
>> the device at wish. We can't ignore that SoCs want different device configs.
> 
> Is it require is my question, how many SoCs take these different
> parameters from platform data.

We can't know, and it doesn't matter. The goal here is not to provide a
way to boot the boards that are currently in mainline via DT with
minimal effort, but to have DT bindings for this driver that reflect the
features of this piece of hardware, so anything is possible.


Regards,
Daniel