Re: [PATCH v5 1/2] SPI: Add SPI driver for Sunplus SP7021

On Mon, Jan 10, 2022 at 8:47 AM Li-hao Kuo [off-list ref] wrote:

Add SPI driver for Sunplus SP7021.

In the subject line use small letters in the prefix. Check with `git
log -- drivers/spi` how people do.

Common comment: Consider to use spi_controller_*() APIs over
spi_master_*() ones. Also your SLA/MAS (and sla/mas) are a bit
confusing: spell them in full and master --> controller or ctrl, slave
--> peripheral or alike.

Signed-off-by: Li-hao Kuo <lhjeff911@gmail.com>
---
Changes in v5:
 - Addressed comments from Mr. Mark Brown
 - Addressed comments from Mr. Andy Shevchenko

You need to elaborate what exactly you addressed.

...

+               writel(readl(pspim->m_base + SP7021_INT_BUSY_REG)
+                       | SP7021_CLR_MAS_INT, pspim->m_base + SP7021_INT_BUSY_REG);

It's better to read with temporary variable being used:

             value = readl(pspim->m_base + SP7021_INT_BUSY_REG);
             value |= SP7021_CLR_MAS_INT;
             writel(value, pspim->m_base + SP7021_INT_BUSY_REG);

...

+       writel(readl(pspim->m_base + SP7021_SPI_STATUS_REG) | SP7021_FD_SW_RST,
+              pspim->m_base + SP7021_SPI_STATUS_REG);

Ditto. And for all other similar cases.

...

+       pspim->xfer_conf |= ((clk_sel & 0xffff) << 16);

Is xfer_conf bigger than 32-bit? If not, why do you need the ' & 0xffff' part?

...

+               ret = 0;

Is it necessary to do this under the lock?

+               if (pspim->xfer_conf & SP7021_CPOL_FD)
+                       writel(pspim->xfer_conf, pspim->m_base + SP7021_SPI_CONFIG_REG);
+
+               mutex_unlock(&pspim->buf_lock);

...

+       if (spi_controller_is_slave(ctlr)) {

Factor out this body to a function, it will increase readability.

+       }
+
+       spi_finalize_current_transfer(ctlr);
+       return ret;

...

+       mode = SP7021_MASTER_MODE;

This...

+       pdev->id = of_alias_get_id(pdev->dev.of_node, "sp_spi");

+       if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
+               mode = SP7021_SLAVE_MODE;

...belongs to this condition, so do not interleave them.

On top of that you may use device property API:

    if (device_property_read_bool(&pdev->dev, "spi-slave"))
        mode = SP7021_SLAVE_MODE;
    else
        mode = SP7021_MASTER_MODE;

...

+       pm_runtime_enable(dev);
+       ret = spi_register_controller(ctlr);
+       if (ret) {
+               pm_runtime_disable(dev);
+               return dev_err_probe(dev, ret, "spi_register_master fail\n");
+       }
+
+       return ret;

return 0;

...

+MODULE_LICENSE("GPL v2");

"GPL", the one you used is legacy.

-- 
With Best Regards,
Andy Shevchenko