* 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
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Linus Walleij @ 2017-03-23 13:41 UTC (permalink / raw)
To: Uwe Kleine-König, Dmitry Torokhov
Cc: 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: <20170323123437.uqdwhfmmsjke3f7s@pengutronix.de>
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?
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Linus Walleij @ 2017-03-23 13:37 UTC (permalink / raw)
To: Uwe Kleine-König, Dmitry Torokhov
Cc: 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: <20170323101045.u3uigdu5xfwjmjc7@pengutronix.de>
On Thu, Mar 23, 2017 at 11:10 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> So you exchanged many obvious and easy to fix problems with a few hard
> ones. I don't agree that's a good idea, but you seem to be willing to
> try it. Good luck.
I think instead of going to sarcastic remarks you can say you NACK the
patch and suggest that it be reverted?
The problem I have here as maintainer is that both you and Dmitry are
very smart people and I have a great deal of trust invested in both of you.
When two valued contributors give me very different advice I get a bit
confused and maybe the best option is not to change anything at all
right now, and just revert Dmitry's patch.
git grep -e 'gpio.*optional(' | wc -l
gives 154 use sites outside drivers/gpio, so it is not impossible to fix
this if we want a good and strict order to it. I'm just a bit overworked to
do it myself right now.
What do you all say, is it better to revert Dmitry's patch and instead go
around and fix the consumers to do it correctly everywhere, after
hammering down the exact semantics?
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Geert Uytterhoeven @ 2017-03-23 12:44 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Linus Walleij, Dmitry Torokhov, 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: <20170323123437.uqdwhfmmsjke3f7s@pengutronix.de>
Hi Uwe,
On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Thu, Mar 23, 2017 at 01:03:56PM +0100, Geert Uytterhoeven wrote:
>> On Thu, Mar 23, 2017 at 12:11 PM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> >> Make sure to enable all drivers and subsystems you need when building
>> >> your kernel. That's always true. And may indeed be hard to debug (e.g. what
>> >> kernel options do I need to make systemd work?).
>> >
>> > It's worse here. If you forget to enable a driver the device isn't bound
>> > and that's obvious to diagnose. When ignoring an optional GPIO there
>> > might be a device that claims to work but fails to do so. (e.g. you
>> > write to memory, write() returns 0, but the data never landed there.)
>> >
>> >> > write(2) and close(2) succeed most of the time, too. Still it's not a
>> >> > good idea to not check the return value. Or let the kernel return
>> >> > success unconditionally.
>> >>
>> >> Writing all bytes passed in the buffer is "optional" in another sense than
>> >> an "optional" GPIO: you must retry the write, while you can continue if
>> >> an optional GPIO is not present.
>> >
>> > And that is the point. You can continue *iff* the optional GPIO is not
>> > present. The patch in question removes the ability to determine if that
>> > GPIO is present and claims it is not present.
>>
>> If you forget to enable a driver/subsystem, you sometimes cannot determine
>> if the device is present or not neither.
>>
>> Hence it boils down to "knowing" if there is a GPIO or not.
>> So, when can there be a GPIO?
>> 1. The GPIO is described in DT.
>> => Not an issue, as DT GPIO implies GPIOLIB,
>> 2. The GPIO is described in legacy platform data.
>> => The platform code should make sure GPIOLIB is selected when needed.
>>
>> Issue solved?
>
> I like it better to not rely on platform code to do the right thing.
;-)
> 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?
Yes, that should do the trick.
No feedback from me about ACPI.
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-23 12:34 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Linus Walleij, Dmitry Torokhov, 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: <CAMuHMdUWybAZXc2RbGMGyp4DYQZU9BS8MxXKE5Kfd+aH8+w9sA@mail.gmail.com>
On Thu, Mar 23, 2017 at 01:03:56PM +0100, Geert Uytterhoeven wrote:
> Hi Uwe,
>
> On Thu, Mar 23, 2017 at 12:11 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >> Make sure to enable all drivers and subsystems you need when building
> >> your kernel. That's always true. And may indeed be hard to debug (e.g. what
> >> kernel options do I need to make systemd work?).
> >
> > It's worse here. If you forget to enable a driver the device isn't bound
> > and that's obvious to diagnose. When ignoring an optional GPIO there
> > might be a device that claims to work but fails to do so. (e.g. you
> > write to memory, write() returns 0, but the data never landed there.)
> >
> >> > write(2) and close(2) succeed most of the time, too. Still it's not a
> >> > good idea to not check the return value. Or let the kernel return
> >> > success unconditionally.
> >>
> >> Writing all bytes passed in the buffer is "optional" in another sense than
> >> an "optional" GPIO: you must retry the write, while you can continue if
> >> an optional GPIO is not present.
> >
> > And that is the point. You can continue *iff* the optional GPIO is not
> > present. The patch in question removes the ability to determine if that
> > GPIO is present and claims it is not present.
>
> If you forget to enable a driver/subsystem, you sometimes cannot determine
> if the device is present or not neither.
>
> Hence it boils down to "knowing" if there is a GPIO or not.
> So, when can there be a GPIO?
> 1. The GPIO is described in DT.
> => Not an issue, as DT GPIO implies GPIOLIB,
> 2. The GPIO is described in legacy platform data.
> => The platform code should make sure GPIOLIB is selected when needed.
>
> Issue solved?
I like it better to not rely on platform code to do the right thing.
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?
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: Vignesh R @ 2017-03-23 12:22 UTC (permalink / raw)
To: Andy Shevchenko
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: <CAHp75VdhHj-7Utganxo1YnSABmnM5KC+bVJe2KgXx65hNHNGng@mail.gmail.com>
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.
Anyways, will make the next version as v4.
Thanks for the review!
--
Regards
Vignesh
^ permalink raw reply
* Re: [PATCH 1/2] tty: serial_core: Add name field to uart_port struct
From: Vignesh R @ 2017-03-23 12:13 UTC (permalink / raw)
To: Andy Shevchenko
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: <CAHp75VfHtTV6L0GE61kMiHMCQrUSwGJPRxsiNuOCD9mY7MPpZQ@mail.gmail.com>
On Thursday 23 March 2017 12:15 AM, Andy Shevchenko wrote:
> On Tue, Mar 21, 2017 at 3:36 PM, 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
>
> uart -> 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
>
> irq(s) -> IRQ(s)
>
>> uart_add_one_port().
>
>> + snprintf(uport->name, sizeof(uport->name), "%s%d", drv->dev_name,
>> + drv->tty_driver->name_base + uport->line);
>
> This is fragile. dev_name is defined as pointer, so, it would be any length.
> So, better to use [devm_]kasprintf().
>
Ok, will you use kasprintf() (and corresponding kfree()) as device is
not yet allocated at this stage to use devm_*.
--
Regards
Vignesh
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Geert Uytterhoeven @ 2017-03-23 12:03 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Boris Brezillon, 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: <20170323111106.7ogh6g2oa3m4cqc6@pengutronix.de>
Hi Uwe,
On Thu, Mar 23, 2017 at 12:11 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>> Make sure to enable all drivers and subsystems you need when building
>> your kernel. That's always true. And may indeed be hard to debug (e.g. what
>> kernel options do I need to make systemd work?).
>
> It's worse here. If you forget to enable a driver the device isn't bound
> and that's obvious to diagnose. When ignoring an optional GPIO there
> might be a device that claims to work but fails to do so. (e.g. you
> write to memory, write() returns 0, but the data never landed there.)
>
>> > write(2) and close(2) succeed most of the time, too. Still it's not a
>> > good idea to not check the return value. Or let the kernel return
>> > success unconditionally.
>>
>> Writing all bytes passed in the buffer is "optional" in another sense than
>> an "optional" GPIO: you must retry the write, while you can continue if
>> an optional GPIO is not present.
>
> And that is the point. You can continue *iff* the optional GPIO is not
> present. The patch in question removes the ability to determine if that
> GPIO is present and claims it is not present.
If you forget to enable a driver/subsystem, you sometimes cannot determine
if the device is present or not neither.
Hence it boils down to "knowing" if there is a GPIO or not.
So, when can there be a GPIO?
1. The GPIO is described in DT.
=> Not an issue, as DT GPIO implies GPIOLIB,
2. The GPIO is described in legacy platform data.
=> The platform code should make sure GPIOLIB is selected when needed.
Issue solved?
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
* [PATCH v2 2/2] serial: samsung: Add missing checks for dma_map_single failure
From: Marek Szyprowski @ 2017-03-23 12:01 UTC (permalink / raw)
To: linux-samsung-soc, linux-serial
Cc: Marek Szyprowski, Sylwester Nawrocki, Krzysztof Kozlowski,
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>
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);
+ if (dma_mapping_error(p->port.dev, dma->rx_addr)) {
+ ret = -EIO;
+ goto err_free_rx;
+ }
/* TX buffer */
dma->tx_addr = dma_map_single(p->port.dev, p->port.state->xmit.buf,
UART_XMIT_SIZE, DMA_TO_DEVICE);
-
- spin_unlock_irqrestore(&p->port.lock, flags);
+ if (dma_mapping_error(p->port.dev, dma->tx_addr)) {
+ ret = -EIO;
+ goto err_unmap_rx;
+ }
return 0;
+
+err_unmap_rx:
+ dma_unmap_single(p->port.dev, dma->rx_addr, dma->rx_size,
+ DMA_FROM_DEVICE);
+err_free_rx:
+ kfree(dma->rx_buf);
+err_release_tx:
+ dma_release_channel(dma->tx_chan);
+err_release_rx:
+ dma_release_channel(dma->rx_chan);
+ return ret;
}
static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
--
1.9.1
^ permalink raw reply related
* [PATCH v2 1/2] serial: samsung: Use right device for DMA-mapping calls
From: Marek Szyprowski @ 2017-03-23 12:01 UTC (permalink / raw)
To: linux-samsung-soc, linux-serial
Cc: Marek Szyprowski, Sylwester Nawrocki, Krzysztof Kozlowski,
Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Seung-Woo Kim,
Joonyoung Shim, Inki Dae, stable
In-Reply-To: <CGME20170323120208eucas1p28df58f312314337e9750d23930de733b@eucas1p2.samsung.com>
Driver should provide its own struct device for all DMA-mapping calls instead
of extracting device pointer from DMA engine channel.
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(-)
diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index 7a17aedbf902..9f3759bdb44f 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -901,14 +901,13 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
return -ENOMEM;
}
- dma->rx_addr = dma_map_single(dma->rx_chan->device->dev, dma->rx_buf,
+ 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);
/* TX buffer */
- dma->tx_addr = dma_map_single(dma->tx_chan->device->dev,
- p->port.state->xmit.buf,
+ dma->tx_addr = dma_map_single(p->port.dev, p->port.state->xmit.buf,
UART_XMIT_SIZE, DMA_TO_DEVICE);
spin_unlock_irqrestore(&p->port.lock, flags);
@@ -922,7 +921,7 @@ static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
if (dma->rx_chan) {
dmaengine_terminate_all(dma->rx_chan);
- dma_unmap_single(dma->rx_chan->device->dev, dma->rx_addr,
+ dma_unmap_single(p->port.dev, dma->rx_addr,
dma->rx_size, DMA_FROM_DEVICE);
kfree(dma->rx_buf);
dma_release_channel(dma->rx_chan);
@@ -931,7 +930,7 @@ static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
if (dma->tx_chan) {
dmaengine_terminate_all(dma->tx_chan);
- dma_unmap_single(dma->tx_chan->device->dev, dma->tx_addr,
+ dma_unmap_single(p->port.dev, dma->tx_addr,
UART_XMIT_SIZE, DMA_TO_DEVICE);
dma_release_channel(dma->tx_chan);
dma->tx_chan = NULL;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 2/2] serial: samsung: Add missing checks for dma_map_single failure
From: Bartlomiej Zolnierkiewicz @ 2017-03-23 11:59 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-samsung-soc, linux-serial, Sylwester Nawrocki,
Krzysztof Kozlowski, Greg Kroah-Hartman, Seung-Woo Kim,
Joonyoung Shim, Inki Dae, stable
In-Reply-To: <3019209.fgIhMIWD3x@amdc3058>
On Thursday, March 23, 2017 12:04:02 PM Bartlomiej Zolnierkiewicz wrote:
> On Thursday, March 23, 2017 08:45:22 AM 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: 2c37eedb74c8 ("serial: samsung: add dma reqest/release functions")
>
> The right commit id is 62c37eedb74c8.
BTW With this fixed you may add:
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
to both patches.
> > CC: stable@vger.kernel.org # v4.10+
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Best regards,
^ 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 11:11 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Linus Walleij, Dmitry Torokhov, 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: <CAMuHMdWDUzwUqrchbhh-TkE3J9Xg4TrwTF6HreikUcNNTf_ozQ@mail.gmail.com>
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?
> Make sure to enable all drivers and subsystems you need when building
> your kernel. That's always true. And may indeed be hard to debug (e.g. what
> kernel options do I need to make systemd work?).
It's worse here. If you forget to enable a driver the device isn't bound
and that's obvious to diagnose. When ignoring an optional GPIO there
might be a device that claims to work but fails to do so. (e.g. you
write to memory, write() returns 0, but the data never landed there.)
> > write(2) and close(2) succeed most of the time, too. Still it's not a
> > good idea to not check the return value. Or let the kernel return
> > success unconditionally.
>
> Writing all bytes passed in the buffer is "optional" in another sense than
> an "optional" GPIO: you must retry the write, while you can continue if
> an optional GPIO is not present.
And that is the point. You can continue *iff* the optional GPIO is not
present. The patch in question removes the ability to determine if that
GPIO is present and claims it is not present.
> > So you exchanged many obvious and easy to fix problems with a few hard
> > ones. I don't agree that's a good idea, but you seem to be willing to
> > try it. Good luck.
>
> Yeah, before drivers had to explicitly ignore -ENOSYS if they want to
> support platforms with and without GPIOLIB. Bad...
Doing things right is sometimes not maximally easy. But that is no
excuse to do it wrong.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 2/2] serial: samsung: Add missing checks for dma_map_single failure
From: Bartlomiej Zolnierkiewicz @ 2017-03-23 11:04 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-samsung-soc, linux-serial, Sylwester Nawrocki,
Krzysztof Kozlowski, Greg Kroah-Hartman, Seung-Woo Kim,
Joonyoung Shim, Inki Dae, stable
In-Reply-To: <1490255131-9597-2-git-send-email-m.szyprowski@samsung.com>
On Thursday, March 23, 2017 08:45:22 AM 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: 2c37eedb74c8 ("serial: samsung: add dma reqest/release functions")
The right commit id is 62c37eedb74c8.
> CC: stable@vger.kernel.org # v4.10+
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Best regards,
^ permalink raw reply
* Re: [PATCH 1/2] serial: samsung: Use right device for DMA-mapping calls
From: Bartlomiej Zolnierkiewicz @ 2017-03-23 11:02 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-samsung-soc, linux-serial, Sylwester Nawrocki,
Krzysztof Kozlowski, Greg Kroah-Hartman, Seung-Woo Kim,
Joonyoung Shim, Inki Dae, stable
In-Reply-To: <1490255131-9597-1-git-send-email-m.szyprowski@samsung.com>
Hi,
On Thursday, March 23, 2017 08:45:21 AM Marek Szyprowski wrote:
> Driver should provide its own struct device for all DMA-mapping calls instead
> of extracting device pointer from DMA engine channel.
>
> Reported-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> Fixes: 2c37eedb74c8 ("serial: samsung: add dma reqest/release functions")
The right commit id is 62c37eedb74c8.
> CC: stable@vger.kernel.org # v4.0+
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls
From: Geert Uytterhoeven @ 2017-03-23 10:20 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Linus Walleij, Dmitry Torokhov, 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: <20170323101045.u3uigdu5xfwjmjc7@pengutronix.de>
Hi Uwe,
On Thu, Mar 23, 2017 at 11:10 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Thu, Mar 23, 2017 at 10:32:01AM +0100, Linus Walleij wrote:
>> On Mon, Mar 20, 2017 at 12:07 PM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> > On Mon, Mar 20, 2017 at 11:38:52AM +0100, Geert Uytterhoeven wrote:
>> >> On Mon, Mar 20, 2017 at 11:31 AM, Uwe Kleine-König
>> >> <u.kleine-koenig@pengutronix.de> wrote:
>> >> > On Mon, Mar 20, 2017 at 10:56:14AM +0100, Geert Uytterhoeven wrote:
>> >> >> Documentation/gpio/consumer.txt rightfully sates:
>> >> >> | Note that gpio_get*_optional() functions (and their managed variants), unlike
>> >> >> | the rest of gpiolib API, also return NULL when gpiolib support is disabled.
>> >> >> | This is helpful to driver authors, since they do not need to special case
>> >> >> | -ENOSYS return codes. System integrators should however be careful to enable
>> >> >> | gpiolib on systems that need it.
>> >> >
>> >> > I cannot find this paragraph in Documentation/gpio/consumer.txt:
>> >> >
>> >> > $ git grep -e 'ENOSYS' v4.11-rc3 -- Documentation/gpio/
>> >> > <void>
>> >>
>> >> It was added by commit 22c403676dbbb7c6 ("gpio: return NULL from
>> >> gpiod_get_optional when GPIOLIB is disabled")
>> >
>> > Ah, that's in next.
>> >
>> > I still think this is wrong and I'm a bit disappointed that Linus merged
>> > this patch (without saying so in the thread even) as I thought Linus and
>> > I agreed on this being a bad idea.
>>
>> I think it is not good, but what we have before this patch is worse.
>>
>> I.e. it is the lesser evil.
>>
>> Before this patch the API is inconsistent: it gives NULL if GPIOLIB
>> is defined and -ENOSYS if GPIOLIB is not defined.
>
> So old is: it worked as intended with GPIOLIB and produced an error
> without GPIOLIB even if no GPIO is there and NULL would be right.
>
>> After this patch it gives NULL in both cases, and that is at least
>> consistent.
>
> And new is: with GPIOLIB it still works as intended and gives you NULL
> even if a GPIO is there and so -ENOSYS would be right.
If I forget to enable a driver, the corresponding class device is also not
there.
> I admit that most of the time with GPIOLIB NULL is the right answer
> because probably there is no GPIO to handle. But if there is a GPIO you
> run in hard to debug problems instead of being able to see the error
> code and act accordingly.
But having the error breaks setups where the GPIO is optional and does
not exist.
Make sure to enable all drivers and subsystems you need when building
your kernel. That's always true. And may indeed be hard to debug (e.g. what
kernel options do I need to make systemd work?).
> write(2) and close(2) succeed most of the time, too. Still it's not a
> good idea to not check the return value. Or let the kernel return
> success unconditionally.
Writing all bytes passed in the buffer is "optional" in another sense than
an "optional" GPIO: you must retry the write, while you can continue if
an optional GPIO is not present.
> So you exchanged many obvious and easy to fix problems with a few hard
> ones. I don't agree that's a good idea, but you seem to be willing to
> try it. Good luck.
Yeah, before drivers had to explicitly ignore -ENOSYS if they want to
support platforms with and without GPIOLIB. Bad...
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-23 10:10 UTC (permalink / raw)
To: Linus Walleij
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: <CACRpkdZy_uguiKfSH-0gdanjr-xkwxoANTx6Fb2VwsvzRwcPRA@mail.gmail.com>
On Thu, Mar 23, 2017 at 10:32:01AM +0100, Linus Walleij wrote:
> On Mon, Mar 20, 2017 at 12:07 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > Hello Geert,
> >
> > On Mon, Mar 20, 2017 at 11:38:52AM +0100, Geert Uytterhoeven wrote:
> >> On Mon, Mar 20, 2017 at 11:31 AM, Uwe Kleine-König
> >> <u.kleine-koenig@pengutronix.de> wrote:
> >> > On Mon, Mar 20, 2017 at 10:56:14AM +0100, Geert Uytterhoeven wrote:
> >> >> Documentation/gpio/consumer.txt rightfully sates:
> >> >> | Note that gpio_get*_optional() functions (and their managed variants), unlike
> >> >> | the rest of gpiolib API, also return NULL when gpiolib support is disabled.
> >> >> | This is helpful to driver authors, since they do not need to special case
> >> >> | -ENOSYS return codes. System integrators should however be careful to enable
> >> >> | gpiolib on systems that need it.
> >> >
> >> > I cannot find this paragraph in Documentation/gpio/consumer.txt:
> >> >
> >> > $ git grep -e 'ENOSYS' v4.11-rc3 -- Documentation/gpio/
> >> > <void>
> >>
> >> It was added by commit 22c403676dbbb7c6 ("gpio: return NULL from
> >> gpiod_get_optional when GPIOLIB is disabled")
> >
> > Ah, that's in next.
> >
> > I still think this is wrong and I'm a bit disappointed that Linus merged
> > this patch (without saying so in the thread even) as I thought Linus and
> > I agreed on this being a bad idea.
>
> I think it is not good, but what we have before this patch is worse.
>
> I.e. it is the lesser evil.
>
> Before this patch the API is inconsistent: it gives NULL if GPIOLIB
> is defined and -ENOSYS if GPIOLIB is not defined.
So old is: it worked as intended with GPIOLIB and produced an error
without GPIOLIB even if no GPIO is there and NULL would be right.
> After this patch it gives NULL in both cases, and that is at least
> consistent.
And new is: with GPIOLIB it still works as intended and gives you NULL
even if a GPIO is there and so -ENOSYS would be right.
I admit that most of the time with GPIOLIB NULL is the right answer
because probably there is no GPIO to handle. But if there is a GPIO you
run in hard to debug problems instead of being able to see the error
code and act accordingly.
write(2) and close(2) succeed most of the time, too. Still it's not a
good idea to not check the return value. Or let the kernel return
success unconditionally.
So you exchanged many obvious and easy to fix problems with a few hard
ones. I don't agree that's a good idea, but you seem to be willing to
try it. Good luck.
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: Linus Walleij @ 2017-03-23 9:32 UTC (permalink / raw)
To: Uwe Kleine-König, Dmitry Torokhov
Cc: 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: <20170320110718.e44s2gip36m75bqj@pengutronix.de>
On Mon, Mar 20, 2017 at 12:07 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello Geert,
>
> On Mon, Mar 20, 2017 at 11:38:52AM +0100, Geert Uytterhoeven wrote:
>> On Mon, Mar 20, 2017 at 11:31 AM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> > On Mon, Mar 20, 2017 at 10:56:14AM +0100, Geert Uytterhoeven wrote:
>> >> Documentation/gpio/consumer.txt rightfully sates:
>> >> | Note that gpio_get*_optional() functions (and their managed variants), unlike
>> >> | the rest of gpiolib API, also return NULL when gpiolib support is disabled.
>> >> | This is helpful to driver authors, since they do not need to special case
>> >> | -ENOSYS return codes. System integrators should however be careful to enable
>> >> | gpiolib on systems that need it.
>> >
>> > I cannot find this paragraph in Documentation/gpio/consumer.txt:
>> >
>> > $ git grep -e 'ENOSYS' v4.11-rc3 -- Documentation/gpio/
>> > <void>
>>
>> It was added by commit 22c403676dbbb7c6 ("gpio: return NULL from
>> gpiod_get_optional when GPIOLIB is disabled")
>
> Ah, that's in next.
>
> I still think this is wrong and I'm a bit disappointed that Linus merged
> this patch (without saying so in the thread even) as I thought Linus and
> I agreed on this being a bad idea.
I think it is not good, but what we have before this patch is worse.
I.e. it is the lesser evil.
Before this patch the API is inconsistent: it gives NULL if GPIOLIB
is defined and -ENOSYS if GPIOLIB is not defined.
After this patch it gives NULL in both cases, and that is at least
consistent.
It would be great if someone steps in and fix it so as to return
relevant errors (or error pointers rather) in both cases. However
I guess it will lead a bunch of fallouts in the consumers, albeit
they are not super-many.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCHv2 09/11] Bluetooth: add nokia driver
From: Sebastian Reichel @ 2017-03-23 9:07 UTC (permalink / raw)
To: Frédéric Danis
Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, Rob Herring,
Samuel Thibault, Pavel Machek, Tony Lindgren, Greg Kroah-Hartman,
Jiri Slaby, Mark Rutland, linux-bluetooth, linux-serial,
devicetree, linux-kernel
In-Reply-To: <07cf4c22-b798-6e77-cca0-548e32e9de3f@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3029 bytes --]
Hi,
On Thu, Mar 23, 2017 at 08:50:42AM +0100, Frédéric Danis wrote:
> Le 21/03/2017 à 23:32, Sebastian Reichel a écrit :
> > This adds a driver for the Nokia H4+ protocol, which is used
> > at least on the Nokia N9, N900 & N950.
> >
> > Signed-off-by: Sebastian Reichel <sre@kernel.org>
> > ---
> > Changes since PATCHv1:
> > * replace __u8 and uint8_t with u8
> > * replace __u16 and uint16_t with u16
> > * drop BT_BAUDRATE_DIVIDER and use btdev->sysclk_speed * 10 instead
> > * fix wording of a sentence
> > * fix error path of negotation & alive package receive functions
> > * replaced nokia_wait_for_cts with newly introduced serdev function
> > * use "nokia,h4p-bluetooth" as compatible string
> > ---
> > drivers/bluetooth/Kconfig | 12 +
> > drivers/bluetooth/Makefile | 2 +
> > drivers/bluetooth/hci_nokia.c | 819 ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 833 insertions(+)
> > create mode 100644 drivers/bluetooth/hci_nokia.c
> >
> > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > index c2c14a12713b..2e3e4d3547ad 100644
> > --- a/drivers/bluetooth/Kconfig
> > +++ b/drivers/bluetooth/Kconfig
> > @@ -86,6 +86,18 @@ config BT_HCIUART_H4
> > Say Y here to compile support for HCI UART (H4) protocol.
> > +config BT_HCIUART_NOKIA
> > + tristate "UART Nokia H4+ protocol support"
> > + depends on BT_HCIUART
> > + depends on SERIAL_DEV_BUS
> > + depends on PM
> > + help
> > + Nokia H4+ is serial protocol for communication between Bluetooth
> > + device and host. This protocol is required for Bluetooth devices
> > + with UART interface in Nokia devices.
> > +
> > + Say Y here to compile support for Nokia's H4+ protocol.
> > +
> > config BT_HCIUART_BCSP
> > bool "BCSP protocol support"
> > depends on BT_HCIUART
> > diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> > index fd571689eed6..a7f237320f4b 100644
> > --- a/drivers/bluetooth/Makefile
> > +++ b/drivers/bluetooth/Makefile
> > @@ -25,6 +25,8 @@ obj-$(CONFIG_BT_BCM) += btbcm.o
> > obj-$(CONFIG_BT_RTL) += btrtl.o
> > obj-$(CONFIG_BT_QCA) += btqca.o
> > +obj-$(CONFIG_BT_HCIUART_NOKIA) += hci_nokia.o
> > +
> > btmrvl-y := btmrvl_main.o
> > btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o
>
> This does not build as module with following error:
> ERROR: "hci_uart_tx_wakeup" [drivers/bluetooth/hci_nokia.ko] undefined!
> ERROR: "hci_uart_register_device" [drivers/bluetooth/hci_nokia.ko]
> undefined!
>
> Should not hci_nokia be part of the hci_uart module?
Yeah, I also received that from kbuild test robot after sending the
patchset. I intentionally did not add this to hci_uart, so that I can
use module_serdev_device_driver(). I think at least for serdev-only
based bluetooth drivers it makes sense to have them in their own module.
I already have added EXPORT_SYMBOL_GPL for those functions in the next
version of this patchset.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2] serial: 8250_lpss: Unconditionally set PCI master for Quark
From: Jan Kiszka @ 2017-03-23 8:26 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Andy Shevchenko, linux-serial@vger.kernel.org,
Linux Kernel Mailing List
MSI needs it as well.
Should have no practical impact, though, as DMA is always available on
the Quark. But given the few users of pci_alloc_irq_vectors so far, this
incorrect pattern may spread otherwise.
Fixes: 3f3a46951e02 ("serial: 8250_lpss: set PCI master only for private DMA")
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Just a repost, no code change - missed to do this with Andy's tag after
the review.
drivers/tty/serial/8250/8250_lpss.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index f3ea90f..7dddd7e 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -183,7 +183,6 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
if (ret)
return;
- pci_set_master(pdev);
pci_try_set_mwi(pdev);
/* Special DMA address for UART */
@@ -216,6 +215,8 @@ static int qrk_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
struct pci_dev *pdev = to_pci_dev(port->dev);
int ret;
+ pci_set_master(pdev);
+
ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
if (ret < 0)
return ret;
^ permalink raw reply related
* Re: [PATCH 1/1] tty: serial: st-asc: Make the locking RT aware
From: Patrice CHOTARD @ 2017-03-23 7:56 UTC (permalink / raw)
To: Lionel DEBIEVE, Greg Kroah-Hartman, Jiri Slaby
Cc: linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-rt-users@vger.kernel.org
In-Reply-To: <1490202751-16581-1-git-send-email-lionel.debieve@st.com>
Hi Lionel
On 03/22/2017 06:12 PM, Lionel Debieve wrote:
> The lock is a sleeping lock and local_irq_save() is not the
> standard implementation now. Working for both -RT and non
> RT.
>
> Signed-off-by: Lionel Debieve <lionel.debieve@st.com>
Acked-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> drivers/tty/serial/st-asc.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> index c334bcc..4889396 100644
> --- a/drivers/tty/serial/st-asc.c
> +++ b/drivers/tty/serial/st-asc.c
> @@ -887,13 +887,12 @@ static void asc_console_write(struct console *co, const char *s, unsigned count)
> int locked = 1;
> u32 intenable;
>
> - local_irq_save(flags);
> if (port->sysrq)
> locked = 0; /* asc_interrupt has already claimed the lock */
> else if (oops_in_progress)
> - locked = spin_trylock(&port->lock);
> + locked = spin_trylock_irqsave(&port->lock, flags);
> else
> - spin_lock(&port->lock);
> + spin_lock_irqsave(&port->lock, flags);
>
> /*
> * Disable interrupts so we don't get the IRQ line bouncing
> @@ -911,8 +910,7 @@ static void asc_console_write(struct console *co, const char *s, unsigned count)
> asc_out(port, ASC_INTEN, intenable);
>
> if (locked)
> - spin_unlock(&port->lock);
> - local_irq_restore(flags);
> + spin_unlock_irqrestore(&port->lock, flags);
> }
>
> static int asc_console_setup(struct console *co, char *options)
>
^ permalink raw reply
* Re: [PATCHv2 09/11] Bluetooth: add nokia driver
From: Frédéric Danis @ 2017-03-23 7:50 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, Rob Herring,
Samuel Thibault, Pavel Machek, Tony Lindgren, Greg Kroah-Hartman,
Jiri Slaby, Mark Rutland, linux-bluetooth, linux-serial,
devicetree, linux-kernel, frederic.danis.oss
In-Reply-To: <20170321223216.11733-10-sre@kernel.org>
Hello Sebastian,
Le 21/03/2017 à 23:32, Sebastian Reichel a écrit :
> This adds a driver for the Nokia H4+ protocol, which is used
> at least on the Nokia N9, N900 & N950.
>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
> Changes since PATCHv1:
> * replace __u8 and uint8_t with u8
> * replace __u16 and uint16_t with u16
> * drop BT_BAUDRATE_DIVIDER and use btdev->sysclk_speed * 10 instead
> * fix wording of a sentence
> * fix error path of negotation & alive package receive functions
> * replaced nokia_wait_for_cts with newly introduced serdev function
> * use "nokia,h4p-bluetooth" as compatible string
> ---
> drivers/bluetooth/Kconfig | 12 +
> drivers/bluetooth/Makefile | 2 +
> drivers/bluetooth/hci_nokia.c | 819 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 833 insertions(+)
> create mode 100644 drivers/bluetooth/hci_nokia.c
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index c2c14a12713b..2e3e4d3547ad 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -86,6 +86,18 @@ config BT_HCIUART_H4
>
> Say Y here to compile support for HCI UART (H4) protocol.
>
> +config BT_HCIUART_NOKIA
> + tristate "UART Nokia H4+ protocol support"
> + depends on BT_HCIUART
> + depends on SERIAL_DEV_BUS
> + depends on PM
> + help
> + Nokia H4+ is serial protocol for communication between Bluetooth
> + device and host. This protocol is required for Bluetooth devices
> + with UART interface in Nokia devices.
> +
> + Say Y here to compile support for Nokia's H4+ protocol.
> +
> config BT_HCIUART_BCSP
> bool "BCSP protocol support"
> depends on BT_HCIUART
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index fd571689eed6..a7f237320f4b 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -25,6 +25,8 @@ obj-$(CONFIG_BT_BCM) += btbcm.o
> obj-$(CONFIG_BT_RTL) += btrtl.o
> obj-$(CONFIG_BT_QCA) += btqca.o
>
> +obj-$(CONFIG_BT_HCIUART_NOKIA) += hci_nokia.o
> +
> btmrvl-y := btmrvl_main.o
> btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o
This does not build as module with following error:
ERROR: "hci_uart_tx_wakeup" [drivers/bluetooth/hci_nokia.ko] undefined!
ERROR: "hci_uart_register_device" [drivers/bluetooth/hci_nokia.ko]
undefined!
Should not hci_nokia be part of the hci_uart module?
Regards,
Fred
^ permalink raw reply
* [PATCH 2/2] serial: samsung: Add missing checks for dma_map_single failure
From: Marek Szyprowski @ 2017-03-23 7:45 UTC (permalink / raw)
To: linux-samsung-soc, linux-serial
Cc: Marek Szyprowski, Sylwester Nawrocki, Krzysztof Kozlowski,
Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Seung-Woo Kim,
Joonyoung Shim, Inki Dae, stable
In-Reply-To: <1490255131-9597-1-git-send-email-m.szyprowski@samsung.com>
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: 2c37eedb74c8 ("serial: samsung: add dma reqest/release functions")
CC: stable@vger.kernel.org # v4.10+
Signed-off-by: Marek Szyprowski <m.szyprowski@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.
---
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);
+ if (dma_mapping_error(p->port.dev, dma->rx_addr)) {
+ ret = -EIO;
+ goto err_free_rx;
+ }
/* TX buffer */
dma->tx_addr = dma_map_single(p->port.dev, p->port.state->xmit.buf,
UART_XMIT_SIZE, DMA_TO_DEVICE);
-
- spin_unlock_irqrestore(&p->port.lock, flags);
+ if (dma_mapping_error(p->port.dev, dma->tx_addr)) {
+ ret = -EIO;
+ goto err_unmap_rx;
+ }
return 0;
+
+err_unmap_rx:
+ dma_unmap_single(p->port.dev, dma->rx_addr, dma->rx_size,
+ DMA_FROM_DEVICE);
+err_free_rx:
+ kfree(dma->rx_buf);
+err_release_tx:
+ dma_release_channel(dma->tx_chan);
+err_release_rx:
+ dma_release_channel(dma->rx_chan);
+ return ret;
}
static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
--
1.9.1
^ permalink raw reply related
* [PATCH 1/2] serial: samsung: Use right device for DMA-mapping calls
From: Marek Szyprowski @ 2017-03-23 7:45 UTC (permalink / raw)
To: linux-samsung-soc, linux-serial
Cc: Marek Szyprowski, Sylwester Nawrocki, Krzysztof Kozlowski,
Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Seung-Woo Kim,
Joonyoung Shim, Inki Dae, stable
In-Reply-To: <CGME20170323074548eucas1p142511884e12d6211bebd9cbabde2f9c9@eucas1p1.samsung.com>
Driver should provide its own struct device for all DMA-mapping calls instead
of extracting device pointer from DMA engine channel.
Reported-by: Seung-Woo Kim <sw0312.kim@samsung.com>
Fixes: 2c37eedb74c8 ("serial: samsung: add dma reqest/release functions")
CC: stable@vger.kernel.org # v4.0+
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/tty/serial/samsung.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index 7a17aedbf902..9f3759bdb44f 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -901,14 +901,13 @@ static int s3c24xx_serial_request_dma(struct s3c24xx_uart_port *p)
return -ENOMEM;
}
- dma->rx_addr = dma_map_single(dma->rx_chan->device->dev, dma->rx_buf,
+ 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);
/* TX buffer */
- dma->tx_addr = dma_map_single(dma->tx_chan->device->dev,
- p->port.state->xmit.buf,
+ dma->tx_addr = dma_map_single(p->port.dev, p->port.state->xmit.buf,
UART_XMIT_SIZE, DMA_TO_DEVICE);
spin_unlock_irqrestore(&p->port.lock, flags);
@@ -922,7 +921,7 @@ static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
if (dma->rx_chan) {
dmaengine_terminate_all(dma->rx_chan);
- dma_unmap_single(dma->rx_chan->device->dev, dma->rx_addr,
+ dma_unmap_single(p->port.dev, dma->rx_addr,
dma->rx_size, DMA_FROM_DEVICE);
kfree(dma->rx_buf);
dma_release_channel(dma->rx_chan);
@@ -931,7 +930,7 @@ static void s3c24xx_serial_release_dma(struct s3c24xx_uart_port *p)
if (dma->tx_chan) {
dmaengine_terminate_all(dma->tx_chan);
- dma_unmap_single(dma->tx_chan->device->dev, dma->tx_addr,
+ dma_unmap_single(p->port.dev, dma->tx_addr,
UART_XMIT_SIZE, DMA_TO_DEVICE);
dma_release_channel(dma->tx_chan);
dma->tx_chan = NULL;
--
1.9.1
^ permalink raw reply related
* Re: [PATCHv2 10/11] ARM: dts: N900: Add bluetooth
From: Tony Lindgren @ 2017-03-22 23:55 UTC (permalink / raw)
To: Rob Herring
Cc: Sebastian Reichel, Marcel Holtmann, Gustavo Padovan,
Johan Hedberg, Samuel Thibault, Pavel Machek, Greg Kroah-Hartman,
Jiri Slaby, Mark Rutland, open list:BLUETOOTH DRIVERS,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAL_JsqLBNSWdbiO2A11c=BDWAKvWVGjOWafaNT+1OxoZgvH+fg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
* Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [170322 14:09]:
> On Tue, Mar 21, 2017 at 5:32 PM, Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > Add bcm2048 node and its system clock to the N900 device tree file.
> > Apart from that a reference to the new clock has been added to
> > wl1251 (which uses it, too).
> >
> > Acked-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> > Signed-off-by: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> > Changes since PATCHv1:
> > - update compatible string
> > ---
> > arch/arm/boot/dts/omap3-n900.dts | 23 +++++++++++++++++++++--
> > 1 file changed, 21 insertions(+), 2 deletions(-)
>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Picking patches 10 and 11 into omap-for-v4.12/dt-v2.
Regards,
Tony
--
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: [PATCHv2 09/11] Bluetooth: add nokia driver
From: Sebastian Reichel @ 2017-03-22 22:48 UTC (permalink / raw)
To: Rob Herring
Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, Samuel Thibault,
Pavel Machek, Tony Lindgren, Greg Kroah-Hartman, Jiri Slaby,
Mark Rutland, open list:BLUETOOTH DRIVERS,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAL_Jsq+UXxocmpSJwJ8dVY0ZSLn1Rk+GfTRKP7Wqvo-_cE4qPg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]
Hi,
On Wed, Mar 22, 2017 at 04:26:28PM -0500, Rob Herring wrote:
> On Tue, Mar 21, 2017 at 5:32 PM, Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > This adds a driver for the Nokia H4+ protocol, which is used
> > at least on the Nokia N9, N900 & N950.
> >
> > Signed-off-by: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
>
> > + btdev->wakeup_host = devm_gpiod_get(dev, "host-wakeup", GPIOD_IN);
> > + if (IS_ERR(btdev->wakeup_host)) {
> > + err = PTR_ERR(btdev->wakeup_host);
> > + dev_err(dev, "could not get host wakeup gpio: %d", err);
> > + return err;
> > + }
> > +
> > + btdev->wake_irq = gpiod_to_irq(btdev->wakeup_host);
>
> Missed this in the binding review, but generally, we make these
> interrupts rather than gpios in the binding.
I also read the state of the GPIO. AFAIK it's not possible to read
the state of an IRQ, so I can't switch to IRQ.
> > +
> > + err = devm_request_threaded_irq(dev, btdev->wake_irq, NULL,
> > + wakeup_handler,
> > + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > + "wakeup", btdev);
> > + if (err) {
> > + dev_err(dev, "could request wakeup irq: %d", err);
> > + return err;
> > + }
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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