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.
Attachment:
signature.asc
Description: PGP signature
