* Re: Shared i2c adapter locking
From: Jean Delvare @ 2009-11-05 14:07 UTC (permalink / raw)
To: Ben Hutchings
Cc: Stephen Rothwell, David Miller, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-next-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mika Kuoppala, Linux I2C
In-Reply-To: <1257429444.2793.2.camel-xQnnTUlwzDrdvaEqJLTMTA9jg9n5Vt1AMm0uRHvK7Nw@public.gmane.org>
On Thu, 05 Nov 2009 13:57:24 +0000, Ben Hutchings wrote:
> On Thu, 2009-11-05 at 14:11 +0100, Jean Delvare wrote:
> [...]
> > What about the following patch?
> >
> > From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > Subject: i2c: Add an interface to lock/unlock I2C bus segment
> >
> > Some drivers need to be able to prevent access to an I2C bus segment
> > for a specific period of time. Add an interface for them to do so
> > without twiddling with i2c-core internals.
> >
> > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > Cc: Ben Hutchings <bhutchings-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
> Acked-by: Ben Hutchings <bhutchings-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
>
> Presumably this is meant for net-next-2.6, and you'll implement
Actually I meant to push this to Linus immediately, through my i2c
tree. This is essentially a no-op: the binary code will be the same as
before the patch, so guaranteed to be safe, and this will solve
conflicts in linux-next.
> i2c_{lock,unlock}_adapter() using rt_mutex in your i2c tree?
Correct.
--
Jean Delvare
^ permalink raw reply
* Re: Shared i2c adapter locking
From: Ben Hutchings @ 2009-11-05 13:57 UTC (permalink / raw)
To: Jean Delvare
Cc: Stephen Rothwell, David Miller, netdev, linux-next, linux-kernel,
Mika Kuoppala, Linux I2C
In-Reply-To: <20091105141122.56b6b4f8@hyperion.delvare>
On Thu, 2009-11-05 at 14:11 +0100, Jean Delvare wrote:
[...]
> What about the following patch?
>
> From: Jean Delvare <khali@linux-fr.org>
> Subject: i2c: Add an interface to lock/unlock I2C bus segment
>
> Some drivers need to be able to prevent access to an I2C bus segment
> for a specific period of time. Add an interface for them to do so
> without twiddling with i2c-core internals.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Ben Hutchings <bhutchings@solarflare.com>
Acked-by: Ben Hutchings <bhutchings@solarflare.com>
Presumably this is meant for net-next-2.6, and you'll implement
i2c_{lock,unlock}_adapter() using rt_mutex in your i2c tree?
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: Shared i2c adapter locking
From: Jean Delvare @ 2009-11-05 13:11 UTC (permalink / raw)
To: Ben Hutchings
Cc: Stephen Rothwell, David Miller, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-next-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mika Kuoppala, Linux I2C
In-Reply-To: <1256828976.2827.27.camel@achroite>
Hi Ben,
On Thu, 29 Oct 2009 15:09:36 +0000, Ben Hutchings wrote:
> On Thu, 2009-10-29 at 15:43 +0100, Jean Delvare wrote:
> > Hi Stephen,
> >
> > On Mon, 26 Oct 2009 13:37:57 +1100, Stephen Rothwell wrote:
> > > Today's linux-next merge of the net tree got a conflict in
> > > drivers/net/sfc/sfe4001.c between commit
> > > 3f7c0648f727a6d5baf6117653e4001dc877b90b ("i2c: Prevent priority
> > > inversion on top of bus lock") from the i2c tree and commit
> > > c9597d4f89565b6562bd3026adbe6eac6c317f47 ("sfc: Merge sfe4001.c into
> > > falcon_boards.c") from the net tree.
> > >
> > > I have applied the following merge fixup patch (after removing
> > > drivers/net/sfc/sfe4001.c) and can carry it as necessary.
> >
> > Thanks for fixing it. The core problem here IMHO is that the sfc
> > network driver touches i2c internals which it would rather leave alone.
>
> I'm just a little proud of having the idea that we could avoid using an
> I/O-expander on this board, but yes, the software side of this
> multiplexing is a hack.
>
> > This is the only driver I know of which does this.
> >
> > I can think of 3 different ways to address the issue.
> >
> > Method #1: add a public API to grab/release an I2C segment.
> >
> > void i2c_adapter_lock(struct i2c_adapter *adapter)
> > {
> > rt_mutex_lock(&adapter->bus_lock);
> > }
> >
> > void i2c_adapter_unlock(struct i2c_adapter *adapter)
> > {
> > rt_mutex_unlock(&adapter->bus_lock);
> > }
> [...]
> > I'm not really sure if I have a preference yet, so please speak up if
> > you do.
>
> Indirect lock operations are a recipe for deadlock, and there doesn't
> seem to be any other user for this, so method 1 seems best.
Well, all 3 methods rely on indirect lock operations to some degree.
But I am fine with method #1 for now. We can always move to something
more complex if the need ever arises.
What about the following patch?
From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Subject: i2c: Add an interface to lock/unlock I2C bus segment
Some drivers need to be able to prevent access to an I2C bus segment
for a specific period of time. Add an interface for them to do so
without twiddling with i2c-core internals.
Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Ben Hutchings <bhutchings-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
---
drivers/net/sfc/sfe4001.c | 4 ++--
include/linux/i2c.h | 18 ++++++++++++++++++
2 files changed, 20 insertions(+), 2 deletions(-)
--- linux-2.6.32-rc6.orig/drivers/net/sfc/sfe4001.c 2009-11-05 10:51:56.000000000 +0100
+++ linux-2.6.32-rc6/drivers/net/sfc/sfe4001.c 2009-11-05 13:40:17.000000000 +0100
@@ -188,7 +188,7 @@ static int sfn4111t_reset(struct efx_nic
efx_oword_t reg;
/* GPIO 3 and the GPIO register are shared with I2C, so block that */
- mutex_lock(&efx->i2c_adap.bus_lock);
+ i2c_lock_adapter(&efx->i2c_adap);
/* Pull RST_N (GPIO 2) low then let it up again, setting the
* FLASH_CFG_1 strap (GPIO 3) appropriately. Only change the
@@ -204,7 +204,7 @@ static int sfn4111t_reset(struct efx_nic
falcon_write(efx, ®, GPIO_CTL_REG_KER);
msleep(1);
- mutex_unlock(&efx->i2c_adap.bus_lock);
+ i2c_unlock_adapter(&efx->i2c_adap);
ssleep(1);
return 0;
--- linux-2.6.32-rc6.orig/include/linux/i2c.h 2009-11-05 10:51:56.000000000 +0100
+++ linux-2.6.32-rc6/include/linux/i2c.h 2009-11-05 14:03:53.000000000 +0100
@@ -361,6 +361,24 @@ static inline void i2c_set_adapdata(stru
dev_set_drvdata(&dev->dev, data);
}
+/**
+ * i2c_lock_adapter - Prevent access to an I2C bus segment
+ * @adapter: Target I2C bus segment
+ */
+static inline void i2c_lock_adapter(struct i2c_adapter *adapter)
+{
+ mutex_lock(&adapter->bus_lock);
+}
+
+/**
+ * i2c_unlock_adapter - Reauthorize access to an I2C bus segment
+ * @adapter: Target I2C bus segment
+ */
+static inline void i2c_unlock_adapter(struct i2c_adapter *adapter)
+{
+ mutex_unlock(&adapter->bus_lock);
+}
+
/*flags for the client struct: */
#define I2C_CLIENT_PEC 0x04 /* Use Packet Error Checking */
#define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */
--
Jean Delvare
^ permalink raw reply
* Re: [PATCH 2/3] i2c-powermac: Simplify pmac_i2c_adapter_to_bus
From: Jean Delvare @ 2009-11-05 9:53 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Linux I2C
In-Reply-To: <1257413363.13611.112.camel@pasglop>
On Thu, 05 Nov 2009 20:29:23 +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2009-11-05 at 10:16 +0100, Jean Delvare wrote:
> >
> > i2c-powermac-01-multiple-msg-not-supported.patch
> > i2c-powermac-02-refactor-xfer.patch
> > i2c-powermac-03-report-errors.patch
> > i2c-powermac-04-include-adapter-in-bus.patch
> > i2c-powermac-05-simplify-pmac_i2c_adapter_to_bus.patch
> > i2c-powermac-06-no-temp-buffer-for-name.patch
> >
> > I only have a formal ack from you for patch 01. Patches 01, 02 and 03
> > have been in linux-next for about a month now. I will delete patch 05
> > now. This leaves us with 4 patches still waiting for your ack (02, 03,
> > 04 and 06.) I'm not sure which ones you tested so far?
>
> I tested 4,5 and 6 though 5 will be dropped. 1 is acked. I think 2 and 3
> slipped through the cracks. Where do I find your patch queue ? I'll test
> tomorrow.
ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/
i2c-powermac-*.patch
Thanks,
--
Jean Delvare
^ permalink raw reply
* Re: [PATCH 2/3] i2c-powermac: Simplify pmac_i2c_adapter_to_bus
From: Benjamin Herrenschmidt @ 2009-11-05 9:29 UTC (permalink / raw)
To: Jean Delvare; +Cc: Linux I2C
In-Reply-To: <20091105101627.0a086c04-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
On Thu, 2009-11-05 at 10:16 +0100, Jean Delvare wrote:
>
> i2c-powermac-01-multiple-msg-not-supported.patch
> i2c-powermac-02-refactor-xfer.patch
> i2c-powermac-03-report-errors.patch
> i2c-powermac-04-include-adapter-in-bus.patch
> i2c-powermac-05-simplify-pmac_i2c_adapter_to_bus.patch
> i2c-powermac-06-no-temp-buffer-for-name.patch
>
> I only have a formal ack from you for patch 01. Patches 01, 02 and 03
> have been in linux-next for about a month now. I will delete patch 05
> now. This leaves us with 4 patches still waiting for your ack (02, 03,
> 04 and 06.) I'm not sure which ones you tested so far?
I tested 4,5 and 6 though 5 will be dropped. 1 is acked. I think 2 and 3
slipped through the cracks. Where do I find your patch queue ? I'll test
tomorrow.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 2/3] i2c-powermac: Simplify pmac_i2c_adapter_to_bus
From: Jean Delvare @ 2009-11-05 9:16 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Linux I2C
In-Reply-To: <1257382888.13611.81.camel@pasglop>
Hi Ben,
On Thu, 05 Nov 2009 12:01:28 +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2009-10-14 at 16:57 +0200, Jean Delvare wrote:
> > pmac_i2c_adapter_to_bus can be simplified. We no longer need to walk
> > the list of buses, we can get the right bus directly.
> >
> > The only case where it makes a difference is if the provided adapter
> > is _not_ a pmac_i2c_bus, but it is not supposed to happen anyway. So
> > we can also drop the error checks.
>
> Allright, I suspect that's the one causing my crashes, I need to confirm
> it still since the crashes are ellusive.
>
> The thing is, there are other i2c adapters on the machine that aren't
> pmac_i2c, for example, the ones creates by the radeon driver for i2c
> probing.
>
> What happens then is that code such as the windfarm stuff does:
>
> static int wf_sat_attach(struct i2c_adapter *adapter)
> {
> struct device_node *busnode, *dev = NULL;
> struct pmac_i2c_bus *bus;
>
> bus = pmac_i2c_adapter_to_bus(adapter);
> busnode = pmac_i2c_get_bus_node(bus);
>
> while ((dev = of_get_next_child(busnode, dev)) != NULL)
> if (of_device_is_compatible(dev, "smu-sat"))
> wf_sat_create(adapter, dev);
> return 0;
> }
>
> That will not work when such an adapter is in the system with
> your new code since pmac_i2c_adapter_to_bus() will return crap,
> causing busnode to basically be random bits of unrelated memory.
You are correct. Not sure how I missed this, as I am aware that the
radeonfb driver has its own I2C adapters.
> Now, what would be nice would be to convert the SMU sat stuff
> to use some new style OF based i2c probing but that is out of
> scope right now, so we need to drop that patch of yours at
> least for now.
I agree. Let's simply drop this one patch. It was only a clean-up and
the rest of the patches are still working and useful without it.
> I'll dbl check later today that this is indeed what's happening
I'm fairly certain your analysis is correct. As long as some drivers
still use i2c_driver.attach_adapter, we can't make any assumption about
which adapter is passed.
I had 6 i2c-powermac patches in my local queue:
i2c-powermac-01-multiple-msg-not-supported.patch
i2c-powermac-02-refactor-xfer.patch
i2c-powermac-03-report-errors.patch
i2c-powermac-04-include-adapter-in-bus.patch
i2c-powermac-05-simplify-pmac_i2c_adapter_to_bus.patch
i2c-powermac-06-no-temp-buffer-for-name.patch
I only have a formal ack from you for patch 01. Patches 01, 02 and 03
have been in linux-next for about a month now. I will delete patch 05
now. This leaves us with 4 patches still waiting for your ack (02, 03,
04 and 06.) I'm not sure which ones you tested so far?
Thanks,
--
Jean Delvare
^ permalink raw reply
* Re: [PATCH 2/3] i2c-powermac: Simplify pmac_i2c_adapter_to_bus
From: Benjamin Herrenschmidt @ 2009-11-05 1:01 UTC (permalink / raw)
To: Jean Delvare; +Cc: Linux I2C
In-Reply-To: <20091014165743.279789a4-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
On Wed, 2009-10-14 at 16:57 +0200, Jean Delvare wrote:
> pmac_i2c_adapter_to_bus can be simplified. We no longer need to walk
> the list of buses, we can get the right bus directly.
>
> The only case where it makes a difference is if the provided adapter
> is _not_ a pmac_i2c_bus, but it is not supposed to happen anyway. So
> we can also drop the error checks.
Allright, I suspect that's the one causing my crashes, I need to confirm
it still since the crashes are ellusive.
The thing is, there are other i2c adapters on the machine that aren't
pmac_i2c, for example, the ones creates by the radeon driver for i2c
probing.
What happens then is that code such as the windfarm stuff does:
static int wf_sat_attach(struct i2c_adapter *adapter)
{
struct device_node *busnode, *dev = NULL;
struct pmac_i2c_bus *bus;
bus = pmac_i2c_adapter_to_bus(adapter);
busnode = pmac_i2c_get_bus_node(bus);
while ((dev = of_get_next_child(busnode, dev)) != NULL)
if (of_device_is_compatible(dev, "smu-sat"))
wf_sat_create(adapter, dev);
return 0;
}
That will not work when such an adapter is in the system with
your new code since pmac_i2c_adapter_to_bus() will return crap,
causing busnode to basically be random bits of unrelated memory.
Now, what would be nice would be to convert the SMU sat stuff
to use some new style OF based i2c probing but that is out of
scope right now, so we need to drop that patch of yours at
least for now.
I'll dbl check later today that this is indeed what's happening
Cheers,
Ben.
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> ---
> arch/powerpc/platforms/powermac/low_i2c.c | 7 +------
> drivers/macintosh/windfarm_lm75_sensor.c | 2 --
> drivers/macintosh/windfarm_max6690_sensor.c | 2 --
> drivers/macintosh/windfarm_smu_sat.c | 2 --
> sound/aoa/codecs/onyx.c | 2 --
> sound/aoa/codecs/tas.c | 2 --
> 6 files changed, 1 insertion(+), 16 deletions(-)
>
> --- linux-2.6.32-rc4.orig/arch/powerpc/platforms/powermac/low_i2c.c 2009-10-14 15:56:23.000000000 +0200
> +++ linux-2.6.32-rc4/arch/powerpc/platforms/powermac/low_i2c.c 2009-10-14 15:57:42.000000000 +0200
> @@ -1020,12 +1020,7 @@ EXPORT_SYMBOL_GPL(pmac_i2c_get_adapter);
>
> struct pmac_i2c_bus *pmac_i2c_adapter_to_bus(struct i2c_adapter *adapter)
> {
> - struct pmac_i2c_bus *bus;
> -
> - list_for_each_entry(bus, &pmac_i2c_busses, link)
> - if (&bus->adapter == adapter)
> - return bus;
> - return NULL;
> + return container_of(adapter, struct pmac_i2c_bus, adapter);
> }
> EXPORT_SYMBOL_GPL(pmac_i2c_adapter_to_bus);
>
> --- linux-2.6.32-rc4.orig/drivers/macintosh/windfarm_lm75_sensor.c 2009-10-05 10:45:49.000000000 +0200
> +++ linux-2.6.32-rc4/drivers/macintosh/windfarm_lm75_sensor.c 2009-10-14 15:57:42.000000000 +0200
> @@ -173,8 +173,6 @@ static int wf_lm75_attach(struct i2c_ada
> DBG("wf_lm75: adapter %s detected\n", adapter->name);
>
> bus = pmac_i2c_adapter_to_bus(adapter);
> - if (bus == NULL)
> - return -ENODEV;
> busnode = pmac_i2c_get_bus_node(bus);
>
> DBG("wf_lm75: bus found, looking for device...\n");
> --- linux-2.6.32-rc4.orig/drivers/macintosh/windfarm_max6690_sensor.c 2009-10-05 10:45:49.000000000 +0200
> +++ linux-2.6.32-rc4/drivers/macintosh/windfarm_max6690_sensor.c 2009-10-14 15:57:42.000000000 +0200
> @@ -135,8 +135,6 @@ static int wf_max6690_attach(struct i2c_
> const char *loc;
>
> bus = pmac_i2c_adapter_to_bus(adapter);
> - if (bus == NULL)
> - return -ENODEV;
> busnode = pmac_i2c_get_bus_node(bus);
>
> while ((dev = of_get_next_child(busnode, dev)) != NULL) {
> --- linux-2.6.32-rc4.orig/drivers/macintosh/windfarm_smu_sat.c 2009-10-05 10:45:49.000000000 +0200
> +++ linux-2.6.32-rc4/drivers/macintosh/windfarm_smu_sat.c 2009-10-14 15:57:42.000000000 +0200
> @@ -359,8 +359,6 @@ static int wf_sat_attach(struct i2c_adap
> struct pmac_i2c_bus *bus;
>
> bus = pmac_i2c_adapter_to_bus(adapter);
> - if (bus == NULL)
> - return -ENODEV;
> busnode = pmac_i2c_get_bus_node(bus);
>
> while ((dev = of_get_next_child(busnode, dev)) != NULL)
> --- linux-2.6.32-rc4.orig/sound/aoa/codecs/onyx.c 2009-10-01 09:44:36.000000000 +0200
> +++ linux-2.6.32-rc4/sound/aoa/codecs/onyx.c 2009-10-14 15:57:42.000000000 +0200
> @@ -1077,8 +1077,6 @@ static int onyx_i2c_attach(struct i2c_ad
> struct pmac_i2c_bus *bus;
>
> bus = pmac_i2c_adapter_to_bus(adapter);
> - if (bus == NULL)
> - return -ENODEV;
> busnode = pmac_i2c_get_bus_node(bus);
>
> while ((dev = of_get_next_child(busnode, dev)) != NULL) {
> --- linux-2.6.32-rc4.orig/sound/aoa/codecs/tas.c 2009-10-05 10:45:51.000000000 +0200
> +++ linux-2.6.32-rc4/sound/aoa/codecs/tas.c 2009-10-14 15:57:42.000000000 +0200
> @@ -958,8 +958,6 @@ static int tas_i2c_attach(struct i2c_ada
> struct pmac_i2c_bus *bus;
>
> bus = pmac_i2c_adapter_to_bus(adapter);
> - if (bus == NULL)
> - return -ENODEV;
> busnode = pmac_i2c_get_bus_node(bus);
>
> while ((dev = of_get_next_child(busnode, dev)) != NULL) {
>
>
^ permalink raw reply
* Re: [PATCH 1/3] i2c-powermac: Include the i2c_adpater in struct pmac_i2c_bus
From: Benjamin Herrenschmidt @ 2009-11-04 23:27 UTC (permalink / raw)
To: Ben Dooks; +Cc: Jean Delvare, Linux I2C
In-Reply-To: <20091030004152.GH13398-elnMNo+KYs3pIgCt6eIbzw@public.gmane.org>
On Fri, 2009-10-30 at 00:41 +0000, Ben Dooks wrote:
> They look ok, anyone else got any objections to putting in -next?
Yeah... finally got to test them and they explode on the G5 :-)
I'll track it down.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/2] [ARM] i2c/pxa: remove unused macro
From: Russell King - ARM Linux @ 2009-11-04 19:06 UTC (permalink / raw)
To: Ben Dooks
Cc: Pavel Machek, Roel Kluin, Eric Miao,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, Uwe Kleine-K?nig, Jean Delvare,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20091103221249.GI13398-elnMNo+KYs3pIgCt6eIbzw@public.gmane.org>
On Tue, Nov 03, 2009 at 10:12:49PM +0000, Ben Dooks wrote:
> On Tue, Nov 03, 2009 at 09:01:15PM +0000, Russell King wrote:
> > On Tue, Nov 03, 2009 at 08:03:19PM +0100, Uwe Kleine-K?nig wrote:
> > > Commit
> > >
> > > beea494 ([ARM] Remove EEPROM slave emulation from i2c-pxa driver.)
> >
> > Oh great. Some people may remember the LX code that I submitted to
> > the mailing list a while back. Removing the slave emulation buggers
> > that platform up. I do intend to submit that once the issue of where
> > the PCON support should be located is resolved - which is still
> > pending.
> >
> > Please revert this change, thanks.
>
> Would you like it reverted in the next merge window, or when the
> LX code is actually added?
Actually, the change was correct, and was part of the LX patchset - I just
misremembered where stuff was with it.
> I'll NAK any changes to remove the eedbg() for the moment.
That change is also fine, consider it acked:
Acked-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Sorry for the noise.
^ permalink raw reply
* Re: [PATCH 1/2] [ARM] i2c/pxa: remove unused macro
From: Uwe Kleine-König @ 2009-11-04 9:00 UTC (permalink / raw)
To: Russell King
Cc: Pavel Machek, Roel Kluin, Eric Miao,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, Ben Dooks, Jean Delvare,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20091103210115.GA24955-f404yB8NqCZvn6HldHNs0ANdhmdF6hFW@public.gmane.org>
Hello,
On Tue, Nov 03, 2009 at 09:01:15PM +0000, Russell King wrote:
> On Tue, Nov 03, 2009 at 08:03:19PM +0100, Uwe Kleine-König wrote:
> > Commit
> >
> > beea494 ([ARM] Remove EEPROM slave emulation from i2c-pxa driver.)
>
> Oh great. Some people may remember the LX code that I submitted to
> the mailing list a while back. Removing the slave emulation buggers
> that platform up. I do intend to submit that once the issue of where
> the PCON support should be located is resolved - which is still
> pending.
>
> Please revert this change, thanks.
"this" means beea494 or my patch? I'm unsure because you (Russell) are
the author of beea494.
The main reason for this series was the 2nd patch. While creating it I
noticed that eedbg was unused.
For this second patch (only defining i2c_pxa_scream_blue_murder if DEBUG
is #defined) should I rebase it to beea494^ or should I revert beea494
and put it on top of the reverted beea494?
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 1/2] [ARM] i2c/pxa: remove unused macro
From: Ben Dooks @ 2009-11-03 22:13 UTC (permalink / raw)
To: Uwe Kleine-K??nig
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jean Delvare,
Ben Dooks, Russell King, Eric Miao, Roel Kluin, Pavel Machek
In-Reply-To: <1257275000-18866-1-git-send-email-u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On Tue, Nov 03, 2009 at 08:03:19PM +0100, Uwe Kleine-K??nig wrote:
> Commit
>
> beea494 ([ARM] Remove EEPROM slave emulation from i2c-pxa driver.)
>
> removed all uses of eedbg, so the definition can go, too.
I belive Russell is trying to sort out the code that was using this
so I would like to put this on hold until we sort out what is going
on.
--
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)
'a smiley only costs 4 bytes'
^ permalink raw reply
* Re: [PATCH 1/2] [ARM] i2c/pxa: remove unused macro
From: Ben Dooks @ 2009-11-03 22:12 UTC (permalink / raw)
To: Russell King
Cc: Uwe Kleine-K?nig, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jean Delvare,
Ben Dooks, Eric Miao, Roel Kluin, Pavel Machek
In-Reply-To: <20091103210115.GA24955-f404yB8NqCZvn6HldHNs0ANdhmdF6hFW@public.gmane.org>
On Tue, Nov 03, 2009 at 09:01:15PM +0000, Russell King wrote:
> On Tue, Nov 03, 2009 at 08:03:19PM +0100, Uwe Kleine-K?nig wrote:
> > Commit
> >
> > beea494 ([ARM] Remove EEPROM slave emulation from i2c-pxa driver.)
>
> Oh great. Some people may remember the LX code that I submitted to
> the mailing list a while back. Removing the slave emulation buggers
> that platform up. I do intend to submit that once the issue of where
> the PCON support should be located is resolved - which is still
> pending.
>
> Please revert this change, thanks.
Would you like it reverted in the next merge window, or when the
LX code is actually added?
I'll NAK any changes to remove the eedbg() for the moment.
--
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)
'a smiley only costs 4 bytes'
^ permalink raw reply
* Re: [PATCH 1/2] [ARM] i2c/pxa: remove unused macro
From: Russell King @ 2009-11-03 21:01 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jean Delvare,
Ben Dooks, Eric Miao, Roel Kluin, Pavel Machek
In-Reply-To: <1257275000-18866-1-git-send-email-u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On Tue, Nov 03, 2009 at 08:03:19PM +0100, Uwe Kleine-König wrote:
> Commit
>
> beea494 ([ARM] Remove EEPROM slave emulation from i2c-pxa driver.)
Oh great. Some people may remember the LX code that I submitted to
the mailing list a while back. Removing the slave emulation buggers
that platform up. I do intend to submit that once the issue of where
the PCON support should be located is resolved - which is still
pending.
Please revert this change, thanks.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
^ permalink raw reply
* Re: [PATCH 1/2] [ARM] i2c/pxa: remove unused macro
From: Uwe Kleine-König @ 2009-11-03 20:00 UTC (permalink / raw)
To: linux-i2c-u79uwXL29TY76Z2rM5mHXA
Cc: Pavel Machek, Roel Kluin, Eric Miao, Ben Dooks, Jean Delvare,
Russell King, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1257275000-18866-1-git-send-email-u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Hello,
> Cc: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
postmaster-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org claims the address ymiao3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org isn't
valid. So I think Eric got a new one.
If you answer to that thread please take care to use
eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org which hopefully works.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 1/2] [ARM] i2c/pxa: remove unused macro
From: Pavel Machek @ 2009-11-03 19:06 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jean Delvare,
Ben Dooks, Russell King, Eric Miao, Roel Kluin
In-Reply-To: <1257275000-18866-1-git-send-email-u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On Tue 2009-11-03 20:03:19, Uwe Kleine-König wrote:
> Commit
>
> beea494 ([ARM] Remove EEPROM slave emulation from i2c-pxa driver.)
>
> removed all uses of eedbg, so the definition can go, too.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> Cc: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Cc: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Cc: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* [PATCH 2/2] [ARM] i2c/pxa: only define 'blue_murder'-function if DEBUG is #defined
From: Uwe Kleine-König @ 2009-11-03 19:03 UTC (permalink / raw)
To: linux-i2c-u79uwXL29TY76Z2rM5mHXA
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Wolfram Sang,
Jean Delvare, Ben Dooks, Russell King, Eric Miao, Roel Kluin,
Pavel Machek
In-Reply-To: <1257275000-18866-1-git-send-email-u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
From: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
This talkative function is also called on timeouts. As timeouts can
happen on regular writes to EEPROMs (no error case), this creates false
positives. Giving lots of details is interesting only for developers
anyhow, so just use the function if DEBUG is #defined.
Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
Cc: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
Cc: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
---
drivers/i2c/busses/i2c-pxa.c | 24 ++++++++++++++----------
1 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 40052c0..009e52e 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -208,16 +208,6 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)
}
#define show_state(i2c) i2c_pxa_show_state(i2c, __LINE__, __func__)
-#else
-#define i2c_debug 0
-
-#define show_state(i2c) do { } while (0)
-#define decode_ISR(val) do { } while (0)
-#define decode_ICR(val) do { } while (0)
-#endif
-
-static void i2c_pxa_master_complete(struct pxa_i2c *i2c, int ret);
-static irqreturn_t i2c_pxa_handler(int this_irq, void *dev_id);
static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)
{
@@ -233,6 +223,20 @@ static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)
printk("\n");
}
+#else /* ifdef DEBUG */
+
+#define i2c_debug 0
+
+#define show_state(i2c) do { } while (0)
+#define decode_ISR(val) do { } while (0)
+#define decode_ICR(val) do { } while (0)
+#define i2c_pxa_scream_blue_murder(i2c, why) do { } while (0)
+
+#endif /* ifdef DEBUG / else */
+
+static void i2c_pxa_master_complete(struct pxa_i2c *i2c, int ret);
+static irqreturn_t i2c_pxa_handler(int this_irq, void *dev_id);
+
static inline int i2c_pxa_is_slavemode(struct pxa_i2c *i2c)
{
return !(readl(_ICR(i2c)) & ICR_SCLE);
--
1.6.5.2
^ permalink raw reply related
* [PATCH 1/2] [ARM] i2c/pxa: remove unused macro
From: Uwe Kleine-König @ 2009-11-03 19:03 UTC (permalink / raw)
To: linux-i2c-u79uwXL29TY76Z2rM5mHXA
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jean Delvare,
Ben Dooks, Russell King, Eric Miao, Roel Kluin, Pavel Machek
Commit
beea494 ([ARM] Remove EEPROM slave emulation from i2c-pxa driver.)
removed all uses of eedbg, so the definition can go, too.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
Cc: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
Cc: Roel Kluin <roel.kluin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
---
drivers/i2c/busses/i2c-pxa.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 0495557..40052c0 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -216,8 +216,6 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)
#define decode_ICR(val) do { } while (0)
#endif
-#define eedbg(lvl, x...) do { if ((lvl) < 1) { printk(KERN_DEBUG "" x); } } while(0)
-
static void i2c_pxa_master_complete(struct pxa_i2c *i2c, int ret);
static irqreturn_t i2c_pxa_handler(int this_irq, void *dev_id);
--
1.6.5.2
^ permalink raw reply related
* [PATCH] i2c-i801: Retry on lost arbitration
From: Jean Delvare @ 2009-11-01 12:25 UTC (permalink / raw)
To: Linux I2C
The Intel 82801 is sometimes used on systems with a BMC connected. The
BMC can access the SMBus, resulting in lost arbitration for the 82801.
We should let i2c-core retry transactions for us in this case.
Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
drivers/i2c/busses/i2c-i801.c | 3 +++
1 file changed, 3 insertions(+)
--- linux-2.6.32-rc5.orig/drivers/i2c/busses/i2c-i801.c 2009-10-05 10:45:49.000000000 +0200
+++ linux-2.6.32-rc5/drivers/i2c/busses/i2c-i801.c 2009-11-01 11:33:29.000000000 +0100
@@ -767,6 +767,9 @@ static int __devinit i801_probe(struct p
/* set up the sysfs linkage to our parent device */
i801_adapter.dev.parent = &dev->dev;
+ /* Retry up to 3 times on arbitration loss */
+ i801_adapter.retries = 3;
+
snprintf(i801_adapter.name, sizeof(i801_adapter.name),
"SMBus I801 adapter at %04lx", i801_smba);
err = i2c_add_adapter(&i801_adapter);
--
Jean Delvare
^ permalink raw reply
* Re: [PATCH V1 1/3] i2c: imx: check busy bit when START/STOP
From: Richard Zhao @ 2009-10-31 2:47 UTC (permalink / raw)
To: Ben Dooks
Cc: kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-i2c-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20091030004025.GG13398-elnMNo+KYs3pIgCt6eIbzw@public.gmane.org>
On Fri, Oct 30, 2009 at 8:40 AM, Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> wrote:
> Does anyone have any comments on these, or shall I look at merging
> them as fixes?
>
> --
> Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)
>
> 'a smiley only costs 4 bytes'
>
Thanks, Ben! I've test these patch on imx31 pdk and imx51 pdk.
Richard
^ permalink raw reply
* Re: [PATCH 1/3] i2c-powermac: Include the i2c_adpater in struct pmac_i2c_bus
From: Benjamin Herrenschmidt @ 2009-10-30 0:50 UTC (permalink / raw)
To: Ben Dooks; +Cc: Jean Delvare, Linux I2C
In-Reply-To: <20091030004152.GH13398-elnMNo+KYs3pIgCt6eIbzw@public.gmane.org>
On Fri, 2009-10-30 at 00:41 +0000, Ben Dooks wrote:
> They look ok, anyone else got any objections to putting in -next?
Haven't had a chance to try them out yet, sorry, too much backlog :-(
I'll try to give those a go later today, else it will have to wait for
next week.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/3] i2c-powermac: Include the i2c_adpater in struct pmac_i2c_bus
From: Ben Dooks @ 2009-10-30 0:41 UTC (permalink / raw)
To: Jean Delvare; +Cc: Linux I2C, Benjamin Herrenschmidt
In-Reply-To: <20091014165629.2e4f6737-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
They look ok, anyone else got any objections to putting in -next?
--
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)
'a smiley only costs 4 bytes'
^ permalink raw reply
* Re: [PATCH V1 1/3] i2c: imx: check busy bit when START/STOP
From: Ben Dooks @ 2009-10-30 0:40 UTC (permalink / raw)
To: Richard Zhao
Cc: kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-i2c-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1255772784-11387-1-git-send-email-linuxzsc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Does anyone have any comments on these, or shall I look at merging
them as fixes?
--
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)
'a smiley only costs 4 bytes'
^ permalink raw reply
* Re: Shared i2c adapter locking (Was: linux-next: manual merge of the net tree with the i2c tree)
From: Ben Hutchings @ 2009-10-29 15:09 UTC (permalink / raw)
To: Jean Delvare
Cc: Stephen Rothwell, David Miller, netdev, linux-next, linux-kernel,
Mika Kuoppala, Linux I2C
In-Reply-To: <20091029154317.651904b9@hyperion.delvare>
On Thu, 2009-10-29 at 15:43 +0100, Jean Delvare wrote:
> Hi Stephen,
>
> On Mon, 26 Oct 2009 13:37:57 +1100, Stephen Rothwell wrote:
> > Today's linux-next merge of the net tree got a conflict in
> > drivers/net/sfc/sfe4001.c between commit
> > 3f7c0648f727a6d5baf6117653e4001dc877b90b ("i2c: Prevent priority
> > inversion on top of bus lock") from the i2c tree and commit
> > c9597d4f89565b6562bd3026adbe6eac6c317f47 ("sfc: Merge sfe4001.c into
> > falcon_boards.c") from the net tree.
> >
> > I have applied the following merge fixup patch (after removing
> > drivers/net/sfc/sfe4001.c) and can carry it as necessary.
>
> Thanks for fixing it. The core problem here IMHO is that the sfc
> network driver touches i2c internals which it would rather leave alone.
I'm just a little proud of having the idea that we could avoid using an
I/O-expander on this board, but yes, the software side of this
multiplexing is a hack.
> This is the only driver I know of which does this.
>
> I can think of 3 different ways to address the issue.
>
> Method #1: add a public API to grab/release an I2C segment.
>
> void i2c_adapter_lock(struct i2c_adapter *adapter)
> {
> rt_mutex_lock(&adapter->bus_lock);
> }
>
> void i2c_adapter_unlock(struct i2c_adapter *adapter)
> {
> rt_mutex_unlock(&adapter->bus_lock);
> }
[...]
> I'm not really sure if I have a preference yet, so please speak up if
> you do.
Indirect lock operations are a recipe for deadlock, and there doesn't
seem to be any other user for this, so method 1 seems best.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Shared i2c adapter locking (Was: linux-next: manual merge of the net tree with the i2c tree)
From: Jean Delvare @ 2009-10-29 14:43 UTC (permalink / raw)
To: Stephen Rothwell
Cc: David Miller, netdev, linux-next, linux-kernel, Mika Kuoppala,
Ben Hutchings, Linux I2C
In-Reply-To: <20091026133757.7cf87e49.sfr@canb.auug.org.au>
Hi Stephen,
On Mon, 26 Oct 2009 13:37:57 +1100, Stephen Rothwell wrote:
> Today's linux-next merge of the net tree got a conflict in
> drivers/net/sfc/sfe4001.c between commit
> 3f7c0648f727a6d5baf6117653e4001dc877b90b ("i2c: Prevent priority
> inversion on top of bus lock") from the i2c tree and commit
> c9597d4f89565b6562bd3026adbe6eac6c317f47 ("sfc: Merge sfe4001.c into
> falcon_boards.c") from the net tree.
>
> I have applied the following merge fixup patch (after removing
> drivers/net/sfc/sfe4001.c) and can carry it as necessary.
Thanks for fixing it. The core problem here IMHO is that the sfc
network driver touches i2c internals which it would rather leave alone.
This is the only driver I know of which does this.
I can think of 3 different ways to address the issue.
Method #1: add a public API to grab/release an I2C segment.
void i2c_adapter_lock(struct i2c_adapter *adapter)
{
rt_mutex_lock(&adapter->bus_lock);
}
void i2c_adapter_unlock(struct i2c_adapter *adapter)
{
rt_mutex_unlock(&adapter->bus_lock);
}
It has the advantage of simplicity. The problem is that it is not
symmetric. Whatever shares the pins with I2C, I2C is considered the
main user in that its mutex is used to guarantee mutual exclusion. If
the other subsystem also has a mandatory locking mechanism, there will
have to be a decision on who is locked first. If not all drivers agree,
lockdep will get confused.
Method #2: let the driver implement its own "main" locking, and have
all pin users (i2c etc.) take it in addition to any subsystem-specific
mutex. As far as the sfc network driver is concerned, that would mean
adding pre-xfer and post-xfer hooks to i2c-algo-bit, where the "main"
mutex in question would be taken resp. released.
It has the advantage of symmetry. The problem is performance, as
regular operation will require to take and release two mutexes instead
of just one. But it is a fairly generic approach which could solve
other issues too.
Method #3: let individual bus drivers implement segment locking
themselves, with custom locking/unlocking hooks. This way, a device
sharing pins between several functions can have its own mutex
protecting these pins globally, and all user subsystems (i2c etc.) use
this single mutex.
It has the advantage of performance and symmetry, at the price of
fragility. If the i2c bus driver implements locking improperly... I am
also worried by inconsistencies that may arise, if different i2c
adapters are protected by different mutex types (mutex vs. real-time
mutex vs. spinlock etc.) but maybe this will be seen as an advantage
rather than a problem.
I'm not really sure if I have a preference yet, so please speak up if
you do.
--
Jean Delvare
^ permalink raw reply
* Re: [PATCH v2 1/2] acpi: support IBM SMBus CMI devices
From: Jean Delvare @ 2009-10-27 17:36 UTC (permalink / raw)
To: djwong-r/Jw6+rmf7HQT0dZR+AlfA
Cc: Crane Cai, Bjorn Helgaas, lenb-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20091027173001.GT26149-bjhdApgbSaxhsM67afOH+sxtgHpCUUYS@public.gmane.org>
On Tue, 27 Oct 2009 10:30:01 -0700, Darrick J. Wong wrote:
> On Tue, Oct 27, 2009 at 06:03:32PM +0100, Jean Delvare wrote:
>
> > I'm only half please with this. You change the function named, but it
> > doesn't follow the calling convention of acpi_dock_match(), which is a
> > little confusing.
> >
> > Anyway, I will need an ack from the ACPI people before I can pick this
> > patch. Or maybe they should even push it upstream themselves.
>
> I am confused. Looking at that bunch of ifs, acpi_is_video_device returns 1
> for a match and 0 for no match. acpi_bay_match returns 0 for a match and
> -ENODEV for no match, which just happens to work with the ACPI_SUCCESS macro.
> acpi_dock_match returns ACPI error codes. Each of the three existing tests has
> different return value semantics, so it is not clear to me which one I should
> use.
Ah, sorry, I looked at one (don't remember which one) and assumed the
other ones followed the same convention.
> I didn't think it was correct for my probe function to use the ACPI_STATUS
> macro unless it returned ACPI error codes... which it does not. -ENODEV seemed
> appropriate for "no device found".
>
> Is it desirable to clean them all up to follow the same convention?
Ideally, yes, but that would be a separate task from what you're up to
at the moment. And anyway this is not in my realm an I am not going to
work on it myself, so my opinion probably doesn't matter that much.
Sorry for bothering you with this in the first place.
--
Jean Delvare
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox