Patchwork [1/4,v2] mfd: add STw481x driver

login
register
mail settings
Submitter Mark Brown
Date Sept. 16, 2013, 1:51 p.m.
Message ID <20130916135118.GD29403@sirena.org.uk>
Download mbox | patch
Permalink /patch/275223/
State RFC
Headers show

Comments

Mark Brown - Sept. 16, 2013, 1:51 p.m.
On Mon, Sep 16, 2013 at 02:44:35PM +0200, Linus Walleij wrote:

> I've tried to fix this for DT-only I2C devices
> and this very driver was the reason.

> But a tiresome regression due to drivers relying on this
> i2c_device_id not being NULL and inability to remove it from the I2C
> core without refactoring the world ensued, see:
> commit c80f52847c50109ca248c22efbf71ff10553dca4

Oh, that was the change...

> Reverted in:
> commit 661f6c1cd926c6c973e03c6b5151d161f3a666ed

> For this reason I think:
> http://marc.info/?l=linux-next&m=137148411231784&w=2

> I have tentatively given up getting pure DT I2C drivers
> to probe, I don't think I have the whole picture, but
> Wolfram has serious doubts about this and say we have
> to be careful ....

> Wolfram, do you have some ideas on how we should
> proceed or ar you happy with merging this as-is?

I'd have expected that it should be possible to change things such that
the change in the core doesn't produce any change in behaviour for
existing drivers.  Can we not change the patch so that i2c_match_id()
copes with getting a NULL id_table?  Something like this:
Wolfram Sang - Sept. 16, 2013, 3:21 p.m.
On Mon, Sep 16, 2013 at 02:51:18PM +0100, Mark Brown wrote:
> On Mon, Sep 16, 2013 at 02:44:35PM +0200, Linus Walleij wrote:
> 
> > I've tried to fix this for DT-only I2C devices
> > and this very driver was the reason.
> 
> > But a tiresome regression due to drivers relying on this
> > i2c_device_id not being NULL and inability to remove it from the I2C
> > core without refactoring the world ensued, see:
> > commit c80f52847c50109ca248c22efbf71ff10553dca4
> 
> Oh, that was the change...
> 
> > Reverted in:
> > commit 661f6c1cd926c6c973e03c6b5151d161f3a666ed
> 
> > For this reason I think:
> > http://marc.info/?l=linux-next&m=137148411231784&w=2
> 
> > I have tentatively given up getting pure DT I2C drivers
> > to probe, I don't think I have the whole picture, but
> > Wolfram has serious doubts about this and say we have
> > to be careful ....
> 
> > Wolfram, do you have some ideas on how we should
> > proceed or ar you happy with merging this as-is?
> 
> I'd have expected that it should be possible to change things such that
> the change in the core doesn't produce any change in behaviour for
> existing drivers.  Can we not change the patch so that i2c_match_id()
> copes with getting a NULL id_table?  Something like this:

I hacked something like this after Linus posted his approach. However, I
found out that run time instanciating ('new_device' file) needs an
id_table. I wasn't to keen on disabling the feature for dt-only drivers.
That's where I stopped, due to lack of time.
Lee Jones - Sept. 16, 2013, 3:34 p.m.
On Mon, 16 Sep 2013, Wolfram Sang wrote:

