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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Mon, Jan 10, 2022 at 8:47 AM Li-hao Kuo <lhjeff911@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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