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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Tue, Apr 23, 2024 at 12:16:37PM +0200, Lorenzo Bianconi kirjoitti:
> > Introduce support for SPI-NAND driver of the Airoha NAND Flash Interface
> > found on Airoha ARM SoCs.
> 
> ...
> 
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/init.h>
> > +#include <linux/iopoll.h>
> 
> > +#include <linux/kernel.h>
> 
> Make sure you are using exact headers you need, this one seems "proxy" and not
> really in use here.
> 
> (Quite likely you wanted minmax.h, types.h, and possible others.)

ack, I will fix it.

> 
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/spi/spi-mem.h>
> 
> ...
> 
> > +#define SPI_NFI_ALL_IRQ_EN			(SPI_NFI_RD_DONE_EN | \
> > +						 SPI_NFI_WR_DONE_EN | \
> > +						 SPI_NFI_RST_DONE_EN | \
> > +						 SPI_NFI_ERASE_DONE_EN | \
> > +						 SPI_NFI_BUSY_RETURN_EN | \
> > +						 SPI_NFI_ACCESS_LOCK_EN | \
> > +						 SPI_NFI_AHB_DONE_EN)
> 
> What about writing this as
> 
> #define SPI_NFI_ALL_IRQ_EN					\
> 	(SPI_NFI_RD_DONE_EN | SPI_NFI_WR_DONE_EN |		\
> 	 SPI_NFI_RST_DONE_EN | SPI_NFI_ERASE_DONE_EN |		\
> 	 SPI_NFI_BUSY_RETURN_EN | SPI_NFI_ACCESS_LOCK_EN |	\
> 	 SPI_NFI_AHB_DONE_EN)
> 
> ?

no strong opinion about it, I will fix it.

> 
> ...
> 
> > +enum airoha_snand_mode {
> > +	SPI_MODE_AUTO,
> > +	SPI_MODE_MANUAL,
> > +	SPI_MODE_DMA,
> > +	SPI_MODE_NO
> 
> Is _NO a termination entry? Meaning there always be only 3 modes no matter
> what. If not, leave the trailing comma as it helps to reduce a burden in case
> this list will be expanded.

I think we can get rid of it

> 
> > +};
> 
> ...
> 
> > +struct airoha_snand_dev {
> > +	size_t buf_len;
> > +
> > +	u8 *txrx_buf;
> > +	dma_addr_t dma_addr;
> > +
> > +	bool data_need_update;
> > +	u64 cur_page_num;
> > +};
> 
> Most likely `pahole` shows better layout to save a few bytes in some cases.

ack, I think we can swap data_need_update and cur_page_num.

