* [PATCH 1/2] serial: pl010: Move uart_register_driver call to device probe
From: Sjoerd Simons @ 2017-03-24 16:26 UTC (permalink / raw)
To: linux-serial
Cc: linux-renesas-soc, Greg Kroah-Hartman, linux-kernel, Russell King,
Jiri Slaby
In-Reply-To: <20170324162634.8880-1-sjoerd.simons@collabora.co.uk>
uart_register_driver call binds the driver to a specific device
node through tty_register_driver call. This should typically
happen during device probe call.
In a multiplatform scenario, it is possible that multiple serial
drivers are part of the kernel. Currently the driver registration fails
if multiple serial drivers with overlapping major/minor numbers are
included.
Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
drivers/tty/serial/amba-pl010.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/amba-pl010.c b/drivers/tty/serial/amba-pl010.c
index f2f251075109..74a1d3b2e45d 100644
--- a/drivers/tty/serial/amba-pl010.c
+++ b/drivers/tty/serial/amba-pl010.c
@@ -749,6 +749,16 @@ static int pl010_probe(struct amba_device *dev, const struct amba_id *id)
amba_ports[i] = uap;
amba_set_drvdata(dev, uap);
+
+ if (!amba_reg.state) {
+ ret = uart_register_driver(&amba_reg);
+ if (ret < 0) {
+ dev_err(uap->port.dev,
+ "Failed to register AMBA-PL010 driver\n");
+ return ret;
+ }
+ }
+
ret = uart_add_one_port(&amba_reg, &uap->port);
if (ret)
amba_ports[i] = NULL;
@@ -760,12 +770,18 @@ static int pl010_remove(struct amba_device *dev)
{
struct uart_amba_port *uap = amba_get_drvdata(dev);
int i;
+ bool busy = false;
uart_remove_one_port(&amba_reg, &uap->port);
for (i = 0; i < ARRAY_SIZE(amba_ports); i++)
if (amba_ports[i] == uap)
amba_ports[i] = NULL;
+ else if (amba_ports[i])
+ busy = true;
+
+ if (!busy)
+ uart_unregister_driver(&amba_reg);
return 0;
}
@@ -816,23 +832,14 @@ static struct amba_driver pl010_driver = {
static int __init pl010_init(void)
{
- int ret;
-
printk(KERN_INFO "Serial: AMBA driver\n");
- ret = uart_register_driver(&amba_reg);
- if (ret == 0) {
- ret = amba_driver_register(&pl010_driver);
- if (ret)
- uart_unregister_driver(&amba_reg);
- }
- return ret;
+ return amba_driver_register(&pl010_driver);
}
static void __exit pl010_exit(void)
{
amba_driver_unregister(&pl010_driver);
- uart_unregister_driver(&amba_reg);
}
module_init(pl010_init);
--
2.11.0
^ permalink raw reply related
* [PATCH 0/2] Move uart_register_driver call to device probe for pl010 and sh-sci
From: Sjoerd Simons @ 2017-03-24 16:26 UTC (permalink / raw)
To: linux-serial
Cc: linux-renesas-soc, Greg Kroah-Hartman, linux-kernel, Russell King,
Jiri Slaby
When testing on a Renesas board with the PL010 serial driver enabled
serial output broke. Turns out the minor device numbers for both
drivers happen to overlap, causing whichever driver happened to be the
second one to register to fail.
The attached patches move the uart_register_driver call to probe time
for both drivers avoiding the issue.
Sjoerd Simons (2):
serial: pl010: Move uart_register_driver call to device probe
serial: sh-sci: Move uart_register_driver call to device probe
drivers/tty/serial/amba-pl010.c | 27 +++++++++++++++++----------
drivers/tty/serial/sh-sci.c | 21 ++++++++++-----------
2 files changed, 27 insertions(+), 21 deletions(-)
--
2.11.0
^ permalink raw reply
* Re: [PATCH v2 1/2] tty: serial_core: Add name field to uart_port struct
From: Andy Shevchenko @ 2017-03-24 11:46 UTC (permalink / raw)
To: Vignesh R
Cc: Jisheng Zhang, Greg Kroah-Hartman, linux-kernel@vger.kernel.org,
linux-serial@vger.kernel.org, Jiri Slaby, linux-arm Mailing List
In-Reply-To: <20170324052800.11526-2-vigneshr@ti.com>
On Fri, Mar 24, 2017 at 7:27 AM, Vignesh R <vigneshr@ti.com> wrote:
> Introduce a field to store name of uart_port that can be used to easily
> identify UART port instances on a system that has more than one UART
> instance. The name is of the form ttyXN(eg. ttyS0, ttyAMA0,..) where N
> is number that particular UART instance.
> This field will be useful when printing debug info for a particular port
> or in register IRQs with unique IRQ name. Port name is populated during
> uart_add_one_port().
>
Looks good to me:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Just in case, have you checked if there any possible scenarios where
memory will be leaked?
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>
> v2:
> use kasprintf() instead of snprintf()
> Fix up commit message.
>
> drivers/tty/serial/serial_core.c | 7 +++++++
> include/linux/serial_core.h | 1 +
> 2 files changed, 8 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 0fb3f7cce62a..f5572e28d16a 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2744,6 +2744,12 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> state->pm_state = UART_PM_STATE_UNDEFINED;
> uport->cons = drv->cons;
> uport->minor = drv->tty_driver->minor_start + uport->line;
> + uport->name = kasprintf(GFP_KERNEL, "%s%d", drv->dev_name,
> + drv->tty_driver->name_base + uport->line);
> + if (!uport->name) {
> + ret = -ENOMEM;
> + goto out;
> + }
>
> /*
> * If this port is a console, then the spinlock is already
> @@ -2861,6 +2867,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> if (uport->type != PORT_UNKNOWN && uport->ops->release_port)
> uport->ops->release_port(uport);
> kfree(uport->tty_groups);
> + kfree(uport->name);
>
> /*
> * Indicate that there isn't a port here anymore.
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 58484fb35cc8..60530678c633 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -247,6 +247,7 @@ struct uart_port {
> unsigned char suspended;
> unsigned char irq_wake;
> unsigned char unused[2];
> + char *name; /* port name */
> struct attribute_group *attr_group; /* port specific attributes */
> const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
> struct serial_rs485 rs485;
> --
> 2.11.0
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Uwe Kleine-König @ 2017-03-24 10:01 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Boris Brezillon, Yoshinori Sato, Geert Uytterhoeven,
Richard Genoud, Greg Kroah-Hartman, Linus Walleij,
Dmitry Torokhov, linux-kernel@vger.kernel.org,
linux-gpio@vger.kernel.org, Janusz Uzycki,
linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAMuHMdWp+e4gmRYYV=kzpUjdkbJUX3UfXGp0zC_LBK0-7+thLQ@mail.gmail.com>
Hello Geert,
On Fri, Mar 24, 2017 at 10:44:50AM +0100, Geert Uytterhoeven wrote:
> On Fri, Mar 24, 2017 at 10:15 AM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Mar 24, 2017 at 09:59:04AM +0100, Geert Uytterhoeven wrote:
> >> On Fri, Mar 24, 2017 at 9:39 AM, Uwe Kleine-König
> >> <u.kleine-koenig@pengutronix.de> wrote:
> >> > On Fri, Mar 24, 2017 at 09:29:02AM +0100, Geert Uytterhoeven wrote:
> >> >> On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-König
> >> >> <u.kleine-koenig@pengutronix.de> wrote:
> >> >> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >> >> > Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled
> >> >> >
> >> >> > People disagree if gpiod_get_optional should return NULL or
> >> >> > ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that
> >> >> > the person who decided to disable GPIOLIB is assumed to know that there
> >> >> > is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might
> >> >> > introduce hard to debug problems if that decision is wrong.
> >> >> >
> >> >> > So this patch introduces a compromise and let gpiod_get_optional (and
> >> >> > its variants) return NULL if the device in question cannot have an
> >> >> > associated GPIO because it is neither instantiated by a device tree nor
> >> >> > by ACPI.
> >> >> >
> >> >> > This should handle most cases that are argued about.
> >> >> >
> >> >> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >> >> > ---
> >> >> > include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++-------
> >> >> > 1 file changed, 46 insertions(+), 9 deletions(-)
> >> >> >
> >> >> > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> >> >> > index fb0fde686cb1..0ca29889290d 100644
> >> >> > --- a/include/linux/gpio/consumer.h
> >> >> > +++ b/include/linux/gpio/consumer.h
> >> >> > @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev,
> >> >> > return ERR_PTR(-ENOSYS);
> >> >> > }
> >> >> >
> >> >> > -static inline struct gpio_desc *__must_check
> >> >> > -gpiod_get_optional(struct device *dev, const char *con_id,
> >> >> > - enum gpiod_flags flags)
> >> >> > +static inline bool __gpiod_no_optional_possible(struct device *dev)
> >> >> > {
> >> >> > - return ERR_PTR(-ENOSYS);
> >> >> > + /*
> >> >> > + * gpiod_get_optional et al can only provide a GPIO if at least one of
> >> >> > + * the backends for specifing a GPIO is available. These are device
> >> >> > + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if
> >> >> > + * GPIOLIB is disabled (which is the case here).
> >> >> > + * So if the provided device is unrelated to device tree and ACPI, we
> >> >> > + * can be sure that there is no optional GPIO and let gpiod_get_optional
> >> >> > + * safely return NULL.
> >> >> > + * Otherwise there is still a chance that there is no GPIO but we cannot
> >> >> > + * be sure without having to enable a part of GPIOLIB (i.e. the lookup
> >> >> > + * part). So lets play safe and return an error. (Though there are also
> >> >> > + * arguments that returning NULL then would be beneficial.)
> >> >> > + */
> >> >> > +
> >> >> > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> >> >> > + return false;
> >> >>
> >> >> At first sight, I though this was OK:
> >> >>
> >> >> 1. On ARM with DT, we can assume CONFIG_GPIOLOB=y.
> >> >>
> >> >> 2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y,
> >> >> and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do
> >> >> not use DT (yet), the check for dev->of_node (false) should handle
> >> >> that.
> >> >>
> >> >> 3. However, I managed to do the same for h8300, which does use DT. Hence
> >> >> if mctrl_gpio would start relying on gpiod_get_optional(), this would
> >> >> break the sh-sci driver on h8300 :-(
> >> >> Note that h8300 doesn't have any GPIO drivers (yet?), so
> >> >> CONFIG_GPIPOLIB=n makes perfect sense!
> >>
> >> >> So I'm afraid the only option is to always return NULL, and put the
> >> >> responsability on the shoulders of the system integrator...
> >> >
> >> > The gpio lines could be provided by an i2c gpio adapter, right? So IMHO
> >> > you don't need platform gpios to justify -ENODEV. So I guess that's a
> >> > case where we don't come to an agreement.
> >>
> >> While you can enable I2C without further dependencies, no I2C GPIO expander
> >> will be offered... unless you have enabled CONFIG_GPIOLIB first.
> >
> > And that is expected, still the device tree could reference such a GPIO
> > and thus create a situation where Dmitry's and my judgement disagree.
>
> If the device tree references such a GPIO, and CONFIG_GPIOLIB is not set,
> the I2C GPIO expander device will not be bound.
> Frank Rowand's DT scripts (http://elinux.org/Device_Tree_frowand) will come
> to the rescue, and inform the user which driver(s) to enable.
>
> > So I think my suggestion is the best we could do now. It minimizes the
> > number of cases where we disagree. The next best thing would be to
> > implement that half gpiolib stuff (i.e. do the full lookup to be sure
> > there is no gpio) but this comes at a price: We need some time to
> > implement it and it adds a bit to the kernel size.
>
> So I still have to handle -ENOSYS in sh-sci.c, to avoid regressions...
How much would it hurt to enable GPIOLIB there now? I assume that the
platform has GPIOs and it's only an intermediate step that there is no
driver for it at present? At some point you have to bite the bullet.
Given that mctrl_gpio is a usage of GPIOs that might be now?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Geert Uytterhoeven @ 2017-03-24 9:44 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Dmitry Torokhov, Linus Walleij, Richard Genoud,
Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre,
Boris Brezillon, linux-arm-kernel@lists.infradead.org,
linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
Janusz Uzycki, linux-gpio@vger.kernel.org, Yoshinori Sato
In-Reply-To: <20170324091558.l2gmqqyced3d345o@pengutronix.de>
Hi Uwe,
On Fri, Mar 24, 2017 at 10:15 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Fri, Mar 24, 2017 at 09:59:04AM +0100, Geert Uytterhoeven wrote:
>> On Fri, Mar 24, 2017 at 9:39 AM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> > On Fri, Mar 24, 2017 at 09:29:02AM +0100, Geert Uytterhoeven wrote:
>> >> On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-König
>> >> <u.kleine-koenig@pengutronix.de> wrote:
>> >> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> >> > Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled
>> >> >
>> >> > People disagree if gpiod_get_optional should return NULL or
>> >> > ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that
>> >> > the person who decided to disable GPIOLIB is assumed to know that there
>> >> > is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might
>> >> > introduce hard to debug problems if that decision is wrong.
>> >> >
>> >> > So this patch introduces a compromise and let gpiod_get_optional (and
>> >> > its variants) return NULL if the device in question cannot have an
>> >> > associated GPIO because it is neither instantiated by a device tree nor
>> >> > by ACPI.
>> >> >
>> >> > This should handle most cases that are argued about.
>> >> >
>> >> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> >> > ---
>> >> > include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++-------
>> >> > 1 file changed, 46 insertions(+), 9 deletions(-)
>> >> >
>> >> > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
>> >> > index fb0fde686cb1..0ca29889290d 100644
>> >> > --- a/include/linux/gpio/consumer.h
>> >> > +++ b/include/linux/gpio/consumer.h
>> >> > @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev,
>> >> > return ERR_PTR(-ENOSYS);
>> >> > }
>> >> >
>> >> > -static inline struct gpio_desc *__must_check
>> >> > -gpiod_get_optional(struct device *dev, const char *con_id,
>> >> > - enum gpiod_flags flags)
>> >> > +static inline bool __gpiod_no_optional_possible(struct device *dev)
>> >> > {
>> >> > - return ERR_PTR(-ENOSYS);
>> >> > + /*
>> >> > + * gpiod_get_optional et al can only provide a GPIO if at least one of
>> >> > + * the backends for specifing a GPIO is available. These are device
>> >> > + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if
>> >> > + * GPIOLIB is disabled (which is the case here).
>> >> > + * So if the provided device is unrelated to device tree and ACPI, we
>> >> > + * can be sure that there is no optional GPIO and let gpiod_get_optional
>> >> > + * safely return NULL.
>> >> > + * Otherwise there is still a chance that there is no GPIO but we cannot
>> >> > + * be sure without having to enable a part of GPIOLIB (i.e. the lookup
>> >> > + * part). So lets play safe and return an error. (Though there are also
>> >> > + * arguments that returning NULL then would be beneficial.)
>> >> > + */
>> >> > +
>> >> > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
>> >> > + return false;
>> >>
>> >> At first sight, I though this was OK:
>> >>
>> >> 1. On ARM with DT, we can assume CONFIG_GPIOLOB=y.
>> >>
>> >> 2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y,
>> >> and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do
>> >> not use DT (yet), the check for dev->of_node (false) should handle
>> >> that.
>> >>
>> >> 3. However, I managed to do the same for h8300, which does use DT. Hence
>> >> if mctrl_gpio would start relying on gpiod_get_optional(), this would
>> >> break the sh-sci driver on h8300 :-(
>> >> Note that h8300 doesn't have any GPIO drivers (yet?), so
>> >> CONFIG_GPIPOLIB=n makes perfect sense!
>> >
>> > Thanks for your efforts.
>>
>> You're welcome.
>>
>> >> So I'm afraid the only option is to always return NULL, and put the
>> >> responsability on the shoulders of the system integrator...
>> >
>> > The gpio lines could be provided by an i2c gpio adapter, right? So IMHO
>> > you don't need platform gpios to justify -ENODEV. So I guess that's a
>> > case where we don't come to an agreement.
>>
>> While you can enable I2C without further dependencies, no I2C GPIO expander
>> will be offered... unless you have enabled CONFIG_GPIOLIB first.
>
> And that is expected, still the device tree could reference such a GPIO
> and thus create a situation where Dmitry's and my judgement disagree.
If the device tree references such a GPIO, and CONFIG_GPIOLIB is not set,
the I2C GPIO expander device will not be bound.
Frank Rowand's DT scripts (http://elinux.org/Device_Tree_frowand) will come
to the rescue, and inform the user which driver(s) to enable.
> So I think my suggestion is the best we could do now. It minimizes the
> number of cases where we disagree. The next best thing would be to
> implement that half gpiolib stuff (i.e. do the full lookup to be sure
> there is no gpio) but this comes at a price: We need some time to
> implement it and it adds a bit to the kernel size.
So I still have to handle -ENOSYS in sh-sci.c, to avoid regressions...
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
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Uwe Kleine-König @ 2017-03-24 9:15 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Dmitry Torokhov, Linus Walleij, Richard Genoud,
Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre,
Boris Brezillon, linux-arm-kernel@lists.infradead.org,
linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
Janusz Uzycki, linux-gpio@vger.kernel.org, Yoshinori Sato
In-Reply-To: <CAMuHMdVoYyDUvQuoPoY347b8tbcuOJK5o7GXoSngiX3LQYekAQ@mail.gmail.com>
On Fri, Mar 24, 2017 at 09:59:04AM +0100, Geert Uytterhoeven wrote:
> Hi Uwe,
>
> On Fri, Mar 24, 2017 at 9:39 AM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Mar 24, 2017 at 09:29:02AM +0100, Geert Uytterhoeven wrote:
> >> On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-König
> >> <u.kleine-koenig@pengutronix.de> wrote:
> >> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >> > Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled
> >> >
> >> > People disagree if gpiod_get_optional should return NULL or
> >> > ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that
> >> > the person who decided to disable GPIOLIB is assumed to know that there
> >> > is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might
> >> > introduce hard to debug problems if that decision is wrong.
> >> >
> >> > So this patch introduces a compromise and let gpiod_get_optional (and
> >> > its variants) return NULL if the device in question cannot have an
> >> > associated GPIO because it is neither instantiated by a device tree nor
> >> > by ACPI.
> >> >
> >> > This should handle most cases that are argued about.
> >> >
> >> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >> > ---
> >> > include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++-------
> >> > 1 file changed, 46 insertions(+), 9 deletions(-)
> >> >
> >> > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> >> > index fb0fde686cb1..0ca29889290d 100644
> >> > --- a/include/linux/gpio/consumer.h
> >> > +++ b/include/linux/gpio/consumer.h
> >> > @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev,
> >> > return ERR_PTR(-ENOSYS);
> >> > }
> >> >
> >> > -static inline struct gpio_desc *__must_check
> >> > -gpiod_get_optional(struct device *dev, const char *con_id,
> >> > - enum gpiod_flags flags)
> >> > +static inline bool __gpiod_no_optional_possible(struct device *dev)
> >> > {
> >> > - return ERR_PTR(-ENOSYS);
> >> > + /*
> >> > + * gpiod_get_optional et al can only provide a GPIO if at least one of
> >> > + * the backends for specifing a GPIO is available. These are device
> >> > + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if
> >> > + * GPIOLIB is disabled (which is the case here).
> >> > + * So if the provided device is unrelated to device tree and ACPI, we
> >> > + * can be sure that there is no optional GPIO and let gpiod_get_optional
> >> > + * safely return NULL.
> >> > + * Otherwise there is still a chance that there is no GPIO but we cannot
> >> > + * be sure without having to enable a part of GPIOLIB (i.e. the lookup
> >> > + * part). So lets play safe and return an error. (Though there are also
> >> > + * arguments that returning NULL then would be beneficial.)
> >> > + */
> >> > +
> >> > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> >> > + return false;
> >>
> >> At first sight, I though this was OK:
> >>
> >> 1. On ARM with DT, we can assume CONFIG_GPIOLOB=y.
> >>
> >> 2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y,
> >> and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do
> >> not use DT (yet), the check for dev->of_node (false) should handle
> >> that.
> >>
> >> 3. However, I managed to do the same for h8300, which does use DT. Hence
> >> if mctrl_gpio would start relying on gpiod_get_optional(), this would
> >> break the sh-sci driver on h8300 :-(
> >> Note that h8300 doesn't have any GPIO drivers (yet?), so
> >> CONFIG_GPIPOLIB=n makes perfect sense!
> >
> > Thanks for your efforts.
>
> You're welcome.
>
> >> So I'm afraid the only option is to always return NULL, and put the
> >> responsability on the shoulders of the system integrator...
> >
> > The gpio lines could be provided by an i2c gpio adapter, right? So IMHO
> > you don't need platform gpios to justify -ENODEV. So I guess that's a
> > case where we don't come to an agreement.
>
> While you can enable I2C without further dependencies, no I2C GPIO expander
> will be offered... unless you have enabled CONFIG_GPIOLIB first.
And that is expected, still the device tree could reference such a GPIO
and thus create a situation where Dmitry's and my judgement disagree.
So I think my suggestion is the best we could do now. It minimizes the
number of cases where we disagree. The next best thing would be to
implement that half gpiolib stuff (i.e. do the full lookup to be sure
there is no gpio) but this comes at a price: We need some time to
implement it and it adds a bit to the kernel size.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Geert Uytterhoeven @ 2017-03-24 8:59 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Dmitry Torokhov, Linus Walleij, Richard Genoud,
Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre,
Boris Brezillon, linux-arm-kernel@lists.infradead.org,
linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
Janusz Uzycki, linux-gpio@vger.kernel.org, Yoshinori Sato
In-Reply-To: <20170324083905.h5uhdgiycchcjcgs@pengutronix.de>
Hi Uwe,
On Fri, Mar 24, 2017 at 9:39 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Fri, Mar 24, 2017 at 09:29:02AM +0100, Geert Uytterhoeven wrote:
>> On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> > Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled
>> >
>> > People disagree if gpiod_get_optional should return NULL or
>> > ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that
>> > the person who decided to disable GPIOLIB is assumed to know that there
>> > is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might
>> > introduce hard to debug problems if that decision is wrong.
>> >
>> > So this patch introduces a compromise and let gpiod_get_optional (and
>> > its variants) return NULL if the device in question cannot have an
>> > associated GPIO because it is neither instantiated by a device tree nor
>> > by ACPI.
>> >
>> > This should handle most cases that are argued about.
>> >
>> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> > ---
>> > include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++-------
>> > 1 file changed, 46 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
>> > index fb0fde686cb1..0ca29889290d 100644
>> > --- a/include/linux/gpio/consumer.h
>> > +++ b/include/linux/gpio/consumer.h
>> > @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev,
>> > return ERR_PTR(-ENOSYS);
>> > }
>> >
>> > -static inline struct gpio_desc *__must_check
>> > -gpiod_get_optional(struct device *dev, const char *con_id,
>> > - enum gpiod_flags flags)
>> > +static inline bool __gpiod_no_optional_possible(struct device *dev)
>> > {
>> > - return ERR_PTR(-ENOSYS);
>> > + /*
>> > + * gpiod_get_optional et al can only provide a GPIO if at least one of
>> > + * the backends for specifing a GPIO is available. These are device
>> > + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if
>> > + * GPIOLIB is disabled (which is the case here).
>> > + * So if the provided device is unrelated to device tree and ACPI, we
>> > + * can be sure that there is no optional GPIO and let gpiod_get_optional
>> > + * safely return NULL.
>> > + * Otherwise there is still a chance that there is no GPIO but we cannot
>> > + * be sure without having to enable a part of GPIOLIB (i.e. the lookup
>> > + * part). So lets play safe and return an error. (Though there are also
>> > + * arguments that returning NULL then would be beneficial.)
>> > + */
>> > +
>> > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
>> > + return false;
>>
>> At first sight, I though this was OK:
>>
>> 1. On ARM with DT, we can assume CONFIG_GPIOLOB=y.
>>
>> 2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y,
>> and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do
>> not use DT (yet), the check for dev->of_node (false) should handle
>> that.
>>
>> 3. However, I managed to do the same for h8300, which does use DT. Hence
>> if mctrl_gpio would start relying on gpiod_get_optional(), this would
>> break the sh-sci driver on h8300 :-(
>> Note that h8300 doesn't have any GPIO drivers (yet?), so
>> CONFIG_GPIPOLIB=n makes perfect sense!
>
> Thanks for your efforts.
You're welcome.
>> So I'm afraid the only option is to always return NULL, and put the
>> responsability on the shoulders of the system integrator...
>
> The gpio lines could be provided by an i2c gpio adapter, right? So IMHO
> you don't need platform gpios to justify -ENODEV. So I guess that's a
> case where we don't come to an agreement.
While you can enable I2C without further dependencies, no I2C GPIO expander
will be offered... unless you have enabled CONFIG_GPIOLIB first.
>> > static inline struct gpio_desc *__must_check
>> > gpiod_get_index_optional(struct device *dev, const char *con_id,
>> > unsigned int index, enum gpiod_flags flags)
>> > {
>> > + if (__gpiod_no_optional_possible(dev))
>> > + return NULL;
>> > +
>> > return ERR_PTR(-ENOSYS);
>>
>> Regardless of the above, given you use the exact same construct in four
>> locations, what about letting __gpiod_no_optional_possible() return the NULL
>> or ERR_PTR itself, and renaming it to e.g. __gpiod_no_optional_return_value()?
>
> I thought about that but didn't find a good name and so considered it
> more clear this way. Another optimisation would be to unconditionally
> define get_optional in terms of get_index_optional which would simplify
> my patch a bit.
>
> I'd consider __gpiod_optional_return_value a better name than
> __gpiod_no_optional_return_value but I'm still not convinced.
No hard feelings about the name from my side.
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
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Linus Walleij @ 2017-03-24 8:58 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Dmitry Torokhov, Geert Uytterhoeven, Richard Genoud,
Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre,
Boris Brezillon, linux-arm-kernel@lists.infradead.org,
linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
Janusz Uzycki, linux-gpio@vger.kernel.org
In-Reply-To: <20170323191020.nrstp2cfh7cuqvf3@pengutronix.de>
On Thu, Mar 23, 2017 at 8:10 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Thu, Mar 23, 2017 at 08:44:41AM -0700, Dmitry Torokhov wrote:
>> On Thu, Mar 23, 2017 at 07:43:25AM -0700, Dmitry Torokhov wrote:
>> > On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote:
>> > > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König
>> > > <u.kleine-koenig@pengutronix.de> wrote:
>> > >
>> > > > Maybe we can make gpiod_get_optional look like this:
>> > > >
>> > > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB))
>> > > > return NULL;
>> > > > else
>> > > > return -ENOSYS;
>> > > >
>> > > > I don't know how isnt_a_acpi_device looks like, probably it involves
>> > > > CONFIG_ACPI and/or dev->acpi_node.
>> > > >
>> > > > This should be safe and still comfortable for legacy platforms, isn't it?
>> > >
>> > > I like the looks of this.
>> > >
>> > > Can we revert Dmitry's patch and apply something like this instead?
>> > >
>> > > Dmitry, how do you feel about this?
>> >
>> > I frankly do not see the point. It still makes driver code more complex
>
> Note that this code is in the gpio header, and not in driver code. So
> the driver just does
>
> gpiod = gpiod_get_optional(...)
> if (IS_ERR(gpiod))
> return PTR_ERR(gpiod);
>
> (as it is supposed to do now). I think that's nice.
It does look nice. Compare this to what we must do for optional regulators:
st->reg = devm_regulator_get_optional(&spi->dev, "vref");
if (IS_ERR(st->reg)) {
/* Any other error indicates that the regulator does exist */
if (PTR_ERR(st->reg) != -ENODEV)
return PTR_ERR(st->reg);
/* Use internal reference */
}
So for optional regulators we get -ENODEV if we don't have it,
and then proceed on an alternate path, such as using an internal
reference voltage.
However the fact that regulators and GPIO optionals are already
handled differently is creating "cognitive dissonance" or what I
should call it.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Uwe Kleine-König @ 2017-03-24 8:39 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Dmitry Torokhov, Linus Walleij, Richard Genoud,
Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre,
Boris Brezillon, linux-arm-kernel@lists.infradead.org,
linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
Janusz Uzycki, linux-gpio@vger.kernel.org, Yoshinori Sato
In-Reply-To: <CAMuHMdXnUzjb5wzMtmzY2ojpGbhr_3KFGDYd2QKEawVipBhdbw@mail.gmail.com>
Hello Geert,
On Fri, Mar 24, 2017 at 09:29:02AM +0100, Geert Uytterhoeven wrote:
> On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled
> >
> > People disagree if gpiod_get_optional should return NULL or
> > ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that
> > the person who decided to disable GPIOLIB is assumed to know that there
> > is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might
> > introduce hard to debug problems if that decision is wrong.
> >
> > So this patch introduces a compromise and let gpiod_get_optional (and
> > its variants) return NULL if the device in question cannot have an
> > associated GPIO because it is neither instantiated by a device tree nor
> > by ACPI.
> >
> > This should handle most cases that are argued about.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 46 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> > index fb0fde686cb1..0ca29889290d 100644
> > --- a/include/linux/gpio/consumer.h
> > +++ b/include/linux/gpio/consumer.h
> > @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev,
> > return ERR_PTR(-ENOSYS);
> > }
> >
> > -static inline struct gpio_desc *__must_check
> > -gpiod_get_optional(struct device *dev, const char *con_id,
> > - enum gpiod_flags flags)
> > +static inline bool __gpiod_no_optional_possible(struct device *dev)
> > {
> > - return ERR_PTR(-ENOSYS);
> > + /*
> > + * gpiod_get_optional et al can only provide a GPIO if at least one of
> > + * the backends for specifing a GPIO is available. These are device
> > + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if
> > + * GPIOLIB is disabled (which is the case here).
> > + * So if the provided device is unrelated to device tree and ACPI, we
> > + * can be sure that there is no optional GPIO and let gpiod_get_optional
> > + * safely return NULL.
> > + * Otherwise there is still a chance that there is no GPIO but we cannot
> > + * be sure without having to enable a part of GPIOLIB (i.e. the lookup
> > + * part). So lets play safe and return an error. (Though there are also
> > + * arguments that returning NULL then would be beneficial.)
> > + */
> > +
> > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> > + return false;
>
> At first sight, I though this was OK:
>
> 1. On ARM with DT, we can assume CONFIG_GPIOLOB=y.
>
> 2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y,
> and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do
> not use DT (yet), the check for dev->of_node (false) should handle
> that.
>
> 3. However, I managed to do the same for h8300, which does use DT. Hence
> if mctrl_gpio would start relying on gpiod_get_optional(), this would
> break the sh-sci driver on h8300 :-(
> Note that h8300 doesn't have any GPIO drivers (yet?), so
> CONFIG_GPIPOLIB=n makes perfect sense!
Thanks for your efforts.
> So I'm afraid the only option is to always return NULL, and put the
> responsability on the shoulders of the system integrator...
The gpio lines could be provided by an i2c gpio adapter, right? So IMHO
you don't need platform gpios to justify -ENODEV. So I guess that's a
case where we don't come to an agreement.
> > + if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_COMPANION(dev))
> > + return false;
>
> No comments about the ACPI case.
>
> > static inline struct gpio_desc *__must_check
> > gpiod_get_index_optional(struct device *dev, const char *con_id,
> > unsigned int index, enum gpiod_flags flags)
> > {
> > + if (__gpiod_no_optional_possible(dev))
> > + return NULL;
> > +
> > return ERR_PTR(-ENOSYS);
>
> Regardless of the above, given you use the exact same construct in four
> locations, what about letting __gpiod_no_optional_possible() return the NULL
> or ERR_PTR itself, and renaming it to e.g. __gpiod_no_optional_return_value()?
I thought about that but didn't find a good name and so considered it
more clear this way. Another optimisation would be to unconditionally
define get_optional in terms of get_index_optional which would simplify
my patch a bit.
I'd consider __gpiod_optional_return_value a better name than
__gpiod_no_optional_return_value but I'm still not convinced.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Geert Uytterhoeven @ 2017-03-24 8:29 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Boris Brezillon, Yoshinori Sato, Geert Uytterhoeven,
Richard Genoud, Greg Kroah-Hartman, Linus Walleij,
Dmitry Torokhov, linux-kernel@vger.kernel.org,
linux-gpio@vger.kernel.org, Janusz Uzycki,
linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20170324080006.tbhyqlgk35xybsna@pengutronix.de>
Hi Uwe,
On Fri, Mar 24, 2017 at 9:00 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled
>
> People disagree if gpiod_get_optional should return NULL or
> ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that
> the person who decided to disable GPIOLIB is assumed to know that there
> is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might
> introduce hard to debug problems if that decision is wrong.
>
> So this patch introduces a compromise and let gpiod_get_optional (and
> its variants) return NULL if the device in question cannot have an
> associated GPIO because it is neither instantiated by a device tree nor
> by ACPI.
>
> This should handle most cases that are argued about.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index fb0fde686cb1..0ca29889290d 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev,
> return ERR_PTR(-ENOSYS);
> }
>
> -static inline struct gpio_desc *__must_check
> -gpiod_get_optional(struct device *dev, const char *con_id,
> - enum gpiod_flags flags)
> +static inline bool __gpiod_no_optional_possible(struct device *dev)
> {
> - return ERR_PTR(-ENOSYS);
> + /*
> + * gpiod_get_optional et al can only provide a GPIO if at least one of
> + * the backends for specifing a GPIO is available. These are device
> + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if
> + * GPIOLIB is disabled (which is the case here).
> + * So if the provided device is unrelated to device tree and ACPI, we
> + * can be sure that there is no optional GPIO and let gpiod_get_optional
> + * safely return NULL.
> + * Otherwise there is still a chance that there is no GPIO but we cannot
> + * be sure without having to enable a part of GPIOLIB (i.e. the lookup
> + * part). So lets play safe and return an error. (Though there are also
> + * arguments that returning NULL then would be beneficial.)
> + */
> +
> + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> + return false;
At first sight, I though this was OK:
1. On ARM with DT, we can assume CONFIG_GPIOLOB=y.
2. I managed to configure an SH kernel with CONFIG_GPIOLOB=n, CONFIG_OF=y,
and CONFIG_SERIAL_SH_SCI=y, but since SH boards with SH-SCI UARTs do
not use DT (yet), the check for dev->of_node (false) should handle
that.
3. However, I managed to do the same for h8300, which does use DT. Hence
if mctrl_gpio would start relying on gpiod_get_optional(), this would
break the sh-sci driver on h8300 :-(
Note that h8300 doesn't have any GPIO drivers (yet?), so
CONFIG_GPIPOLIB=n makes perfect sense!
So I'm afraid the only option is to always return NULL, and put the
responsability on the shoulders of the system integrator...
> + if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_COMPANION(dev))
> + return false;
No comments about the ACPI case.
> static inline struct gpio_desc *__must_check
> gpiod_get_index_optional(struct device *dev, const char *con_id,
> unsigned int index, enum gpiod_flags flags)
> {
> + if (__gpiod_no_optional_possible(dev))
> + return NULL;
> +
> return ERR_PTR(-ENOSYS);
Regardless of the above, given you use the exact same construct in four
locations, what about letting __gpiod_no_optional_possible() return the NULL
or ERR_PTR itself, and renaming it to e.g. __gpiod_no_optional_return_value()?
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Uwe Kleine-König @ 2017-03-24 8:00 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud,
Greg Kroah-Hartman, Linus Walleij, linux-kernel@vger.kernel.org,
linux-gpio@vger.kernel.org, Janusz Uzycki, Geert Uytterhoeven,
linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20170323195804.GA2502@dtor-ws>
Hello Dmitry,
On Thu, Mar 23, 2017 at 12:58:04PM -0700, Dmitry Torokhov wrote:
> On Thu, Mar 23, 2017 at 08:10:20PM +0100, Uwe Kleine-König wrote:
> > On Thu, Mar 23, 2017 at 08:44:41AM -0700, Dmitry Torokhov wrote:
> > > On Thu, Mar 23, 2017 at 07:43:25AM -0700, Dmitry Torokhov wrote:
> > > > On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote:
> > > > > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > >
> > > > > > Maybe we can make gpiod_get_optional look like this:
> > > > > >
> > > > > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB))
> > > > > > return NULL;
> > > > > > else
> > > > > > return -ENOSYS;
> > > > > >
> > > > > > I don't know how isnt_a_acpi_device looks like, probably it involves
> > > > > > CONFIG_ACPI and/or dev->acpi_node.
> > > > > >
> > > > > > This should be safe and still comfortable for legacy platforms, isn't it?
> > > > >
> > > > > I like the looks of this.
> > > > >
> > > > > Can we revert Dmitry's patch and apply something like this instead?
> > > > >
> > > > > Dmitry, how do you feel about this?
> > > >
> > > > I frankly do not see the point. It still makes driver code more complex
> >
> > Note that this code is in the gpio header, and not in driver code. So
> > the driver just does
> >
> > gpiod = gpiod_get_optional(...)
> > if (IS_ERR(gpiod))
> > return PTR_ERR(gpiod);
> >
> > (as it is supposed to do now). I think that's nice.
>
> Except that it breaks if !CONFIG_GPIOLIB which is legitimate config in
> many cases. Can I have a DT platform or ACPI platform that does not
> expose any GPIOs for kernel to control and thus want to disable GPIOLIB?
> I can't see why not.
>
> >
> > > > for no good reason. I also think that not having optional GPIO is not an
> > > > error, so returning value from error space is not correct. NULL is value
> > > > from another space altogether.
> >
> > It seems you didn't understand my concern.
>
> Likewise.
OK, lets agree that we don't agree then. You think that if someone
disabled GPIOLIB it should be safe to assume that there is no GPIO, I
think that's wrong.
Still I think it should be possible that we agree on my suggestion to
return NULL in some cases only. Here is a prototype (not even compile
tested without GPIOLIB):
------->8--------
From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled
People disagree if gpiod_get_optional should return NULL or
ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that
the person who decided to disable GPIOLIB is assumed to know that there
is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might
introduce hard to debug problems if that decision is wrong.
So this patch introduces a compromise and let gpiod_get_optional (and
its variants) return NULL if the device in question cannot have an
associated GPIO because it is neither instantiated by a device tree nor
by ACPI.
This should handle most cases that are argued about.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 46 insertions(+), 9 deletions(-)
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index fb0fde686cb1..0ca29889290d 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev,
return ERR_PTR(-ENOSYS);
}
-static inline struct gpio_desc *__must_check
-gpiod_get_optional(struct device *dev, const char *con_id,
- enum gpiod_flags flags)
+static inline bool __gpiod_no_optional_possible(struct device *dev)
{
- return ERR_PTR(-ENOSYS);
+ /*
+ * gpiod_get_optional et al can only provide a GPIO if at least one of
+ * the backends for specifing a GPIO is available. These are device
+ * tree, ACPI and gpiolib's lookup tables. The latter isn't available if
+ * GPIOLIB is disabled (which is the case here).
+ * So if the provided device is unrelated to device tree and ACPI, we
+ * can be sure that there is no optional GPIO and let gpiod_get_optional
+ * safely return NULL.
+ * Otherwise there is still a chance that there is no GPIO but we cannot
+ * be sure without having to enable a part of GPIOLIB (i.e. the lookup
+ * part). So lets play safe and return an error. (Though there are also
+ * arguments that returning NULL then would be beneficial.)
+ */
+
+ if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
+ return false;
+
+ if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_COMPANION(dev))
+ return false;
+
+ return true;
}
static inline struct gpio_desc *__must_check
gpiod_get_index_optional(struct device *dev, const char *con_id,
unsigned int index, enum gpiod_flags flags)
{
+ if (__gpiod_no_optional_possible(dev))
+ return NULL;
+
return ERR_PTR(-ENOSYS);
}
+static inline struct gpio_desc *__must_check
+gpiod_get_optional(struct device *dev, const char *con_id,
+ enum gpiod_flags flags)
+{
+ return gpiod_get_index_optional(dev, con_id, 0, flags);
+}
+
static inline struct gpio_descs *__must_check
gpiod_get_array(struct device *dev, const char *con_id,
enum gpiod_flags flags)
@@ -186,6 +214,9 @@ static inline struct gpio_descs *__must_check
gpiod_get_array_optional(struct device *dev, const char *con_id,
enum gpiod_flags flags)
{
+ if (__gpiod_no_optional_possible(dev))
+ return NULL;
+
return ERR_PTR(-ENOSYS);
}
@@ -223,17 +254,20 @@ devm_gpiod_get_index(struct device *dev,
}
static inline struct gpio_desc *__must_check
-devm_gpiod_get_optional(struct device *dev, const char *con_id,
- enum gpiod_flags flags)
+devm_gpiod_get_index_optional(struct device *dev, const char *con_id,
+ unsigned int index, enum gpiod_flags flags)
{
+ if (__gpiod_no_optional_possible(dev))
+ return NULL;
+
return ERR_PTR(-ENOSYS);
}
static inline struct gpio_desc *__must_check
-devm_gpiod_get_index_optional(struct device *dev, const char *con_id,
- unsigned int index, enum gpiod_flags flags)
+devm_gpiod_get_optional(struct device *dev, const char *con_id,
+ enum gpiod_flags flags)
{
- return ERR_PTR(-ENOSYS);
+ return devm_gpiod_get_index_optional(dev, con_id, 0, flags);
}
static inline struct gpio_descs *__must_check
@@ -247,6 +281,9 @@ static inline struct gpio_descs *__must_check
devm_gpiod_get_array_optional(struct device *dev, const char *con_id,
enum gpiod_flags flags)
{
+ if (__gpiod_no_optional_possible(dev))
+ return NULL;
+
return ERR_PTR(-ENOSYS);
}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply related
* [PATCH v2 2/2] serial: 8250: 8250_core: Fix irq name for 8250 serial IRQ
From: Vignesh R @ 2017-03-24 5:28 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jisheng Zhang, Vignesh R, linux-kernel, Andy Shevchenko,
linux-serial, Jiri Slaby, linux-arm-kernel
In-Reply-To: <20170324052800.11526-1-vigneshr@ti.com>
Using dev_name() as IRQ name during request_irq() might be misleading in
case of serial over PCI. Therefore identify serial port IRQ using
uart_port's name field. This will help mapping IRQs to appropriate
ttySN(where N is the serial port index) instances.
Signed-off-by: Vignesh R <vigneshr@ti.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
v2:
Pick up reviewed by
s/irq(s)/IRQ(s) in commit message
drivers/tty/serial/8250/8250_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index f83b69f30987..48a07e2f617f 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -218,7 +218,7 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
spin_unlock_irq(&i->lock);
irq_flags |= up->port.irqflags;
ret = request_irq(up->port.irq, serial8250_interrupt,
- irq_flags, dev_name(up->port.dev), i);
+ irq_flags, up->port.name, i);
if (ret < 0)
serial_do_unlink(i, up);
}
--
2.11.0
^ permalink raw reply related
* [PATCH v2 1/2] tty: serial_core: Add name field to uart_port struct
From: Vignesh R @ 2017-03-24 5:27 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jisheng Zhang, Vignesh R, linux-kernel, Andy Shevchenko,
linux-serial, Jiri Slaby, linux-arm-kernel
In-Reply-To: <20170324052800.11526-1-vigneshr@ti.com>
Introduce a field to store name of uart_port that can be used to easily
identify UART port instances on a system that has more than one UART
instance. The name is of the form ttyXN(eg. ttyS0, ttyAMA0,..) where N
is number that particular UART instance.
This field will be useful when printing debug info for a particular port
or in register IRQs with unique IRQ name. Port name is populated during
uart_add_one_port().
Signed-off-by: Vignesh R <vigneshr@ti.com>
---
v2:
use kasprintf() instead of snprintf()
Fix up commit message.
drivers/tty/serial/serial_core.c | 7 +++++++
include/linux/serial_core.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 0fb3f7cce62a..f5572e28d16a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2744,6 +2744,12 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
state->pm_state = UART_PM_STATE_UNDEFINED;
uport->cons = drv->cons;
uport->minor = drv->tty_driver->minor_start + uport->line;
+ uport->name = kasprintf(GFP_KERNEL, "%s%d", drv->dev_name,
+ drv->tty_driver->name_base + uport->line);
+ if (!uport->name) {
+ ret = -ENOMEM;
+ goto out;
+ }
/*
* If this port is a console, then the spinlock is already
@@ -2861,6 +2867,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
if (uport->type != PORT_UNKNOWN && uport->ops->release_port)
uport->ops->release_port(uport);
kfree(uport->tty_groups);
+ kfree(uport->name);
/*
* Indicate that there isn't a port here anymore.
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 58484fb35cc8..60530678c633 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -247,6 +247,7 @@ struct uart_port {
unsigned char suspended;
unsigned char irq_wake;
unsigned char unused[2];
+ char *name; /* port name */
struct attribute_group *attr_group; /* port specific attributes */
const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
struct serial_rs485 rs485;
--
2.11.0
^ permalink raw reply related
* [PATCH v2 0/2] serial: Add name field to uart_port
From: Vignesh R @ 2017-03-24 5:27 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jisheng Zhang, Vignesh R, linux-kernel, Andy Shevchenko,
linux-serial, Jiri Slaby, linux-arm-kernel
This series adds name attribute to uart_port struct inorder to store
name of the tty port that will help in identify different uart
instances.
This patch series is based on discussion here[1].
[1] https://www.spinics.net/lists/arm-kernel/msg569188.html
Previous version: https://lkml.org/lkml/2017/3/21/328
Vignesh R (2):
tty: serial_core: Add name field to uart_port struct
serial: 8250: 8250_core: Fix irq name for 8250 serial IRQ
drivers/tty/serial/8250/8250_core.c | 2 +-
drivers/tty/serial/serial_core.c | 7 +++++++
include/linux/serial_core.h | 1 +
3 files changed, 9 insertions(+), 1 deletion(-)
--
2.11.0
^ permalink raw reply
* RE: [PATCH V7 5/7] ACPI: Delay the enumeration on the devices whose dependency has not met
From: Gabriele Paoloni @ 2017-03-24 0:23 UTC (permalink / raw)
To: Gabriele Paoloni, Arnd Bergmann, Yuanzhichang
Cc: Mark Rutland, Rafael Wysocki, Catalin Marinas, Will Deacon,
Linuxarm, Frank Rowand, Lorenzo Pieralisi, ACPI Devel Maling List,
linux-serial@vger.kernel.org, Benjamin Herrenschmidt,
devicetree@vger.kernel.org, Corey Minyard, liviu.dudau@arm.com,
zhichang.yuan02@gmail.com, Rob Herring, Bjorn Helgaas,
kantyzc@163.com, Linux ARM, linux-pci, Rafael J. Wysocki
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E20492A4C@FRAEML521-MBX.china.huawei.com>
Hi Arnd
> -----Original Message-----
> From: linuxarm-bounces@huawei.com [mailto:linuxarm-bounces@huawei.com]
> On Behalf Of Gabriele Paoloni
> Sent: 16 March 2017 16:14
> To: Arnd Bergmann; Yuanzhichang
> Cc: Mark Rutland; Benjamin Herrenschmidt; Rafael Wysocki; linux-pci;
> Will Deacon; Linuxarm; Frank Rowand; Lorenzo Pieralisi; ACPI Devel
> Maling List; linux-serial@vger.kernel.org; Catalin Marinas;
> devicetree@vger.kernel.org; Corey Minyard; liviu.dudau@arm.com; Rob
> Herring; Bjorn Helgaas; kantyzc@163.com; zhichang.yuan02@gmail.com;
> Linux ARM; Rafael J. Wysocki; Linux Kernel Mailing List; Zou Rongrong
> Subject: RE: [PATCH V7 5/7] ACPI: Delay the enumeration on the devices
> whose dependency has not met
>
> Hi Arnd
>
> > -----Original Message-----
> > From: arndbergmann@gmail.com [mailto:arndbergmann@gmail.com] On
> Behalf
> > Of Arnd Bergmann
> > Sent: 16 March 2017 10:13
> > To: Yuanzhichang
> > Cc: Rafael J. Wysocki; Catalin Marinas; Will Deacon; Rob Herring;
> Frank
> > Rowand; Bjorn Helgaas; Rafael Wysocki; Mark Rutland; Linux ARM; ACPI
> > Devel Maling List; Lorenzo Pieralisi; Benjamin Herrenschmidt; Linux
> > Kernel Mailing List; Linuxarm; devicetree@vger.kernel.org; linux-pci;
> > linux-serial@vger.kernel.org; Corey Minyard; liviu.dudau@arm.com; Zou
> > Rongrong; John Garry; Gabriele Paoloni; zhichang.yuan02@gmail.com;
> > kantyzc@163.com; xuwei (O)
> > Subject: Re: [PATCH V7 5/7] ACPI: Delay the enumeration on the
> devices
> > whose dependency has not met
> >
> > On Thu, Mar 16, 2017 at 3:21 AM, zhichang.yuan
> > <yuanzhichang@hisilicon.com> wrote:
> > > Hi, Rafael,
> > >
> > > Thanks for your review!
> > >
> > > On 2017/3/14 5:24, Rafael J. Wysocki wrote:
> > >> On Monday, March 13, 2017 10:42:41 AM zhichang.yuan wrote:
> > >>> In commit 40e7fcb1929(ACPI: Add _DEP support to fix battery issue
> > on Asus
> > >>> T100TA), the '_DEP' was supported to solve the dependency of Asus
> > battery. But
> > >>> this patch is specific to Asus battery device.
> > >>> In the real world, there are other devices which need the
> > dependency to play the
> > >>> role on the enumeration order. For example, all the Hip06 LPC
> > >>> periperals(IPMI-BT, uart, etc) must be scanned after the LPC host
> > driver
> > >>> finished the probing. So, it makes sense to add a checking
> whether
> > the ACPI
> > >>> device meet all the dependencies during its enumeration slot, if
> > not, the
> > >>> enumeration will be delayed till all dependency master finish
> their
> > work.
> > >>>
> > >>> This patch adds the dependency checking in ACPI enumeration, also
> > the
> > >>> corresponding handling to retrigger the Hip06 LPC peripherals'
> > scanning.
> > >>
> > >> AFAICS, _DEP is generally abused in the wild and cannot be made
> > generic. Sorry.
> > >>
> > >
> > > From the ACPI specification, _DEP is for operation region accesses.
> > > You are right...
> > >
> > > How about we add a ACPI handler for our LPC bus?? Just like amba.
> > > In this way, we also can solve the issue about LPC enumeration
> order.
> >
> > As far as I can tell, PCI and LPC have exactly the same requirement
> > here,
> > so whatever you end up doing for one should be used for the other as
> > well.
>
> Well as you know PCI has got his own handler, identified by his own
> namespace id "PNP0A03".
> Now when you say "you end up doing for one should be used for the
> other"
> are you saying that we should introduce a new class of devices?
> i.e. should we have an ACPI namespace identifier for non-PCI IO Host
> Controllers?
>
> Otherwise, if my understanding is correct, having a specific new ACPI
> handler for HiSilicon LPC would mean to adding another function_init()
> in the list of acpi handlers inits in acpi_scan_init().
>
> But then every vendor would declare his own one...is this really
> correct?
Do you have any feedback on this?
Otherwise I think that maybe we could consider moving back to the
arch_initcall approach as proposed in V6:
https://lkml.org/lkml/2017/1/24/28
Thanks
Gab
>
> Many Thanks
> Gab
>
> >
> > Arnd
> _______________________________________________
> linuxarm mailing list
> linuxarm@huawei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
^ permalink raw reply
* Re: [PATCH v2 2/2] serial: samsung: Add missing checks for dma_map_single failure
From: Krzysztof Kozlowski @ 2017-03-23 20:04 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-samsung-soc, linux-serial, Sylwester Nawrocki,
Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Seung-Woo Kim,
Joonyoung Shim, Inki Dae, stable
In-Reply-To: <1490270519-4046-2-git-send-email-m.szyprowski@samsung.com>
On Thu, Mar 23, 2017 at 01:01:53PM +0100, Marek Szyprowski wrote:
> This patch adds missing checks for dma_map_single() failure and proper error
> reporting. While touching this part of the code, it also removes unnecessary
> spinlock calls around dma_map_single() for TX buffer. This finally solves all
> the issues reported by DMA API debug framework.
>
> Reported-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> Fixes: 62c37eedb74c8 ("serial: samsung: add dma reqest/release functions")
> CC: stable@vger.kernel.org # v4.10+
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
> This issue was there since adding DMA support, but this patch applies cleanly
> only to v4.10+ kernels due to other changes in the surrounding code.
>
> v2:
> - fixed commit id in 'fixes' tag, added 'reviewed-by' tag
> ---
> drivers/tty/serial/samsung.c | 35 ++++++++++++++++++++++++-----------
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> index 9f3759bdb44f..8aca18c4cdea 100644
> --- a/drivers/tty/serial/samsung.c
> +++ b/drivers/tty/serial/samsung.c
> @@ -859,7 +859,7 @@ static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
> static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
> {
> struct s3c24xx_uart_dma *dma = p->dma;
> - unsigned long flags;
> + int ret;
>
> /* Default slave configuration parameters */
> dma->rx_conf.direction = DMA_DEV_TO_MEM;
> @@ -884,8 +884,8 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
>
> dma->tx_chan = dma_request_chan(p->port.dev, "tx");
> if (IS_ERR(dma->tx_chan)) {
> - dma_release_channel(dma->rx_chan);
> - return PTR_ERR(dma->tx_chan);
> + ret = PTR_ERR(dma->tx_chan);
> + goto err_release_rx;
> }
>
> dmaengine_slave_config(dma->tx_chan, &dma->tx_conf);
> @@ -894,25 +894,38 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
> dma->rx_size = PAGE_SIZE;
>
> dma->rx_buf = kmalloc(dma->rx_size, GFP_KERNEL);
> -
> if (!dma->rx_buf) {
> - dma_release_channel(dma->rx_chan);
> - dma_release_channel(dma->tx_chan);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto err_release_tx;
> }
>
> dma->rx_addr = dma_map_single(p->port.dev, dma->rx_buf,
> dma->rx_size, DMA_FROM_DEVICE);
> -
> - spin_lock_irqsave(&p->port.lock, flags);
Error paths look fine but how about splitting spinlock removal out of
this patch? Logically they are two different changes and unrelated code
should not be backported.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Dmitry Torokhov @ 2017-03-23 19:58 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Linus Walleij, Geert Uytterhoeven, Richard Genoud,
Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre,
Boris Brezillon, linux-arm-kernel@lists.infradead.org,
linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
Janusz Uzycki, linux-gpio@vger.kernel.org
In-Reply-To: <20170323191020.nrstp2cfh7cuqvf3@pengutronix.de>
On Thu, Mar 23, 2017 at 08:10:20PM +0100, Uwe Kleine-König wrote:
> On Thu, Mar 23, 2017 at 08:44:41AM -0700, Dmitry Torokhov wrote:
> > On Thu, Mar 23, 2017 at 07:43:25AM -0700, Dmitry Torokhov wrote:
> > > On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote:
> > > > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > > > > Maybe we can make gpiod_get_optional look like this:
> > > > >
> > > > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB))
> > > > > return NULL;
> > > > > else
> > > > > return -ENOSYS;
> > > > >
> > > > > I don't know how isnt_a_acpi_device looks like, probably it involves
> > > > > CONFIG_ACPI and/or dev->acpi_node.
> > > > >
> > > > > This should be safe and still comfortable for legacy platforms, isn't it?
> > > >
> > > > I like the looks of this.
> > > >
> > > > Can we revert Dmitry's patch and apply something like this instead?
> > > >
> > > > Dmitry, how do you feel about this?
> > >
> > > I frankly do not see the point. It still makes driver code more complex
>
> Note that this code is in the gpio header, and not in driver code. So
> the driver just does
>
> gpiod = gpiod_get_optional(...)
> if (IS_ERR(gpiod))
> return PTR_ERR(gpiod);
>
> (as it is supposed to do now). I think that's nice.
Except that it breaks if !CONFIG_GPIOLIB which is legitimate config in
many cases. Can I have a DT platform or ACPI platform that does not
expose any GPIOs for kernel to control and thus want to disable GPIOLIB?
I can't see why not.
>
> > > for no good reason. I also think that not having optional GPIO is not an
> > > error, so returning value from error space is not correct. NULL is value
> > > from another space altogether.
>
> It seems you didn't understand my concern.
Likewise.
>
> > > Uwe seems to be concerned about case that I find extremely unlikely. We
> > > are talking about a system that does not have GPIO support and behaves
> > > just fine, with the exception that it actually has (physically) a
> > > *single* GPIO, and that GPIO happens to be optional in a single driver,
> > > but in this particular system is actually needed (but that need
> > > manifests in a non-obvious way). And we have system integrator that has
> > > no idea what they are doing (no schematic, etc).
>
> IMHO this is not an excuse to have lax error checking.
IMO it is not an error altogether, so there is no lax checking.
>
> > > I think that if there is one optional GPIO there will be mandatiry GPIOs
> > > in such system as well and selection of GPIOLIB will be forced early on
> > > in board bringup.
> >
> > One more thing: if we keep reporting -ENOSYS in case of !CONFIG_GPIOLIB,
> > then most of the non platform-sepcific drivers will eventually gain code
> > silently coping with this -ENOSYS:
> >
> > data->gpiod = gpiod_getptional(...);
> > if (IS_ERR(data->gpiod)) {
> > error = PTR_ERR(data->gpiod);
> > if (error != -ENOSYS)
> > return error;
> >
> > data->gpiod = NULL; /* This GPIO _is_ optional */
>
> Do you agree that this is wrong? Yes, it behaves right for most cases.
> But there are cases where it behaves wrong and so it needs fixing.
I think by now it should be obvious that I do not find it wrong. In fact
that is what I, as a maintainer of drivers that supposed to work on
multitude of platforms, will be forced to do, if the change to stop
reporting -ENOSYS gets reverted. A generic driver has no way to know
that kernel configuration or firmware configuration is wrong and should
not be trusted (except for piling up horrendous DMI hacks in some
cases on X86).
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/2] serial: samsung: Use right device for DMA-mapping calls
From: Krzysztof Kozlowski @ 2017-03-23 19:51 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-samsung-soc, linux-serial, Sylwester Nawrocki,
Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Seung-Woo Kim,
Joonyoung Shim, Inki Dae, stable
In-Reply-To: <1490270519-4046-1-git-send-email-m.szyprowski@samsung.com>
On Thu, Mar 23, 2017 at 01:01:52PM +0100, Marek Szyprowski wrote:
> Driver should provide its own struct device for all DMA-mapping calls instead
> of extracting device pointer from DMA engine channel.
For the purpose of cc-stable it might be useful to answer here what
visible error is fixed (or why driver should not use DMA's device) but
on the other hand this is actually obvious... Anyway:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
> Reported-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> Fixes: 62c37eedb74c8 ("serial: samsung: add dma reqest/release functions")
> CC: stable@vger.kernel.org # v4.0+
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
> v2:
> - fixed commit id in 'fixes' tag, added 'reviewed-by' tag
> ---
> drivers/tty/serial/samsung.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Uwe Kleine-König @ 2017-03-23 19:10 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linus Walleij, Geert Uytterhoeven, Richard Genoud,
Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre,
Boris Brezillon, linux-arm-kernel@lists.infradead.org,
linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
Janusz Uzycki, linux-gpio@vger.kernel.org
In-Reply-To: <20170323154441.GA9460@dtor-ws>
On Thu, Mar 23, 2017 at 08:44:41AM -0700, Dmitry Torokhov wrote:
> On Thu, Mar 23, 2017 at 07:43:25AM -0700, Dmitry Torokhov wrote:
> > On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote:
> > > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > > Maybe we can make gpiod_get_optional look like this:
> > > >
> > > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB))
> > > > return NULL;
> > > > else
> > > > return -ENOSYS;
> > > >
> > > > I don't know how isnt_a_acpi_device looks like, probably it involves
> > > > CONFIG_ACPI and/or dev->acpi_node.
> > > >
> > > > This should be safe and still comfortable for legacy platforms, isn't it?
> > >
> > > I like the looks of this.
> > >
> > > Can we revert Dmitry's patch and apply something like this instead?
> > >
> > > Dmitry, how do you feel about this?
> >
> > I frankly do not see the point. It still makes driver code more complex
Note that this code is in the gpio header, and not in driver code. So
the driver just does
gpiod = gpiod_get_optional(...)
if (IS_ERR(gpiod))
return PTR_ERR(gpiod);
(as it is supposed to do now). I think that's nice.
> > for no good reason. I also think that not having optional GPIO is not an
> > error, so returning value from error space is not correct. NULL is value
> > from another space altogether.
It seems you didn't understand my concern.
> > Uwe seems to be concerned about case that I find extremely unlikely. We
> > are talking about a system that does not have GPIO support and behaves
> > just fine, with the exception that it actually has (physically) a
> > *single* GPIO, and that GPIO happens to be optional in a single driver,
> > but in this particular system is actually needed (but that need
> > manifests in a non-obvious way). And we have system integrator that has
> > no idea what they are doing (no schematic, etc).
IMHO this is not an excuse to have lax error checking.
> > I think that if there is one optional GPIO there will be mandatiry GPIOs
> > in such system as well and selection of GPIOLIB will be forced early on
> > in board bringup.
>
> One more thing: if we keep reporting -ENOSYS in case of !CONFIG_GPIOLIB,
> then most of the non platform-sepcific drivers will eventually gain code
> silently coping with this -ENOSYS:
>
> data->gpiod = gpiod_getptional(...);
> if (IS_ERR(data->gpiod)) {
> error = PTR_ERR(data->gpiod);
> if (error != -ENOSYS)
> return error;
>
> data->gpiod = NULL; /* This GPIO _is_ optional */
Do you agree that this is wrong? Yes, it behaves right for most cases.
But there are cases where it behaves wrong and so it needs fixing.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 0/2] serial: Add name field to uart_port
From: Andy Shevchenko @ 2017-03-23 17:07 UTC (permalink / raw)
To: Vignesh R
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-arm Mailing List,
Jisheng Zhang
In-Reply-To: <21173bda-55e5-b9d0-5eb1-335091fbca6a@ti.com>
On Thu, Mar 23, 2017 at 2:22 PM, Vignesh R <vigneshr@ti.com> wrote:
> On Thursday 23 March 2017 12:16 AM, Andy Shevchenko wrote:
>> On Tue, Mar 21, 2017 at 3:36 PM, Vignesh R <vigneshr@ti.com> wrote:
>>> This series adds name attribute to uart_port struct inorder to store
>>> name of the tty port that will help in identify different uart
>>> instances.
>>> This patch series is based on discussion here[1].
>>
>> Please, use versioning for the series. This AFAIU should be v3, and
>> next (which is expecting based on comments) would be v4.
>
> The very first patch I submitted was accepted. So I did not make the
> previous patch as v2 as it was an incremental fix. Since, this series is
> very different from previous patch, I chose not to label this as v3.
Fair enough.
> Anyways, will make the next version as v4.
No need, then it makes sense to mark it as v2.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC RESEND] serdev binding for QCA7000 UART driver
From: Rob Herring @ 2017-03-23 15:55 UTC (permalink / raw)
To: Stefan Wahren
Cc: Frank Rowand, Greg Kroah-Hartman, Jiri Slaby,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <9127690c-3b6e-d60d-6af1-97bb9ad34628-eS4NqCHxEME@public.gmane.org>
On Thu, Mar 23, 2017 at 10:13 AM, Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org> wrote:
> Hi Rob,
>
> Am 22.03.2017 um 21:32 schrieb Rob Herring:
>> On Mon, Mar 13, 2017 at 11:16 AM, Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org> wrote:
>>> Hi Rob,
>>>
>>> since serdev has been merged, i'm currently working on a
>>> suitable ethernet driver for the QCA7000 [1]. In order to provide a plug
>>> and play solution for the network interface i suggest the following
>>> binding, which uses common properties for UART configuration:
>>>
>>> * Qualcomm QCA7000 (Ethernet over UART protocol)
>>>
>>> Note: The QCA7000 is also useable as a UART slave device.
>> Should this be s/UART/SPI/ ?
>
> i rewrote this note in my second version below.
>
>>
>>> Required properties:
>>> - compatible : Should be "qca,qca7000-uart"
>>>
>>> Optional properties:
>>> - local-mac-address : 6 bytes, Specifies MAC address
>>> - current-speed : Specifies current serial device speed in
>>> bits per second (default = 115200)
>>> - data-bits : Specifies number of data bits (default = 8)
>>> - use-parity : If present, this enables the parity
>>> error detection (default = off)
>>> - odd-parity : If present, this specifies that the parity
>>> of each character must be odd (default = even)
>>>
>>> Example:
>>>
>>> /* Freescale i.MX28 UART */
>>> auart0: serial@8006a000 {
>>> compatible = "fsl,imx28-auart", "fsl,imx23-auart";
>>> reg = <0x8006a000 0x2000>;
>>> pinctrl-names = "default";
>>> pinctrl-0 = <&auart0_2pins_a>;
>>> status = "okay";
>>>
>>> qca7000: ethernet {
>>> compatible = "qca,qca7000-uart";
>>> local-mac-address = [ A0 B0 C0 D0 E0 F0 ];
>>> current-speed = <38400>;
>> Unless this device supports auto-baud (generally only AT command set
>> devices do), this is setup by a bootloader (and you have no way to
>> reset the device), or the speed changes with different firmware, I
>> don't think you should need this. Devices typically have a fixed,
>> initial baudrate that is known, so the driver should know it.
>
> Unfortunately the QCA7000 firmware can be freely preconfigured to a
> fixed baudrate and there is no way to determine, because Ethernet frames
> are send nearly directly to the UART and vice versa. I agree this isn't
> best practice.
>
>>> data-bits = <8>;
>> Nearly everything uses 8 bits, so this should be the default.
>
> I will drop it completly.
>
>>
>>> use-parity;
>>> odd-parity;
>> I don't think we need 2 properties here. "even-parity" and
>> "odd-parity" booleans should be enough to enable and set parity type.
>
> Your suggestion is better, but it's possible to define wrongly both
> which isn't nice per design.
> Anyway i decide to drop this, too. So it could be implemented later.
could also do 'parity = "(even|odd)";' or a line-config property with
"8N1", "7E1", etc. for values though I tend to prefer booleans and
discreet properties per setting.
>
> So here is the new version:
>
> * Qualcomm QCA7000 (Ethernet over UART protocol)
>
> Note: This binding applies in case the QCA7000 is configured as a
> UART slave device. It is possible to preconfigure the UART settings
> of the QCA7000 firmware, which can't be changed during runtime.
>
> Required properties:
> - compatible : Should be "qca,qca7000-uart"
>
> Optional properties:
> - local-mac-address : 6 bytes, Specifies MAC address
> - current-speed : Specifies the serial device speed in
> bits per second (default = 115200), which is
> predefined by the QCA7000 firmware configuration
>
> Example:
>
> /* Freescale i.MX28 UART */
> auart0: serial@8006a000 {
> compatible = "fsl,imx28-auart", "fsl,imx23-auart";
> reg = <0x8006a000 0x2000>;
> pinctrl-names = "default";
> pinctrl-0 = <&auart0_2pins_a>;
> status = "okay";
>
> qca7000: ethernet {
> compatible = "qca,qca7000-uart";
> local-mac-address = [ A0 B0 C0 D0 E0 F0 ];
> current-speed = <38400>;
> };
> };
Looks good to me.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Dmitry Torokhov @ 2017-03-23 15:55 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Geert Uytterhoeven, Linus Walleij, Richard Genoud,
Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre,
Boris Brezillon, linux-arm-kernel@lists.infradead.org,
linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
Janusz Uzycki, linux-gpio@vger.kernel.org
In-Reply-To: <20170323111106.7ogh6g2oa3m4cqc6@pengutronix.de>
On Thu, Mar 23, 2017 at 12:11:06PM +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Thu, Mar 23, 2017 at 11:20:39AM +0100, Geert Uytterhoeven wrote:
> > But having the error breaks setups where the GPIO is optional and does
> > not exist.
>
> so the right way forward is to check harder in the situation where
> -ENOSYS was returned before to determine if there is really no GPIO to
> be used. "Oh, there are hints that there is no GPIO (GPIOLIB=n), so lets
> assume there isn't." is wrong.
>
> Can we please properly fix the problem instead of papering over it?
I think I once already said what would need to _attempt_ to fix it
"properly". You would need to implement custom parsing of ACPI tables in
GPIOLIB (what if they disable ACPI by mistake?), do the same for OF,
call board's manufacturer hotline to ensure that they indeed did not
forget to describe GPIOs, etc, etc.
Or you could trust that person responsible for selecting kernel
configuration has a clue, and if GPIOLIB is disabled it was disabled for
a reason.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Dmitry Torokhov @ 2017-03-23 15:44 UTC (permalink / raw)
To: Linus Walleij
Cc: Uwe Kleine-König, Geert Uytterhoeven, Richard Genoud,
Greg Kroah-Hartman, Geert Uytterhoeven, Nicolas Ferre,
Boris Brezillon, linux-arm-kernel@lists.infradead.org,
linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
Janusz Uzycki, linux-gpio@vger.kernel.org
In-Reply-To: <20170323144325.GA952@dtor-ws>
On Thu, Mar 23, 2017 at 07:43:25AM -0700, Dmitry Torokhov wrote:
> On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote:
> > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> >
> > > Maybe we can make gpiod_get_optional look like this:
> > >
> > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB))
> > > return NULL;
> > > else
> > > return -ENOSYS;
> > >
> > > I don't know how isnt_a_acpi_device looks like, probably it involves
> > > CONFIG_ACPI and/or dev->acpi_node.
> > >
> > > This should be safe and still comfortable for legacy platforms, isn't it?
> >
> > I like the looks of this.
> >
> > Can we revert Dmitry's patch and apply something like this instead?
> >
> > Dmitry, how do you feel about this?
>
> I frankly do not see the point. It still makes driver code more complex
> for no good reason. I also think that not having optional GPIO is not an
> error, so returning value from error space is not correct. NULL is value
> from another space altogether.
>
> Uwe seems to be concerned about case that I find extremely unlikely. We
> are talking about a system that does not have GPIO support and behaves
> just fine, with the exception that it actually has (physically) a
> *single* GPIO, and that GPIO happens to be optional in a single driver,
> but in this particular system is actually needed (but that need
> manifests in a non-obvious way). And we have system integrator that has
> no idea what they are doing (no schematic, etc).
>
> I think that if there is one optional GPIO there will be mandatiry GPIOs
> in such system as well and selection of GPIOLIB will be forced early on
> in board bringup.
One more thing: if we keep reporting -ENOSYS in case of !CONFIG_GPIOLIB,
then most of the non platform-sepcific drivers will eventually gain code
silently coping with this -ENOSYS:
data->gpiod = gpiod_getptional(...);
if (IS_ERR(data->gpiod)) {
error = PTR_ERR(data->gpiod);
if (error != -ENOSYS)
return error;
data->gpiod = NULL; /* This GPIO _is_ optional */
}
which will negate Uwe's claim that it will help debugging issues.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RFC RESEND] serdev binding for QCA7000 UART driver
From: Stefan Wahren @ 2017-03-23 15:13 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, Greg Kroah-Hartman, Jiri Slaby,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAL_JsqJS05gw80B4oUWZHEHOg5iPpwKie8XdBr82UD=7WB1cdg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Rob,
Am 22.03.2017 um 21:32 schrieb Rob Herring:
> On Mon, Mar 13, 2017 at 11:16 AM, Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org> wrote:
>> Hi Rob,
>>
>> since serdev has been merged, i'm currently working on a
>> suitable ethernet driver for the QCA7000 [1]. In order to provide a plug
>> and play solution for the network interface i suggest the following
>> binding, which uses common properties for UART configuration:
>>
>> * Qualcomm QCA7000 (Ethernet over UART protocol)
>>
>> Note: The QCA7000 is also useable as a UART slave device.
> Should this be s/UART/SPI/ ?
i rewrote this note in my second version below.
>
>> Required properties:
>> - compatible : Should be "qca,qca7000-uart"
>>
>> Optional properties:
>> - local-mac-address : 6 bytes, Specifies MAC address
>> - current-speed : Specifies current serial device speed in
>> bits per second (default = 115200)
>> - data-bits : Specifies number of data bits (default = 8)
>> - use-parity : If present, this enables the parity
>> error detection (default = off)
>> - odd-parity : If present, this specifies that the parity
>> of each character must be odd (default = even)
>>
>> Example:
>>
>> /* Freescale i.MX28 UART */
>> auart0: serial@8006a000 {
>> compatible = "fsl,imx28-auart", "fsl,imx23-auart";
>> reg = <0x8006a000 0x2000>;
>> pinctrl-names = "default";
>> pinctrl-0 = <&auart0_2pins_a>;
>> status = "okay";
>>
>> qca7000: ethernet {
>> compatible = "qca,qca7000-uart";
>> local-mac-address = [ A0 B0 C0 D0 E0 F0 ];
>> current-speed = <38400>;
> Unless this device supports auto-baud (generally only AT command set
> devices do), this is setup by a bootloader (and you have no way to
> reset the device), or the speed changes with different firmware, I
> don't think you should need this. Devices typically have a fixed,
> initial baudrate that is known, so the driver should know it.
Unfortunately the QCA7000 firmware can be freely preconfigured to a
fixed baudrate and there is no way to determine, because Ethernet frames
are send nearly directly to the UART and vice versa. I agree this isn't
best practice.
>> data-bits = <8>;
> Nearly everything uses 8 bits, so this should be the default.
I will drop it completly.
>
>> use-parity;
>> odd-parity;
> I don't think we need 2 properties here. "even-parity" and
> "odd-parity" booleans should be enough to enable and set parity type.
Your suggestion is better, but it's possible to define wrongly both
which isn't nice per design.
Anyway i decide to drop this, too. So it could be implemented later.
So here is the new version:
* Qualcomm QCA7000 (Ethernet over UART protocol)
Note: This binding applies in case the QCA7000 is configured as a
UART slave device. It is possible to preconfigure the UART settings
of the QCA7000 firmware, which can't be changed during runtime.
Required properties:
- compatible : Should be "qca,qca7000-uart"
Optional properties:
- local-mac-address : 6 bytes, Specifies MAC address
- current-speed : Specifies the serial device speed in
bits per second (default = 115200), which is
predefined by the QCA7000 firmware configuration
Example:
/* Freescale i.MX28 UART */
auart0: serial@8006a000 {
compatible = "fsl,imx28-auart", "fsl,imx23-auart";
reg = <0x8006a000 0x2000>;
pinctrl-names = "default";
pinctrl-0 = <&auart0_2pins_a>;
status = "okay";
qca7000: ethernet {
compatible = "qca,qca7000-uart";
local-mac-address = [ A0 B0 C0 D0 E0 F0 ];
current-speed = <38400>;
};
};
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Dmitry Torokhov @ 2017-03-23 14:43 UTC (permalink / raw)
To: Linus Walleij
Cc: Boris Brezillon, Geert Uytterhoeven, Richard Genoud,
Greg Kroah-Hartman, linux-kernel@vger.kernel.org,
linux-gpio@vger.kernel.org, Janusz Uzycki, Geert Uytterhoeven,
linux-serial@vger.kernel.org, Uwe Kleine-König,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CACRpkdZRfLh9RXRycW_cAxuTYE6tPd7LR32EkAW8y2fh=AxhXA@mail.gmail.com>
On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote:
> On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>
> > Maybe we can make gpiod_get_optional look like this:
> >
> > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB))
> > return NULL;
> > else
> > return -ENOSYS;
> >
> > I don't know how isnt_a_acpi_device looks like, probably it involves
> > CONFIG_ACPI and/or dev->acpi_node.
> >
> > This should be safe and still comfortable for legacy platforms, isn't it?
>
> I like the looks of this.
>
> Can we revert Dmitry's patch and apply something like this instead?
>
> Dmitry, how do you feel about this?
I frankly do not see the point. It still makes driver code more complex
for no good reason. I also think that not having optional GPIO is not an
error, so returning value from error space is not correct. NULL is value
from another space altogether.
Uwe seems to be concerned about case that I find extremely unlikely. We
are talking about a system that does not have GPIO support and behaves
just fine, with the exception that it actually has (physically) a
*single* GPIO, and that GPIO happens to be optional in a single driver,
but in this particular system is actually needed (but that need
manifests in a non-obvious way). And we have system integrator that has
no idea what they are doing (no schematic, etc).
I think that if there is one optional GPIO there will be mandatiry GPIOs
in such system as well and selection of GPIOLIB will be forced early on
in board bringup.
Thanks.
--
Dmitry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox