Re: [PATCH v4 3/3] spi: airoha: add SPI-NAND Flash controller driver

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Fri, Apr 26, 2024 at 11:31 AM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote:
>
> Introduce support for SPI-NAND driver of the Airoha NAND Flash Interface
> found on Airoha ARM SoCs.

...

> +#include <asm-generic/unaligned.h>

No driver should include asm-generic, basically 99.9% of the kernel
code must not do that. I.o.w. asm-generic is very special.

> +#include <linux/bitfield.h>
> +#include <linux/clk.h>

+ delay.h

> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>

+ errno.h

> +#include <linux/types.h>

Can you make it ordered (I noticed this after a while)?

+ limits.h

> +#include <linux/math.h>

+ minmax.h

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/sizes.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>

+ types.h

Also note, we usually place headers from more generic to less, hence
linux/* followed by asm/* and not vice versa.

...

> +struct airoha_snand_dev {
> +       size_t buf_len;
> +
> +       u8 *txrx_buf;
> +       dma_addr_t dma_addr;
> +
> +       u64 cur_page_num;
> +       bool data_need_update;
> +};

...

> +               /* quad io / quad out */

io --> in ?

...

> +               /* dual io / dual out */

Ditto.

...

> +       case SPI_MEM_DATA_OUT:
> +               /* check dummy cycle first */
> +               if (op->dummy.nbytes)
> +                       return false;
> +
> +               /* program load quad out */
> +               if (op->addr.buswidth == 1 && op->data.buswidth == 4)
> +                       return true;
> +
> +               /* standard spi */
> +               if (op->addr.buswidth == 1 && op->data.buswidth == 1)
> +                       return true;

> +       default:
> +               break;
> +       }
> +
> +       return false;

Why not return false directly from the default case?

...

> +               op->data.nbytes = min_t(size_t, op->data.nbytes, 160 - len);

You probably wanted clamp(). It's discouraged to use min_t() for unsigned types.

...

> +       err = regmap_read_poll_timeout(as_ctrl->regmap_nfi, REG_SPI_NFI_INTR,
> +                                      val, (val & SPI_NFI_AHB_DONE), 0,
> +                                      USEC_PER_SEC);

Perhaps
  1 * USEC_PER_SEC
?

Easy to read plain numbers like this to get the idea "this is 1 SEC
timeout". Also editors highlight plain integers with a different
colour.

...

> +       /* addr part */
> +       cmd = opcode == SPI_NAND_OP_GET_FEATURE ? 0x11 : 0x8;
> +       put_unaligned_be64(op->addr.val, data);

> +       for (i = 0; i < op->addr.nbytes; i++) {
> +               err = airoha_snand_write_data(as_ctrl, cmd,
> +                                             &data[8 - op->addr.nbytes + i],

Now you can update a for loop to make this prettier, right?

> +                                             sizeof(data[0]));
> +               if (err)
> +                       return err;
> +       }

       for (i = 8 - op->addr.nbytes; i < 8; i++) {
               err = airoha_snand_write_data(as_ctrl, cmd, &data[i],
                                             sizeof(data[0]));
               ...
       }

Note, 8 can be replaced by sizeof() / ARRAY_SIZE() but I'm not insisting.

...

> +       devm_kfree(as_ctrl->dev, as_dev->txrx_buf);
> +       devm_kfree(as_ctrl->dev, as_dev);

Why?! Using devm_*free() explicitly hints about either
misunderstanding of devm concept, or object's lifetime.

...

> +       spi_set_ctldata(spi, NULL);

Seems there is no consensus on NULLifying this (when, if even needed),
but it's fine.

...

> +       base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);

How is 'res' being used exactly?

> +       if (IS_ERR(base))
> +               return PTR_ERR(base);

...

> +       base = devm_platform_get_and_ioremap_resource(pdev, 1, &res);

Ditto.

> +       if (IS_ERR(base))
> +               return PTR_ERR(base);


...

> +       ctrl->dev.of_node = dev->of_node;

Use device_set_node() instead.
You might need dev_fwnode() from property.h.

-- 
With Best Regards,
Andy Shevchenko