On Wed, Jul 26, 2017 at 3:35 PM, Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> wrote:
>> > > > On Mon, Jul 24, 2017 at 8:39 PM, Andy Shevchenko
>> > > > <andriy.shevchenko@xxxxxxxxxxxxxxx> 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.
Something like that:
diff --git a/drivers/tty/serial/serial_mctrl_gpio.c
b/drivers/tty/serial/serial_mctrl_gpio.c
index d2da6aa..8871213 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -19,6 +19,7 @@
#include <linux/irq.h>
#include <linux/gpio/consumer.h>
#include <linux/termios.h>
+#include <linux/property.h>
#include <linux/serial_core.h>
#include <linux/module.h>
@@ -118,6 +119,9 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct
device *dev, unsigned int idx)
struct mctrl_gpios *gpios;
enum mctrl_gpio_idx i;
+ if (device_property_present(dev, "wakeup-source"))
+ return ERR_PTR(-EINVAL);
+
gpios = devm_kzalloc(dev, sizeof(*gpios), GFP_KERNEL);
if (!gpios)
return ERR_PTR(-ENOMEM);
--
> 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).
OK. Let's wait for suggestions.
Yegor
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
