Re: [PATCH v2 4/5] spi: Add OF binding support for SPI busses

2008-07-11 Thread Grant Likely 
On Wed, Jul 02, 2008 at 11:02:23PM -0400, Jon Smirl wrote:
 On 7/2/08, Grant Likely [EMAIL PROTECTED] wrote:
  From: Grant Likely [EMAIL PROTECTED]
 
   This patch adds support for populating an SPI bus based on data in the
   OF device tree.  This is useful for powerpc platforms which use the
   device tree instead of discrete code for describing platform layout.
 
   Signed-off-by: Grant Likely [EMAIL PROTECTED]
   ---
   +   /* Select device driver */
   +   sprop = of_get_property(nc, linux,modalias, len);
   +   if (sprop  len  0)
   +   strncpy(spi-modalias, sprop, KOBJ_NAME_LEN);
   +   else
   +   strncpy(spi-modalias, spidev, KOBJ_NAME_LEN);
 
 You're missing a request_module(%s, info.type) to make sure the
 module is loaded.
 
 It might make sense to share code with of_find_i2c_driver() so we have
 a common way of guessing module names.

You're right.  I've refactored the i2c code to make it usable by SPI
also.  I'll post the new patch series this evening.

g.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

RE: [PATCH v2 4/5] spi: Add OF binding support for SPI busses

2008-07-03 Thread Chen Gong 
 

 -Original Message-
 From: [EMAIL PROTECTED] 
 [mailto:[EMAIL PROTECTED] 
 On Behalf Of Grant Likely
 Sent: 2008?7?3? 9:03
 To: linuxppc-dev@ozlabs.org; 
 [EMAIL PROTECTED]; [EMAIL PROTECTED]
 Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]
 Subject: [PATCH v2 4/5] spi: Add OF binding support for SPI busses
 
 From: Grant Likely [EMAIL PROTECTED]
 
 This patch adds support for populating an SPI bus based on data in the
 OF device tree.  This is useful for powerpc platforms which use the
 device tree instead of discrete code for describing platform layout.
 
 Signed-off-by: Grant Likely [EMAIL PROTECTED]
 ---
 
  drivers/of/Kconfig |6 +++
  drivers/of/Makefile|1 +
  drivers/of/of_spi.c|   88 
 
  include/linux/of_spi.h |   18 ++
  4 files changed, 113 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
 index 3a7a11a..edd6e92 100644
 --- a/drivers/of/Kconfig
 +++ b/drivers/of/Kconfig
 @@ -13,3 +13,9 @@ config OF_I2C
   depends on PPC_OF  I2C
   help
 OpenFirmware I2C accessors
 +
 +config OF_SPI
 + def_tristate SPI
 + depends on OF  PPC_OF  SPI
 + help
 +   OpenFirmware SPI accessors
 diff --git a/drivers/of/Makefile b/drivers/of/Makefile
 index 548772e..4c3c6f8 100644
 --- a/drivers/of/Makefile
 +++ b/drivers/of/Makefile
 @@ -2,3 +2,4 @@ obj-y = base.o
  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
  obj-$(CONFIG_OF_GPIO)   += gpio.o
  obj-$(CONFIG_OF_I2C) += of_i2c.o
 +obj-$(CONFIG_OF_SPI) += of_spi.o
 diff --git a/drivers/of/of_spi.c b/drivers/of/of_spi.c
 new file mode 100644
 index 000..ed0c807
 --- /dev/null
 +++ b/drivers/of/of_spi.c
 @@ -0,0 +1,88 @@
 +/*
 + * SPI OF support routines
 + * Copyright (C) 2008 Secret Lab Technologies Ltd.
 + *
 + * Support routines for deriving SPI device attachments from 
 the device
 + * tree.
 + */
 +
 +#include linux/of.h
 +#include linux/device.h
 +#include linux/spi/spi.h
 +#include linux/of_spi.h
 +
 +/**
 + * of_register_spi_devices - Register child devices onto the SPI bus
 + * @master:  Pointer to spi_master device
 + * @np:  parent node of SPI device nodes
 + *
 + * Registers an spi_device for each child node of 'np' which 
 has a 'reg'
 + * property.
 + */
 +void of_register_spi_devices(struct spi_master *master, 
 struct device_node *np)
 +{
 + struct spi_device *spi;
 + struct device_node *nc;
 + const u32 *prop;
 + const char *sprop;
 + int rc;
 + int len;
 +
 + for_each_child_of_node(np, nc) {
 + /* Alloc an spi_device */
 + spi = spi_alloc_device(master);
 + if (!spi) {
 + dev_err(master-dev, spi_device alloc 
 error for %s\n,
 + nc-full_name);
 + continue;
 + }
 +
 + /* Device address */
 + prop = of_get_property(nc, reg, len);
 + if (!prop || len  sizeof(*prop)) {
 + dev_err(master-dev, %s has no 'reg' 
 property\n,
 + nc-full_name);
 + continue;
 + }
 + spi-chip_select = *prop;
 +
 + /* Mode (clock phase/polarity/etc.) */
 + if (of_find_property(nc, spi,cpha, NULL))
 + spi-mode |= SPI_CPHA;
 + if (of_find_property(nc, spi,cpol, NULL))
 + spi-mode |= SPI_CPOL;

so becuase in function spi_alloc_deive, spi is allocated by kzalloc,
how about writing as follows:
/* Mode (clock phase/polarity/etc.) */
prop = of_get_property(nc, spi,cpha, NULL))
if (prop)
spi-mode |= *prop;
prop = of_get_property(nc, spi,cpol, NULL))
if (prop)
spi-mode |= *prop;

 +
 + /* Device speed */
 + prop = of_get_property(nc, max-speed, len);
 + if (!prop || len  sizeof(*prop)) {
 + dev_err(master-dev, %s has no 
 'max-speed' property\n,
 + nc-full_name);
 + continue;
 + }
 + spi-max_speed_hz = *prop;
 +
 + /* IRQ */
 + spi-irq = irq_of_parse_and_map(nc, 0);
 +
 + /* Select device driver */
 + sprop = of_get_property(nc, linux,modalias, len);
 + if (sprop  len  0)
 + strncpy(spi-modalias, sprop, KOBJ_NAME_LEN);
 + else
 + strncpy(spi-modalias, spidev, KOBJ_NAME_LEN);
 +
 how about writing as follows:

