linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] iio: core: Propagate error codes from OF layer to client drivers
@ 2014-08-25 12:57 Ivan T. Ivanov
  2014-08-25 16:10 ` Jonathan Cameron
  0 siblings, 1 reply; 9+ messages in thread
From: Ivan T. Ivanov @ 2014-08-25 12:57 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Ivan T. Ivanov, Adam.Thomson.Opensource, knaack.h, lars, pmeerw,
	linux-iio, linux-kernel

Do not overwrite error codes returned from of_iio_channel_get().
Error codes are used to distinguish between "io-channel-names"
not present in DT bindings, property is optional, and IIO channel
provider driver still not being loaded, defer probe.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 drivers/iio/inkern.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index c749700..66a6cde 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -162,7 +162,7 @@ err_free_channel:
 static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
 						      const char *name)
 {
-	struct iio_channel *chan = NULL;
+	struct iio_channel *chan = ERR_PTR(-ENODEV);
 
 	/* Walk up the tree of devices looking for a matching iio channel */
 	while (np) {
@@ -183,7 +183,7 @@ static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
 		else if (name && index >= 0) {
 			pr_err("ERROR: could not get IIO channel %s:%s(%i)\n",
 				np->full_name, name ? name : "", index);
-			return NULL;
+			break;
 		}
 
 		/*
@@ -193,7 +193,7 @@ static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
 		 */
 		np = np->parent;
 		if (np && !of_get_property(np, "io-channel-ranges", NULL))
-			return NULL;
+			break;
 	}
 
 	return chan;
@@ -243,12 +243,12 @@ error_free_chans:
 static inline struct iio_channel *
 of_iio_channel_get_by_name(struct device_node *np, const char *name)
 {
-	return NULL;
+	return ERR_PTR(-ENODEV);
 }
 
 static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
 {
-	return NULL;
+	return ERR_PTR(-ENODEV);
 }
 
 #endif /* CONFIG_OF */
@@ -312,14 +312,14 @@ struct iio_channel *iio_channel_get(struct device *dev,
 	const char *name = dev ? dev_name(dev) : NULL;
 	struct iio_channel *channel;
 
-	if (dev) {
-		channel = of_iio_channel_get_by_name(dev->of_node,
-						     channel_name);
-		if (channel != NULL)
-			return channel;
-	}
+	channel = iio_channel_get_sys(name, channel_name);
+	if (!IS_ERR(channel))
+		return channel;
+
+	if (!dev)
+		return channel;
 
-	return iio_channel_get_sys(name, channel_name);
+	return of_iio_channel_get_by_name(dev->of_node, channel_name);
 }
 EXPORT_SYMBOL_GPL(iio_channel_get);
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] iio: core: Propagate error codes from OF layer to client drivers
  2014-08-25 12:57 [PATCH] iio: core: Propagate error codes from OF layer to client drivers Ivan T. Ivanov
@ 2014-08-25 16:10 ` Jonathan Cameron
  2014-08-25 16:54   ` Guenter Roeck
  0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2014-08-25 16:10 UTC (permalink / raw)
  To: Ivan T. Ivanov
  Cc: Adam.Thomson.Opensource, knaack.h, lars, pmeerw, linux-iio,
	linux-kernel, Guenter Roeck

On 25/08/14 13:57, Ivan T. Ivanov wrote:
> Do not overwrite error codes returned from of_iio_channel_get().
> Error codes are used to distinguish between "io-channel-names"
> not present in DT bindings, property is optional, and IIO channel
> provider driver still not being loaded, defer probe.
> 
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
Cc'd Guenter who often takes an interest in this code (and wrote it ;)

Mostly seems logical to me, though I don't like the change of
priority in the last bit.  I've also just taken a fix for this
code so there may be some fuzz from that once it's propogated
through to mainline and back to the togreg tree of iio.git

Thanks,

Jonathan
> ---
>  drivers/iio/inkern.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index c749700..66a6cde 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -162,7 +162,7 @@ err_free_channel:
>  static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
>  						      const char *name)
>  {
> -	struct iio_channel *chan = NULL;
> +	struct iio_channel *chan = ERR_PTR(-ENODEV);
>  
>  	/* Walk up the tree of devices looking for a matching iio channel */
>  	while (np) {
> @@ -183,7 +183,7 @@ static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
>  		else if (name && index >= 0) {
>  			pr_err("ERROR: could not get IIO channel %s:%s(%i)\n",
>  				np->full_name, name ? name : "", index);
> -			return NULL;
> +			break;
>  		}
>  
>  		/*
> @@ -193,7 +193,7 @@ static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
>  		 */
>  		np = np->parent;
>  		if (np && !of_get_property(np, "io-channel-ranges", NULL))
> -			return NULL;
> +			break;
>  	}
>  
>  	return chan;
> @@ -243,12 +243,12 @@ error_free_chans:
>  static inline struct iio_channel *
>  of_iio_channel_get_by_name(struct device_node *np, const char *name)
>  {
> -	return NULL;
> +	return ERR_PTR(-ENODEV);
>  }
>  
>  static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
>  {
> -	return NULL;
> +	return ERR_PTR(-ENODEV);
>  }
>  
>  #endif /* CONFIG_OF */
> @@ -312,14 +312,14 @@ struct iio_channel *iio_channel_get(struct device *dev,
>  	const char *name = dev ? dev_name(dev) : NULL;
>  	struct iio_channel *channel;
>  
> -	if (dev) {
> -		channel = of_iio_channel_get_by_name(dev->of_node,
> -						     channel_name);
> -		if (channel != NULL)
> -			return channel;
> -	}
> +	channel = iio_channel_get_sys(name, channel_name);
> +	if (!IS_ERR(channel))
> +		return channel;
> +
> +	if (!dev)
> +		return channel;
>  
> -	return iio_channel_get_sys(name, channel_name);
> +	return of_iio_channel_get_by_name(dev->of_node, channel_name);
>  }
Why reorder the logic?   This makes this patch less obviously
correct for limited obvious gain?

Previously the priority was clearly given to device tree bindings
wherease now it is given to board file provided map elements.  It
would be interesting to see boards with both provided, but it is
possible.
>  EXPORT_SYMBOL_GPL(iio_channel_get);
>  
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] iio: core: Propagate error codes from OF layer to client drivers
  2014-08-25 16:10 ` Jonathan Cameron
