mbox series

[v5,0/2] Add SPI control driver for Sunplus SP7021 SoC

Message ID cover.1641797029.git.lhjeff911@gmail.com
Headers show
Series Add SPI control driver for Sunplus SP7021 SoC | expand

Message

Li-hao Kuo Jan. 10, 2022, 6:47 a.m. UTC 
This is a patch series for SPI driver for Sunplus SP7021 SoC.

Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates
many peripherals (ex: UART, I2C, SPI, SDIO, eMMC, USB, SD card and
etc.) into a single chip. It is designed for industrial control.

Refer to:
https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
https://tibbo.com/store/plus1.html

Li-hao Kuo (2):
  SPI: Add SPI driver for Sunplus SP7021
  devicetree: bindings SPI Add bindings doc for Sunplus SP7021

 .../bindings/spi/spi-sunplus-sp7021.yaml           |  81 +++
 MAINTAINERS                                        |   7 +
 drivers/spi/Kconfig                                |  11 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-sunplus_sp7021.c                   | 603 +++++++++++++++++++++
 5 files changed, 703 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
 create mode 100644 drivers/spi/spi-sunplus_sp7021.c

Comments

Lh Kuo 郭力豪 Jan. 14, 2022, 1:31 a.m. UTC | #1 
> > +               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);

I will add it in next next submission

> > +       pspim->xfer_conf |= ((clk_sel & 0xffff) << 16);
> 
> Is xfer_conf bigger than 32-bit? If not, why do you need the ' & 0xffff' part?
> 
> ...

I will add it in next next submission

> > +               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;


I will add it in next next submission


> > +               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.
>


I will add it in next next submission