lkml.org 
[lkml]   [2012]   [Nov]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH V2 2/2] mfd: stmpe: Extend DT support in stmpe driver
From
On 22 November 2012 16:54, Lee Jones <lee.jones@linaro.org> wrote:
> 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

>> 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 :(

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

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.

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?

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

>> +stmpe-ts:
>> +-----------

> See "When adding new bindings, ask yourself" above.

Same. Can you explicitly point out, which bindings you didn't like.

>> +spi@e0100000 {
>
> This shouldn't be a child of the SPI device becuase it uses SPI.
>
> Drivers use clocks, regulators, IRQ controller too, but they're
> not children of those devices.

Yes.

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

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

>> 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 :)

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

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

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

Thanks for your review though. :)

--
viresh


\
 
 \ /
  Last update: 2012-11-26 10:21    [W:0.189 / U:0.504 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site