Re: [PATCH 1/4 v2] mfd: add STw481x driver

Re: [PATCH 1/4 v2] mfd: add STw481x driver

[Linus Walleij]

On Mon, Sep 16, 2013 at 12:40 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Sep 16, 2013 at 10:19:56AM +0100, Lee Jones wrote:
>
>> Can't you just NULL .id_table?

No. That is not OK with the I2C core. It's
not easy to get rid of the requirement for .id_table
not to be NULL either.

>> Here's the code which would use it:
>> > /* match on an id table if there is one */
>> > if (driver->id_table)
>> >         return i2c_match_id(driver->id_table, client) != NULL;
>
>> Matching for "dummy" will just waste cycles.
>
> i2c_device_probe() will return -ENODEV if id_table is NULL before we get
> to actually matching.  We could remove that check though...

Copy+pasting from my own reply earlier:

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

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?

Yours,
Linus Walleij

Re: [PATCH 1/4 v2] mfd: add STw481x driver

[Mark Brown]

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

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:

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))

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

Re: [PATCH 1/4 v2] mfd: add STw481x driver

[Wolfram Sang]

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


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.


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

Re: [PATCH 1/4 v2] mfd: add STw481x driver

[Lee Jones]

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?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4 v2] mfd: add STw481x driver

[Wolfram Sang]

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


> 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.


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

Re: [PATCH 1/4 v2] mfd: add STw481x driver

[Lee Jones]

> > 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.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4 v2] mfd: add STw481x driver

[Mark Brown]

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

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).

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

Re: [PATCH 1/4 v2] mfd: add STw481x driver

[Lee Jones]

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.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog