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.)
> +#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)
?
...
> +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.
> +};
...
> +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.
...
> +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?
> + } 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.
> + 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.
...
> + /* 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()?
> + 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)
> + return -EINVAL;
> +}
...
> + err = regmap_read(as_ctrl->regmap_nfi,
> + REG_SPI_NFI_SECCUS_SIZE, &val);
One line?
> + 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.
...
> + 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.
> + 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.
> +module_platform_driver(airoha_snand_driver);
--
With Best Regards,
Andy Shevchenko