@ 2014-08-25 16:54   ` Guenter Roeck
  2014-08-26  7:51     ` Ivan T. Ivanov
  0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2014-08-25 16:54 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Ivan T. Ivanov, Adam.Thomson.Opensource, knaack.h, lars, pmeerw,
	linux-iio, linux-kernel

On Mon, Aug 25, 2014 at 05:10:31PM +0100, Jonathan Cameron wrote:
> On 25/08/14 13:57, Ivan T. Ivanov wrote:
> > Do not overwrite error codes returned from of_iio_channel_get().
> > Error codes are used to distinguish between "io-channel-names"
> > not present in DT bindings, property is optional, and IIO channel
> > provider driver still not being loaded, defer probe.
> > 
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> Cc'd Guenter who often takes an interest in this code (and wrote it ;)
> 
> Mostly seems logical to me, though I don't like the change of
> priority in the last bit.  I've also just taken a fix for this
> code so there may be some fuzz from that once it's propogated
> through to mainline and back to the togreg tree of iio.git
> 
> Thanks,
> 
> Jonathan
> > ---
> >  drivers/iio/inkern.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index c749700..66a6cde 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -162,7 +162,7 @@ err_free_channel:
> >  static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
> >  						      const char *name)
> >  {
> > -	struct iio_channel *chan = NULL;
> > +	struct iio_channel *chan = ERR_PTR(-ENODEV);
> >  
> >  	/* Walk up the tree of devices looking for a matching iio channel */
> >  	while (np) {
> > @@ -183,7 +183,7 @@ static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
> >  		else if (name && index >= 0) {
> >  			pr_err("ERROR: could not get IIO channel %s:%s(%i)\n",
> >  				np->full_name, name ? name : "", index);
> > -			return NULL;
> > +			break;
> >  		}
> >  
> >  		/*
> > @@ -193,7 +193,7 @@ static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
> >  		 */
> >  		np = np->parent;
> >  		if (np && !of_get_property(np, "io-channel-ranges", NULL))
> > -			return NULL;
> > +			break;
> >  	}
> >  
> >  	return chan;
> > @@ -243,12 +243,12 @@ error_free_chans:
> >  static inline struct iio_channel *
> >  of_iio_channel_get_by_name(struct device_node *np, const char *name)
> >  {
> > -	return NULL;
> > +	return ERR_PTR(-ENODEV);
> >  }
> >  
> >  static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
> >  {
> > -	return NULL;
> > +	return ERR_PTR(-ENODEV);
> >  }
> >  
> >  #endif /* CONFIG_OF */
> > @@ -312,14 +312,14 @@ struct iio_channel *iio_channel_get(struct device *dev,
> >  	const char *name = dev ? dev_name(dev) : NULL;
> >  	struct iio_channel *channel;
> >  
> > -	if (dev) {
> > -		channel = of_iio_channel_get_by_name(dev->of_node,
> > -						     channel_name);
> > -		if (channel != NULL)
> > -			return channel;
> > -	}
> > +	channel = iio_channel_get_sys(name, channel_name);
> > +	if (!IS_ERR(channel))
> > +		return channel;
> > +
> > +	if (!dev)
> > +		return channel;
> >  
> > -	return iio_channel_get_sys(name, channel_name);
> > +	return of_iio_channel_get_by_name(dev->of_node, channel_name);
> >  }
> Why reorder the logic?   This makes this patch less obviously
> correct for limited obvious gain?
> 
> Previously the priority was clearly given to device tree bindings
> wherease now it is given to board file provided map elements.  It
> would be interesting to see boards with both provided, but it is
> possible.

I am not entirely sure I understand what problem this patch is supposed
to fix on top of the patch you just applied, and I am also a bit concerned
about reversing the logic. Also, iio_channel_get_sys can return -ENOMEM
and -EINVAL besides -ENODEV, all of which is now being ignored unless dev is
set, and then it is returned unconditionally. So instead of ignoring error
codes from of_iio_channel_get_by_name, the code now ignores error codes
from iio_channel_get_sys under some circumstances (which, coincidentially,
does not return -EPROBE_DEFER), and in other circumstances may return an error
even if devicetree data exists. Why and how is that better than before ?
Seems to me it just introduces a whole number of new failure conditions.

Guenter

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] iio: core: Propagate error codes from OF layer to client drivers
  2014-08-25 16:54   ` Guenter Roeck
@ 2014-08-26  7:51     ` Ivan T. Ivanov
  2014-08-26 13:25       ` Guenter Roeck
  0 siblings, 1 reply; 9+ messages in thread
From: Ivan T. Ivanov @ 2014-08-26  7:51 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Jonathan Cameron, Adam.Thomson.Opensource, knaack.h, lars,
	pmeerw, linux-iio, linux-kernel

Hi,

On Mon, 2014-08-25 at 09:54 -0700, Guenter Roeck wrote:
> On Mon, Aug 25, 2014 at 05:10:31PM +0100, Jonathan Cameron wrote:
> > On 25/08/14 13:57, Ivan T. Ivanov wrote:
> > > Do not overwrite error codes returned from of_iio_channel_get().
> > > Error codes are used to distinguish between "io-channel-names"
> > > not present in DT bindings, property is optional, and IIO channel
> > > provider driver still not being loaded, defer probe.
> > > 
> > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > Cc'd Guenter who often takes an interest in this code (and wrote it ;)
> > 
> > Mostly seems logical to me, though I don't like the change of
> > priority in the last bit.  I've also just taken a fix for this
> > code so there may be some fuzz from that once it's propogated
> > through to mainline and back to the togreg tree of iio.git

It is propagated, but exactly this patch [1] is causing the troubles :-)

> > 
> > Thanks,
> > 
> > Jonathan
> > > ---
> > >  drivers/iio/inkern.c | 24 ++++++++++++------------
> > >  1 file changed, 12 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > index c749700..66a6cde 100644
> > > --- a/drivers/iio/inkern.c
> > > +++ b/drivers/iio/inkern.c
> > > @@ -162,7 +162,7 @@ err_free_channel:
> > >  static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
> > >  						      const char *name)
> > >  {
> > > -	struct iio_channel *chan = NULL;
> > > +	struct iio_channel *chan = ERR_PTR(-ENODEV);
> > >  
> > >  	/* Walk up the tree of devices looking for a matching iio channel */
> > >  	while (np) {
> > > @@ -183,7 +183,7 @@ static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
> > >  		else if (name && index >= 0) {
> > >  			pr_err("ERROR: could not get IIO channel %s:%s(%i)\n",
> > >  				np->full_name, name ? name : "", index);
> > > -			return NULL;
> > > +			break;
> > >  		}
> > >  
> > >  		/*
> > > @@ -193,7 +193,7 @@ static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
> > >  		 */
> > >  		np = np->parent;
> > >  		if (np && !of_get_property(np, "io-channel-ranges", NULL))
> > > -			return NULL;
> > > +			break;
> > >  	}
> > >  
> > >  	return chan;
> > > @@ -243,12 +243,12 @@ error_free_chans:
> > >  static inline struct iio_channel *
> > >  of_iio_channel_get_by_name(struct device_node *np, const char *name)
> > >  {
> > > -	return NULL;
> > > +	return ERR_PTR(-ENODEV);
> > >  }
> > >  
> > >  static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
> > >  {
> > > -	return NULL;
> > > +	return ERR_PTR(-ENODEV);
> > >  }
> > >  
> > >  #endif /* CONFIG_OF */
> > > @@ -312,14 +312,14 @@ struct iio_channel *iio_channel_get(struct device *dev,
> > >  	const char *name = dev ? dev_name(dev) : NULL;
> > >  	struct iio_channel *channel;
> > >  
> > > -	if (dev) {
> > > -		channel = of_iio_channel_get_by_name(dev->of_node,
> > > -						     channel_name);
> > > -		if (channel != NULL)
> > > -			return channel;
> > > -	}
> > > +	channel = iio_channel_get_sys(name, channel_name);
> > > +	if (!IS_ERR(channel))
> > > +		return channel;
> > > +
> > > +	if (!dev)
> > > +		return channel;
> > >  
> > > -	return iio_channel_get_sys(name, channel_name);
> > > +	return of_iio_channel_get_by_name(dev->of_node, channel_name);
> > >  }
> > Why reorder the logic?   This makes this patch less obviously
> > correct for limited obvious gain?

I would like to avoid white listing EPROBE_DEFER error code on return of
of_get() function.

> > 
> > Previously the priority was clearly given to device tree bindings
> > wherease now it is given to board file provided map elements.  It
> > would be interesting to see boards with both provided, but it is
> > possible.

I see.

> 
> I am not entirely sure I understand what problem this patch is supposed
> to fix on top of the patch you just applied, 

Patch [1], Guenter please fix you date, introduce regression. It
breaks deferred probe mechanism.

> and I am also a bit concerned
> about reversing the logic. Also, iio_channel_get_sys can return -ENOMEM
> and -EINVAL besides -ENODEV, all of which is now being ignored 

Yes, and thats why we continue with trying to find channel using OF.

> unless dev is set, and then it is returned unconditionally. 

If dev is not valid there is no point to go and ask OF layer for
channel, so jut go out with error code from get_sys()

> So instead of ignoring error
> codes from of_iio_channel_get_by_name, the code now ignores error codes
> from iio_channel_get_sys under some circumstances (which, coincidentially,
> does not return -EPROBE_DEFER), and in other circumstances may return an error
> even if devicetree data exists. Why and how is that better than before ?
> Seems to me it just introduces a whole number of new failure conditions.

We should not mask error codes from OF layer. EPROBE_DEFER is one of
them. So instead of checking for them explicitly on return from of_iio_get(),
I decided that will be future proof if I just reverse the order of functions. 

Another, less intrusive, solution will be if we revert last patch and explicitly
check for EPROBE_DEFER on of_ by_name() return. How this sounds?

Regards,
Ivan


[1] commit a2c12493ed7e63a18cef33a71686d12ffcd6600e
Author: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Date:   Thu Nov 6 12:11:00 2014 +0000

    iio: of_iio_channel_get_by_name() returns non-null pointers for error legs
 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] iio: core: Propagate error codes from OF layer to client drivers
  2014-08-26  7:51     ` Ivan T. Ivanov
