Message-ID: <20121122154612.GC10986@gmail.com>
Date:	Thu, 22 Nov 2012 15:46:12 +0000
From:	Lee Jones <lee.jones@...aro.org>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	sameo@...ux.intel.com, devicetree-discuss@...ts.ozlabs.org,
	linux-kernel@...r.kernel.org, spear-devel@...t.st.com,
	Vipul Kumar Samar <vipulkumar.samar@...com>
Subject: Re: [PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver

> > Big fat NACK.
> >
> > You've just overwritten the current implementation with your own.
> > Please take time to understand the mechanisms in place before
> > you submit any changes or additions to it.
> 
> :)
> 
> My fault. Comments on all overwritten stuff accepted

Okay, great.

> >> diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
> >> +- irq-over-gpio: bool, true if gpio is used to get irq
> >> +- irq-gpios: gpio number over which irq will be requested (significant only if
> >> +  irq-over-gpio is true)
> >
> > You don't need these. Use gpio_to_irq() instead.
> 
> I am passing gpio numbers here and am doing gpio_to_irq() in driver.
> Didn't get this one :(

For a start you have 'irq-over-gpio' in the binding document and Device Tree
and 'irq_over_gpio' in the code. Has it even been tested?

GPIOs are used as IRQ lines in many other previously DT:ed drivers. Take a
look to see how they are handled without adding unnecessary DT bindings. 

> >>  Optional properties:
> >> - - interrupts                   : The interrupt outputs from the controller
> >> - - interrupt-controller         : Marks the device node as an interrupt controller
> >> - - interrupt-parent             : Specifies which IRQ controller we're connected to
> >> - - i2c-client-wake              : Marks the input device as wakable
> >> - - st,autosleep-timeout         : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024
> >
> > And you've removed these why?
> 
> No. They are readjusted...

Please don't readjust them.

They are clear and easily readable in their current state. In the new
implementation they are messy and hard to decipher.

> One thing removed is interrupt-controller. I had a doubt on this.
> stmpe, by itself doesn't give any interrupt lines to SoC to freely use
> them. Instead
> gpio controller driver part of it does. And so adding
> interrupt-controller for that is
> the right option.

That's fine, I don't have an issue with that.

> stmpe is an interrupt controller for the IP's which are present inside
> it: gpio, adc.
> But interrupt lines for them are managed by stmpe driver internally. So should
> we really add interrupt-controller for it?

You can't manage IRQ lines internally, you have to go through
the IRQ subsystem. When you request an IRQ via device tree you
will do so like this:

 interrupts = <25 0x1>;
 interrupt-parent = <&stmpe-gpio>;

In this example the stmpe-gpio node must advertise itself using
the interrupt-controller property.

The STMPE GPIO controller can't be used by Device Tree yet in any case,
because it doesn't have an IRQ domain. This is compulsory, or it won't
work. Have you tried to test this functionality yet?

> >> +- keypad,scan-count: number of key scanning cycles to confirm key data. Maximum
> >> +  is STMPE_KEYPAD_MAX_SCAN_COUNT.
> >> +- keypad,debounce-ms: debounce interval, in ms. Maximum is
> >> +  STMPE_KEYPAD_MAX_DEBOUNCE.
> >> +- keypad,no-autorepeat: bool, disable key autorepeat
> >
> > See "When adding new bindings, ask yourself" above.
> 
> Yes, these are required. This is part of platform data it expects.

Okay, I've just had a look at my tested bindings.

We already have these:

  debounce-interval   /* This is a generic binding */
  st,scan-count       /* These are vendor specific */
  st,no-autorepeat    /*            "              */

Vendor specific bindings should be "<vendor>,<binding>", rather than
"<type_of_driver>,<binding>"

> >> +             reg = <0>;
> >
> > You have reg twice here. Also reg should never be '0'.
> 
> For SPI, there are chip selects and there is no reg offset.

I understand the addressing issues, but you have 'reg' twice.

Which one should be used?

> >> +             stmpe610-ts {
> >> +                     compatible = "stmpe,ts";
> >> +                     ts,sample-time = <4>;
> >> +                     ts,mod-12b = <1>;
> >> +                     ts,ref-sel = <0>;
> >> +                     ts,adc-freq = <1>;
> >> +                     ts,ave-ctrl = <1>;
> >> +                     ts,touch-det-delay = <2>;
> >> +                     ts,settling = <2>;
> >> +                     ts,fraction-z = <7>;
> >> +                     ts,i-drive = <1>;
> >
> > Wow! See "When adding new bindings, ask yourself" above.
> 
> :)
> They are required. I didn't get your point, sorry.

I didn't go through them, but are you sure that:

1. Can I do without them?
   1.1 Can I derive the configuration from other things?
   2.2 Are they _really_ required, or am I just blindly copying platform data?
2. Does a similar binding already exist?
3. Can other drivers make use of them?
   3.1 If so, create a generic binding
   3.2 If not, prepend the binding with "<vendor>,"

