linux-kernel - Re: [PATCH 1/3] spi: Loongson: Add Loongson 3A+7A SPI controller driver support

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d86df590-0091-ea6b-eeaf-a310d5ef6843@loongson.cn>
Date:   Tue, 24 Nov 2020 09:54:51 +0800
From:   zhangqing <zhangqing@...ngson.cn>
To:     Mark Brown <broonie@...nel.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        linux-spi@...r.kernel.org, Huacai Chen <chenhc@...ote.com>,
        Jiaxun Yang <jiaxun.yang@...goat.com>,
        devicetree@...r.kernel.org, linux-mips@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Philippe Mathieu-Daudé <f4bug@...at.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Krzysztof Kozlowski <krzk@...nel.org>
Subject: Re: [PATCH 1/3] spi: Loongson: Add Loongson 3A+7A SPI controller
 driver support

Hi Mark,

Thank you for your suggestion, I will do it and send the v2  in the future.

Thanks,
Qing


On 11/23/2020 09:10 PM, Mark Brown wrote:
> On Mon, Nov 23, 2020 at 05:19:06PM +0800, Qing Zhang wrote:
>
>> This module is integrated into the Loongson-3A SoC and the LS7A bridge chip.
> It looks like this needs quite a bit of update to fit into the SPI
> subsystem properly, fortunately most of that is cutting code out of the
> driver so you can use core features so it shouldn't be too bad.  There's
> also a bunch of pretty minor stylistic issues none of which look too
> difficult to address.
>
>> @@ -968,6 +968,12 @@ config SPI_AMD
>>   	help
>>   	  Enables SPI controller driver for AMD SoC.
>>   
>> +config SPI_LOONGSON
>> +        tristate "Loongson SPI Controller Support"
> Please keep Kconfig and Makefile sorted.
>
>> +        depends on CPU_LOONGSON32 || CPU_LOONGSON64
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Loongson3A+7A SPI driver
>> + *
> Please make the entire comment a C++ one so things look more
> intentional.
>
>> +#include <linux/pci.h>
>> +#include <linux/of.h>
>> +/*define spi register */
>> +#define	SPCR	0x00
> Missing blank line.
>
>> +#define	SPSR	0x01
>> +#define FIFO	0x02
> This indentation is unclear, also all these defines could use some
> namespacing to avoid collisions with anything that gets defined in a
> header.
>
>> +	hz  = t ? t->speed_hz : spi->max_speed_hz;
>> +
>> +	if (!hz)
>> +		hz = spi->max_speed_hz;
> Please write normal conditional statements to improve legibility, and
> note that the core will ensure that the transfer always has a speed set.
>
>> +	if ((hz && loongson_spi->hz != hz) || ((spi->mode ^ loongson_spi->mode) & (SPI_CPOL | SPI_CPHA))) {
> Please try to keep your lines less than 80 columns (there's not a hard
> limit here but it helps legibility).
>
>> +		bit = fls(div) - 1;
>> +		if ((1<<bit) == div)
> 1 << bit
>
>> +static int loongson_spi_setup(struct spi_device *spi)
>> +{
>> +	struct loongson_spi *loongson_spi;
>> +
>> +	loongson_spi = spi_master_get_devdata(spi->master);
>> +	if (spi->bits_per_word % 8)
>> +		return -EINVAL;
>> +
>> +	if (spi->chip_select >= spi->master->num_chipselect)
>> +		return -EINVAL;
>> +
>> +	loongson_spi_update_state(loongson_spi, spi, NULL);
>> +	set_cs(loongson_spi, spi, 1);
> The setup() operation shouldn't configure the physical controller state
> unless there are separate configuration registers per chip select -
> another device could be active when setup() is called.
>
>
>> +static int loongson_spi_write_read_8bit(struct spi_device *spi,
>> +		const u8 **tx_buf, u8 **rx_buf, unsigned int num)
>> +{
>> +	struct loongson_spi *loongson_spi;
>> +	loongson_spi = spi_master_get_devdata(spi->master);
>> +
>> +	if (tx_buf && *tx_buf) {
>> +		loongson_spi_write_reg(loongson_spi, FIFO, *((*tx_buf)++));
>> +		while ((loongson_spi_read_reg(loongson_spi, SPSR) & 0x1) == 1);
>> +        } else {
>> +		loongson_spi_write_reg(loongson_spi, FIFO, 0);
>> +		while ((loongson_spi_read_reg(loongson_spi, SPSR) & 0x1) == 1);
>> +        }
> The indentation is messed up here, looks like you have some kind of
> tab/space confusion.
>
>> +	count = xfer->len;
>> +
>> +	do {
>> +		if (loongson_spi_write_read_8bit(spi, &tx, &rx, count) < 0)
>> +			goto out;
> This is the only caller of _write_read_8bit(), may sa well inline it?
>
>> +static inline int set_cs(struct loongson_spi *loongson_spi, struct spi_device  *spi, int val)
> Why is this static inline?  This should be an operation provided to the
> SPI core.
>
>> +{
>> +	int cs = loongson_spi_read_reg(loongson_spi, SFCS) & ~(0x11 << spi->chip_select);
>> +
>> +        if (spi->mode  & SPI_CS_HIGH)
>> +		val = !val;
>> +	loongson_spi_write_reg(loongson_spi, SFCS,
>> +                        (val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) | cs);
> There's mult
>
>> +static void loongson_spi_work(struct work_struct *work)
>> +{
> Drivers shouldn't be open coding a message queue, implement
> transfer_one_message() and let the core handle it for you.
>
>> +static const struct of_device_id loongson_spi_id_table[] = {
>> +	{ .compatible = "loongson,loongson-spi", },
>> +	{ },
>> +};
> This is introducing a new DT binding, there should also be a new binding
> document added.
>
>> +static int loongson_spi_pci_register(struct pci_dev *pdev,
>> +		const struct pci_device_id *ent)
>> +{
>> +	int ret;
>> +	unsigned char v8;
> I would expect the PCI device to be a separate module with a dependency
> on PCI, I'm kind of surprised that this builds on !PCI systems but I
> guess it's possible there's stubs.