@ 2014-08-26 13:25       ` Guenter Roeck
  2014-08-26 13:48         ` Ivan T. Ivanov
  0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2014-08-26 13:25 UTC (permalink / raw)
  To: Ivan T. Ivanov
  Cc: Jonathan Cameron, Adam.Thomson.Opensource, knaack.h, lars,
	pmeerw, linux-iio, linux-kernel

On 08/26/2014 12:51 AM, Ivan T. Ivanov wrote:
> Hi,
>
[ ... ]
>
> Another, less intrusive, solution will be if we revert last patch and explicitly
> check for EPROBE_DEFER on of_ by_name() return. How this sounds?
>
How is that different to the just accepted patch ?

Guenter


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] iio: core: Propagate error codes from OF layer to client drivers
  2014-08-26 13:25       ` Guenter Roeck
@ 2014-08-26 13:48         ` Ivan T. Ivanov
  2014-08-26 14:35           ` Opensource [Adam Thomson]
  2014-08-27 15:59           ` Adam Thomson
  0 siblings, 2 replies; 9+ messages in thread
From: Ivan T. Ivanov @ 2014-08-26 13:48 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Jonathan Cameron, Adam.Thomson.Opensource, knaack.h, lars,
	pmeerw, linux-iio, linux-kernel

On Tue, 2014-08-26 at 06:25 -0700, Guenter Roeck wrote:
> On 08/26/2014 12:51 AM, Ivan T. Ivanov wrote:
> > Hi,
> >
> [ ... ]
> >
> > Another, less intrusive, solution will be if we revert last patch and explicitly
> > check for EPROBE_DEFER on of_ by_name() return. How this sounds?
> >
> How is that different to the just accepted patch ?

