linux-kernel - Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A 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] [day] [month] [year] [list]

Date:   Wed, 9 Dec 2020 11:59:47 +0000
From:   Mark Brown <broonie@...nel.org>
To:     zhangqing <zhangqing@...ngson.cn>
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, gaojuxin@...ngson.cn,
        yangtiezhu@...ngson.cn
Subject: Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller
 driver support

On Wed, Dec 09, 2020 at 03:24:15PM +0800, zhangqing wrote:

> > > +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);

I don't know what this does but is it better split out into a
prepare_message()?  It was only done once per message in your previous
implementation.  Or possibly runtime PM would be even better if that's
what it's doing.

> > ...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);
> }

You also need to change to using plain spi_register_master() but yes.
Otherwise everything looked good.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)