if (sprop  len  0)
strncpy(spi-modalias, sprop, KOBJ_NAME_LEN -
1);
else
strncpy(spi-modalias, spidev, KOBJ_NAME_LEN -
1);
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org

Re: [PATCH v2 4/5] spi: Add OF binding support for SPI busses

2008-07-03 Thread Grant Likely 
On Fri, Jul 04, 2008 at 11:54:57AM +0800, Chen Gong wrote:
 Grant Likely wrote:
  +   /* Mode (clock phase/polarity/etc.) */
  +   if (of_find_property(nc, spi,cpha, NULL))
  +   spi-mode |= SPI_CPHA;
  +   if (of_find_property(nc, spi,cpol, NULL))
  +   spi-mode |= SPI_CPOL;
 
 so becuase in function spi_alloc_deive, spi is allocated by kzalloc,
 how about writing as follows:
   /* Mode (clock phase/polarity/etc.) */
   prop = of_get_property(nc, spi,cpha, NULL))
 if (prop)
   spi-mode |= *prop;
   prop = of_get_property(nc, spi,cpol, NULL))
 if (prop)
   spi-mode |= *prop;

spi,cpha and spi,cpol are defined as empty properties.  The presence of
the property in the node means I need to set the flag in the spi_device.

  +   /* Select device driver */
  +   sprop = of_get_property(nc, linux,modalias, len);
  +   if (sprop  len  0)
  +   strncpy(spi-modalias, sprop, KOBJ_NAME_LEN);
  +   else
  +   strncpy(spi-modalias, spidev, KOBJ_NAME_LEN);
  +
  how about writing as follows:
 
   if (sprop  len  0)
   strncpy(spi-modalias, sprop, KOBJ_NAME_LEN -
 1);
   else
   strncpy(spi-modalias, spidev, KOBJ_NAME_LEN -
 1);

Actually, neither are very good.  What it should really be is:

if (sprop  len  0)
strlcpy(spi-modalias, sprop, sizeof (spi-modalias));
else
strlcpy(spi-modalias, spidev, sizeof (spi-modalias));

This ensures that the string is always null terminated and always the
right size.

... But the whole argument is a bit moot.  The fact that I even defined
a linux,modalias property is a great big hairy hack that I would
never want my mother to see.  Instead, I need a method to bind to an SPI
driver based on the compatible list.  Jon Smirl has done some work
in this regard for i2c, but I haven't looked at it very deeply, and so
do not at all understand it (yet).

Thanks for the comments.
g.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH v2 4/5] spi: Add OF binding support for SPI busses

