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

From: Daniel Mack
Date: Tue Aug 07 2012 - 14:49:06 EST

Next message: Konrad Rzeszutek Wilk: "Re: [PATCH v2 23/23] [HACK] xen/arm: implementxen_remap_domain_mfn_range"
Previous message: Maarten Lankhorst: "Re: [PATCH 1/3] dma-fence: dma-buf synchronization (v7)"
In reply to: AnilKumar, Chimata: "RE: [PATCH v3 1/2] lis3: add generic DT matching code"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

This is a multi-part message in MIME format.Hi,

thanks for your review.

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@xxxxxxxxx>
>>> ---
>>> 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
>>>
>>> diff --git a/Documentation/devicetree/bindings/misc/lis302.txt b/Documentation/devicetree/bindings/misc/lis302.txt
>>> new file mode 100644
>>> index 0000000..66230fd
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/misc/lis302.txt
>>> @@ -0,0 +1,74 @@
>>> +LIS302 accelerometer devicetree bindings
>>> +
>>> +This device is matched via its bus drivers, and has a number of properties
>>> +that apply in on the generic device (independent from the bus).
>>> +
>>> +
>>> +Required properties for the SPI bindings:
>>> + - compatible: should be set to "st,lis3lv02d_spi"
>>> + - reg: the chipselect index
>>> + - spi-max-frequency: maximal bus speed, should be set to 1000000 unless
>>> + constrained by external circuitry
>>> + - interrupts: the interrupt generated by the device
>>> +
>>> +
>>> +Optional properties for all bus drivers:
>>> +
>>> + - st,click-single-{x,y,z}: if present, tells the device to issue an
>>> + interrupt on single click events on the
>>> + x/y/z axis.
>>> + - st,click-double-{x,y,z}: if present, tells the device to issue an
>>> + interrupt on double click events on the
>>> + x/y/z axis.
>>> + - st,click-thresh-{x,y,z}: set the x/y/z axis threshold
>>> + - st,click-click-time-limit: click time limit, from 0 to 127.5msec
>>> + with step of 0.5 msec
>>> + - st,click-latency: click latency, from 0 to 255 msec with
>>> + step of 1 msec.
>>> + - st,click-window: click window, from 0 to 255 msec with
>>> + step of 1 msec.
>>> + - st,irq{1,2}-disable: disable IRQ 1/2
>>> + - st,irq{1,2}-ff-wu-1: raise IRQ 1/2 on FF_WU_1 condition
>>> + - st,irq{1,2}-ff-wu-2: raise IRQ 1/2 on FF_WU_2 condition
>>> + - st,irq{1,2}-data-ready: raise IRQ 1/2 on data ready contition
>>> + - st,irq{1,2}-click: raise IRQ 1/2 on click condition
>>> + - st,irq-open-drain: consider IRQ lines open-drain
>>> + - st,irq-active-low: make IRQ lines active low
>>> + - st,wu-duration-1: duration register for Free-Fall/Wake-Up
>>> + interrupt 1
>>> + - st,wu-duration-2: duration register for Free-Fall/Wake-Up
>>> + interrupt 2
>>> + - st,wakeup-{x,y,z}-{lo,hi}: set wakeup condition on x/y/z axis for
>>> + upper/lower limit
>>> + - st,highpass-cutoff-hz=: 1, 2, 4 or 8 for 1Hz, 2Hz, 4Hz or 8Hz of
>>> + highpass cut-off frequency
>>> + - st,hipass{1,2}-disable: disable highpass 1/2.
>>> + - st,default-rate=: set the default rate
>>> + - st,axis-{x,y,z}=: set the axis to map to the three coordinates
>
> Some more parameters missing, what about st_min_limits and st_max_limits
> required for selftest.

Right. Added them now.

>>> +
>>> +
>>> +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
generic part is not something the device tree knows about.

Hence I put the generic parsing of common DT bindings to the generic
part of the driver, and made the SPI driver just pass through the
of_node pointer.

> #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.

>>> +
>>> diff --git a/drivers/misc/lis3lv02d/lis3lv02d.c b/drivers/misc/lis3lv02d/lis3lv02d.c
>>> index a981e2a..1411fdc 100644
>>> --- a/drivers/misc/lis3lv02d/lis3lv02d.c
>>> +++ b/drivers/misc/lis3lv02d/lis3lv02d.c
>>> @@ -39,6 +39,7 @@
>>> #include <linux/miscdevice.h>
>>> #include <linux/pm_runtime.h>
>>> #include <linux/atomic.h>
>>> +#include <linux/of_device.h>
>>> #include "lis3lv02d.h"
>>>
>>> #define DRIVER_NAME "lis3lv02d"
>>> @@ -912,6 +913,138 @@ static void lis3lv02d_8b_configure(struct lis3lv02d *lis3,
>>> }
>>> }
>>>
>>> +#ifdef CONFIG_OF
>>> +static int lis3lv02d_init_dt(struct lis3lv02d *lis3)
>>> +{
>>> + struct lis3lv02d_platform_data *pdata;
>>> + struct device_node *np = lis3->of_node;
>>> + u32 tmp;
>
> Can you use some better name than tmp, if require?

Yes, done.

[...]

>>> + if (of_get_property(np, "st,axis-x", NULL))
>
> &tmp missed here.
>
>>> + pdata->axis_x = tmp;
>>> + if (of_get_property(np, "st,axis-y", NULL))
>
> &tmp missed here.

True, thanks.

I fixed all these issues now and attached a v4.

Best regards,
Daniel

Next message: Konrad Rzeszutek Wilk: "Re: [PATCH v2 23/23] [HACK] xen/arm: implementxen_remap_domain_mfn_range"
Previous message: Maarten Lankhorst: "Re: [PATCH 1/3] dma-fence: dma-buf synchronization (v7)"
In reply to: AnilKumar, Chimata: "RE: [PATCH v3 1/2] lis3: add generic DT matching code"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]