lkml.org 
[lkml]   [2023]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v11 2/2] spi: loongson: add bus driver for the loongson spi controller
From
Date


在 2023/5/23 下午8:54, andy.shevchenko@gmail.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 */
>

\
 
 \ /
  Last update: 2023-05-24 09:53    [W:0.152 / U:0.568 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site