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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 10 Jan 2022 12:58:56 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Li-hao Kuo <lhjeff911@...il.com>
Cc:     Philipp Zabel <p.zabel@...gutronix.de>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        linux-spi <linux-spi@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Wells Lu 呂芳騰 <wells.lu@...plus.com>,
        "LH.Kuo" <lh.kuo@...plus.com>
Subject: Re: [PATCH v5 1/2] SPI: Add SPI driver for Sunplus SP7021

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