> 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.
>
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:
signature.asc
Description: PGP signature