You mean this one[1]. I have prepared fix last Friday and forget to
check again before posting it, sorry. Please ignore my patch.

Thanks, 
Ivan

[1] iio:inkern: fix overwritten -EPROBE_DEFER in of_iio_channel_get_by_name



^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] iio: core: Propagate error codes from OF layer to client drivers
  2014-08-26 13:48         ` Ivan T. Ivanov
@ 2014-08-26 14:35           ` Opensource [Adam Thomson]
  2014-08-27 15:59           ` Adam Thomson
  1 sibling, 0 replies; 9+ messages in thread
From: Opensource [Adam Thomson] @ 2014-08-26 14:35 UTC (permalink / raw)
  To: Ivan T. Ivanov, Guenter Roeck
  Cc: Jonathan Cameron, Opensource [Adam Thomson],
	knaack.h, lars, pmeerw, linux-iio, linux-kernel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1162 bytes --]

On August 26, 2014 14:48, Ivan T. Ivanov wrote:

> On Tue, 2014-08-26 at 06:25 -0700, Guenter Roeck wrote:
> > On 08/26/2014 12:51 AM, Ivan T. Ivanov wrote:
> > > Hi,
> > >
> > [ ... ]
> > >
> > > Another, less intrusive, solution will be if we revert last patch and explicitly
> > > check for EPROBE_DEFER on of_ by_name() return. How this sounds?
> > >
> > How is that different to the just accepted patch ?
> 
> You mean this one[1]. I have prepared fix last Friday and forget to
> check again before posting it, sorry. Please ignore my patch.
> 
> Thanks,
> Ivan
> 
> [1] iio:inkern: fix overwritten -EPROBE_DEFER in of_iio_channel_get_by_name
> 

Apologies on my part for fixing one problem and introducing another. Didn't see
that in my testing, and missed that potential return value when examining the
code. At the time It looked like the parent function would only expect NULL
pointers for failure, especially given the non CONFIG_OF definitions of the
functions. Should've delved further. :(
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] iio: core: Propagate error codes from OF layer to client drivers
  2014-08-26 13:48         ` Ivan T. Ivanov
  2014-08-26 14:35           ` Opensource [Adam Thomson]
@ 2014-08-27 15:59           ` Adam Thomson
  2014-08-27 17:41             ` Jonathan Cameron
  1 sibling, 1 reply; 9+ messages in thread
From: Adam Thomson @ 2014-08-27 15:59 UTC (permalink / raw)
  To: Opensource [Adam Thomson], Ivan T. Ivanov, Guenter Roeck
  Cc: Jonathan Cameron, knaack.h, lars, pmeerw, linux-iio, linux-kernel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3375 bytes --]

On August 26, 2014 15:36, Adam Thomson wrote:

> On August 26, 2014 14:48, Ivan T. Ivanov wrote:
>
> > On Tue, 2014-08-26 at 06:25 -0700, Guenter Roeck wrote:
> > > On 08/26/2014 12:51 AM, Ivan T. Ivanov wrote:
> > > > Hi,
> > > >
> > > [ ... ]
> > > >
> > > > Another, less intrusive, solution will be if we revert last patch and explicitly
> > > > check for EPROBE_DEFER on of_ by_name() return. How this sounds?
> > > >
> > > How is that different to the just accepted patch ?
> >
> > You mean this one[1]. I have prepared fix last Friday and forget to
> > check again before posting it, sorry. Please ignore my patch.
> >
> > Thanks,
> > Ivan
> >
> > [1] iio:inkern: fix overwritten -EPROBE_DEFER in of_iio_channel_get_by_name
> >
>
> Apologies on my part for fixing one problem and introducing another. Didn't see
> that in my testing, and missed that potential return value when examining the
> code. At the time It looked like the parent function would only expect NULL
> pointers for failure, especially given the non CONFIG_OF definitions of the
> functions. Should've delved further. :(

On August 26, 2014 15:36, Adam Thomson wrote:

> On August 26, 2014 14:48, Ivan T. Ivanov wrote:
>
> > On Tue, 2014-08-26 at 06:25 -0700, Guenter Roeck wrote:
> > > On 08/26/2014 12:51 AM, Ivan T. Ivanov wrote:
> > > > Hi,
> > > >
> > > [ ... ]
> > > >
> > > > Another, less intrusive, solution will be if we revert last patch and explicitly
> > > > check for EPROBE_DEFER on of_ by_name() return. How this sounds?
> > > >
> > > How is that different to the just accepted patch ?
> >
> > You mean this one[1]. I have prepared fix last Friday and forget to
> > check again before posting it, sorry. Please ignore my patch.
> >
> > Thanks,
> > Ivan
> >
> > [1] iio:inkern: fix overwritten -EPROBE_DEFER in of_iio_channel_get_by_name
> >
>
> Apologies on my part for fixing one problem and introducing another. Didn't see
> that in my testing, and missed that potential return value when examining the
> code. At the time It looked like the parent function would only expect NULL
> pointers for failure, especially given the non CONFIG_OF definitions of the
> functions. Should've delved further. :(

In testing patch code for one of our devices, I've verified what Guenter
mentioned previously in this thread, in that iio_channel_get_sys() doesn't
return -EPROBE_DEFER. This means that drivers using the default map approach of
accessing IIO channels will fail to instantiate as they do not know they
need to defer their probe (have seen this with my driver too).

Haven't looked in detail yet as to how, but I guess this is something that will
also need to be addressed.
Legal Disclaimer: This e-mail communication (and any attachment/s) is confidential and contains proprietary information, some or all of which may be legally privileged. It is intended solely for the use of the individual or entity to which it is addressed. Access to this email by anyone else is unauthorized. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.

Please consider the environment before printing this e-mail


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] iio: core: Propagate error codes from OF layer to client drivers
  2014-08-27 15:59           ` Adam Thomson
