Re: [PATCH v1 2/2] spi: loongson: add bus driver for the loongson spi controller

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

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

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

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

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

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

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

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

> +subsys_initcall(loongson_spi_init);
> +module_exit(loongson_spi_exit);

Why not just a regular module initcall like most SPI drivers?

Attachment: signature.asc
Description: PGP signature