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