Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver
 support
To:     Mark Brown <broonie@kernel.org>
References: <1607413467-17698-1-git-send-email-zhangqing@loongson.cn>
 <20201208135644.GC6686@sirena.org.uk>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        linux-spi@vger.kernel.org, Huacai Chen <chenhc@lemote.com>,
        Jiaxun Yang <jiaxun.yang@flygoat.com>,
        devicetree@vger.kernel.org, linux-mips@vger.kernel.org,
        linux-kernel@vger.kernel.org, gaojuxin@loongson.cn,
        yangtiezhu@loongson.cn
From:   zhangqing <zhangqing@loongson.cn>
Message-ID: <c916c525-7308-12a7-824b-7068fcead4cc@loongson.cn>
Date:   Wed, 9 Dec 2020 15:24:15 +0800
User-Agent: Mozilla/5.0 (X11; Linux mips64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <20201208135644.GC6686@sirena.org.uk>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Brown,

Thank you for your suggestions, these are achievable, I will send v3 in 
the soon.

Before sending v3, I would like to trouble you to see if this is 
correct. It has been tested locally.

On 12/08/2020 09:56 PM, Mark Brown wrote:
> On Tue, Dec 08, 2020 at 03:44:24PM +0800, Qing Zhang wrote:
>
>> v2:
>> - keep Kconfig and Makefile sorted
>> - make the entire comment a C++ one so things look more intentional
> You say this but...
>
>> +++ b/drivers/spi/spi-ls7a.c
>> @@ -0,0 +1,324 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Loongson LS7A SPI Controller driver
>> + *
>> + * Copyright (C) 2020 Loongson Technology Corporation Limited
>> + */
> ...this is still a mix of C and C++ comments?
       Replace all with //

>
>> +static int set_cs(struct ls7a_spi *ls7a_spi, struct spi_device  *spi, int val)
>> +{
>> +	int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << spi->chip_select);
>> +
>> +	if (spi->mode  & SPI_CS_HIGH)
>> +		val = !val;
>> +	ls7a_spi_write_reg(ls7a_spi, SFCS,
>> +		(val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) | cs);
>> +
>> +	return 0;
>> +}
> Why not just expose this to the core and let it handle things?
>
> Please also write normal conditional statements to improve legibility.
> There's quite a lot of coding style issues in this with things like
> missing spaces
     static void ls7a_spi_set_cs(struct spi_device *spi, bool enable)
{
         struct ls7a_spi *ls7a_spi;

         int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 << 
spi->chip_select));

         ls7a_spi = spi_master_get_devdata(spi->master);

         if (!!(spi->mode & SPI_CS_HIGH) == enable)
                 val = (0x11 << spi->chip_select) | cs;
         else
                 val = (0x1 << spi->chip_select) | cs;

         ls7a_spi_write_reg(ls7a_spi, SFCS, val);
}

      static int ls7a_spi_pci_probe---->

      +master->set_cs = ls7a_spi_set_cs;

>
>> +	if (t) {
>> +		hz = t->speed_hz;
>> +		if (!hz)
>> +			hz = spi->max_speed_hz;
>> +	} else
>> +		hz = spi->max_speed_hz;
> If one branch of the conditional has braces please use them on both to
> improve legibility.
>
>> +static int  ls7a_spi_transfer_one_message(struct spi_master *master,
>> +                                         struct spi_message *m)
> I don't understand why the driver is implementing transfer_one_message()
> - it looks like this is just open coding the standard loop that the
> framework provides and should just be using transfer_one().

static int  ls7a_spi_transfer_one(struct spi_master *master,
                       struct spi_device *spi,
                                   struct spi_transfer *t)
{
     struct ls7a_spi *ls7a_spi;
     int param, status;

     ls7a_spi = spi_master_get_devdata(master);

     spin_lock(&ls7a_spi->lock);
     param = ls7a_spi_read_reg(ls7a_spi, PARA);
     ls7a_spi_write_reg(ls7a_spi, PARA, param&~1);
     spin_unlock(&ls7a_spi->lock);

         status = ls7a_spi_do_transfer(ls7a_spi, spi, t);
         if(status < 0)
                 return status;

         if(t->len)
         r = ls7a_spi_write_read(spi, t);

         spin_lock(&ls7a_spi->lock);
     ls7a_spi_write_reg(ls7a_spi, PARA, param);
     spin_unlock(&ls7a_spi->lock);

     return status;
}

   static int ls7a_spi_pci_probe---->

  - master->transfer_one_message = ls7a_spi_transfer_one_message;
  +master->transfer_one = ls7a_spi_transfer_one;
>
>> +		r = ls7a_spi_write_read(spi, t);
>> +		if (r < 0) {
>> +			status = r;
>> +			goto error;
>> +			}
> The indentation here isn't following the kernel coding style.
>
>> +	master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));
>> +	if (!master)
>> +		return -ENOMEM;
> Why not use devm_ here?

- master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));

   error:
- spi_put_master(master);

+ master = devm_spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));

>
>> +	ret = devm_spi_register_master(dev, master);
>> +	if (ret)
>> +		goto err_free_master;
> The driver uses devm_spi_register_master() here but...
>
>> +static void ls7a_spi_pci_remove(struct pci_dev *pdev)
>> +{
>> +	struct spi_master *master = pci_get_drvdata(pdev);
>> +	struct ls7a_spi *spi;
>> +
>> +	spi = spi_master_get_devdata(master);
>> +	if (!spi)
>> +		return;
>> +
>> +	pci_release_regions(pdev);
> ...releases the PCI regions in the remove() function before the SPI
> controller is freed so the controller could still be active.

      static void ls7a_spi_pci_remove(struct pci_dev *pdev)
{
         struct spi_master *master = pci_get_drvdata(pdev);

      + spi_unregister_master(master);
         pci_release_regions(pdev);
}

Thanks,

-Qing