| Message ID | 1455876982-6743-4-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
On Fri, Feb 19, 2016 at 11:16 AM, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > Make use of the device property API in this driver so that both OF based > systems and ACPI based systems can use this driver. > > Based on commits b26d4e2283b6d9b6 ("input: gpio_keys_polled: Make use of > device property API"), 99b4ffbd84ea4191 ("Input: gpio_keys[_polled] - > change name of wakeup property"), and 1feb57a245a4910b ("gpio: add > parameter to allow the use named gpios"). > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > Tested with DT only. > --- > drivers/input/keyboard/gpio_keys.c | 77 +++++++++++++++----------------------- > 1 file changed, 30 insertions(+), 47 deletions(-) > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c > index b6262d94aff19f70..5764308e3b26314a 100644 > --- a/drivers/input/keyboard/gpio_keys.c > +++ b/drivers/input/keyboard/gpio_keys.c > @@ -655,32 +646,29 @@ gpio_keys_get_devtree_pdata(struct platform_device *pdev) > return ERR_PTR(-ENOMEM); > > pdata->buttons = (struct gpio_keys_button *)(pdata + 1); > - pdata->nbuttons = nbuttons; > - > - pdata->rep = !!of_get_property(node, "autorepeat", NULL); > > - of_property_read_string(node, "label", &pdata->name); > + pdata->rep = device_property_present(dev, "autorepeat"); > + device_property_read_string(dev, "label", &pdata->name); > > - i = 0; > - for_each_available_child_of_node(node, pp) { > - enum of_gpio_flags flags; > + device_for_each_child_node(dev, child) { > + struct gpio_desc *desc; > > - button = &pdata->buttons[i++]; > - > - button->gpio = of_get_gpio_flags(pp, 0, &flags); > - if (button->gpio < 0) { > - error = button->gpio; > + desc = devm_get_gpiod_from_child(dev, NULL, child); > + if (IS_ERR(desc)) { > + error = PTR_ERR(desc); > if (error != -ENOENT) { > if (error != -EPROBE_DEFER) > dev_err(dev, > "Failed to get gpio flags, error: %d\n", > error); > + fwnode_handle_put(child); > return ERR_PTR(error); > } > - } else { > - button->active_low = flags & OF_GPIO_ACTIVE_LOW; > } > > + button = &pdata->buttons[pdata->nbuttons++]; > + button->gpiod = desc; > + > button->irq = platform_get_irq(pdev, 0); > > if (!gpio_is_valid(button->gpio) && button->irq < 0) { Woops, I missed to convert one check. The above line should become: if (IS_ERR(button->gpiod) && button->irq < 0) { Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 19, 2016 at 11:16:22AM +0100, Geert Uytterhoeven wrote: > @@ -887,7 +870,7 @@ static struct platform_driver gpio_keys_device_driver = { > .driver = { > .name = "gpio-keys", > .pm = &gpio_keys_pm_ops, > - .of_match_table = of_match_ptr(gpio_keys_of_match), > + .of_match_table = gpio_keys_of_match, Why are we changing this? I think match table should still be guarded by #ifdef CONFIG_OF. Thanks.
On Mon, Feb 22, 2016 at 11:58:15AM -0800, Dmitry Torokhov wrote: > On Fri, Feb 19, 2016 at 11:16:22AM +0100, Geert Uytterhoeven wrote: > > @@ -887,7 +870,7 @@ static struct platform_driver gpio_keys_device_driver = { > > .driver = { > > .name = "gpio-keys", > > .pm = &gpio_keys_pm_ops, > > - .of_match_table = of_match_ptr(gpio_keys_of_match), > > + .of_match_table = gpio_keys_of_match, > > Why are we changing this? I think match table should still be guarded > by #ifdef CONFIG_OF. This allows ACPI "PRP0001" ID to match DT .compatible strings in the gpio_keys_of_match[] table. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 23, 2016 at 09:29:50AM +0200, Mika Westerberg wrote: > On Mon, Feb 22, 2016 at 11:58:15AM -0800, Dmitry Torokhov wrote: > > On Fri, Feb 19, 2016 at 11:16:22AM +0100, Geert Uytterhoeven wrote: > > > @@ -887,7 +870,7 @@ static struct platform_driver gpio_keys_device_driver = { > > > .driver = { > > > .name = "gpio-keys", > > > .pm = &gpio_keys_pm_ops, > > > - .of_match_table = of_match_ptr(gpio_keys_of_match), > > > + .of_match_table = gpio_keys_of_match, > > > > Why are we changing this? I think match table should still be guarded > > by #ifdef CONFIG_OF. > > This allows ACPI "PRP0001" ID to match DT .compatible strings in the > gpio_keys_of_match[] table. Ah, I see.
Sorry to interrupt with question, but I guess this thread has right
people for related to this topic question.
The question is reversed to the topic - how to, having working touch
driver in Android and Windows determine which exactly gpio pin is used
as INT\WAKE gpio by the driver?
Or for example even when I have some variant of such gpio pin how to
ensure that it is exactly this gpio?
For example practical task: I am trying to determine which exactly
gpio pin is responsible for INT/WAKE touchscreen on Chuwi Vi10
(baytrail x86_64 Arch 4.4.2 vanilla kernel with my custom config).
Exploring /sys/class/gpio/* have guessed that in Android it is
probably gpio134 but in Linux 4.4.2 probably gpio393. How to compare
such pins and ensure that in Linux 393 gpio is the same as 134 in
Android?
Is this possible to do in any predictable (not guessing or enumerating
all range) way?
Regards and thanks for replies,
Serge Kolotylo.
On Mon, Feb 22, 2016 at 7:58 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Feb 19, 2016 at 11:16:22AM +0100, Geert Uytterhoeven wrote:
>> @@ -887,7 +870,7 @@ static struct platform_driver gpio_keys_device_driver = {
>> .driver = {
>> .name = "gpio-keys",
>> .pm = &gpio_keys_pm_ops,
>> - .of_match_table = of_match_ptr(gpio_keys_of_match),
>> + .of_match_table = gpio_keys_of_match,
>
> Why are we changing this? I think match table should still be guarded
> by #ifdef CONFIG_OF.
>
> Thanks.
>
> --
> Dmitry
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 28, 2016 at 02:03:34AM +0000, sergk sergk2mail wrote: > Sorry to interrupt with question, but I guess this thread has right > people for related to this topic question. > The question is reversed to the topic - how to, having working touch > driver in Android and Windows determine which exactly gpio pin is used > as INT\WAKE gpio by the driver? On ACPI systems the device description has either Interrupt() or GpioInt() resource in its _CRS list (see Documentation/acpi/gpio-properties.txt). > Or for example even when I have some variant of such gpio pin how to > ensure that it is exactly this gpio? > For example practical task: I am trying to determine which exactly > gpio pin is responsible for INT/WAKE touchscreen on Chuwi Vi10 > (baytrail x86_64 Arch 4.4.2 vanilla kernel with my custom config). > Exploring /sys/class/gpio/* have guessed that in Android it is > probably gpio134 but in Linux 4.4.2 probably gpio393. How to compare > such pins and ensure that in Linux 393 gpio is the same as 134 in > Android? > Is this possible to do in any predictable (not guessing or enumerating > all range) way? Both DT and ACPI provide means to assign GPIOs to devices. We can then use Linux APIs to query those. For example getting GPIO with name "reset" is done like: struct gpio_desc *reset_desc; reset_desc = gpiod_get(dev, "reset", GPIOD_OUT_HIGH); This will retrieve the GPIO using firmware description (ACPI, DT) if available. No need to deal with the GPIO numbers. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Both DT and ACPI provide means to assign GPIOs to devices. We can then > use Linux APIs to query those. For example getting GPIO with name > "reset" is done like: > > struct gpio_desc *reset_desc; > > reset_desc = gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > > This will retrieve the GPIO using firmware description (ACPI, DT) if > available. No need to deal with the GPIO numbers. Hi Mika, Thanks for reply, But how then to obtain gpio name or if it is possible the list of all available names? For example decoded ACPI DSDT shows the following: how to get gpio name for mentioned in your reply function? Does it according below DSDT should be "GPO1" or "INT33FC" or something other? Kind regards, Serge Kolotylo. Device (TCS5) { Name (_ADR, Zero) // _ADR: Address Name (_HID, "CHPN0001") // _HID: Hardware ID Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID: Compatible ID Name (_S0W, Zero) // _S0W: S0 Device Wake State Name (_DEP, Package (0x02) // _DEP: Dependencies { GPO1, I2C5 }) Method (_PS3, 0, Serialized) // _PS3: Power State 3 { } Method (_PS0, 0, Serialized) // _PS0: Power State 0 { If ((^^^GPO1.AVBL == One)) { ^^^GPO1.TCD3 = Zero } Sleep (0x05) If ((^^^I2C5.PMI1.AVBG == One)) { ^^^I2C5.PMI1.TCON = One } Sleep (0x1E) If ((^^^GPO1.AVBL == One)) { ^^^GPO1.TCD3 = One } Sleep (0x78) } and GPO1 descr Device (GPO1) { Name (_ADR, Zero) // _ADR: Address Name (_HID, "INT33FC" /* Intel Baytrail GPIO Controller */) // _HID: Hardware ID Name (_CID, "INT33FC" /* Intel Baytrail GPIO Controller */) // _CID: Compatible ID Name (_DDN, "ValleyView GPNCORE controller") // _DDN: DOS Device Name Name (_UID, 0x02) // _UID: Unique ID Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings { Name (RBUF, ResourceTemplate () { Memory32Fixed (ReadWrite, 0xFED0D000, // Address Base 0x00001000, // Address Length ) Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, ) { 0x00000030, } }) Return (RBUF) /* \_SB_.GPO1._CRS.RBUF */ } Name (AVBL, Zero) Method (_REG, 2, NotSerialized) // _REG: Region Availability { If ((Arg0 == 0x08)) { AVBL = Arg1 } } OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x0C) Field (GPOP, ByteAcc, NoLock, Preserve) { Connection ( GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\\_SB.GPO1", 0x00, ResourceConsumer, , ) { // Pin list 0x000F } ), BST5, 1, Connection ( GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\\_SB.GPO1", 0x00, ResourceConsumer, , ) { // Pin list 0x001A } ), TCD3, 1 } -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 29, 2016 at 04:24:16PM +0000, sergk sergk2mail wrote: > But how then to obtain gpio name or if it is possible the list of all > available names? > For example decoded ACPI DSDT shows the following: > how to get gpio name for mentioned in your reply function? For existing systems that do not provide _DSD naming for GPIOs you still can provide them in the driver itself (ugly but works). See Documentation/acpi/gpio-properties.txt chapter "ACPI GPIO Mappings Provided by Drivers". > Does it according below DSDT should be "GPO1" or "INT33FC" or something other? No. The DSDT below does not have any names. > Kind regards, > Serge Kolotylo. > > Device (TCS5) > { > Name (_ADR, Zero) // _ADR: Address > Name (_HID, "CHPN0001") // _HID: Hardware ID > Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) > */) // _CID: Compatible ID > Name (_S0W, Zero) // _S0W: S0 Device Wake State > Name (_DEP, Package (0x02) // _DEP: Dependencies > { > GPO1, > I2C5 > }) > Method (_PS3, 0, Serialized) // _PS3: Power State 3 > { > } > > Method (_PS0, 0, Serialized) // _PS0: Power State 0 > { > If ((^^^GPO1.AVBL == One)) > { > ^^^GPO1.TCD3 = Zero Note that all these are part of GPIO Operation Region and not accessible to the i2c-hid driver. The will be used when the device is powered on and the pinctrl-baytrail has been loaded (that provides the Operation Region). If you need to use GPIOs from driver, they are listed in _CRS of the device. > } > > Sleep (0x05) > If ((^^^I2C5.PMI1.AVBG == One)) > { > ^^^I2C5.PMI1.TCON = One > } > > Sleep (0x1E) > If ((^^^GPO1.AVBL == One)) > { > ^^^GPO1.TCD3 = One > } > > Sleep (0x78) > } -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 1, 2016 at 2:52 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Mon, Feb 29, 2016 at 04:24:16PM +0000, sergk sergk2mail wrote: >> But how then to obtain gpio name or if it is possible the list of all >> available names? >> For example decoded ACPI DSDT shows the following: >> how to get gpio name for mentioned in your reply function? > > For existing systems that do not provide _DSD naming for GPIOs you still > can provide them in the driver itself (ugly but works). See > Documentation/acpi/gpio-properties.txt chapter "ACPI GPIO Mappings > Provided by Drivers". I just this merge window added an ABI for userspace to read name of the GPIO line and also consumer name ("label") by using the two strings stored in struct gpio_desc. Currently this will be initialized per-offset from the seldom used char names[] in struct gpio_chip, if not NULL. We're working on DT bindings to set this per-line from the DT, and if ACPI has a mechanism to name individual lines, please submit patches for assigning this properly! (Hi Mika ;) For consumers struct gpio_desc is opaque, but if they actually need to know the name of a line we can add gpiod_get_name(struct gpio_desc *) but then I want to know a usecase. debugfs and userspace ABI is already displaying it just fine. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 09, 2016 at 10:36:08AM +0700, Linus Walleij wrote: > I just this merge window added an ABI for userspace to read name > of the GPIO line and also consumer name ("label") by using the > two strings stored in struct gpio_desc. > > Currently this will be initialized per-offset from the seldom used > char names[] in struct gpio_chip, if not NULL. > > We're working on DT bindings to set this per-line from the DT, and > if ACPI has a mechanism to name individual lines, please submit > patches for assigning this properly! (Hi Mika ;) Hi :) Yes, ACPI nowadays has mechanism for assigning names to GPIOs (_DSD). We will look into the DT implementation and try to come up with corresponding ACPI version. Thanks for the heads-up. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index b6262d94aff19f70..5764308e3b26314a 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -27,9 +27,6 @@ #include <linux/workqueue.h> #include <linux/gpio.h> #include <linux/gpio/consumer.h> -#include <linux/of.h> -#include <linux/of_platform.h> -#include <linux/of_gpio.h> #include <linux/spinlock.h> struct gpio_button_data { @@ -625,26 +622,20 @@ static void gpio_keys_close(struct input_dev *input) * Handlers for alternative sources of platform_data */ -#ifdef CONFIG_OF /* - * Translate OpenFirmware node properties into platform_data + * Translate properties into platform_data */ static struct gpio_keys_platform_data * gpio_keys_get_devtree_pdata(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct device_node *node, *pp; struct gpio_keys_platform_data *pdata; struct gpio_keys_button *button; + struct fwnode_handle *child; int error; int nbuttons; - int i; - - node = dev->of_node; - if (!node) - return ERR_PTR(-ENODEV); - nbuttons = of_get_available_child_count(node); + nbuttons = device_get_child_node_count(dev); if (nbuttons == 0) return ERR_PTR(-ENODEV); @@ -655,32 +646,29 @@ gpio_keys_get_devtree_pdata(struct platform_device *pdev) return ERR_PTR(-ENOMEM); pdata->buttons = (struct gpio_keys_button *)(pdata + 1); - pdata->nbuttons = nbuttons; - - pdata->rep = !!of_get_property(node, "autorepeat", NULL); - of_property_read_string(node, "label", &pdata->name); + pdata->rep = device_property_present(dev, "autorepeat"); + device_property_read_string(dev, "label", &pdata->name); - i = 0; - for_each_available_child_of_node(node, pp) { - enum of_gpio_flags flags; + device_for_each_child_node(dev, child) { + struct gpio_desc *desc; - button = &pdata->buttons[i++]; - - button->gpio = of_get_gpio_flags(pp, 0, &flags); - if (button->gpio < 0) { - error = button->gpio; + desc = devm_get_gpiod_from_child(dev, NULL, child); + if (IS_ERR(desc)) { + error = PTR_ERR(desc); if (error != -ENOENT) { if (error != -EPROBE_DEFER) dev_err(dev, "Failed to get gpio flags, error: %d\n", error); + fwnode_handle_put(child); return ERR_PTR(error); } - } else { - button->active_low = flags & OF_GPIO_ACTIVE_LOW; } + button = &pdata->buttons[pdata->nbuttons++]; + button->gpiod = desc; + button->irq = platform_get_irq(pdev, 0); if (!gpio_is_valid(button->gpio) && button->irq < 0) { @@ -688,24 +676,29 @@ gpio_keys_get_devtree_pdata(struct platform_device *pdev) return ERR_PTR(-EINVAL); } - if (of_property_read_u32(pp, "linux,code", &button->code)) { - dev_err(dev, "Button without keycode: 0x%x\n", - button->gpio); + if (fwnode_property_read_u32(child, "linux,code", + &button->code)) { + dev_err(dev, "Button without keycode: %d\n", + pdata->nbuttons - 1); + fwnode_handle_put(child); return ERR_PTR(-EINVAL); } - button->desc = of_get_property(pp, "label", NULL); + fwnode_property_read_string(child, "label", &button->desc); - if (of_property_read_u32(pp, "linux,input-type", &button->type)) + if (fwnode_property_read_u32(child, "linux,input-type", + &button->type)) button->type = EV_KEY; - button->wakeup = of_property_read_bool(pp, "wakeup-source") || - /* legacy name */ - of_property_read_bool(pp, "gpio-key,wakeup"); + button->wakeup = + fwnode_property_read_bool(child, "wakeup-source") || + /* legacy name */ + fwnode_property_read_bool(child, "gpio-key,wakeup"); - button->can_disable = !!of_get_property(pp, "linux,can-disable", NULL); + button->can_disable = + fwnode_property_present(child, "linux,can-disable"); - if (of_property_read_u32(pp, "debounce-interval", + if (fwnode_property_read_u32(child, "debounce-interval", &button->debounce_interval)) button->debounce_interval = 5; } @@ -722,16 +715,6 @@ static const struct of_device_id gpio_keys_of_match[] = { }; MODULE_DEVICE_TABLE(of, gpio_keys_of_match); -#else - -static inline struct gpio_keys_platform_data * -gpio_keys_get_devtree_pdata(struct platform_device *pdev) -{ - return ERR_PTR(-ENODEV); -} - -#endif - static int gpio_keys_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -887,7 +870,7 @@ static struct platform_driver gpio_keys_device_driver = { .driver = { .name = "gpio-keys", .pm = &gpio_keys_pm_ops, - .of_match_table = of_match_ptr(gpio_keys_of_match), + .of_match_table = gpio_keys_of_match, } };
Make use of the device property API in this driver so that both OF based systems and ACPI based systems can use this driver. Based on commits b26d4e2283b6d9b6 ("input: gpio_keys_polled: Make use of device property API"), 99b4ffbd84ea4191 ("Input: gpio_keys[_polled] - change name of wakeup property"), and 1feb57a245a4910b ("gpio: add parameter to allow the use named gpios"). Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Tested with DT only. --- drivers/input/keyboard/gpio_keys.c | 77 +++++++++++++++----------------------- 1 file changed, 30 insertions(+), 47 deletions(-)