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

andy.shevchenko
2023-05-08T15:04:30Z

Wed, Apr 26, 2023 at 03:10:45PM +0800, Yinbo Zhu kirjoitti:
> This bus driver supports the Loongson spi hardware controller in the
> Loongson platforms and supports to use DTS and PCI framework to
> register spi device resources.

SPI

...

> +config SPI_LOONGSON_CORE
> + tristate "Loongson SPI Controller Core Driver Support"

Does it need to be visible to the user?

> + depends on LOONGARCH || COMPILE_TEST
> + help
> + This core driver supports the Loongson spi hardware controller in
> + the Loongson platforms.
> + Say Y or M here if you want to use the SPI controller on
> + Loongson platform.

...

> +config SPI_LOONGSON_PLATFORM
> + tristate "Loongson SPI Controller Platform Driver Support"
> + select SPI_LOONGSON_CORE
> + depends on OF && (LOONGARCH || COMPILE_TEST)

Is it really dependent to OF? Why?

> + help
> + This bus driver supports the Loongson spi hardware controller in
> + the Loongson platforms and supports to use DTS framework to
> + register spi device resources.
> + Say Y or M here if you want to use the SPI controller on
> + Loongson platform.

...

> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/spi/spi.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>

Ordered?

...

> + if (loongson_spi->mode & SPI_NO_CS)
> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SFCS_REG, 0);

Missing {}

> + else {
> + cs = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SFCS_REG)
> + & ~(0x11 << spi->chip_select);
> + loongson_spi_write_reg(loongson_spi,
> + LOONGSON_SPI_SFCS_REG,
> + (val ? (0x11 << spi->chip_select) :
> + (0x1 << spi->chip_select)) | cs);

Too many parentheses.

> + }

...

> + const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};

Oh, why?!

...

> + if ((hz && loongson_spi->hz != hz) ||
> + ((spi->mode ^ loongson_spi->mode) & (SPI_CPOL | SPI_CPHA))) {
> + div = DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz);

> + if (div < 2)
> + div = 2;
> + if (div > 4096)
> + div = 4096;

NIH clamp_val()

> + bit = fls(div) - 1;
> + if ((1<<bit) == div)
> + bit--;
> + div_tmp = rdiv[bit];

I believe this can be optimized.

> + dev_dbg(&spi->dev, "clk_rate = %llu hz = %d div_tmp = %d bit = %d\n",
> + loongson_spi->clk_rate, hz, div_tmp, bit);
> +
> + loongson_spi->hz = hz;
> + loongson_spi->spcr = div_tmp & 3;
> + loongson_spi->sper = (div_tmp >> 2) & 3;
> + val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
> + val &= ~0xc;

GENMASK()

> + if (spi->mode & SPI_CPOL)
> + val |= 8;

BIT()

> + if (spi->mode & SPI_CPHA)
> + val |= 4;

> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, (val & ~3) |
> + loongson_spi->spcr);
> + val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPER_REG);
> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPER_REG, (val & ~3) |
> + loongson_spi->sper);
> + loongson_spi->mode &= SPI_NO_CS;
> + loongson_spi->mode |= spi->mode;
> + }

...

> + while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1 &&
> + time_after(timeout, jiffies))
> + cpu_relax();

iopoll.h has a suitable macro for this.

...

> + while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1 &&
> + time_after(timeout, jiffies))
> + cpu_relax();

Ditto.

...

> + master = devm_spi_alloc_master(dev, sizeof(struct loongson_spi));
> + if (master == NULL) {

> + dev_info(dev, "master allocation failed\n");

We do not issue a message for ENOMEM

> + return -ENOMEM;
> + }

...

> + master->dev.of_node = of_node_get(dev->of_node);

device_set_node()

...

> + spi->base = devm_ioremap(dev, res->start, resource_size(res));

Why not devm_ioremap_resource()?

> + if (spi->base == NULL) {
> + dev_err(dev, "cannot map io\n");
> + return -ENXIO;

return dev_err_probe();

> + }

...

> + clk = devm_clk_get(dev, NULL);

Can we hav

> + if (!IS_ERR(clk))

Use _optional variant above instead of this.
Do not forget about deferred probe.

> + spi->clk_rate = clk_get_rate(clk);

...

> + if (of_get_property(dev->of_node, "spi-nocs", NULL))
> + spi->mode |= SPI_NO_CS;

