linux-kernel - Re: [PATCH v1 2/2] spi: loongson: add bus driver for the loongson spi controller

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]

Date:   Fri, 10 Mar 2023 18:01:23 +0800
From:   zhuyinbo <zhuyinbo@...ngson.cn>
To:     Mark Brown <broonie@...nel.org>
Cc:     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 v1 2/2] spi: loongson: add bus driver for the loongson spi
 controller


在 2023/3/8 下午11:03, Mark Brown 写道:
> On Wed, Mar 08, 2023 at 10:59:08AM +0800, Yinbo Zhu wrote:
>
>> +config SPI_LOONGSON
>> +	tristate "Loongson SPI Controller Support"
>> +	depends on LOONGARCH && OF && PCI
> I'm not seeing any build time dependencies here (possibly PCI?) so
> please add an || COMPILE_TEST to improve build coverage.  It'd be better
> to have separate modules for the platform and PCI functionality, that
> way someone who has a system without PCI can still use the driver even
> with PCI support disabled.

I will add an || COMPILE_TEST and drop && PCI then add some CONFIG_PCI 
macro

to limit some pci code.

>
>> +++ b/drivers/spi/spi-loongson.c
>> @@ -0,0 +1,502 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Loongson SPI Support
>> + *
>> + * Copyright (C) 2023 Loongson Technology Corporation Limited
> Please make the entire comment block a C++ one so things look more
> intentional.
okay, I got it.
>
>> +static int loongson_spi_update_state(struct loongson_spi *loongson_spi,
>> +				     struct spi_device *spi, struct spi_transfer *t)
>> +{
>> +	unsigned int hz;
>> +	unsigned int div, div_tmp;
>> +	unsigned int bit;
>> +	unsigned char val;
>> +	const char rdiv[12] = {0, 1, 4, 2, 3, 5, 6, 7, 8, 9, 10, 11};
>> +
>> +	hz  = t ? t->speed_hz : spi->max_speed_hz;
> Please write normal conditional statements so that things are legible,
> though in this case the core will ensure that there's a speed_hz in
> every transfer so there's no need for any of the logic around ensuring
> it's set.
Do you mean to achieve the following ?  and drop spi->max_speed_hz.

if (t)

       hz = t->speed_hz;


>
>> +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);
>> +	loongson_spi_set_cs(loongson_spi, spi, 1);
> Note that setup() needs to be able to run for one device while there are
> transfers for other devices on the same controller active.
okay, I will add a spin_lock for it.
>
>> +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 = spi_master_get_devdata(spi->master);
>> +
>> +	if (tx_buf && *tx_buf) {
>> +		loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_FIFO_REG, *((*tx_buf)++));
>> +		while ((loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_SPSR_REG) & 0x1) == 1)
>> +			;
> A timeout would be good on these spins in case the controller gets
> stuck.  It'd also be polite to have a cpu_relax() somewhere either here
> or in the caller given that it's busy waiting.
okay, I got it.
>
>> +static void loongson_spi_work(struct work_struct *work)
>> +{
>> +	int param;
>> +	struct spi_message *m;
>> +	struct spi_device  *spi;
>> +	struct spi_transfer *t = NULL;
>> +	struct loongson_spi *loongson_spi = container_of(work, struct loongson_spi, work);
>> +
>> +	spin_lock(&loongson_spi->lock);
>> +	param = loongson_spi_read_reg(loongson_spi, LOONGSON_SPI_PARA_REG);
>> +	loongson_spi_write_reg(loongson_spi, LOONGSON_SPI_PARA_REG, param&~1);
>> +	while (!list_empty(&loongson_spi->msg_queue)) {
>> +		m = container_of(loongson_spi->msg_queue.next, struct spi_message, queue);
>> +
> This all looks like it's open coding the core's message pump, only
> without the heavy optimisation work that the core has and missing some
> handling of cs_change and delays.  You should implement
> spi_transfer_one() instead, this will save a lot of code and should be
> more performant.
okay, I will try to add a spi_transfer_one for this.
>
>> +static int loongson_spi_transfer(struct spi_device *spi, struct spi_message *m)
>> +{
> In general you'd need an extremely strong reason to implement transfer()
> in a new driver.
okay, I got it.
>
>> +static int __maybe_unused loongson_spi_resume(struct device *dev)
>> +{
>> +static const struct dev_pm_ops loongson_spi_dev_pm_ops = {
>> +	.suspend = loongson_spi_suspend,
>> +	.resume = loongson_spi_resume,
>> +};
> The suspend/resume ops are assigned unconditionally.
sorry, I don't got it,  you mean was to  add a CONFIG_PM to limit code ?
>
>> +subsys_initcall(loongson_spi_init);
>> +module_exit(loongson_spi_exit);
> Why not just a regular module initcall like most SPI drivers?
okay, I will use module_init for register spi drivers.