Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

2020-12-09 Thread Mark Brown 
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(_spi->lock);
> param = ls7a_spi_read_reg(ls7a_spi, PARA);
> ls7a_spi_write_reg(ls7a_spi, PARA, param&~1);
> spin_unlock(_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.


signature.asc
Description: PGP signature

Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

2020-12-08 Thread zhangqing 

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(_spi->lock);
param = ls7a_spi_read_reg(ls7a_spi, PARA);
ls7a_spi_write_reg(ls7a_spi, PARA, param&~1);
spin_unlock(_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(_spi->lock);
ls7a_spi_write_reg(ls7a_spi, PARA, param);
spin_unlock(_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(>dev, sizeof(struct ls7a_spi));
+   if (!master)
+   return -ENOMEM;

Why not use devm_ here?


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

  error:
- spi_put_master(master);

+ master = devm_spi_alloc_master(>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

Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

2020-12-08 Thread kernel test robot 
Hi Qing,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on spi/for-next]
[also build test ERROR on robh/for-next linus/master v5.10-rc7 next-20201208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:
https://github.com/0day-ci/linux/commits/Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# 
https://github.com/0day-ci/linux/commit/6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review 
Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
git checkout 6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 

All error/warnings (new ones prefixed by >>):

   drivers/spi/spi-ls7a.c: In function 'ls7a_spi_write_read':
   drivers/spi/spi-ls7a.c:162:19: warning: variable 'ls7a_spi' set but not used 
[-Wunused-but-set-variable]
 162 |  struct ls7a_spi *ls7a_spi;
 |   ^~~~
   drivers/spi/spi-ls7a.c: At top level:
>> drivers/spi/spi-ls7a.c:320:1: warning: data definition has no type or 
>> storage class
 320 | module_pci_driver(ls7a_spi_pci_driver);
 | ^
>> drivers/spi/spi-ls7a.c:320:1: error: type defaults to 'int' in declaration 
>> of 'module_pci_driver' [-Werror=implicit-int]
>> drivers/spi/spi-ls7a.c:320:1: warning: parameter names (without types) in 
>> function declaration
   drivers/spi/spi-ls7a.c:313:26: warning: 'ls7a_spi_pci_driver' defined but 
not used [-Wunused-variable]
 313 | static struct pci_driver ls7a_spi_pci_driver = {
 |  ^~~
   cc1: some warnings being treated as errors

vim +320 drivers/spi/spi-ls7a.c

   319  
 > 320  module_pci_driver(ls7a_spi_pci_driver);
   321  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org


.config.gz
Description: application/gzip

Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

2020-12-08 Thread Mark Brown 
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?

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

> + 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().

> + 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(>dev, sizeof(struct ls7a_spi));
> + if (!master)
> + return -ENOMEM;

Why not use devm_ here?

> + 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.


signature.asc
Description: PGP signature

Re: [PATCH v2 1/4] spi: LS7A: Add Loongson LS7A SPI controller driver support

2020-12-08 Thread kernel test robot 
Hi Qing,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on spi/for-next]
[also build test WARNING on robh/for-next linus/master v5.10-rc7 next-20201207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:
https://github.com/0day-ci/linux/commits/Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# 
https://github.com/0day-ci/linux/commit/6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review 
Qing-Zhang/spi-LS7A-Add-Loongson-LS7A-SPI-controller-driver-support/20201208-154822
git checkout 6b2c3d42e1460dd3472a2c74d6a6c46d8693ce79
# save the attached .config to linux build tree
make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 

All warnings (new ones prefixed by >>):

   drivers/spi/spi-ls7a.c: In function 'ls7a_spi_write_read':
>> drivers/spi/spi-ls7a.c:162:19: warning: variable 'ls7a_spi' set but not used 
>> [-Wunused-but-set-variable]
 162 |  struct ls7a_spi *ls7a_spi;
 |   ^~~~

vim +/ls7a_spi +162 drivers/spi/spi-ls7a.c

   158  
   159  static unsigned int ls7a_spi_write_read(struct spi_device *spi,
   160  struct spi_transfer *xfer)
   161  {
 > 162  struct ls7a_spi *ls7a_spi;
   163  unsigned int count;
   164  const u8 *tx = xfer->tx_buf;
   165  u8 *rx = xfer->rx_buf;
   166  
   167  ls7a_spi = spi_master_get_devdata(spi->master);
   168  count = xfer->len;
   169  
   170  do {
   171  if (ls7a_spi_write_read_8bit(spi, , , count) < 0)
   172  goto out;
   173  count--;
   174  } while (count);
   175  
   176  out:
   177  return xfer->len - count;
   178  }
   179  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org


.config.gz
Description: application/gzip