Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.

Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Mark Brown]

On Tue, Dec 18, 2012 at 11:29:34AM -0500, Jun Chen wrote:

>   * @master: Controller to which device is connected
> + * device_was_children_of_master is flag which the device is registed
> + * as the children of the bus

This isn't a kerneldoc style comment (it needs the @XXX: format).  The
name is also extremely long, can't we think of something more concise?

> -	spi->dev.parent = &master->dev;
> +	if (device_was_children_of_master == true)
> +		spi->dev.parent = &master->dev;
> +	else
> +		spi->dev.parent = dev;

Can you provide an example of where this is useful?  Your changelog
didn't make it clear and the code doesn't make it obvious either.

[PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Jun Chen]

Because there are two aim when allocating the new device, one is for children of master,
other is for master. So this patch add one flag to indicate different purpose.

Signed-off-by: Bi Chao <chao.bi@intel.com>
Signed-off-by: Chen Jun <jun.d.chen@intel.com>
---
 drivers/spi/spi.c       |   16 +++++++++++-----
 include/linux/spi/spi.h |    3 ++-
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 718cc1f..06f69ce 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -300,6 +300,8 @@ static DEFINE_MUTEX(board_lock);
 /**
  * spi_alloc_device - Allocate a new SPI device
  * @master: Controller to which device is connected
+ * device_was_children_of_master is flag which the device is registed
+ * as the children of the bus
  * Context: can sleep
  *
  * Allows a driver to allocate and initialize a spi_device without
@@ -314,7 +316,8 @@ static DEFINE_MUTEX(board_lock);
  *
  * Returns a pointer to the new device, or NULL.
  */
-struct spi_device *spi_alloc_device(struct spi_master *master)
+struct spi_device *spi_alloc_device(struct spi_master *master,
+				bool device_was_children_of_master)
 {
 	struct spi_device	*spi;
 	struct device		*dev = master->dev.parent;
@@ -330,7 +333,10 @@ struct spi_device *spi_alloc_device(struct spi_master *master)
 	}
 
 	spi->master = master;
-	spi->dev.parent = &master->dev;
+	if (device_was_children_of_master == true)
+		spi->dev.parent = &master->dev;
+	else
+		spi->dev.parent = dev;
 	spi->dev.bus = &spi_bus_type;
 	spi->dev.release = spidev_release;
 	device_initialize(&spi->dev);