@ 2014-08-27 17:41             ` Jonathan Cameron
  0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2014-08-27 17:41 UTC (permalink / raw)
  To: Adam Thomson, Opensource [Adam Thomson], Ivan T. Ivanov, Guenter Roeck
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel

On 27/08/14 16:59, Adam Thomson wrote:
> On August 26, 2014 15:36, Adam Thomson wrote:
> 
>> On August 26, 2014 14:48, Ivan T. Ivanov wrote:
>>
>>> On Tue, 2014-08-26 at 06:25 -0700, Guenter Roeck wrote:
>>>> On 08/26/2014 12:51 AM, Ivan T. Ivanov wrote:
>>>>> Hi,
>>>>>
>>>> [ ... ]
>>>>>
>>>>> Another, less intrusive, solution will be if we revert last patch and explicitly
>>>>> check for EPROBE_DEFER on of_ by_name() return. How this sounds?
>>>>>
>>>> How is that different to the just accepted patch ?
>>>
>>> You mean this one[1]. I have prepared fix last Friday and forget to
>>> check again before posting it, sorry. Please ignore my patch.
>>>
>>> Thanks,
>>> Ivan
>>>
>>> [1] iio:inkern: fix overwritten -EPROBE_DEFER in of_iio_channel_get_by_name
>>>
>>
>> Apologies on my part for fixing one problem and introducing another. Didn't see
>> that in my testing, and missed that potential return value when examining the
>> code. At the time It looked like the parent function would only expect NULL
>> pointers for failure, especially given the non CONFIG_OF definitions of the
>> functions. Should've delved further. :(
> 
> On August 26, 2014 15:36, Adam Thomson wrote:
> 
>> On August 26, 2014 14:48, Ivan T. Ivanov wrote:
>>
>>> On Tue, 2014-08-26 at 06:25 -0700, Guenter Roeck wrote:
>>>> On 08/26/2014 12:51 AM, Ivan T. Ivanov wrote:
>>>>> Hi,
>>>>>
>>>> [ ... ]
>>>>>
>>>>> Another, less intrusive, solution will be if we revert last patch and explicitly
>>>>> check for EPROBE_DEFER on of_ by_name() return. How this sounds?
>>>>>
>>>> How is that different to the just accepted patch ?
>>>
>>> You mean this one[1]. I have prepared fix last Friday and forget to
>>> check again before posting it, sorry. Please ignore my patch.
>>>
>>> Thanks,
>>> Ivan
>>>
>>> [1] iio:inkern: fix overwritten -EPROBE_DEFER in of_iio_channel_get_by_name
>>>
>>
>> Apologies on my part for fixing one problem and introducing another. Didn't see
>> that in my testing, and missed that potential return value when examining the
>> code. At the time It looked like the parent function would only expect NULL
>> pointers for failure, especially given the non CONFIG_OF definitions of the
>> functions. Should've delved further. :(
> 
> In testing patch code for one of our devices, I've verified what Guenter
> mentioned previously in this thread, in that iio_channel_get_sys() doesn't
> return -EPROBE_DEFER. This means that drivers using the default map approach of
> accessing IIO channels will fail to instantiate as they do not know they
> need to defer their probe (have seen this with my driver too).
> 
> Haven't looked in detail yet as to how, but I guess this is something that will
> also need to be addressed.
Yes, that stuff (I think) predates deferred probing and no one has gotten around
to bringing it up to date as yet.

All contributions welcome!

J

> Legal Disclaimer: This e-mail communication (and any attachment/s) is confidential and contains proprietary information, some or all of which may be legally privileged. It is intended solely for the use of the individual or entity to which it is addressed. Access to this email by anyone else is unauthorized. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.
> 
> Please consider the environment before printing this e-mail
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2014-08-27 17:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-25 12:57 [PATCH] iio: core: Propagate error codes from OF layer to client drivers Ivan T. Ivanov
2014-08-25 16:10 ` Jonathan Cameron
2014-08-25 16:54   ` Guenter Roeck
2014-08-26  7:51     ` Ivan T. Ivanov
2014-08-26 13:25       ` Guenter Roeck
2014-08-26 13:48         ` Ivan T. Ivanov
2014-08-26 14:35           ` Opensource [Adam Thomson]
2014-08-27 15:59           ` Adam Thomson
2014-08-27 17:41             ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).