Re: [PATCH] ARM: PL011: Fix DMA support

[Gregory CLEMENT <gregory.clement@bootlin.com>]

Hello Linus,

> On Wed, Nov 22, 2023 at 8:47 AM Gregory CLEMENT
> <gregory.clement@bootlin.com> wrote:
>
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> Since there is no guarantee that the memory returned by
>> dma_alloc_coherent() is associated with a 'struct page', using the
>> architecture specific phys_to_page() is wrong, but using
>> virt_to_page() would be as well.
>>
>> Stop using sg lists altogether and just use the *_single() functions
>> instead. This also simplifies the code a bit since the scatterlists in
>> this driver always have only one entry anyway.
>>
>> gcl: Add a commit log from the initial thread:
>
> First I thought this was Grant C. Likely, but now I realized it is
> Gregory :)

:)

Next time I will only use gc then.

>
>> https://lore.kernel.org/lkml/86db0fe5-930d-4cbb-bd7d-03367da38951@app.fastmail.com/
>>
>> Fixes: cb06ff102e2d7 ("ARM: PL011: Add support for Rx DMA buffer polling.")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Tested-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>
> Fair enough.
>
>>  struct pl011_sgbuf {
>> -       struct scatterlist sg;
>> -       char *buf;
>> +       dma_addr_t              dma;
>> +       size_t                  len;
>> +       char                    *buf;
>>  };
>
> Should this struct really be named "pl011_sgbuf" after this?
>
> It breaks Rusty Russell's API naming rules.
>
> What about renaming it pl011_dmabuf?

OK

>
>> @@ -369,18 +371,11 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap)
>>  static int pl011_sgbuf_init(struct dma_chan *chan, struct pl011_sgbuf *sg,
>
> And then parameters and variables named "sg" don't make much sense
> either, so just db or so for "dma buffer" if you wanna keep it short.

I am preparing a v2.

Thanks,

Gregory

>
> Yours,
> Linus Walleij

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com