Messages in this thread | | | From | andy.shevchenko@gmail ... | Date | Tue, 23 May 2023 15:54:07 +0300 | Subject | Re: [PATCH v11 2/2] spi: loongson: add bus driver for the loongson spi controller |
| |
Mon, May 22, 2023 at 03:10:30PM +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.
It's polite to add reviewers of the previous versions to the Cc list.
...
> +static void loongson_spi_set_clk(struct loongson_spi *loongson_spi, unsigned int hz) > +{ > + unsigned char val; > + unsigned int div, div_tmp;
> + const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
static?
> + > + div = clamp_val(DIV_ROUND_UP_ULL(loongson_spi->clk_rate, hz), 2, 4096); > + div_tmp = rdiv[fls(div - 1)]; > + loongson_spi->spcr = (div_tmp & GENMASK(1, 0)) >> 0; > + loongson_spi->sper = (div_tmp & GENMASK(3, 2)) >> 2; > + val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG); > + 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->hz = hz; > +}
...
> +static int loongson_spi_update_state(struct loongson_spi *loongson_spi, > + struct spi_device *spi, struct spi_transfer *t) > +{ > + unsigned int hz; > + > + if (t) > + hz = t->speed_hz;
And if t is NULL? hz will be uninitialized. Don't you get a compiler warning? (Always test your code with `make W=1 ...`)
> + if (hz && loongson_spi->hz != hz) > + loongson_spi_set_clk(loongson_spi, hz); > + > + if ((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK) > + loongson_spi_set_mode(loongson_spi, spi); > + > + return 0; > +}
...
> + readb_poll_timeout(loongson_spi->base + LOONGSON_SPI_SPSR_REG, loongson_spi->spsr, > + (loongson_spi->spsr & 0x1) != 1, 1, MSEC_PER_SEC);
Wouldn't be better to use ' == 0' in the conditional? Or if you think your approach is better (to show the exact expectation) the definition of the bit 0 might help
#define LOONGSON_... BIT(0)
readb_poll_timeout(loongson_spi->base + LOONGSON_SPI_SPSR_REG, loongson_spi->spsr, (loongson_spi->spsr & LOONGSON_...) != LOONGSON_..., 1, MSEC_PER_SEC);
...
> + do { > + if (loongson_spi_write_read_8bit(spi, &tx, &rx, count) < 0)
> + goto out;
break;
> + count--; > + } while (count);
} while (--count);
?
> +out: > + return xfer->len - count;
Shouldn't you return an error code if the write failed?
...
> + master = devm_spi_alloc_master(dev, sizeof(struct loongson_spi));
> + if (master == NULL)
if (!master)
> + return -ENOMEM;
Why do you use deprecated naming? Can you use spi_controller* instead of spi_master* in all cases?
...
> + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
= SPI_MODE_X_MASK | SPI_CS_HIGH;
...
> + clk = devm_clk_get_optional(dev, NULL); > + if (!IS_ERR(clk)) > + spi->clk_rate = clk_get_rate(clk);
> + else
Redundant. Just check for the error first as it's very usual pattern in the Linux kernel.
> + return dev_err_probe(dev, PTR_ERR(clk), "unable to get clock\n");
...
> +static void loongson_spi_pci_unregister(struct pci_dev *pdev) > +{
> + pcim_iounmap_regions(pdev, BIT(0));
Not needed due to 'm' in the API name, which means "managed".
> + pci_disable_device(pdev);
This is simply wrong. We don't do explicit clean up for managed resources.
> +}
That said, drop the ->remove() completely.
...
> +static struct pci_device_id loongson_spi_devices[] = { > + { PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a0b) }, > + { PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, 0x7a1b) }, > + { },
No comma for the terminator entry. It's by definition "terminating" something.
> +};
...
> +#include <linux/of.h>
There is no user of this header. Please, replace with what actually is being used (presumably mod_devicetable.h and maybe others).
> +#include <linux/platform_device.h> > + > +#include "spi-loongson.h" > + > +static int loongson_spi_platform_probe(struct platform_device *pdev) > +{ > + int ret; > + void __iomem *reg_base; > + struct device *dev = &pdev->dev; > + > + reg_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(reg_base)) > + return PTR_ERR(reg_base); > + > + ret = loongson_spi_init_master(dev, reg_base); > + if (ret) > + return dev_err_probe(dev, ret, "failed to initialize master\n"); > + > + return ret;
return 0;
> +}
...
> +#ifndef __LINUX_SPI_LOONGSON_H > +#define __LINUX_SPI_LOONGSON_H > + > +#include <linux/bits.h>
> +#include <linux/device.h>
This header is not used.
> +#include <linux/pm.h>
> +#include <linux/spi/spi.h>
This neither.
> +#include <linux/types.h>
For them use forward declarations
struct device; struct spi_controller;
The rest of the inclusions is correct.
...
> +#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
Where is this used outside of the main driver?
> +/* Bits definition for Loongson SPI register */ > +#define LOONGSON_SPI_PARA_MEM_EN BIT(0) > +#define LOONGSON_SPI_SPCR_CPHA BIT(2) > +#define LOONGSON_SPI_SPCR_CPOL BIT(3) > +#define LOONGSON_SPI_SPCR_SPE BIT(6) > +#define LOONGSON_SPI_SPSR_WCOL BIT(6) > +#define LOONGSON_SPI_SPSR_SPIF BIT(7) > + > +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; > +}; > + > +int loongson_spi_init_master(struct device *dev, void __iomem *reg); > +extern const struct dev_pm_ops loongson_spi_dev_pm_ops; > +#endif /* __LINUX_SPI_LOONGSON_H */
-- With Best Regards, Andy Shevchenko
| |