| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 9 Jun 2023 15:10:05 +0800
From: zhuyinbo <zhuyinbo@...ngson.cn>
To: Andy Shevchenko <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 v12 2/2] spi: loongson: add bus driver for the loongson
spi controller
在 2023/6/8 下午6:15, Andy Shevchenko 写道:
> On Thu, Jun 8, 2023 at 10:28 AM Yinbo Zhu <zhuyinbo@...ngson.cn> wrote:
>>
>> This bus driver supports the Loongson SPI hardware controller in the
>> Loongson platforms and supports to use DTS and PCI framework to
>
> the use
okay, I got it.
>
>> register SPI device resources.
>
> Thank you for an update. I have a few nit-picks below, but in general
> this version is good (esp. if you address them)
> Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
>
You're welcome, I will add the reviewed-by in v13.
...
>> +static void loongson_spi_set_cs(struct spi_device *spi, bool val)
>> +{
>> + int cs;
>> + struct loongson_spi *loongson_spi = spi_controller_get_devdata(spi->controller);
>> +
>> + cs = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SFCS_REG)
>> + & ~(0x11 << spi_get_chipselect(spi, 0));
>> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SFCS_REG,
>> + (val ? (0x11 << spi_get_chipselect(spi, 0)) :
>> + (0x1 << spi_get_chipselect(spi, 0))) | cs);
>
> Can be done as
>
> static void loongson_spi_set_cs(struct spi_device *spi, bool en)
>
> unsigned char mask = (BIT(4) | BIT(0)) << spi_get_chipselect(spi, 0);
> unsigned char val = en ? mask : (BIT(0) << spi_get_chipselect(spi, 0));
>
> cs = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SFCS_REG) & ~mask;
> loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SFCS_REG, val | cs);
okay, I will do it.
>
> (Renamed variables to be consistent with the other uses in the driver below)
sorry, I don't got it. Are you referring to the following changes ?
loongson_spi_set_cs(spi, 1) => loongson_spi_set_cs(spi, true)
>
>> +}
>> +
>> +static void loongson_spi_set_clk(struct loongson_spi *loongson_spi, unsigned int hz)
>> +{
>> + unsigned char val;
>> + unsigned int div, div_tmp;
>> + static const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
>> +
>> + 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);
>
> val &= GENMASK(1, 0);
This seems to be "val &= ~GENMASK(1, 0);"
>
>> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, (val & ~3) |
>> + loongson_spi->spcr);
>
> loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, val
> | loongson_spi->spcr);
okay, I got it.
>
>> + val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPER_REG);
>
> val &= GENMASK(1, 0);
This seems to be "val &= ~GENMASK(1, 0);"
>
>> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPER_REG, (val & ~3) |
>> + loongson_spi->sper);
>
> loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPER_REG, val
> | loongson_spi->sper);
>
okay, I got it.
>> + loongson_spi->hz = hz;
>> +}
>> +
>> +static void loongson_spi_set_mode(struct loongson_spi *loongson_spi,
>> + struct spi_device *spi)
>> +{
>> + unsigned char val;
>> +
>> + val = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPCR_REG);
>> + val &= ~(LOONGSON_SPI_SPCR_CPOL | LOONGSON_SPI_SPCR_CPHA);
>> + if (spi->mode & SPI_CPOL)
>> + val |= LOONGSON_SPI_SPCR_CPOL;
>> + if (spi->mode & SPI_CPHA)
>> + val |= LOONGSON_SPI_SPCR_CPHA;
>> +
>> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_SPCR_REG, val);
>> + loongson_spi->mode |= spi->mode;
>> +}
>> +
>> +static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
>> + struct spi_device *spi, struct spi_transfer *t)
>> +{
>> + if (t && loongson_spi->hz != t->speed_hz)
>> + loongson_spi_set_clk(loongson_spi, t->speed_hz);
>> +
>> + if ((spi->mode ^ loongson_spi->mode) & SPI_MODE_X_MASK)
>> + loongson_spi_set_mode(loongson_spi, spi);
>> +
>> + return 0;
>> +}
>> +
>> +static int loongson_spi_setup(struct spi_device *spi)
>> +{
>> + struct loongson_spi *loongson_spi;
>> +
>> + loongson_spi = spi_controller_get_devdata(spi->controller);
>> + if (spi->bits_per_word % 8)
>> + return -EINVAL;
>> +
>> + if (spi_get_chipselect(spi, 0) >= spi->controller->num_chipselect)
>> + return -EINVAL;
>> +
>> + loongson_spi->hz = 0;
>> + loongson_spi_set_cs(spi, 1);
>> +
>> + return 0;
>> +}
>> +
>> +static int loongson_spi_write_read_8bit(struct spi_device *spi, const u8 **tx_buf,
>> + u8 **rx_buf, unsigned int num)
>> +{
>> + int ret;
>> + struct loongson_spi *loongson_spi = spi_controller_get_devdata(spi->controller);
>> +
>> + if (tx_buf && *tx_buf)
>> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, *((*tx_buf)++));
>> + else
>> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, 0);
>
> + Blank line
okay, I will add the blank line.
>
>> + ret = readb_poll_timeout(loongson_spi->base + LOONGSON_SPI_SPSR_REG, loongson_spi->spsr,
>> + (loongson_spi->spsr & 0x1) != LOONGSON_SPI_SPSR_RFEMPTY, 1, MSEC_PER_SEC);
>
> (loongson_spi->spsr &
> LOONGSON_SPI_SPSR_RFEMPTY) != LOONGSON_SPI_SPSR_RFEMPTY,
> 1, MSEC_PER_SEC);
okay, I got it.
>
>> + if (rx_buf && *rx_buf)
>> + *(*rx_buf)++ = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_FIFO_REG);
>> + else
>> + loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_FIFO_REG);
>> +
>> + return ret;
>> +}
>> +
>> +static int loongson_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
>> +{
>> + int ret;
>> + unsigned int count;
>> + const u8 *tx = xfer->tx_buf;
>> + u8 *rx = xfer->rx_buf;
>> +
>> + count = xfer->len;
>
>> +
>
> Unneeded blank line.
okay, I will remove the blank line.
>
>> + do {
>> + ret = loongson_spi_write_read_8bit(spi, &tx, &rx, count);
>> + if (ret)
>> + break;
>> + } while (--count);
>> +
>> + return ret;
>> +}
>> +
>> +static int loongson_spi_prepare_message(struct spi_controller *ctlr, struct spi_message *m)
>> +{
>> + struct loongson_spi *loongson_spi = spi_controller_get_devdata(ctlr);
>
>> + loongson_spi->para = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_PARA_REG);
>
>> + loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, loongson_spi->para & ~1);
>
> BIT(0) ?
> LOONGSON_SPI_PARA_MEM_EN ?
I will use LOONGSON_SPI_PARA_MEM_EN.
>
>> + return 0;
>> +}
>> +
...
>> +int loongson_spi_init_controller(struct device *dev, void __iomem *regs)
>> +{
>> + struct spi_controller *controller;
>> + struct loongson_spi *spi;
>> + struct clk *clk;
>> +
>> + controller = devm_spi_alloc_host(dev, sizeof(struct loongson_spi));
>> + if (controller == NULL)
>> + return -ENOMEM;
>> +
>> + controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
>
> ... = SPI_MODE_X_MASK | SPI_CS_HIGH;
okay, I got it.
Thanks,
Yinbo
Powered by blists - more mailing lists