@@ -434,7 +440,7 @@ struct spi_device *spi_new_device(struct spi_master *master,
 	 * suggests syslogged diagnostics are best here (ugh).
 	 */
 
-	proxy = spi_alloc_device(master);
+	proxy = spi_alloc_device(master, false);
 	if (!proxy)
 		return NULL;
 
@@ -827,7 +833,7 @@ static void of_register_spi_devices(struct spi_master *master)
 
 	for_each_available_child_of_node(master->dev.of_node, nc) {
 		/* Alloc an spi_device */
-		spi = spi_alloc_device(master);
+		spi = spi_alloc_device(master, true);
 		if (!spi) {
 			dev_err(&master->dev, "spi_device alloc error for %s\n",
 				nc->full_name);
@@ -939,7 +945,7 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
 	if (acpi_bus_get_status(adev) || !adev->status.present)
 		return AE_OK;
 
-	spi = spi_alloc_device(master);
+	spi = spi_alloc_device(master, false);
 	if (!spi) {
 		dev_err(&master->dev, "failed to allocate SPI device for %s\n",
 			dev_name(&adev->dev));
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index fa702ae..43d2f8e 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -838,7 +838,8 @@ spi_register_board_info(struct spi_board_info const *info, unsigned n)
  * be defined using the board info.
  */
 extern struct spi_device *
-spi_alloc_device(struct spi_master *master);
+spi_alloc_device(struct spi_master *master,
+				bool device_was_children_of_master);
 
 extern int
 spi_add_device(struct spi_device *spi);
-- 
1.7.4.1

[PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Jun Chen]

Because there are two aim when allocating the new device, one is for children of master,
other is for master. So this patch add one flag to indicate different purpose.

Signed-off-by: Bi Chao <chao.bi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Chen Jun <jun.d.chen-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi.c       |   16 +++++++++++-----
 include/linux/spi/spi.h |    3 ++-
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 718cc1f..06f69ce 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -300,6 +300,8 @@ static DEFINE_MUTEX(board_lock);
 /**
  * spi_alloc_device - Allocate a new SPI device
  * @master: Controller to which device is connected
+ * device_was_children_of_master is flag which the device is registed
+ * as the children of the bus
  * Context: can sleep
  *
  * Allows a driver to allocate and initialize a spi_device without
@@ -314,7 +316,8 @@ static DEFINE_MUTEX(board_lock);
  *
  * Returns a pointer to the new device, or NULL.
  */
-struct spi_device *spi_alloc_device(struct spi_master *master)
+struct spi_device *spi_alloc_device(struct spi_master *master,
+				bool device_was_children_of_master)
 {
 	struct spi_device	*spi;
 	struct device		*dev = master->dev.parent;
@@ -330,7 +333,10 @@ struct spi_device *spi_alloc_device(struct spi_master *master)
 	}
 
 	spi->master = master;
-	spi->dev.parent = &master->dev;
+	if (device_was_children_of_master == true)
+		spi->dev.parent = &master->dev;
+	else
+		spi->dev.parent = dev;
 	spi->dev.bus = &spi_bus_type;
 	spi->dev.release = spidev_release;
 	device_initialize(&spi->dev);
@@ -434,7 +440,7 @@ struct spi_device *spi_new_device(struct spi_master *master,
 	 * suggests syslogged diagnostics are best here (ugh).
 	 */
 
-	proxy = spi_alloc_device(master);
+	proxy = spi_alloc_device(master, false);
 	if (!proxy)
 		return NULL;
 
@@ -827,7 +833,7 @@ static void of_register_spi_devices(struct spi_master *master)
 
 	for_each_available_child_of_node(master->dev.of_node, nc) {
 		/* Alloc an spi_device */
-		spi = spi_alloc_device(master);
+		spi = spi_alloc_device(master, true);
 		if (!spi) {
 			dev_err(&master->dev, "spi_device alloc error for %s\n",
 				nc->full_name);
@@ -939,7 +945,7 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
 	if (acpi_bus_get_status(adev) || !adev->status.present)
 		return AE_OK;
 
-	spi = spi_alloc_device(master);
+	spi = spi_alloc_device(master, false);
 	if (!spi) {
 		dev_err(&master->dev, "failed to allocate SPI device for %s\n",
 			dev_name(&adev->dev));
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index fa702ae..43d2f8e 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -838,7 +838,8 @@ spi_register_board_info(struct spi_board_info const *info, unsigned n)
  * be defined using the board info.
  */
 extern struct spi_device *
-spi_alloc_device(struct spi_master *master);
+spi_alloc_device(struct spi_master *master,
+				bool device_was_children_of_master);
 
 extern int
 spi_add_device(struct spi_device *spi);
-- 
1.7.4.1




------------------------------------------------------------------------------
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d

Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]

On Wed, Dec 19, 2012 at 04:44:03AM -0500, Jun Chen wrote:

> This spi_alloc_device function will be called in the spi_new_device
> function to alloc new device as the master. But other way, it is called
> by the of_register_spi_devices function to register new device as the
> children of the master. I will update changlog to add it. 

But why is this a bad thing?  You've said what's happening but not why
it's a problem.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Jun Chen]

On Tue, 2012-12-18 at 15:26 +0000, Mark Brown wrote:
> On Tue, Dec 18, 2012 at 11:29:34AM -0500, Jun Chen wrote:
> 
> >   * @master: Controller to which device is connected
> > + * device_was_children_of_master is flag which the device is registed
> > + * as the children of the bus
> 
> This isn't a kerneldoc style comment (it needs the @XXX: format).  The
> name is also extremely long, can't we think of something more concise?
> 
Thank for your suggestion, I will correct this comment and use concise
flag.

> > -	spi->dev.parent = &master->dev;
> > +	if (device_was_children_of_master == true)
> > +		spi->dev.parent = &master->dev;
> > +	else
> > +		spi->dev.parent = dev;
> 
> Can you provide an example of where this is useful?  Your changelog
> didn't make it clear and the code doesn't make it obvious either.

This spi_alloc_device function will be called in the spi_new_device
function to alloc new device as the master. But other way, it is called
by the of_register_spi_devices function to register new device as the
children of the master. I will update changlog to add it. 

@@ -434,7 +440,7 @@ struct spi_device *spi_new_device(struct spi_master
*master,
         * suggests syslogged diagnostics are best here (ugh).
         */
 
-       proxy = spi_alloc_device(master);
+       proxy = spi_alloc_device(master, false);
        if (!proxy)
                return NULL;
 
@@ -827,7 +833,7 @@ static void of_register_spi_devices(struct
spi_master *master)
 
        for_each_available_child_of_node(master->dev.of_node, nc) {
                /* Alloc an spi_device */
-               spi = spi_alloc_device(master);
+               spi = spi_alloc_device(master, true);


If I have mistake, pls correct me, thanks!

Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Jun Chen]

On Tue, 2012-12-18 at 15:26 +0000, Mark Brown wrote:
> On Tue, Dec 18, 2012 at 11:29:34AM -0500, Jun Chen wrote:
> 
> >   * @master: Controller to which device is connected
> > + * device_was_children_of_master is flag which the device is registed
> > + * as the children of the bus
> 
> This isn't a kerneldoc style comment (it needs the @XXX: format).  The
> name is also extremely long, can't we think of something more concise?
> 
Thank for your suggestion, I will correct this comment and use concise
flag.

> > -	spi->dev.parent = &master->dev;
> > +	if (device_was_children_of_master == true)
> > +		spi->dev.parent = &master->dev;
> > +	else
> > +		spi->dev.parent = dev;
> 
> Can you provide an example of where this is useful?  Your changelog
> didn't make it clear and the code doesn't make it obvious either.

This spi_alloc_device function will be called in the spi_new_device
function to alloc new device as the master. But other way, it is called
by the of_register_spi_devices function to register new device as the
children of the master. I will update changlog to add it. 

@@ -434,7 +440,7 @@ struct spi_device *spi_new_device(struct spi_master
*master,
         * suggests syslogged diagnostics are best here (ugh).
         */
 
-       proxy = spi_alloc_device(master);
+       proxy = spi_alloc_device(master, false);
        if (!proxy)
                return NULL;
 
@@ -827,7 +833,7 @@ static void of_register_spi_devices(struct
spi_master *master)
 
        for_each_available_child_of_node(master->dev.of_node, nc) {
                /* Alloc an spi_device */
-               spi = spi_alloc_device(master);
+               spi = spi_alloc_device(master, true);


If I have mistake, pls correct me, thanks!




------------------------------------------------------------------------------
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d

Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Grant Likely]

On Wed, 19 Dec 2012 09:04:16 +0000, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Dec 19, 2012 at 04:44:03AM -0500, Jun Chen wrote:
> 
> > This spi_alloc_device function will be called in the spi_new_device
> > function to alloc new device as the master. But other way, it is called
> > by the of_register_spi_devices function to register new device as the
> > children of the master. I will update changlog to add it. 
> 
> But why is this a bad thing?  You've said what's happening but not why
> it's a problem.

spi_devices should always be children of the spi_master. If that is not
the case then it is a bug to be fixed.

g.

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Grant Likely]

On Wed, 19 Dec 2012 09:04:16 +0000, Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Wed, Dec 19, 2012 at 04:44:03AM -0500, Jun Chen wrote:
> 
> > This spi_alloc_device function will be called in the spi_new_device
> > function to alloc new device as the master. But other way, it is called
> > by the of_register_spi_devices function to register new device as the
> > children of the master. I will update changlog to add it. 
> 
> But why is this a bad thing?  You've said what's happening but not why
> it's a problem.

spi_devices should always be children of the spi_master. If that is not
the case then it is a bug to be fixed.

g.

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

------------------------------------------------------------------------------
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d

Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Jun Chen]

On Wed, 2012-12-19 at 16:21 +0000, Grant Likely wrote:
> On Wed, 19 Dec 2012 09:04:16 +0000, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> > On Wed, Dec 19, 2012 at 04:44:03AM -0500, Jun Chen wrote:
> > 
> > > This spi_alloc_device function will be called in the spi_new_device
> > > function to alloc new device as the master. But other way, it is called
> > > by the of_register_spi_devices function to register new device as the
> > > children of the master. I will update changlog to add it. 
> > 
> > But why is this a bad thing?  You've said what's happening but not why
> > it's a problem.
> 
> spi_devices should always be children of the spi_master. If that is not
> the case then it is a bug to be fixed.
> 

When many boards initializing, boards will call function
spi_register_board_info to create bi->board_info,Then spi driver probe
to call spi_register_master to register the driver and in the function
spi_match_master_to_boardinfo To create new spi device, and this cases
the spi_devices are not children of the spi_master.
Many drivers do these steps. If all spi_devices must be children of the
spi_master, Do spi core have plan to delete this way? 
Or spi core can hold this way for many drivers. 
Thanks! 

Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Jun Chen]

On Wed, 2012-12-19 at 16:21 +0000, Grant Likely wrote:
> On Wed, 19 Dec 2012 09:04:16 +0000, Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> > On Wed, Dec 19, 2012 at 04:44:03AM -0500, Jun Chen wrote:
> > 
> > > This spi_alloc_device function will be called in the spi_new_device
> > > function to alloc new device as the master. But other way, it is called
> > > by the of_register_spi_devices function to register new device as the
> > > children of the master. I will update changlog to add it. 
> > 
> > But why is this a bad thing?  You've said what's happening but not why
> > it's a problem.
> 
> spi_devices should always be children of the spi_master. If that is not
> the case then it is a bug to be fixed.
> 

When many boards initializing, boards will call function
spi_register_board_info to create bi->board_info,Then spi driver probe
to call spi_register_master to register the driver and in the function
spi_match_master_to_boardinfo To create new spi device, and this cases
the spi_devices are not children of the spi_master.
Many drivers do these steps. If all spi_devices must be children of the
spi_master, Do spi core have plan to delete this way? 
Or spi core can hold this way for many drivers. 
Thanks! 



------------------------------------------------------------------------------
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d

Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Grant Likely]

On Fri, 21 Dec 2012 12:39:52 -0500, Jun Chen <jun.d.chen@intel.com> wrote:
> On Wed, 2012-12-19 at 16:21 +0000, Grant Likely wrote:
> > On Wed, 19 Dec 2012 09:04:16 +0000, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> > > On Wed, Dec 19, 2012 at 04:44:03AM -0500, Jun Chen wrote:
> > > 
> > > > This spi_alloc_device function will be called in the spi_new_device
> > > > function to alloc new device as the master. But other way, it is called
> > > > by the of_register_spi_devices function to register new device as the
> > > > children of the master. I will update changlog to add it. 
> > > 
> > > But why is this a bad thing?  You've said what's happening but not why
> > > it's a problem.
> > 
> > spi_devices should always be children of the spi_master. If that is not
> > the case then it is a bug to be fixed.
> > 
> 
> When many boards initializing, boards will call function
> spi_register_board_info to create bi->board_info,Then spi driver probe
> to call spi_register_master to register the driver and in the function
> spi_match_master_to_boardinfo To create new spi device, and this cases
> the spi_devices are not children of the spi_master.
> Many drivers do these steps. If all spi_devices must be children of the
> spi_master, Do spi core have plan to delete this way? 
> Or spi core can hold this way for many drivers. 

Let me make sure I understand what you're saying...

Right now, every spi_device object is registered as a child of an
spi_master object.

With this proposed patch, spi_devices registered via spi_register_board_info
will be siblings of the spi_master instead of children.

Do I understand correctly so far?

The problem is that I don't understand why this change is necessary.
spi_devices should always be children of an spi_master, not siblings.
What is the problem you're trying to solve with this change?

g.

Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Jun Chen]

On Fri, 2012-12-21 at 19:06 +0000, Grant Likely wrote:
> On Fri, 21 Dec 2012 12:39:52 -0500, Jun Chen <jun.d.chen@intel.com> wrote:
> > On Wed, 2012-12-19 at 16:21 +0000, Grant Likely wrote:
> > > On Wed, 19 Dec 2012 09:04:16 +0000, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> > > > On Wed, Dec 19, 2012 at 04:44:03AM -0500, Jun Chen wrote:
> > > > 
> > > > > This spi_alloc_device function will be called in the spi_new_device
> > > > > function to alloc new device as the master. But other way, it is called
> > > > > by the of_register_spi_devices function to register new device as the
> > > > > children of the master. I will update changlog to add it. 
> > > > 
> > > > But why is this a bad thing?  You've said what's happening but not why
> > > > it's a problem.
> > > 
> > > spi_devices should always be children of the spi_master. If that is not
> > > the case then it is a bug to be fixed.
> > > 
> > 
> > When many boards initializing, boards will call function
> > spi_register_board_info to create bi->board_info,Then spi driver probe
> > to call spi_register_master to register the driver and in the function
> > spi_match_master_to_boardinfo To create new spi device, and this cases
> > the spi_devices are not children of the spi_master.
> > Many drivers do these steps. If all spi_devices must be children of the
> > spi_master, Do spi core have plan to delete this way? 
> > Or spi core can hold this way for many drivers. 
> 
> Let me make sure I understand what you're saying...
> 
> Right now, every spi_device object is registered as a child of an
> spi_master object.
> 
> With this proposed patch, spi_devices registered via spi_register_board_info
> will be siblings of the spi_master instead of children.
> 
> Do I understand correctly so far?
> 
Yes, 

> The problem is that I don't understand why this change is necessary.
> spi_devices should always be children of an spi_master, not siblings.
> What is the problem you're trying to solve with this change?
> 
When spi drivers try to use the core function(spi_register_master),it
will trigger error,because they use the function
spi_match_master_to_boardinfo  to create new spi device as the children
of the master.
In the old version of spi core, the new devices are registered as
siblings of the spi_master. My spi driver based on the old version runs
normal.

But after applying for this patch:
{
    spi: Fix device unregistration when unregistering the bus master
    
    Device are added as children of the bus master's parent device, but
    spi_unregister_master() looks for devices to unregister in the bus
    master's children. This results in the child devices not being
    unregistered.
    
    Fix this by registering devices as direct children of the bus
master.


-	spi->dev.parent = dev;
+	spi->dev.parent = &master->dev;
}

Then my driver will be crash.
Maybe I have mistake on this issue, thank for your more explanation and
detail replay. 

> g.
> 

Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.

[Grant Likely]

On Mon, 24 Dec 2012 11:16:52 -0500, Jun Chen <jun.d.chen@intel.com> wrote:
> On Fri, 2012-12-21 at 19:06 +0000, Grant Likely wrote:
> > The problem is that I don't understand why this change is necessary.
> > spi_devices should always be children of an spi_master, not siblings.
> > What is the problem you're trying to solve with this change?
> > 
> When spi drivers try to use the core function(spi_register_master),it
> will trigger error,because they use the function
> spi_match_master_to_boardinfo  to create new spi device as the children
> of the master.
> In the old version of spi core, the new devices are registered as
> siblings of the spi_master. My spi driver based on the old version runs
> normal.
> 
> But after applying for this patch:
> {
>     spi: Fix device unregistration when unregistering the bus master
>     
>     Device are added as children of the bus master's parent device, but
>     spi_unregister_master() looks for devices to unregister in the bus
>     master's children. This results in the child devices not being
>     unregistered.
>     
>     Fix this by registering devices as direct children of the bus
> master.
> 
> 
> -	spi->dev.parent = dev;
> +	spi->dev.parent = &master->dev;
> }
> 
> Then my driver will be crash.
> Maybe I have mistake on this issue, thank for your more explanation and
> detail replay. 

Sounds like you've got a driver bug. Make sure it isn't trying to use
the spi_client parent pointer to find the device instance.

g.