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
