| Message ID | cover.1641797029.git.lhjeff911@gmail.com |
|---|---|
| Headers | show |
| Series | Add SPI control driver for Sunplus SP7021 SoC | expand |
On Mon, Jan 10, 2022 at 8:47 AM Li-hao Kuo <lhjeff911@gmail.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@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.
> > + 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