> 
> On Mon, Sep 16, 2013 at 02:51:18PM +0100, Mark Brown wrote:
> > On Mon, Sep 16, 2013 at 02:44:35PM +0200, Linus Walleij wrote:
> > 
> > > I've tried to fix this for DT-only I2C devices
> > > and this very driver was the reason.
> > 
> > > But a tiresome regression due to drivers relying on this
> > > i2c_device_id not being NULL and inability to remove it from the I2C
> > > core without refactoring the world ensued, see:
> > > commit c80f52847c50109ca248c22efbf71ff10553dca4
> > 
> > Oh, that was the change...
> > 
> > > Reverted in:
> > > commit 661f6c1cd926c6c973e03c6b5151d161f3a666ed
> > 
> > > For this reason I think:
> > > http://marc.info/?l=linux-next&m=137148411231784&w=2
> > 
> > > I have tentatively given up getting pure DT I2C drivers
> > > to probe, I don't think I have the whole picture, but
> > > Wolfram has serious doubts about this and say we have
> > > to be careful ....
> > 
> > > Wolfram, do you have some ideas on how we should
> > > proceed or ar you happy with merging this as-is?
> > 
> > I'd have expected that it should be possible to change things such that
> > the change in the core doesn't produce any change in behaviour for
> > existing drivers.  Can we not change the patch so that i2c_match_id()
> > copes with getting a NULL id_table?  Something like this:
> 
> I hacked something like this after Linus posted his approach. However, I
> found out that run time instanciating ('new_device' file) needs an
> id_table. I wasn't to keen on disabling the feature for dt-only drivers.
> That's where I stopped, due to lack of time.

So in the mean time are you happy with this "dummy" approach?
Wolfram Sang - Sept. 16, 2013, 4 p.m.
> So in the mean time are you happy with this "dummy" approach?

No. "dummy" is reserved for a dummy device in case an i2c slave needs
more than one address. The proper solution would be if
i2c_sysfs_new_device() could recognize the of_device_ids.
Lee Jones - Sept. 16, 2013, 4:05 p.m.
> > So in the mean time are you happy with this "dummy" approach?
> 
> No. "dummy" is reserved for a dummy device in case an i2c slave needs
> more than one address. The proper solution would be if
> i2c_sysfs_new_device() could recognize the of_device_ids.

Okay, thanks for clarifying.
Mark Brown - Sept. 16, 2013, 4:24 p.m.
On Mon, Sep 16, 2013 at 05:05:37PM +0100, Lee Jones wrote:
> > > So in the mean time are you happy with this "dummy" approach?

> > No. "dummy" is reserved for a dummy device in case an i2c slave needs
> > more than one address. The proper solution would be if
> > i2c_sysfs_new_device() could recognize the of_device_ids.

> Okay, thanks for clarifying.

Or for now to use something like stw481x (even if it's never expected to
actually make a difference).
Lee Jones - Sept. 17, 2013, 8:06 a.m.
On Mon, 16 Sep 2013, Mark Brown wrote:

> On Mon, Sep 16, 2013 at 05:05:37PM +0100, Lee Jones wrote:
> > > > So in the mean time are you happy with this "dummy" approach?
> 
> > > No. "dummy" is reserved for a dummy device in case an i2c slave needs
> > > more than one address. The proper solution would be if
> > > i2c_sysfs_new_device() could recognize the of_device_ids.
> 
> > Okay, thanks for clarifying.
> 
> Or for now to use something like stw481x (even if it's never expected to
> actually make a difference).

If Linus wants to resubmit, I'm happy with that.

Patch

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 29d3f04..61087ea 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -67,6 +67,9 @@  static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver);
 static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
 						const struct i2c_client *client)
 {
+	if (!id)
+		return NULL;
+
 	while (id->name[0]) {
 		if (strcmp(client->name, id->name) == 0)
 			return id;
@@ -92,11 +95,8 @@  static int i2c_device_match(struct device *dev, struct device_driver *drv)
 		return 1;
 
 	driver = to_i2c_driver(drv);
-	/* match on an id table if there is one */
-	if (driver->id_table)
-		return i2c_match_id(driver->id_table, client) != NULL;
 
-	return 0;
+	return i2c_match_id(driver->id_table, client) != NULL;
 }
 
 
@@ -246,7 +246,7 @@  static int i2c_device_probe(struct device *dev)
 		return 0;
 
 	driver = to_i2c_driver(dev->driver);
-	if (!driver->probe || !driver->id_table)
+	if (!driver->probe)
 		return -ENODEV;
 	client->driver = driver;
 	if (!device_can_wakeup(&client->dev))