There looks like a lot there. So each of those values vary, or can
any of them be hard-coded? Just because they're in platform code,
it doesn't mean they should be in the DT binding. A lot of platform
code is actually crap.

> >> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> >> +static struct stmpe_keypad_platform_data *
> >> +get_keyboard_pdata_dt(struct device *dev, struct device_node *np)
> >> +{
> >> +     struct stmpe_keypad_platform_data *pdata;
> >> +     u32 val;
> >> +
> >> +     pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> >> +     if (!pdata) {
> >> +             dev_warn(dev, "stmpe keypad kzalloc fail\n");
> >> +             return NULL;
> >> +     }
> >> +
> >> +     if (!of_property_read_u32(np, "keypad,scan-count", &val))
> >> +             pdata->scan_count = val;
> >> +     if (!of_property_read_u32(np, "keypad,debounce-ms", &val))
> >> +             pdata->debounce_ms = val;
> >> +     if (of_property_read_bool(np, "keypad,no-autorepeat"))
> >> +             pdata->no_autorepeat = true;
> >
> > Why are you (re)adding these here? Have you even looked in the driver?
> 
> Because i wanted to keep all DT stuff together. Obviously i have seen keypad
> driver earlier :)

If you had, you'd realise that these bindings already exist. ;)

I appreciate that the patches haven't landed yet, but they've been
on the MLs for some time now, and already have some important Acks.

> I am not setting pdata of stmpe here, but pdata of keypad.

Exactly, so why are you doing it in the STMPE driver?

It's keypad data, set it up in the keypad driver.

> >> +static struct stmpe_gpio_platform_data *get_gpio_pdata_dt(struct device *dev,
> >> +             struct device_node *np)
> >> +{
> >> +     struct stmpe_gpio_platform_data *pdata;
> >> +     u32 val;
> >> +
> >> +     pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> >> +     if (!pdata) {
> >> +             dev_warn(dev, "stmpe gpio kzalloc fail\n");
> >> +             return NULL;
> >> +     }
> >> +
> >> +     if (!of_property_read_u32(np, "gpio,norequest-mask", &val))
> >> +             pdata->norequest_mask = val;
> >> +
> >> +     /* assign gpio numbers dynamically */
> >> +     pdata->gpio_base = -1;
> >> +
> >> +     return pdata;
> >> +}
> >
> > Is this function really required? Even if is is, should it live here
> > or in the STMPE driver?
> 
> As said earlier, either i can do DT parsing of sub-modules of stmpe in
> their specific files or in stmpe driver itself. Because currently platform
> data of those sub-modules is passed from stmpe, i kept them here only.
> stmpe sub modules can't live without stmpe driver and so keeping all
> binding stuff here isn't that bad of an idea.

Yes, don't do that.

Do DT parsing in the driver it references, not in the most common denominator.

> >> +static struct stmpe_ts_platform_data *get_ts_pdata_dt(struct device *dev,
> >> +             struct device_node *np)
> >> +{
> >> +     struct stmpe_ts_platform_data *pdata;
> >> +     u32 val;
> >> +
> >> +     pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> >> +     if (!pdata) {
> >> +             dev_warn(dev, "stmpe ts kzalloc fail\n");
> >> +             return NULL;
> >> +     }
> >> +
> >> +     if (!of_property_read_u32(np, "ts,sample-time", &val))
> >> +             pdata->sample_time = val;
> >> +     if (!of_property_read_u32(np, "ts,mod-12b", &val))
> >> +             pdata->mod_12b = val;
> >> +     if (!of_property_read_u32(np, "ts,ref-sel", &val))
> >> +             pdata->ref_sel = val;
> >> +     if (!of_property_read_u32(np, "ts,adc-freq", &val))
> >> +             pdata->adc_freq = val;
> >> +     if (!of_property_read_u32(np, "ts,ave-ctrl", &val))
> >> +             pdata->ave_ctrl = val;
> >> +     if (!of_property_read_u32(np, "ts,touch-det-delay", &val))
> >> +             pdata->touch_det_delay = val;
> >> +     if (!of_property_read_u32(np, "ts,settling", &val))
> >> +             pdata->settling = val;
> >> +     if (!of_property_read_u32(np, "ts,fraction-z", &val))
> >> +             pdata->fraction_z = val;
> >> +     if (!of_property_read_u32(np, "ts,i-drive", &val))
> >> +             pdata->i_drive = val;
> >> +
> >> +     return pdata;
> >> +}
> >
> > As above.
> >
> > I'm going to stop my review here. I think you get the idea.
> 
> I got most of your worries, but couldn't understand the issue of passing
> all pdata fields as DT bindings.

It's a really big issue with DT enablement. Platform data has been filled
with lots of junk over the years, to blindly move it all over to DT would
be foolish. Now is the time to go through each of them and pull out the
ones that we can do without, can be hard-coded or derived in the driver.
I'm more than sure that at least some of the above can be removed with
some thoughtful coding.

> Thanks for your review though. :)

No problem.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists