[PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[Rodolfo Giometti]

It could happen that an i2c adapter may lock the bus due due
electrical problems, so the user may recover this stale state by using:

	$ echo 1 > /sys/class/i2c-adapter/i2c-0/reset

Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
---
 drivers/i2c/i2c-core.c |   21 +++++++++++++++++++++
 include/linux/i2c.h    |    2 ++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index b1c9abe..b0c4053 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -393,8 +393,29 @@ show_adapter_name(struct device *dev, struct device_attribute *attr, char *buf)
 	return sprintf(buf, "%s\n", adap->name);
 }
 
+static ssize_t
+write_adapter_reset(struct device *dev, struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct i2c_adapter *adap = to_i2c_adapter(dev);
+	unsigned long val;
+	int ret;
+
+	if (!adap->reset)
+		return -EINVAL;
+
+	ret = strict_strtoul(buf, 10, &val);
+	if (ret || val != 1)
+		return -EINVAL;
+
+	adap->reset(adap);
+
+	return count;
+}
+
 static struct device_attribute i2c_adapter_attrs[] = {
 	__ATTR(name, S_IRUGO, show_adapter_name, NULL),
+	__ATTR(reset, S_IWUSR, NULL, write_adapter_reset),
 	{ },
 };
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fcfbfea..0f84345 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -369,6 +369,8 @@ struct i2c_adapter {
 	struct list_head clients;	/* DEPRECATED */
 	char name[48];
 	struct completion dev_released;
+
+	void (*reset)(struct i2c_adapter *);	/* user request adap reset */
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
-- 
1.5.6.3

[PATCH 2/2] i2c pxa: add the force reset method.

[Rodolfo Giometti]

Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
---
 drivers/i2c/busses/i2c-pxa.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 6af6814..ccbc9ba 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -963,6 +963,13 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num
 	return ret;
 }
 
+static void i2c_pxa_force_reset(struct i2c_adapter *adap)
+{
+	struct pxa_i2c *i2c = adap->algo_data;
+
+	i2c_pxa_reset(i2c);
+}
+
 static u32 i2c_pxa_functionality(struct i2c_adapter *adap)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
@@ -1065,6 +1072,7 @@ static int i2c_pxa_probe(struct platform_device *dev)
 
 	i2c->adap.algo_data = i2c;
 	i2c->adap.dev.parent = &dev->dev;
