Messages in this thread Patch in this message | | | Subject | Re: [PATCH v1 2/2] spi: loongson: add bus driver for the loongson spi controller | From | zhuyinbo <> | Date | Fri, 10 Mar 2023 18:01:23 +0800 |
| |
在 2023/3/8 下午11:03, Mark Brown 写道: > On Wed, Mar 08, 2023 at 10:59:08AM +0800, Yinbo Zhu wrote: > >> +config SPI_LOONGSON >> + tristate "Loongson SPI Controller Support" >> + depends on LOONGARCH && OF && PCI > I'm not seeing any build time dependencies here (possibly PCI?) so > please add an || COMPILE_TEST to improve build coverage. It'd be better > to have separate modules for the platform and PCI functionality, that > way someone who has a system without PCI can still use the driver even > with PCI support disabled.
I will add an || COMPILE_TEST and drop && PCI then add some CONFIG_PCI macro
to limit some pci code.
> >> +++ b/drivers/spi/spi-loongson.c >> @@ -0,0 +1,502 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Loongson SPI Support >> + * >> + * Copyright (C) 2023 Loongson Technology Corporation Limited > Please make the entire comment block a C++ one so things look more > intentional. okay, I got it. >> +static int loongson_spi_update_state(struct loongson_spi *loongson_spi, >> + struct spi_device *spi, struct spi_transfer *t) >> +{ >> + unsigned int hz; >> + unsigned int div, div_tmp; >> + unsigned int bit; >> + unsigned char val; >> + const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11}; >> + >> + hz = t ? t->speed_hz : spi->max_speed_hz; > Please write normal conditional statements so that things are legible, > though in this case the core will ensure that there's a speed_hz in > every transfer so there's no need for any of the logic around ensuring > it's set. Do you mean to achieve the following ? and drop spi->max_speed_hz.
if (t) hz = t->speed_hz;
> >> +static int loongson_spi_setup(struct spi_device *spi) >> +{ >> + struct loongson_spi *loongson_spi; >> + >> + loongson_spi = spi_master_get_devdata(spi->master); >> + if (spi->bits_per_word % 8) >> + return -EINVAL; >> + >> + if (spi->chip_select >= spi->master->num_chipselect) >> + return -EINVAL; >> + >> + loongson_spi_update_state(loongson_spi, spi, NULL); >> + loongson_spi_set_cs(loongson_spi, spi, 1); > Note that setup() needs to be able to run for one device while there are > transfers for other devices on the same controller active. okay, I will add a spin_lock for it. > >> +static int loongson_spi_write_read_8bit(struct spi_device *spi, const u8 **tx_buf, >> + u8 **rx_buf, unsigned int num) >> +{ >> + struct loongson_spi *loongson_spi = spi_master_get_devdata(spi->master); >> + >> + if (tx_buf && *tx_buf) { >> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, *((*tx_buf)++)); >> + while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1) >> + ; > A timeout would be good on these spins in case the controller gets > stuck. It'd also be polite to have a cpu_relax() somewhere either here > or in the caller given that it's busy waiting. okay, I got it. > >> +static void loongson_spi_work(struct work_struct *work) >> +{ >> + int param; >> + struct spi_message *m; >> + struct spi_device *spi; >> + struct spi_transfer *t = NULL; >> + struct loongson_spi *loongson_spi = container_of(work, struct loongson_spi, work); >> + >> + spin_lock(&loongson_spi->lock); >> + param = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_PARA_REG); >> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, param&~1); >> + while (!list_empty(&loongson_spi->msg_queue)) { >> + m = container_of(loongson_spi->msg_queue.next, struct spi_message, queue); >> + > This all looks like it's open coding the core's message pump, only > without the heavy optimisation work that the core has and missing some > handling of cs_change and delays. You should implement > spi_transfer_one() instead, this will save a lot of code and should be > more performant. okay, I will try to add a spi_transfer_one for this. > >> +static int loongson_spi_transfer(struct spi_device *spi, struct spi_message *m) >> +{ > In general you'd need an extremely strong reason to implement transfer() > in a new driver. okay, I got it. > >> +static int __maybe_unused loongson_spi_resume(struct device *dev) >> +{ >> +static const struct dev_pm_ops loongson_spi_dev_pm_ops = { >> + .suspend = loongson_spi_suspend, >> + .resume = loongson_spi_resume, >> +}; > The suspend/resume ops are assigned unconditionally. sorry, I don't got it, you mean was to add a CONFIG_PM to limit code ? > >> +subsys_initcall(loongson_spi_init); >> +module_exit(loongson_spi_exit); > Why not just a regular module initcall like most SPI drivers? okay, I will use module_init for register spi drivers.
| |