| Message ID | CAGm1_ktq0ed2fHMHfY6F7sUAuB0Py7OkFe2APXNpQP6Vmuwc9g@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2017-07-26 at 15:26 +0200, Yegor Yefremov wrote: > On Tue, Jul 25, 2017 at 11:18 AM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, 2017-07-25 at 07:43 +0200, Yegor Yefremov wrote: > > > On Mon, Jul 24, 2017 at 8:48 PM, Yegor Yefremov > > > <yegorslists@googlemail.com> wrote: > > > > On Mon, Jul 24, 2017 at 8:39 PM, Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > Hi! > > > > > > > > > > Since my big ACPI GPIO fix made the vanilla, I think we may > > > > > return > > > > > back > > > > > the commit 4ef03d328769 ("tty/serial/8250: use mctrl_gpio > > > > > helpers"). > > > > > > > > > > I just have tested it on two x86 boards: > > > > > 1) Broxton (without _DSD properties) > > > > > 2) ApolloLake (with _DSD properties for RX and CTS pins) > > > > > > > > > > Opinions, more testing? > > > > Alas, I did more deep testing and the patch breaks the console. > > > > It looks like we need to distinguish what those GPIOs are used for: > > a) modem control lines, or > > b) wakeup source. > > > > There are few options to distinguish: > > 1) check if GPIO resource is marked as wakeup source (ACPI only) > > 2) use "wakeup-source" device property, for now looks like there is > > no > > serial driver is using it (might be collision with the real wake > > capable > > serial drivers) > > 3) similar to 2), though introduce another property like "oob- > > wakeup- > > source" or any variations of that > > 4) something else? > > > AFAIK this patch was needed in order to avoid serial port breakage, > where some modem signals were defined in ACPI tables. Mika and some > other devs reported the issue as "tty/serial/8250: use mctrl_gpio > helpers" was part of the kernel. I am one from those devs. > Perhaps I should just augment my patch like this? > > + sprintf(mctrl_property, "%s-gpios", > mctrl_gpios_desc[i].name); > + if (!device_property_present(dev, mctrl_property) This is not needed any more. My patch series for GPIO ACPI library makes this clean (no fallback is allowed if name is supplied). The problen now while property _is_ there in ACPI table. > || > + of_property_read_bool(dev->of_node, > "wakeup-source")) ...and this should be device_property besides that fact that you need to check it once in _noauto() and forbid requesting GPIOs by returning an error immediately. P.S. Take into account that use of this property should be described in the bindings. Before that it should be discussed (I pointed above what might be cons of use this name/property).
diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c index d2da6aa..a6245efb 100644 --- a/drivers/tty/serial/serial_mctrl_gpio.c +++ b/drivers/tty/serial/serial_mctrl_gpio.c @@ -19,6 +19,8 @@ #include <linux/irq.h> #include <linux/gpio/consumer.h> #include <linux/termios.h> +#include <linux/of_device.h> +#include <linux/property.h> #include <linux/serial_core.h> #include <linux/module.h> @@ -124,6 +126,12 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx) for (i = 0; i < UART_GPIO_MAX; i++) { enum gpiod_flags flags; + char mctrl_property[10]; + + sprintf(mctrl_property, "%s-gpios", mctrl_gpios_desc[i].name); + if (!device_property_present(dev, mctrl_property) || + of_property_read_bool(dev->of_node, "wakeup-source")) + continue;