Don't we have something in the SPI core to handle this in a generic way?

...

> +EXPORT_SYMBOL_GPL(loongson_spi_init_master);

Please, use _NS variant.

...

> +MODULE_DESCRIPTION("Loongson spi core driver");

SPI

...

> + struct resource res[2];
> + struct device *dev = &pdev->dev;
> +
> + ret = pci_enable_device(pdev);

pcim_enable_device()

> + if (ret < 0) {
> + dev_err(dev, "cannot enable pci device\n");
> + goto err_out;

return dev_err_probe();

> + }
> +
> + ret = pci_request_region(pdev, 0, "loongson-spi io");
> + if (ret < 0) {
> + dev_err(dev, "cannot request region 0.\n");
> + goto err_out;
> + }
> +
> + res[0].start = pci_resource_start(pdev, 0);
> + res[0].end = pci_resource_end(pdev, 0);

What's wrong with pcim_iomap_regions()?

...

> + ret = pci_read_config_byte(pdev, PCI_INTERRUPT_LINE, &v8);

What?!

What's wrong with pci_alloc_irq_vectors()?

> +
> + if (ret == PCIBIOS_SUCCESSFUL) {
> + res[1].start = v8;
> + res[1].end = v8;
> + }
> +
> + ret = loongson_spi_init_master(dev, res);

Why not passing the remapped address and IRQ number instead?

> + if (ret)
> + dev_err(dev, "failed to initialize master\n");

return dev_err_probe();

> +
> +err_out:

Completely useless. Return in-line.

> + return ret;
> +}

...

> +static struct pci_device_id loongson_spi_devices[] = {
> + {PCI_DEVICE(0x14, 0x7a0b)},
> + {PCI_DEVICE(0x14, 0x7a1b)},

Can you define vendor ID in pci_ids.h?

> + {0, 0, 0, 0, 0, 0, 0}

What is this? Why {} is not working for you?

> +};

> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {

Why not using devm_platform_ioremap_resource()?

> + dev_err(dev, "cannot get io resource memory\n");
> + return -ENOENT;

return dev_err_probe();

> + }
> +
> + ret = loongson_spi_init_master(dev, res);
> + if (ret)
> + dev_err(dev, "failed to initialize master\n");

Ditto.

...

> +static const struct of_device_id loongson_spi_id_table[] = {
> + { .compatible = "loongson,ls2k-spi", },

Inned comma is redundant.

> + { }
> +};

...

> +#ifndef __LINUX_SPI_LOONGSON_H
> +#define __LINUX_SPI_LOONGSON_H

Missing bits.h
Missing types.h
Missing declaration for msecs_to_jiffies()
Missing forward declarations for struct spi_master and struct device.
MIssing declaration for dev_pm_ops.

> +#define LOONGSON_SPI_SPCR_REG 0x00
> +#define LOONGSON_SPI_SPSR_REG 0x01
> +#define LOONGSON_SPI_FIFO_REG 0x02
> +#define LOONGSON_SPI_SPER_REG 0x03
> +#define LOONGSON_SPI_PARA_REG 0x04
> +#define LOONGSON_SPI_SFCS_REG 0x05
> +#define LOONGSON_SPI_TIMI_REG 0x06
> +
> +/* Bits definition for Loongson SPI register */
> +#define LOONGSON_SPI_PARA_MEM_EN BIT(0)
> +#define LOONGSON_SPI_SPSR_SPIF BIT(7)
> +#define LOONGSON_SPI_SPSR_WCOL BIT(6)
> +#define LOONGSON_SPI_SPCR_SPE BIT(6)
> +
> +#define SPI_COMPLETION_TIMEOUT msecs_to_jiffies(2000)
> +
> +struct loongson_spi {
> + struct spi_master *master;
> + void __iomem *base;
> + int cs_active;
> + unsigned int hz;
> + unsigned char spcr;
> + unsigned char sper;
> + unsigned char spsr;
> + unsigned char para;
> + unsigned char sfcs;
> + unsigned char timi;
> + unsigned int mode;
> + u64 clk_rate;
> +};
> +
> +extern int loongson_spi_init_master(struct device *dev, struct resource *res);

No extern for the function declarations.

> +extern const struct dev_pm_ops loongson_spi_dev_pm_ops;

> +#endif /* __LINUX_SPI_LOONGSON_H */

--
With Best Regards,
Andy Shevchenko