> 
> ...
> 
> > +struct airoha_snand_ctrl {
> > +	struct device *dev;
> > +	struct regmap *regmap_ctrl;
> > +	struct regmap *regmap_nfi;
> > +	struct clk *spi_clk;
> > +
> > +	struct {
> > +		size_t page_size;
> > +		size_t sec_size;
> 
> > +		unsigned char sec_num;
> > +		unsigned char spare_size;
> 
> Hmm... Why not u8 for both of these?

ack, I will fix it.

> 
> > +	} nfi_cfg;
> > +};
> 
> ...
> 
> > +static int airoha_snand_write_data(struct airoha_snand_ctrl *as_ctrl, u8 cmd,
> > +				   const u8 *data, int len)
> > +{
> > +	int i = 0;
> > +
> > +	while (i < len) {
> 
> Seems nothing prevents you from using for-loop here as well.

ack, I will fix it.

> 
> > +		int data_len = min(len, MAX_TRANSFER_SIZE);
> > +		int err;
> > +
> > +		err = airoha_snand_set_fifo_op(as_ctrl, cmd, data_len);
> > +		if (err)
> > +			return err;
> > +
> > +		err = airoha_snand_write_data_to_fifo(as_ctrl, &data[i],
> > +						      data_len);
> > +		if (err < 0)
> > +			return err;
> > +
> > +		i += data_len;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int airoha_snand_read_data(struct airoha_snand_ctrl *as_ctrl, u8 *data,
> > +				  int len)
> 
> As per above.

ack, I will fix it.

> 
> ...
> 
> > +	/* addr part */
> > +	for (i = 0; i < op->addr.nbytes; i++) {
> > +		u8 cmd = opcode == SPI_NAND_OP_GET_FEATURE ? 0x11 : 0x8;
> > +
> > +		data = op->addr.val >> ((op->addr.nbytes - i - 1) * 8);
> 
> Seems like you wanted to have always the same endianess and hence can be done
> outside the loop via cpu_to_xxx()?

sorry, I did not get what you mean here, data value relies on the loop
iteration.

> 
> > +		err = airoha_snand_write_data(as_ctrl, cmd, &data,
> > +					      sizeof(data));
> > +		if (err)
> > +			return err;
> > +	}
> 
> ...
> 
> > +static int airoha_snand_setup(struct spi_device *spi)
> > +{
> > +	struct airoha_snand_dev *as_dev = spi_get_ctldata(spi);
> > +	struct airoha_snand_ctrl *as_ctrl;
> > +
> > +	as_dev = kzalloc(sizeof(*as_dev), GFP_KERNEL);
> > +	if (!as_dev)
> > +		return -ENOMEM;
> > +
> > +	spi_set_ctldata(spi, as_dev);
> > +	as_dev->data_need_update = true;
> > +
> > +	/* prepare device buffer */
> > +	as_dev->buf_len = SPI_NAND_CACHE_SIZE;
> > +	as_dev->txrx_buf = kzalloc(as_dev->buf_len, GFP_KERNEL);
> > +	if (!as_dev->txrx_buf)
> > +		goto error_dev_free;
> > +
> > +	as_ctrl = spi_controller_get_devdata(spi->controller);
> > +	as_dev->dma_addr = dma_map_single(as_ctrl->dev, as_dev->txrx_buf,
> > +					  as_dev->buf_len, DMA_BIDIRECTIONAL);
> > +	if (dma_mapping_error(as_ctrl->dev, as_dev->dma_addr))
> > +		goto error_buf_free;
> > +
> > +	return 0;
> > +
> > +error_buf_free:
> > +	kfree(as_dev->txrx_buf);
> > +error_dev_free:
> > +	kfree(as_dev);
> 
> Why not utilising cleanup.h? (__free(), no_free_ptr(), etc)

I guess we can just allocate as_dev and as_dev->txrx_buf with devm_kzalloc()
here, agree?

> 
> > +	return -EINVAL;
> > +}
> 
> ...
> 
> > +	err = regmap_read(as_ctrl->regmap_nfi,
> > +			  REG_SPI_NFI_SECCUS_SIZE, &val);
> 
> One line?

ack, I will fix it.

> 
> > +	if (err)
> > +		return err;
> 
> ...
> 
> > +	as_ctrl->nfi_cfg.page_size = rounddown(sec_size * sec_num, 1024);
> 
> round_down() is optimised for power-of-2.
> You would need to include math.h IIRC.

ack, I will fix it.
> 
> ...
> 
> > +	as_ctrl->regmap_ctrl = devm_regmap_init_mmio(&pdev->dev, base,
> > +						     &spi_ctrl_regmap_config);
> 
> With help of
> 
> 	struct device *dev = &pdev->dev;
> 
> at the top of the function the entire code will become neater.

ack, I will fix it.

> 
> > +	if (IS_ERR(as_ctrl->regmap_ctrl)) {
> > +		dev_err(&pdev->dev, "failed to init spi ctrl regmap: %ld\n",
> > +			PTR_ERR(as_ctrl->regmap_ctrl));
> > +		return PTR_ERR(as_ctrl->regmap_ctrl);
> 
> 		return dev_err_probe(...);
> 
> > +	}
> 
> ...
> 
> > +		dev_err(&pdev->dev, "failed to init spi nfi regmap: %ld\n",
> > +			PTR_ERR(as_ctrl->regmap_nfi));
> > +		return PTR_ERR(as_ctrl->regmap_nfi);
> 
> 		return dev_err_probe(...);
> 
> ...
> 
> > +		dev_err(&pdev->dev, "unable to get spi clk");
> > +		return PTR_ERR(as_ctrl->spi_clk);
> 
> Ditto.
> 
> ...
> 
> > +
> 
> Unneeded blank line.

ack, I will fix it.

Regards,
Lorenzo

> 
> > +module_platform_driver(airoha_snand_driver);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

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