Re: [PATCH v16 6/6] soc: amd: Add support for AMD Pensando SoC Controller

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Tue, Sep 26, 2023 at 11:05 PM Brad Larson <blarson@xxxxxxx> wrote:
> On Thu, Sep 21, 2023 at 18:19:57 +0300 Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > On Thu, Sep 14, 2023 at 12:52 AM Brad Larson <blarson@xxxxxxx> wrote:

...

> >> +       u8 tx_buf[PENCTRL_MAX_MSG_LEN];
> >> +       u8 rx_buf[PENCTRL_MAX_MSG_LEN];
> >
> > These are not DMA-safe, is this a problem?
>
> It's not a problem, the peripheral is PIO FIFO driven only.

The question was about the SPI controller itself.
Also, depending on the driver it may or may not require DMA-safe
pointers. I believe with the new pump queue used in the SPI core all
drivers that are using it are DMA-safe and the caller needs no
additional care.

...

> >> +       msg = memdup_user((struct penctrl_spi_xfer *)arg, size);
> >> +       if (IS_ERR(msg)) {
> >> +               ret = PTR_ERR(msg);
> >> +               goto out_unlock;
> >> +       }
> >
> > Wondering if you can start using cleanup.h.
>
> Perhaps if recommended, I don't see DEFINE_(FREE,UNLOCK,...) being used.

It's guard()() and scoped_guard() for locks and __free() for the allocations.

Plenty of uses in a few modules already (talking about Linux Next snapshot).

...

> >> +       spi->chip_select = current_cs;
> >
> > spi_set_chipselect()
>
> Yes, I'll change to inline function spi_set_chipselect(spi, 0, current_cs).  The
> second arg must be legacy as its unused.

Actually, it's a placeholder for the future support of indexed CS'.

...

> >> +       u8 txbuf[3];
> >> +       u8 rxbuf[1];
> >
> > Not DMA-safe. Is it a problem?
>
> Not a problem, the peripheral is PIO only using FIFOs.

Same as somewhere above.

-- 
With Best Regards,
Andy Shevchenko