+	i2c->adap.reset	  = i2c_pxa_force_reset;
 
 	ret = i2c_add_numbered_adapter(&i2c->adap);
 	if (ret < 0) {
-- 
1.5.6.3

Re: [PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[Ben Dooks]

On Fri, Feb 06, 2009 at 04:23:17PM +0100, Rodolfo Giometti wrote:
> It could happen that an i2c adapter may lock the bus due due
> electrical problems, so the user may recover this stale state by using:
> 
> 	$ echo 1 > /sys/class/i2c-adapter/i2c-0/reset
> 
> Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>

There doesn't seem to be any locking to stop issuing a reset during
an extant transaction... should the controller driver deal with
cancelling any outstanding transactions?

> ---
>  drivers/i2c/i2c-core.c |   21 +++++++++++++++++++++
>  include/linux/i2c.h    |    2 ++
>  2 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index b1c9abe..b0c4053 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -393,8 +393,29 @@ show_adapter_name(struct device *dev, struct device_attribute *attr, char *buf)
>  	return sprintf(buf, "%s\n", adap->name);
>  }
>  
> +static ssize_t
> +write_adapter_reset(struct device *dev, struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct i2c_adapter *adap = to_i2c_adapter(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	if (!adap->reset)
> +		return -EINVAL;
> +
> +	ret = strict_strtoul(buf, 10, &val);
> +	if (ret || val != 1)
> +		return -EINVAL;
> +
> +	adap->reset(adap);
> +
> +	return count;
> +}
> +
>  static struct device_attribute i2c_adapter_attrs[] = {
>  	__ATTR(name, S_IRUGO, show_adapter_name, NULL),
> +	__ATTR(reset, S_IWUSR, NULL, write_adapter_reset),
>  	{ },
>  };
>  
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index fcfbfea..0f84345 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -369,6 +369,8 @@ struct i2c_adapter {
>  	struct list_head clients;	/* DEPRECATED */
>  	char name[48];
>  	struct completion dev_released;
> +
> +	void (*reset)(struct i2c_adapter *);	/* user request adap reset */
>  };
>  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
>  
> -- 
> 1.5.6.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[Rodolfo Giometti]

On Mon, Feb 09, 2009 at 02:55:46PM +0000, Ben Dooks wrote:
> On Fri, Feb 06, 2009 at 04:23:17PM +0100, Rodolfo Giometti wrote:
> > It could happen that an i2c adapter may lock the bus due due
> > electrical problems, so the user may recover this stale state by using:
> > 
> > 	$ echo 1 > /sys/class/i2c-adapter/i2c-0/reset
> > 
> > Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
> 
> There doesn't seem to be any locking to stop issuing a reset during
> an extant transaction... should the controller driver deal with
> cancelling any outstanding transactions?

I think this should be done inside each adapter driver since it's
an adapter specific issue.

If you see next patch of this patchset you can see that the adapter
i2c-pxa aborts any transfer currently under way during reset.

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org
Linux Device Driver                          giometti-k2GhghHVRtY@public.gmane.org
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: [PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[Jean Delvare]

On Mon, 9 Feb 2009 16:02:55 +0100, Rodolfo Giometti wrote:
> On Mon, Feb 09, 2009 at 02:55:46PM +0000, Ben Dooks wrote:
> > On Fri, Feb 06, 2009 at 04:23:17PM +0100, Rodolfo Giometti wrote:
> > > It could happen that an i2c adapter may lock the bus due due
> > > electrical problems, so the user may recover this stale state by using:
> > > 
> > > 	$ echo 1 > /sys/class/i2c-adapter/i2c-0/reset
> > > 
> > > Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
> > 
> > There doesn't seem to be any locking to stop issuing a reset during
> > an extant transaction... should the controller driver deal with
> > cancelling any outstanding transactions?
> 
> I think this should be done inside each adapter driver since it's
> an adapter specific issue.
> 
> If you see next patch of this patchset you can see that the adapter
> i2c-pxa aborts any transfer currently under way during reset.

I disagree. An i2c bus driver should never get stuck in the middle of a
transaction (that is, with i2c_adapter->bus_lock held.) If a
transaction fails, the driver should timeout and return. Thus I think
Ben is right and it would make a lot of sense to have function
write_adapter_reset take i2c_adapter->bus_lock. This guarantees that
all bus drivers will get locking right and won't allow resetting in the
middle of a (non-stuck) transaction.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[Rodolfo Giometti]

On Mon, Feb 09, 2009 at 04:28:38PM +0100, Jean Delvare wrote:
> On Mon, 9 Feb 2009 16:02:55 +0100, Rodolfo Giometti wrote:
> > On Mon, Feb 09, 2009 at 02:55:46PM +0000, Ben Dooks wrote:
> > > On Fri, Feb 06, 2009 at 04:23:17PM +0100, Rodolfo Giometti wrote:
> > > > It could happen that an i2c adapter may lock the bus due due
> > > > electrical problems, so the user may recover this stale state by using:
> > > > 
> > > > 	$ echo 1 > /sys/class/i2c-adapter/i2c-0/reset
> > > > 
> > > > Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
> > > 
> > > There doesn't seem to be any locking to stop issuing a reset during
> > > an extant transaction... should the controller driver deal with
> > > cancelling any outstanding transactions?
> > 
> > I think this should be done inside each adapter driver since it's
> > an adapter specific issue.
> > 
> > If you see next patch of this patchset you can see that the adapter
> > i2c-pxa aborts any transfer currently under way during reset.
> 
> I disagree. An i2c bus driver should never get stuck in the middle of a
> transaction (that is, with i2c_adapter->bus_lock held.) If a
> transaction fails, the driver should timeout and return. Thus I think
> Ben is right and it would make a lot of sense to have function
> write_adapter_reset take i2c_adapter->bus_lock. This guarantees that
> all bus drivers will get locking right and won't allow resetting in the
> middle of a (non-stuck) transaction.

Ok, I'll modify the code as you suggested ASAP.

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org
Linux Device Driver                          giometti-k2GhghHVRtY@public.gmane.org
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: [PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[Jean Delvare]

On Fri,  6 Feb 2009 16:23:17 +0100, Rodolfo Giometti wrote:
> It could happen that an i2c adapter may lock the bus due due

Typo: "due due" -> "due to".

> electrical problems, so the user may recover this stale state by using:
> 
> 	$ echo 1 > /sys/class/i2c-adapter/i2c-0/reset

What is the intended purpose, debugging? I would certainly hope that
drivers know by themselves when they need a reset. Requiring users to
reset themselves doesn't sound terribly friendly :(

> Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
> ---
>  drivers/i2c/i2c-core.c |   21 +++++++++++++++++++++
>  include/linux/i2c.h    |    2 ++
>  2 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index b1c9abe..b0c4053 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -393,8 +393,29 @@ show_adapter_name(struct device *dev, struct device_attribute *attr, char *buf)
>  	return sprintf(buf, "%s\n", adap->name);
>  }
>  
> +static ssize_t
> +write_adapter_reset(struct device *dev, struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct i2c_adapter *adap = to_i2c_adapter(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	if (!adap->reset)
> +		return -EINVAL;
> +
> +	ret = strict_strtoul(buf, 10, &val);
> +	if (ret || val != 1)
> +		return -EINVAL;
> +
> +	adap->reset(adap);

I consider it good practice to always test for function pointers
_right_ before dereferencing them. Makes it harder to accidentally
break things later.

> +
> +	return count;
> +}
> +
>  static struct device_attribute i2c_adapter_attrs[] = {
>  	__ATTR(name, S_IRUGO, show_adapter_name, NULL),
> +	__ATTR(reset, S_IWUSR, NULL, write_adapter_reset),
>  	{ },
>  };
>  
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index fcfbfea..0f84345 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -369,6 +369,8 @@ struct i2c_adapter {
>  	struct list_head clients;	/* DEPRECATED */
>  	char name[48];
>  	struct completion dev_released;
> +
> +	void (*reset)(struct i2c_adapter *);	/* user request adap reset */
>  };
>  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
>  


-- 
Jean Delvare

Re: [PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[Rodolfo Giometti]

On Mon, Feb 09, 2009 at 05:17:20PM +0100, Jean Delvare wrote:
> On Fri,  6 Feb 2009 16:23:17 +0100, Rodolfo Giometti wrote:
> > It could happen that an i2c adapter may lock the bus due due
> 
> Typo: "due due" -> "due to".

Ok.

> > electrical problems, so the user may recover this stale state by using:
> > 
> > 	$ echo 1 > /sys/class/i2c-adapter/i2c-0/reset
> 
> What is the intended purpose, debugging? I would certainly hope that
> drivers know by themselves when they need a reset. Requiring users to
> reset themselves doesn't sound terribly friendly :(

Unluckely not all adapter does it by themselfes! At least not PXA
one. I'm currently work on adding this new feature.

> > Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
> > ---
> >  drivers/i2c/i2c-core.c |   21 +++++++++++++++++++++
> >  include/linux/i2c.h    |    2 ++
> >  2 files changed, 23 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index b1c9abe..b0c4053 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -393,8 +393,29 @@ show_adapter_name(struct device *dev, struct device_attribute *attr, char *buf)
> >  	return sprintf(buf, "%s\n", adap->name);
> >  }
> >  
> > +static ssize_t
> > +write_adapter_reset(struct device *dev, struct device_attribute *attr,
> > +				const char *buf, size_t count)
> > +{
> > +	struct i2c_adapter *adap = to_i2c_adapter(dev);
> > +	unsigned long val;
> > +	int ret;
> > +
> > +	if (!adap->reset)
> > +		return -EINVAL;
> > +
> > +	ret = strict_strtoul(buf, 10, &val);
> > +	if (ret || val != 1)
> > +		return -EINVAL;
> > +
> > +	adap->reset(adap);
> 
> I consider it good practice to always test for function pointers
> _right_ before dereferencing them. Makes it harder to accidentally
> break things later.

Ok.

> > +
> > +	return count;
> > +}
> > +
> >  static struct device_attribute i2c_adapter_attrs[] = {
> >  	__ATTR(name, S_IRUGO, show_adapter_name, NULL),
> > +	__ATTR(reset, S_IWUSR, NULL, write_adapter_reset),
> >  	{ },
> >  };
> >  
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index fcfbfea..0f84345 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -369,6 +369,8 @@ struct i2c_adapter {
> >  	struct list_head clients;	/* DEPRECATED */
> >  	char name[48];
> >  	struct completion dev_released;
> > +
> > +	void (*reset)(struct i2c_adapter *);	/* user request adap reset */
> >  };
> >  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
> >  

I'll repropose a new patch ASAP.

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org
Linux Device Driver                          giometti-k2GhghHVRtY@public.gmane.org
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: [PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[Jean Delvare]

On Fri,  6 Feb 2009 16:23:17 +0100, Rodolfo Giometti wrote:
> It could happen that an i2c adapter may lock the bus due due
> electrical problems, so the user may recover this stale state by using:
> 
> 	$ echo 1 > /sys/class/i2c-adapter/i2c-0/reset
> 
> Signed-off-by: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
> ---
>  drivers/i2c/i2c-core.c |   21 +++++++++++++++++++++
>  include/linux/i2c.h    |    2 ++
>  2 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index b1c9abe..b0c4053 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -393,8 +393,29 @@ show_adapter_name(struct device *dev, struct device_attribute *attr, char *buf)
>  	return sprintf(buf, "%s\n", adap->name);
>  }
>  
> +static ssize_t
> +write_adapter_reset(struct device *dev, struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct i2c_adapter *adap = to_i2c_adapter(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	if (!adap->reset)
> +		return -EINVAL;

On second thought I believe that -ENOTSUP (or -EOPNOTSUPP) would be a
better error code for this case.

> +
> +	ret = strict_strtoul(buf, 10, &val);
> +	if (ret || val != 1)
> +		return -EINVAL;
> +
> +	adap->reset(adap);
> +
> +	return count;
> +}
> +
>  static struct device_attribute i2c_adapter_attrs[] = {
>  	__ATTR(name, S_IRUGO, show_adapter_name, NULL),
> +	__ATTR(reset, S_IWUSR, NULL, write_adapter_reset),
>  	{ },
>  };
>  
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index fcfbfea..0f84345 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -369,6 +369,8 @@ struct i2c_adapter {
>  	struct list_head clients;	/* DEPRECATED */
>  	char name[48];
>  	struct completion dev_released;
> +
> +	void (*reset)(struct i2c_adapter *);	/* user request adap reset */
>  };
>  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
>  


-- 
Jean Delvare

Re: [PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[Jean Delvare]

On Mon, 9 Feb 2009 21:41:08 +0100, Rodolfo Giometti wrote:
> On Mon, Feb 09, 2009 at 05:17:20PM +0100, Jean Delvare wrote:
> > On Fri,  6 Feb 2009 16:23:17 +0100, Rodolfo Giometti wrote:
> > > It could happen that an i2c adapter may lock the bus due due
> > 
> > Typo: "due due" -> "due to".
> 
> Ok.
> 
> > > electrical problems, so the user may recover this stale state by using:
> > > 
> > > 	$ echo 1 > /sys/class/i2c-adapter/i2c-0/reset
> > 
> > What is the intended purpose, debugging? I would certainly hope that
> > drivers know by themselves when they need a reset. Requiring users to
> > reset themselves doesn't sound terribly friendly :(
> 
> Unluckely not all adapter does it by themselfes! At least not PXA
> one. I'm currently work on adding this new feature.

Well, if you are going to fix the i2c-pxa driver, and it's the only
driver using the manual reset feature, then do we really need to add
this feature?

Don't get me wrong, if this feature is still useful then I am fine
adding it. But OTOH we still have to think twice when doing such core
changes, because they increase the memory footprint for all systems.

I am curious what other developers think. Me, I never felt the need for
a manual reset feature. Usually when I have managed to break an I2C
controller, cycling the driver would fix it, or if not, it was so
broken that I had to power-cycle the box anyway.

Are there other developers out there who consider the proposed manual
reset feature a good thing to have? Please speak up.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[Rodolfo Giometti]

On Tue, Feb 10, 2009 at 11:44:38AM +0100, Jean Delvare wrote:
> On Mon, 9 Feb 2009 21:41:08 +0100, Rodolfo Giometti wrote:
> > On Mon, Feb 09, 2009 at 05:17:20PM +0100, Jean Delvare wrote:
> > > On Fri,  6 Feb 2009 16:23:17 +0100, Rodolfo Giometti wrote:
> > > > It could happen that an i2c adapter may lock the bus due due
> > > 
> > > Typo: "due due" -> "due to".
> > 
> > Ok.
> > 
> > > > electrical problems, so the user may recover this stale state by using:
> > > > 
> > > > 	$ echo 1 > /sys/class/i2c-adapter/i2c-0/reset
> > > 
> > > What is the intended purpose, debugging? I would certainly hope that
> > > drivers know by themselves when they need a reset. Requiring users to
> > > reset themselves doesn't sound terribly friendly :(
> > 
> > Unluckely not all adapter does it by themselfes! At least not PXA
> > one. I'm currently work on adding this new feature.
> 
> Well, if you are going to fix the i2c-pxa driver, and it's the only
> driver using the manual reset feature, then do we really need to add
> this feature?

I don't know about other controllers, I know about PXA one because I'm
currently working on it. :)

> 
> Don't get me wrong, if this feature is still useful then I am fine
> adding it. But OTOH we still have to think twice when doing such core
> changes, because they increase the memory footprint for all systems.

I agree. Maybe I can add such entry with a conditional #ifdef DEBUG
and setting it off by default?

> 
> I am curious what other developers think. Me, I never felt the need for
> a manual reset feature. Usually when I have managed to break an I2C
> controller, cycling the driver would fix it, or if not, it was so
> broken that I had to power-cycle the box anyway.
> 
> Are there other developers out there who consider the proposed manual
> reset feature a good thing to have? Please speak up.

However, regarding PXA controller I found a way to automagically reset
it when it locks. I'll send a patchset ASAP.

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti-AVVDYK/kqiJWk0Htik3J/w@public.gmane.org
Linux Device Driver                          giometti-k2GhghHVRtY@public.gmane.org
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: [PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[Wolfram Sang]

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


Sorry, I missed the original post.

> > Are there other developers out there who consider the proposed manual
> > reset feature a good thing to have? Please speak up.

Hmm, never needed it so far. Can't really think of a reason for it
besides debugging...

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH 1/2] i2c: add "reset" sysfs entry for adapters.

[Jean Delvare]

On Wed, 11 Feb 2009 14:00:29 +0100, Rodolfo Giometti wrote:
> On Tue, Feb 10, 2009 at 11:44:38AM +0100, Jean Delvare wrote:
> > On Mon, 9 Feb 2009 21:41:08 +0100, Rodolfo Giometti wrote:
> > > On Mon, Feb 09, 2009 at 05:17:20PM +0100, Jean Delvare wrote:
> > > > On Fri,  6 Feb 2009 16:23:17 +0100, Rodolfo Giometti wrote:
> > > > > It could happen that an i2c adapter may lock the bus due due
> > > > 
> > > > Typo: "due due" -> "due to".
> > > 
> > > Ok.
> > > 
> > > > > electrical problems, so the user may recover this stale state by using:
> > > > > 
> > > > > 	$ echo 1 > /sys/class/i2c-adapter/i2c-0/reset
> > > > 
> > > > What is the intended purpose, debugging? I would certainly hope that
> > > > drivers know by themselves when they need a reset. Requiring users to
> > > > reset themselves doesn't sound terribly friendly :(
> > > 
> > > Unluckely not all adapter does it by themselfes! At least not PXA
> > > one. I'm currently work on adding this new feature.
> > 
> > Well, if you are going to fix the i2c-pxa driver, and it's the only
> > driver using the manual reset feature, then do we really need to add
> > this feature?
> 
> I don't know about other controllers, I know about PXA one because I'm
> currently working on it. :)
> 
> > 
> > Don't get me wrong, if this feature is still useful then I am fine
> > adding it. But OTOH we still have to think twice when doing such core
> > changes, because they increase the memory footprint for all systems.
> 
> I agree. Maybe I can add such entry with a conditional #ifdef DEBUG
> and setting it off by default?

Yes, I would make it conditional upon CONFIG_I2C_DEBUG_BUS, and
possibly make it a private attribute of i2c-pxa if no other developers
are interested.

> > I am curious what other developers think. Me, I never felt the need for
> > a manual reset feature. Usually when I have managed to break an I2C
> > controller, cycling the driver would fix it, or if not, it was so
> > broken that I had to power-cycle the box anyway.
> > 
> > Are there other developers out there who consider the proposed manual
> > reset feature a good thing to have? Please speak up.
> 
> However, regarding PXA controller I found a way to automagically reset
> it when it locks. I'll send a patchset ASAP.

Well, then I'm not sure if we still care about this debugging patch...

-- 
Jean Delvare