| Message ID | 1607413467-17698-1-git-send-email-zhangqing@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support | expand |
On Tue, Dec 08, 2020 at 03:44:24PM +0800, Qing Zhang wrote: > v2: > - keep Kconfig and Makefile sorted > - make the entire comment a C++ one so things look more intentional You say this but... > +++ b/drivers/spi/spi-ls7a.c > @@ -0,0 +1,324 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Loongson LS7A SPI Controller driver > + * > + * Copyright (C) 2020 Loongson Technology Corporation Limited > + */ ...this is still a mix of C and C++ comments? > +static int set_cs(struct ls7a_spi *ls7a_spi, struct spi_device *spi, int val) > +{ > + int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << spi->chip_select); > + > + if (spi->mode & SPI_CS_HIGH) > + val = !val; > + ls7a_spi_write_reg(ls7a_spi, SFCS, > + (val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) | cs); > + > + return 0; > +} Why not just expose this to the core and let it handle things? Please also write normal conditional statements to improve legibility. There's quite a lot of coding style issues in this with things like missing spaces > + if (t) { > + hz = t->speed_hz; > + if (!hz) > + hz = spi->max_speed_hz; > + } else > + hz = spi->max_speed_hz; If one branch of the conditional has braces please use them on both to improve legibility. > +static int ls7a_spi_transfer_one_message(struct spi_master *master, > + struct spi_message *m) I don't understand why the driver is implementing transfer_one_message() - it looks like this is just open coding the standard loop that the framework provides and should just be using transfer_one(). > + r = ls7a_spi_write_read(spi, t); > + if (r < 0) { > + status = r; > + goto error; > + } The indentation here isn't following the kernel coding style. > + master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi)); > + if (!master) > + return -ENOMEM; Why not use devm_ here? > + ret = devm_spi_register_master(dev, master); > + if (ret) > + goto err_free_master; The driver uses devm_spi_register_master() here but... > +static void ls7a_spi_pci_remove(struct pci_dev *pdev) > +{ > + struct spi_master *master = pci_get_drvdata(pdev); > + struct ls7a_spi *spi; > + > + spi = spi_master_get_devdata(master); > + if (!spi) > + return; > + > + pci_release_regions(pdev); ...releases the PCI regions in the remove() function before the SPI controller is freed so the controller could still be active.
Hi Qing,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on spi/for-next]
[also build test ERROR on robh/for-next linus/master v5.10-rc7 next-20201208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
git checkout 6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=c6x
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
drivers/spi/spi-ls7a.c: In function 'ls7a_spi_write_read':
drivers/spi/spi-ls7a.c:162:19: warning: variable 'ls7a_spi' set but not used [-Wunused-but-set-variable]
162 | struct ls7a_spi *ls7a_spi;
| ^~~~~~~~
drivers/spi/spi-ls7a.c: At top level:
>> drivers/spi/spi-ls7a.c:320:1: warning: data definition has no type or storage class
320 | module_pci_driver(ls7a_spi_pci_driver);
| ^~~~~~~~~~~~~~~~~
>> drivers/spi/spi-ls7a.c:320:1: error: type defaults to 'int' in declaration of 'module_pci_driver' [-Werror=implicit-int]
>> drivers/spi/spi-ls7a.c:320:1: warning: parameter names (without types) in function declaration
drivers/spi/spi-ls7a.c:313:26: warning: 'ls7a_spi_pci_driver' defined but not used [-Wunused-variable]
313 | static struct pci_driver ls7a_spi_pci_driver = {
| ^~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +320 drivers/spi/spi-ls7a.c
319
> 320 module_pci_driver(ls7a_spi_pci_driver);
321
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Brown, Thank you for your suggestions, these are achievable, I will send v3 in the soon. Before sending v3, I would like to trouble you to see if this is correct. It has been tested locally. On 12/08/2020 09:56 PM, Mark Brown wrote: > On Tue, Dec 08, 2020 at 03:44:24PM +0800, Qing Zhang wrote: > >> v2: >> - keep Kconfig and Makefile sorted >> - make the entire comment a C++ one so things look more intentional > You say this but... > >> +++ b/drivers/spi/spi-ls7a.c >> @@ -0,0 +1,324 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Loongson LS7A SPI Controller driver >> + * >> + * Copyright (C) 2020 Loongson Technology Corporation Limited >> + */ > ...this is still a mix of C and C++ comments? Replace all with // > >> +static int set_cs(struct ls7a_spi *ls7a_spi, struct spi_device *spi, int val) >> +{ >> + int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << spi->chip_select); >> + >> + if (spi->mode & SPI_CS_HIGH) >> + val = !val; >> + ls7a_spi_write_reg(ls7a_spi, SFCS, >> + (val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) | cs); >> + >> + return 0; >> +} > Why not just expose this to the core and let it handle things? > > Please also write normal conditional statements to improve legibility. > There's quite a lot of coding style issues in this with things like > missing spaces static void ls7a_spi_set_cs(struct spi_device *spi, bool enable) { struct ls7a_spi *ls7a_spi; int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << spi->chip_select)); ls7a_spi = spi_master_get_devdata(spi->master); if (!!(spi->mode & SPI_CS_HIGH) == enable) val = (0x11 << spi->chip_select) | cs; else val = (0x1 << spi->chip_select) | cs; ls7a_spi_write_reg(ls7a_spi, SFCS, val); } static int ls7a_spi_pci_probe----> +master->set_cs = ls7a_spi_set_cs; > >> + if (t) { >> + hz = t->speed_hz; >> + if (!hz) >> + hz = spi->max_speed_hz; >> + } else >> + hz = spi->max_speed_hz; > If one branch of the conditional has braces please use them on both to > improve legibility. > >> +static int ls7a_spi_transfer_one_message(struct spi_master *master, >> + struct spi_message *m) > I don't understand why the driver is implementing transfer_one_message() > - it looks like this is just open coding the standard loop that the > framework provides and should just be using transfer_one(). static int ls7a_spi_transfer_one(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t) { struct ls7a_spi *ls7a_spi; int param, status; ls7a_spi = spi_master_get_devdata(master); spin_lock(&ls7a_spi->lock); param = ls7a_spi_read_reg(ls7a_spi, PARA); ls7a_spi_write_reg(ls7a_spi, PARA, param&~1); spin_unlock(&ls7a_spi->lock); status = ls7a_spi_do_transfer(ls7a_spi, spi, t); if(status < 0) return status; if(t->len) r = ls7a_spi_write_read(spi, t); spin_lock(&ls7a_spi->lock); ls7a_spi_write_reg(ls7a_spi, PARA, param); spin_unlock(&ls7a_spi->lock); return status; } static int ls7a_spi_pci_probe----> - master->transfer_one_message = ls7a_spi_transfer_one_message; +master->transfer_one = ls7a_spi_transfer_one; > >> + r = ls7a_spi_write_read(spi, t); >> + if (r < 0) { >> + status = r; >> + goto error; >> + } > The indentation here isn't following the kernel coding style. > >> + master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi)); >> + if (!master) >> + return -ENOMEM; > Why not use devm_ here? - master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi)); error: - spi_put_master(master); + master = devm_spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi)); > >> + ret = devm_spi_register_master(dev, master); >> + if (ret) >> + goto err_free_master; > The driver uses devm_spi_register_master() here but... > >> +static void ls7a_spi_pci_remove(struct pci_dev *pdev) >> +{ >> + struct spi_master *master = pci_get_drvdata(pdev); >> + struct ls7a_spi *spi; >> + >> + spi = spi_master_get_devdata(master); >> + if (!spi) >> + return; >> + >> + pci_release_regions(pdev); > ...releases the PCI regions in the remove() function before the SPI > controller is freed so the controller could still be active. static void ls7a_spi_pci_remove(struct pci_dev *pdev) { struct spi_master *master = pci_get_drvdata(pdev); + spi_unregister_master(master); pci_release_regions(pdev); } Thanks, -Qing
On Wed, Dec 09, 2020 at 03:24:15PM +0800, zhangqing wrote: > > > +static int ls7a_spi_transfer_one_message(struct spi_master *master, > > > + struct spi_message *m) > > I don't understand why the driver is implementing transfer_one_message() > > - it looks like this is just open coding the standard loop that the > > framework provides and should just be using transfer_one(). > static int ls7a_spi_transfer_one(struct spi_master *master, > struct spi_device *spi, > struct spi_transfer *t) > { > struct ls7a_spi *ls7a_spi; > int param, status; > > ls7a_spi = spi_master_get_devdata(master); > > spin_lock(&ls7a_spi->lock); > param = ls7a_spi_read_reg(ls7a_spi, PARA); > ls7a_spi_write_reg(ls7a_spi, PARA, param&~1); > spin_unlock(&ls7a_spi->lock); I don't know what this does but is it better split out into a prepare_message()? It was only done once per message in your previous implementation. Or possibly runtime PM would be even better if that's what it's doing. > > ...releases the PCI regions in the remove() function before the SPI > > controller is freed so the controller could still be active. > > static void ls7a_spi_pci_remove(struct pci_dev *pdev) > { > struct spi_master *master = pci_get_drvdata(pdev); > > + spi_unregister_master(master); > pci_release_regions(pdev); > } You also need to change to using plain spi_register_master() but yes. Otherwise everything looked good.
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index aadaea0..af7c0d4 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -413,6 +413,13 @@ config SPI_LP8841_RTC Say N here unless you plan to run the kernel on an ICP DAS LP-8x4x industrial computer. +config SPI_LS7A + tristate "Loongson LS7A SPI Controller Support" + depends on CPU_LOONGSON64 || COMPILE_TEST + help + This drivers supports the Loongson LS7A SPI controller in master + SPI mode. + config SPI_MPC52xx tristate "Freescale MPC52xx SPI (non-PSC) controller support" depends on PPC_MPC52xx diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index 6fea582..d015cf2 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -61,6 +61,7 @@ obj-$(CONFIG_SPI_LANTIQ_SSC) += spi-lantiq-ssc.o obj-$(CONFIG_SPI_JCORE) += spi-jcore.o obj-$(CONFIG_SPI_LM70_LLP) += spi-lm70llp.o obj-$(CONFIG_SPI_LP8841_RTC) += spi-lp8841-rtc.o +obj-$(CONFIG_SPI_LS7A) += spi-ls7a.o obj-$(CONFIG_SPI_MESON_SPICC) += spi-meson-spicc.o obj-$(CONFIG_SPI_MESON_SPIFC) += spi-meson-spifc.o obj-$(CONFIG_SPI_MPC512x_PSC) += spi-mpc512x-psc.o diff --git a/drivers/spi/spi-ls7a.c b/drivers/spi/spi-ls7a.c new file mode 100644 index 0000000..21ca1ab --- /dev/null +++ b/drivers/spi/spi-ls7a.c @@ -0,0 +1,324 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Loongson LS7A SPI Controller driver + * + * Copyright (C) 2020 Loongson Technology Corporation Limited + */ + +#include <linux/init.h> +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/interrupt.h> +#include <linux/delay.h> +#include <linux/platform_device.h> +#include <linux/err.h> +#include <linux/spi/spi.h> +#include <linux/pci.h> +#include <linux/of.h> + +/* define spi register */ +#define SPCR 0x00 +#define SPSR 0x01 +#define FIFO 0x02 +#define SPER 0x03 +#define PARA 0x04 +#define SFCS 0x05 +#define TIMI 0x06 + +struct ls7a_spi { + spinlock_t lock; + struct spi_master *master; + void __iomem *base; + int cs_active; + unsigned int hz; + unsigned char spcr, sper; + unsigned int mode; +}; + +static void ls7a_spi_write_reg(struct ls7a_spi *spi, + unsigned char reg, + unsigned char data) +{ + writeb(data, spi->base + reg); +} + +static char ls7a_spi_read_reg(struct ls7a_spi *spi, + unsigned char reg) +{ + return readb(spi->base + reg); +} + +static int set_cs(struct ls7a_spi *ls7a_spi, struct spi_device *spi, int val) +{ + int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << spi->chip_select); + + if (spi->mode & SPI_CS_HIGH) + val = !val; + ls7a_spi_write_reg(ls7a_spi, SFCS, + (val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) | cs); + + return 0; +} + +static int ls7a_spi_do_transfer(struct ls7a_spi *ls7a_spi, + struct spi_device *spi, + struct spi_transfer *t) +{ + unsigned int hz; + unsigned int div, div_tmp; + unsigned int bit; + unsigned long clk; + unsigned char val; + const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11}; + + if (t) { + hz = t->speed_hz; + if (!hz) + hz = spi->max_speed_hz; + } else + hz = spi->max_speed_hz; + + if (((spi->mode ^ ls7a_spi->mode) & (SPI_CPOL | SPI_CPHA)) + || (hz && ls7a_spi->hz != hz)) { + clk = 100000000; + + div = DIV_ROUND_UP(clk, hz); + if (div < 2) + div = 2; + if (div > 4096) + div = 4096; + + bit = fls(div) - 1; + if ((1<<bit) == div) + bit--; + div_tmp = rdiv[bit]; + + dev_dbg(&spi->dev, "clk = %ld hz = %d div_tmp = %d bit = %d\n", + clk, hz, div_tmp, bit); + + ls7a_spi->hz = hz; + ls7a_spi->spcr = div_tmp & 3; + ls7a_spi->sper = (div_tmp >> 2) & 3; + + val = ls7a_spi_read_reg(ls7a_spi, SPCR); + val &= ~0xc; + if (spi->mode & SPI_CPOL) + val |= 8; + if (spi->mode & SPI_CPHA) + val |= 4; + ls7a_spi_write_reg(ls7a_spi, SPCR, (val & ~3) | ls7a_spi->spcr); + val = ls7a_spi_read_reg(ls7a_spi, SPER); + ls7a_spi_write_reg(ls7a_spi, SPER, (val & ~3) | ls7a_spi->sper); + ls7a_spi->mode = spi->mode; + } + return 0; +} + +static int ls7a_spi_setup(struct spi_device *spi) +{ + struct ls7a_spi *ls7a_spi; + + ls7a_spi = spi_master_get_devdata(spi->master); + + if (spi->chip_select >= spi->master->num_chipselect) + return -EINVAL; + + set_cs(ls7a_spi, spi, 1); + + return 0; +} + +static int ls7a_spi_write_read_8bit(struct spi_device *spi, + const u8 **tx_buf, u8 **rx_buf, + unsigned int num) +{ + struct ls7a_spi *ls7a_spi; + + ls7a_spi = spi_master_get_devdata(spi->master); + + if (tx_buf && *tx_buf) { + ls7a_spi_write_reg(ls7a_spi, FIFO, *((*tx_buf)++)); + + while ((ls7a_spi_read_reg(ls7a_spi, SPSR) & 0x1) == 1) + ; + } else { + ls7a_spi_write_reg(ls7a_spi, FIFO, 0); + + while ((ls7a_spi_read_reg(ls7a_spi, SPSR) & 0x1) == 1) + ; + } + + if (rx_buf && *rx_buf) + *(*rx_buf)++ = ls7a_spi_read_reg(ls7a_spi, FIFO); + else + ls7a_spi_read_reg(ls7a_spi, FIFO); + + return 1; +} + +static unsigned int ls7a_spi_write_read(struct spi_device *spi, + struct spi_transfer *xfer) +{ + struct ls7a_spi *ls7a_spi; + unsigned int count; + const u8 *tx = xfer->tx_buf; + u8 *rx = xfer->rx_buf; + + ls7a_spi = spi_master_get_devdata(spi->master); + count = xfer->len; + + do { + if (ls7a_spi_write_read_8bit(spi, &tx, &rx, count) < 0) + goto out; + count--; + } while (count); + +out: + return xfer->len - count; +} + +static int ls7a_spi_transfer_one_message(struct spi_master *master, + struct spi_message *m) +{ + struct ls7a_spi *ls7a_spi; + struct spi_device *spi; + struct spi_transfer *t = NULL; + int param, status, r; + unsigned int total_len = 0; + unsigned int cs_change; + const int nsecs = 50; + + ls7a_spi = spi_master_get_devdata(master); + spi = m->spi; + + cs_change = 1; + + spin_lock(&ls7a_spi->lock); + param = ls7a_spi_read_reg(ls7a_spi, PARA); + ls7a_spi_write_reg(ls7a_spi, PARA, param&~1); + spin_unlock(&ls7a_spi->lock); + list_for_each_entry(t, &m->transfers, transfer_list) { + if (cs_change) { + set_cs(ls7a_spi, spi, 0); + ls7a_spi_do_transfer(ls7a_spi, spi, t); + ndelay(nsecs); + } + cs_change = t->cs_change; + + r = ls7a_spi_write_read(spi, t); + if (r < 0) { + status = r; + goto error; + } + total_len += r; + + spi_transfer_delay_exec(t); + + if (cs_change) { + set_cs(ls7a_spi, spi, 1); + ndelay(nsecs); + } + } + + spin_lock(&ls7a_spi->lock); + ls7a_spi_write_reg(ls7a_spi, PARA, param); + spin_unlock(&ls7a_spi->lock); + + if (!cs_change) { + ndelay(nsecs); + set_cs(ls7a_spi, spi, 1); + } + +error: + m->status = status; + m->actual_length = total_len; + spi_finalize_current_message(master); + return status; +} + +static int ls7a_spi_pci_probe(struct pci_dev *pdev, + const struct pci_device_id *ent) +{ + struct device *dev = &pdev->dev; + struct spi_master *master; + struct ls7a_spi *spi; + int ret; + + master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi)); + if (!master) + return -ENOMEM; + + spi = spi_master_get_devdata(master); + ret = pcim_enable_device(pdev); + if (ret) + goto err_free_master; + + ret = pci_request_regions(pdev, "ls7a-spi"); + if (ret) + goto err_free_master; + + spi->base = pcim_iomap(pdev, 0, pci_resource_len(pdev, 0)); + if (!spi->base) { + ret = -EINVAL; + goto err_free_master; + } + ls7a_spi_write_reg(spi, SPCR, 0x51); + ls7a_spi_write_reg(spi, SPER, 0x00); + ls7a_spi_write_reg(spi, TIMI, 0x01); + ls7a_spi_write_reg(spi, PARA, 0x40); + spi->mode = 0; + + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH; + master->setup = ls7a_spi_setup; + master->transfer_one_message = ls7a_spi_transfer_one_message; + master->bits_per_word_mask = SPI_BPW_MASK(8); + master->num_chipselect = 4; + master->dev.of_node = of_node_get(pdev->dev.of_node); + + spi->master = master; + + pci_set_drvdata(pdev, master); + + ret = devm_spi_register_master(dev, master); + if (ret) + goto err_free_master; + + return 0; + +err_free_master: + pci_release_regions(pdev); + spi_master_put(master); + return ret; +} + +static void ls7a_spi_pci_remove(struct pci_dev *pdev) +{ + struct spi_master *master = pci_get_drvdata(pdev); + struct ls7a_spi *spi; + + spi = spi_master_get_devdata(master); + if (!spi) + return; + + pci_release_regions(pdev); +} + +static const struct pci_device_id ls7a_spi_pci_id_table[] = { + { PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a0b) }, + { 0, } +}; + +MODULE_DEVICE_TABLE(pci, ls7a_spi_pci_id_table); + +static struct pci_driver ls7a_spi_pci_driver = { + .name = "ls7a-spi", + .id_table = ls7a_spi_pci_id_table, + .probe = ls7a_spi_pci_probe, + .remove = ls7a_spi_pci_remove, +}; + +module_pci_driver(ls7a_spi_pci_driver); + +MODULE_AUTHOR("Juxin Gao <gaojuxin@loongson.cn>"); +MODULE_AUTHOR("Qing Zhang <zhangqing@loongson.cn>"); +MODULE_DESCRIPTION("Loongson LS7A SPI controller driver");