2008-07-02 Thread Jon Smirl 
On 7/2/08, Grant Likely [EMAIL PROTECTED] wrote:
 From: Grant Likely [EMAIL PROTECTED]

  This patch adds support for populating an SPI bus based on data in the
  OF device tree.  This is useful for powerpc platforms which use the
  device tree instead of discrete code for describing platform layout.

  Signed-off-by: Grant Likely [EMAIL PROTECTED]
  ---

   drivers/of/Kconfig |6 +++
   drivers/of/Makefile|1 +
   drivers/of/of_spi.c|   88 
 
   include/linux/of_spi.h |   18 ++
   4 files changed, 113 insertions(+), 0 deletions(-)

  diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
  index 3a7a11a..edd6e92 100644
  --- a/drivers/of/Kconfig
  +++ b/drivers/of/Kconfig
  @@ -13,3 +13,9 @@ config OF_I2C
 depends on PPC_OF  I2C
 help
   OpenFirmware I2C accessors
  +
  +config OF_SPI
  +   def_tristate SPI
  +   depends on OF  PPC_OF  SPI
  +   help
  + OpenFirmware SPI accessors
  diff --git a/drivers/of/Makefile b/drivers/of/Makefile
  index 548772e..4c3c6f8 100644
  --- a/drivers/of/Makefile
  +++ b/drivers/of/Makefile
  @@ -2,3 +2,4 @@ obj-y = base.o
   obj-$(CONFIG_OF_DEVICE) += device.o platform.o
   obj-$(CONFIG_OF_GPIO)   += gpio.o
   obj-$(CONFIG_OF_I2C)   += of_i2c.o
  +obj-$(CONFIG_OF_SPI)   += of_spi.o
  diff --git a/drivers/of/of_spi.c b/drivers/of/of_spi.c
  new file mode 100644
  index 000..ed0c807
  --- /dev/null
  +++ b/drivers/of/of_spi.c
  @@ -0,0 +1,88 @@
  +/*
  + * SPI OF support routines
  + * Copyright (C) 2008 Secret Lab Technologies Ltd.
  + *
  + * Support routines for deriving SPI device attachments from the device
  + * tree.
  + */
  +
  +#include linux/of.h
  +#include linux/device.h
  +#include linux/spi/spi.h
  +#include linux/of_spi.h
  +
  +/**
  + * of_register_spi_devices - Register child devices onto the SPI bus
  + * @master:Pointer to spi_master device
  + * @np:parent node of SPI device nodes
  + *
  + * Registers an spi_device for each child node of 'np' which has a 'reg'
  + * property.
  + */
  +void of_register_spi_devices(struct spi_master *master, struct device_node 
 *np)
  +{
  +   struct spi_device *spi;
  +   struct device_node *nc;
  +   const u32 *prop;
  +   const char *sprop;
  +   int rc;
  +   int len;
  +
  +   for_each_child_of_node(np, nc) {
  +   /* Alloc an spi_device */
  +   spi = spi_alloc_device(master);
  +   if (!spi) {
  +   dev_err(master-dev, spi_device alloc error for 
 %s\n,
  +   nc-full_name);
  +   continue;
  +   }
  +
  +   /* Device address */
  +   prop = of_get_property(nc, reg, len);
  +   if (!prop || len  sizeof(*prop)) {
  +   dev_err(master-dev, %s has no 'reg' property\n,
  +   nc-full_name);
  +   continue;
  +   }
  +   spi-chip_select = *prop;
  +
  +   /* Mode (clock phase/polarity/etc.) */
  +   if (of_find_property(nc, spi,cpha, NULL))
  +   spi-mode |= SPI_CPHA;
  +   if (of_find_property(nc, spi,cpol, NULL))
  +   spi-mode |= SPI_CPOL;
  +
  +   /* Device speed */
  +   prop = of_get_property(nc, max-speed, len);
  +   if (!prop || len  sizeof(*prop)) {
  +   dev_err(master-dev, %s has no 'max-speed' 
 property\n,
  +   nc-full_name);
  +   continue;
  +   }
  +   spi-max_speed_hz = *prop;
  +
  +   /* IRQ */
  +   spi-irq = irq_of_parse_and_map(nc, 0);
  +
  +   /* Select device driver */
  +   sprop = of_get_property(nc, linux,modalias, len);
  +   if (sprop  len  0)
  +   strncpy(spi-modalias, sprop, KOBJ_NAME_LEN);
  +   else
  +   strncpy(spi-modalias, spidev, KOBJ_NAME_LEN);

You're missing a request_module(%s, info.type) to make sure the
module is loaded.

It might make sense to share code with of_find_i2c_driver() so we have
a common way of guessing module names.

  +
  +   /* Store a pointer to the node in the device structure */
  +   of_node_get(nc);
  +   spi-dev.archdata.of_node = nc;
  +
  +   /* Register the new device */
  +   rc = spi_add_device(spi);
  +   if (rc) {
  +   dev_err(master-dev, spi_device register error 
 %s\n,
  +   nc-full_name);
  +   spi_dev_put(spi);
  +   }
  +
  +   }
  +}
  +EXPORT_SYMBOL(of_register_spi_devices);
  diff --git a/include/linux/of_spi.h b/include/linux/of_spi.h
  new file mode 100644