| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAKohpo=F91sZ7St3XVXqwAEgmSWXjAxxXwRs1RD5qPyW99P1vA@mail.gmail.com>
Date: Thu, 22 Nov 2012 19:24:09 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Lee Jones <lee.jones@...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
On 22 November 2012 16:54, Lee Jones <lee.jones@...aro.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@...00000 {
>
> 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
--
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