Date:   Wed, 24 May 2023 15:52:18 +0800
From:   zhuyinbo <zhuyinbo@...ngson.cn>
To:     andy.shevchenko@...il.com
Cc:     Mark Brown <broonie@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-spi@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jianmin Lv <lvjianmin@...ngson.cn>,
        wanghongliang@...ngson.cn, Liu Peibao <liupeibao@...ngson.cn>,
        loongson-kernel@...ts.loongnix.cn, zhuyinbo@...ngson.cn
Subject: Re: [PATCH v11 2/2] spi: loongson: add bus driver for the loongson
 spi controller



在 2023/5/23 下午8:54, andy.shevchenko@...il.com 写道:
> 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.

okay, I got it.
> 
> ...
> 
>> +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?

okay, I will define "static const char rdiv".

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

I alwasy use `make W=1` and I don't find any warnig, but that what you
said was right and I will initial hz.

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

okay, I got it.
> 
> 
> 	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);
> 
> ?

okay, I got it.

> 
>> +out:
>> +	return xfer->len - count;
> 
> Shouldn't you return an error code if the write failed?

okay, I got it. I will add a error code for return when 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?


It seems was a personal code style issue and I don't find the
differences between spi_controller and spi_master, Using spi_controller*
is also acceptable to me and I will 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.

Like below ?

         clk = devm_clk_get_optional(dev, NULL);
-       if (!IS_ERR(clk))
-               spi->clk_rate = clk_get_rate(clk);
-       else
+       if (IS_ERR(clk))
                 return dev_err_probe(dev, PTR_ERR(clk), "unable to get 
clock\n");

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


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

okay,  I will 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.

okay, I got it.
> 
>> +};
> 
> ...
> 
>> +#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).
> 

okay, I got it.

>> +#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;

It seems was more appropriate that initialize ret then return ret.
Do you think so ?

> 
>> +}
> 
> ...
> 
>> +#ifndef __LINUX_SPI_LOONGSON_H
>> +#define __LINUX_SPI_LOONGSON_H
>> +
>> +#include <linux/bits.h>
> 
>> +#include <linux/device.h>
> 
> This header is not used.

okay, I got it.
> 
>> +#include <linux/pm.h>
> 
>> +#include <linux/spi/spi.h>
> 
> This neither.

That other .c file seems to need it and I will move it to other .c
code file.
> 
>> +#include <linux/types.h>
> 
> 
> For them use forward declarations
> 
> struct device;
> struct spi_controller;
> 
> The rest of the inclusions is correct.

okay, I got it.
> 
> ...
> 
>> +#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?

These definitions are only used in core.c

> 
>> +/* 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 */
> 

Powered by blists - more mailing lists