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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5306 bytes --]

> On Fri, Apr 26, 2024 at 11:31 AM Lorenzo Bianconi <lorenzo@kernel.org> 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.
> 

ack we can use <asm/unaligned.h> instead

> > +#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.

ack, I will fix it.

> 
> ...
> 
> > +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 ?

ack, I will fix it.

> 
> ...
> 
> > +               /* 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?

it is because we still need the 'return false' at the end of routine for the
other cases due to SPI_MEM_DATA_IN and SPI_MEM_DATA_OUT.

> 
> ...
> 
> > +               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.

do you mean doing something like:

op->data.nbytes = clamp(op->data.nbytes, op->data.nbytes, 160 - len);

maybe an 'if' condition is more readable, what do you think?

> 
> ...
> 
> > +       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.

ack, I agree. I will fix it.

> 
> ...
> 
> > +       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.

ack, I agree, we can get rid of them.

> 
> ...
> 
> > +       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?

right, we can pass NULL here to devm_platform_get_and_ioremap_resource()

> 
> > +       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.

ack, I will fix it.

Regards,
Lorenzo

> 
> -- 
> With Best Regards,
> Andy Shevchenko

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]