Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1418596 > unrolled thread

Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing Wolfram Sang <wsa@the-dreams.de> - 2016-06-09 21:20 +0200
    Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing Javier Martinez Canillas <javier@osg.samsung.com> - 2016-06-09 21:50 +0200
      Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing Wolfram Sang <wsa@the-dreams.de> - 2016-06-09 22:10 +0200
        Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing Kieran Bingham <kieran@ksquared.org.uk> - 2016-06-10 12:10 +0200
          Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing Wolfram Sang <wsa@the-dreams.de> - 2016-06-10 13:10 +0200
            Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing Kieran Bingham <kieran@ksquared.org.uk> - 2016-06-10 14:10 +0200
              Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing Wolfram Sang <wsa@the-dreams.de> - 2016-06-10 15:40 +0200
                [TEST PATCH] rtc: convert ds1307 to interim probe_new Kieran Bingham <kieran@ksquared.org.uk> - 2016-06-12 23:20 +0200
                  Re: [TEST PATCH] rtc: convert ds1307 to interim probe_new Wolfram Sang <wsa@the-dreams.de> - 2016-06-13 19:20 +0200

#1418596 — Re: [PATCHv5 0/8] 2c: Relax mandatory I2C ID table passing

[Multipart message — attachments visible in raw view] — view raw

Hi Kieran,

> * Device Tree
>   I tested that the device would still register by adding a node in the device
>   tree for the board, and testing with a built-in module. 
> 
>  - This worked fine.
> 
> * Module Autoloading
>   With the device tree node in the board dts file, it wouldn't automatically
>   load from the external module. This was due to the rtc-ds1307 module not
>   exporting an of_match table, and not yet having Javier's "report OF style
>   modalias when probing using DT" [0]  patch applied

What I didn't get here: did your version of the RTC driver use probe()
or probe_new() without i2c_device_id table or did you try both? I assume
module autoloading only fails with probe_new(), otherwise we would be in
serious trouble. But I'd wonder then that userspace instantiation works.

Thanks to you and Javier for the testing. I pushed the patches to a
local branch for now and will merge once this question is clear.

Regards,

   Wolfram

[toc] | [next] | [standalone]

#1418607

Hello Wolfram,

On 06/09/2016 03:15 PM, Wolfram Sang wrote:
> Hi Kieran,
> 
>> * Device Tree
>>   I tested that the device would still register by adding a node in the device
>>   tree for the board, and testing with a built-in module. 
>>
>>  - This worked fine.
>>
>> * Module Autoloading
>>   With the device tree node in the board dts file, it wouldn't automatically
>>   load from the external module. This was due to the rtc-ds1307 module not
>>   exporting an of_match table, and not yet having Javier's "report OF style
>>   modalias when probing using DT" [0]  patch applied
> 
> What I didn't get here: did your version of the RTC driver use probe()
> or probe_new() without i2c_device_id table or did you try both? I assume
> module autoloading only fails with probe_new(), otherwise we would be in
> serious trouble. But I'd wonder then that userspace instantiation works.
>

I can't answer for Kieran but you trimmed this last sentence from him:

>  - With the module updated, and Javiers patch applied, the module autoloads
>

So my understanding is that by updated he meant a patched rtc-ds1307 driver
using a .probe_new, whose i2c_device_id table was removed and of_device_id
table added (that's not present in the mainline driver).

And that's why he needed my RFC patch to report a MODALIAS=of:N*T*Cfoo,bar
and match what's exported to the module using the of_device_id table.

Because drivers that only use .probe and have an i2c_device_id table will
continue to match and report MODALIAS=i2c:foo as before after this series.

> Thanks to you and Javier for the testing. I pushed the patches to a
> local branch for now and will merge once this question is clear.
>
> Regards,
> 
>    Wolfram
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[toc] | [prev] | [next] | [standalone]

#1418620

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jun 09, 2016 at 03:45:52PM -0400, Javier Martinez Canillas wrote:
> Hello Wolfram,
> 
> On 06/09/2016 03:15 PM, Wolfram Sang wrote:
> > Hi Kieran,
> > 
> >> * Device Tree
> >>   I tested that the device would still register by adding a node in the device
> >>   tree for the board, and testing with a built-in module. 
> >>
> >>  - This worked fine.
> >>
> >> * Module Autoloading
> >>   With the device tree node in the board dts file, it wouldn't automatically
> >>   load from the external module. This was due to the rtc-ds1307 module not
> >>   exporting an of_match table, and not yet having Javier's "report OF style
> >>   modalias when probing using DT" [0]  patch applied

Let's call this a)

> > 
> > What I didn't get here: did your version of the RTC driver use probe()
> > or probe_new() without i2c_device_id table or did you try both? I assume
> > module autoloading only fails with probe_new(), otherwise we would be in
> > serious trouble. But I'd wonder then that userspace instantiation works.
> >
> 
> I can't answer for Kieran but you trimmed this last sentence from him:
> 
> >  - With the module updated, and Javiers patch applied, the module autoloads

Let's call this b)

> >
> 
> So my understanding is that by updated he meant a patched rtc-ds1307 driver
> using a .probe_new, whose i2c_device_id table was removed and of_device_id
> table added (that's not present in the mainline driver).
> 
> And that's why he needed my RFC patch to report a MODALIAS=of:N*T*Cfoo,bar
> and match what's exported to the module using the of_device_id table.
> 
> Because drivers that only use .probe and have an i2c_device_id table will
> continue to match and report MODALIAS=i2c:foo as before after this series.

Yes, this is my understanding and expectation, too. However, he wrote
that module loading fails on a) where he never wrote anything about an
updated module before. That comes only later with b). So what I would
have expected:

1) update the module (which is "b)" above)
2) autoloading fails (which is "a)" above)
3) applying your patch
4) everything works

which implies

0) nothing done, everything works

But I don't see this in the text, so I better ask. This also raises the
question open how userspace instantiation was tested. With or without
updating (ideally both).

Thanks,

   Wolfram

[toc] | [prev] | [next] | [standalone]

#1419190

Hi Wolfram,

On 09/06/16 21:04, Wolfram Sang wrote:
> On Thu, Jun 09, 2016 at 03:45:52PM -0400, Javier Martinez Canillas wrote:
>> Hello Wolfram,
>>
>> On 06/09/2016 03:15 PM, Wolfram Sang wrote:
>>> Hi Kieran,
>>>
>>>> * Device Tree
>>>>   I tested that the device would still register by adding a node in the device
>>>>   tree for the board, and testing with a built-in module. 
>>>>
>>>>  - This worked fine.
>>>>
>>>> * Module Autoloading
>>>>   With the device tree node in the board dts file, it wouldn't automatically
>>>>   load from the external module. This was due to the rtc-ds1307 module not
>>>>   exporting an of_match table, and not yet having Javier's "report OF style
>>>>   modalias when probing using DT" [0]  patch applied
> 
> Let's call this a)
> 
>>>
>>> What I didn't get here: did your version of the RTC driver use probe()
>>> or probe_new() without i2c_device_id table or did you try both? I assume
>>> module autoloading only fails with probe_new(), otherwise we would be in
>>> serious trouble. But I'd wonder then that userspace instantiation works.
>>>
>>
>> I can't answer for Kieran but you trimmed this last sentence from him:
>>
>>>  - With the module updated, and Javiers patch applied, the module autoloads

> Let's call this b)
> 
>>>
>>
>> So my understanding is that by updated he meant a patched rtc-ds1307 driver
>> using a .probe_new, whose i2c_device_id table was removed and of_device_id
>> table added (that's not present in the mainline driver).
>>
>> And that's why he needed my RFC patch to report a MODALIAS=of:N*T*Cfoo,bar
>> and match what's exported to the module using the of_device_id table.
>>
>> Because drivers that only use .probe and have an i2c_device_id table will
>> continue to match and report MODALIAS=i2c:foo as before after this series.

Yes, Javier's interpretation is correct here.

> Yes, this is my understanding and expectation, too. However, he wrote
> that module loading fails on a) where he never wrote anything about an
> updated module before. That comes only later with b). So what I would
> have expected:
> 
> 1) update the module (which is "b)" above)
> 2) autoloading fails (which is "a)" above)
> 3) applying your patch
> 4) everything works
> 
> which implies
> 
> 0) nothing done, everything works
> 
> But I don't see this in the text, so I better ask. This also raises the
> question open how userspace instantiation was tested. With or without
> updating (ideally both).

Well it's a month later, and I can't remember - so I've retested.

Kernel built with this patchset - and *without* Javiers patch for 0)
position test.

RTC_DS1307 driver *unmodified*

root@arm:~# uname -a
Linux arm 4.7.0-rc2-00027-g72baec98c9f0 #27 SMP Fri Jun 10 10:42:09 BST
2016 armv7l GNU/Linux

root@arm:~# lsmod
Module                  Size  Used by    Not tainted
omap_rng                3927  0
rng_core                6410  1 omap_rng
rtc_ds1307             12121  0

root@arm:~# cat /sys/class/rtc/rtc0/device/modalias
i2c:ds1307

root@arm:~# cat /sys/class/rtc/rtc0/date
2016-06-10


* Module autoload successful,
* i2c read successful.


Code tested at
repository https://github.com/kbingham/linux.git
tag i2c-dt/v4.7-rc2-relax-conversion-zero-test

Is this what you were looking for?


> Thanks,
> 
>    Wolfram
> 

-- 
Regards

Kieran Bingham

[toc] | [prev] | [next] | [standalone]

#1419266

[Multipart message — attachments visible in raw view] — view raw

> Is this what you were looking for?

Mostly, thanks. This verifies that the old way still works. Good!

The new way (no i2c_device_ids, just compatibles) will need Javier's
patch to work with module auto loading, I know. But what about userspace
instantiation with built-in driver? I didn't understand if this was
tested using the new way. And do you need then the full-compatible or
the vendor-stripped string?

Thanks,

   Wolfram

[toc] | [prev] | [next] | [standalone]

#1419317

Hi Wolfram,

On 10/06/16 12:00, Wolfram Sang wrote:
> 
>> Is this what you were looking for?
> 
> Mostly, thanks. This verifies that the old way still works. Good!
> 
> The new way (no i2c_device_ids, just compatibles) will need Javier's
> patch to work with module auto loading, I know. But what about userspace
> instantiation with built-in driver? I didn't understand if this was
> tested using the new way. And do you need then the full-compatible or
> the vendor-stripped string?

When I reported :

> * new_device (built-in, and external module)
> echo ds1307 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
> cat /sys/class/rtc/rtc0/date
> 
> - Both of those worked fine.

That was *without* Javier's patch, but hopefully obviously *with* Lee's
patchset.

Do you need this testing *with* Javiers patch as well?
I have not tested this combination.

I've already switched my dev board environment back to the Salvator-X
for $DAYJOB and switching back now will have to wait to the weekend.


> Thanks,
> 
>    Wolfram
> 

-- 
Regards

Kieran Bingham

[toc] | [prev] | [next] | [standalone]

#1419426

[Multipart message — attachments visible in raw view] — view raw

> When I reported :
> 
> > * new_device (built-in, and external module)
> > echo ds1307 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
> > cat /sys/class/rtc/rtc0/date
> > 
> > - Both of those worked fine.
> 
> That was *without* Javier's patch, but hopefully obviously *with* Lee's
> patchset.
> 
> Do you need this testing *with* Javiers patch as well?

Without Javiers patch. But with a modified rtc driver which will only
use proper compatibles, no i2c_device_ids, and probe_new. And this one
should be able to instantiate via userspace with the driver builtin.

We have documented ways of instantiating. All I ask for is to make sure
the old style and new style work with them. Check the attached sketch
for an example (only compile-tested and conversion to probe_new is
missing). Can you instantiate the "maxim,ds1307" (with the additional
printout) and "dallas,ds1307" via userspace?

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 821d9c089cdb48..10a4e5bc923e07 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -31,6 +31,7 @@
  */
 enum ds_type {
 	ds_1307,
+	maxim_1307,
 	ds_1337,
 	ds_1338,
 	ds_1339,
@@ -144,6 +145,10 @@ static struct chip_desc chips[last_ds_type] = {
 		.nvram_offset	= 8,
 		.nvram_size	= 56,
 	},
+	[maxim_1307] = {
+		.nvram_offset	= 8,
+		.nvram_size	= 56,
+	},
 	[ds_1337] = {
 		.alarm		= 1,
 	},
@@ -173,23 +178,6 @@ static struct chip_desc chips[last_ds_type] = {
 	},
 };
 
-static const struct i2c_device_id ds1307_id[] = {
-	{ "ds1307", ds_1307 },
-	{ "ds1337", ds_1337 },
-	{ "ds1338", ds_1338 },
-	{ "ds1339", ds_1339 },
-	{ "ds1388", ds_1388 },
-	{ "ds1340", ds_1340 },
-	{ "ds3231", ds_3231 },
-	{ "m41t00", m41t00 },
-	{ "mcp7940x", mcp794xx },
-	{ "mcp7941x", mcp794xx },
-	{ "pt7c4338", ds_1307 },
-	{ "rx8025", rx_8025 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, ds1307_id);
-
 /*----------------------------------------------------------------------*/
 
 #define BLOCK_DATA_MAX_TRIES 10
@@ -1435,6 +1423,9 @@ read_rtc:
 	 */
 	tmp = ds1307->regs[DS1307_REG_SECS];
 	switch (ds1307->type) {
+	case maxim_1307:
+		dev_info(&client->dev, "I'm a Maxim\n");
+		/* fallthrough */
 	case ds_1307:
 	case m41t00:
 		/* clock halted?  turn it on, so clock can tick. */
@@ -1610,13 +1601,22 @@ static int ds1307_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id ds1307_dt_ids[] = {
+	{ .compatible = "dallas,ds1307", .data = (void *)ds_1307 },
+	{ .compatible = "maxim,ds1307", .data = (void *)maxim_1307 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ds1302_dt_ids);
+#endif
+
 static struct i2c_driver ds1307_driver = {
 	.driver = {
 		.name	= "rtc-ds1307",
+		.of_match_table = of_match_ptr(ds1307_dt_ids),
 	},
 	.probe		= ds1307_probe,
 	.remove		= ds1307_remove,
-	.id_table	= ds1307_id,
 };
 
 module_i2c_driver(ds1307_driver);

[toc] | [prev] | [next] | [standalone]

#1420330 — [TEST PATCH] rtc: convert ds1307 to interim probe_new

Just for testing, specify a ds9999 device to identify the code path used when
instantiating the driver from userspace.

As we match on only the device, not the manufacturer, I've changed your sketch
so that the test maxim line is on a compatible with name ds9999 to make it
unique. Otherwise, we would match to the dallas,ds1307 id type.

If you would prefer that we support separate manufacturers, I can update the
match function so that it attempts a full match first, followed by a stripped
manufacturer match. I'm not certain if we have a need for that at the moment
though, as the current drivers simply match on the device name:

This patch also demonstrates a method to obtain the device ID from the new
match system for drivers which would normally have expected this information to
be passed in.


Testing:
=======-

root@arm:~# echo ds9999 0x68 > /sys/bus/i2c/devices/i2c-2/new_device
[ 43.262432] rtc-ds1307 2-0068: I'm a Maxim ...
[ 43.268707] rtc-ds1307 2-0068: rtc core: registered ds9999 as rtc0
[ 43.275276] rtc-ds1307 2-0068: 56 bytes nvram
[ 43.279920] i2c i2c-2: new_device: Instantiated device ds9999 at 0x68

root@arm:~# cat /sys/class/rtc/rtc0/date 
2016-06-12

root@arm:~# cat /sys/class/rtc/rtc0/name 
ds9999


---
 drivers/rtc/rtc-ds1307.c | 60 ++++++++++++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 821d9c089cdb..d97e8adb866b 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -31,6 +31,7 @@
  */
 enum ds_type {
 	ds_1307,
+	maxim_1307,
 	ds_1337,
 	ds_1338,
 	ds_1339,
@@ -144,6 +145,10 @@ static struct chip_desc chips[last_ds_type] = {
 		.nvram_offset	= 8,
 		.nvram_size	= 56,
 	},
+	[maxim_1307] = {
+		.nvram_offset	= 8,
+		.nvram_size	= 56,
+	},
 	[ds_1337] = {
 		.alarm		= 1,
 	},
@@ -173,22 +178,6 @@ static struct chip_desc chips[last_ds_type] = {
 	},
 };
 
-static const struct i2c_device_id ds1307_id[] = {
-	{ "ds1307", ds_1307 },
-	{ "ds1337", ds_1337 },
-	{ "ds1338", ds_1338 },
-	{ "ds1339", ds_1339 },
-	{ "ds1388", ds_1388 },
-	{ "ds1340", ds_1340 },
-	{ "ds3231", ds_3231 },
-	{ "m41t00", m41t00 },
-	{ "mcp7940x", mcp794xx },
-	{ "mcp7941x", mcp794xx },
-	{ "pt7c4338", ds_1307 },
-	{ "rx8025", rx_8025 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, ds1307_id);
 
 /*----------------------------------------------------------------------*/
 
@@ -1226,13 +1215,27 @@ static void ds1307_clks_register(struct ds1307 *ds1307)
 
 #endif /* CONFIG_COMMON_CLK */
 
-static int ds1307_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static const struct of_device_id ds1307_dt_ids[] = {
+	/* We are only matching on the device name, *NOT* the manufacturer name
+	 * I.e. dallas, maxim, are dropped in the search when someone tries to load a
+	 * 'ds1307', and hence first match wins.
+	 *
+	 * We could extend this to do a full match first, followed by a fallback match
+	 * to just the device name.
+	 */
+	{ .compatible = "dallas,ds1307", .data = (void *)ds_1307 },
+	{ .compatible = "maxim,ds9999", .data = (void *)maxim_1307 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ds1307_dt_ids);
+
+static int ds1307_probe(struct i2c_client *client)
 {
 	struct ds1307		*ds1307;
 	int			err = -ENODEV;
 	int			tmp;
-	struct chip_desc	*chip = &chips[id->driver_data];
+	const struct of_device_id 	*idof;
+	struct chip_desc	*chip;
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
 	bool			want_irq = false;
 	bool			ds1307_can_wakeup_device = false;
@@ -1255,10 +1258,20 @@ static int ds1307_probe(struct i2c_client *client,
 	if (!ds1307)
 		return -ENOMEM;
 
+	/* If we've got this far, this shouldn't be able to fail - but check anyway for now */
+	idof = i2c_of_match_device(ds1307_dt_ids, client);
+	if (!idof) {
+		dev_err(&client->dev, "Probe failed to find an id entry\n");
+		return -ENODEV;
+	}
+
+	/* Now we can set our chip entry */
+	chip = &chips[(int)idof->data];
+
 	i2c_set_clientdata(client, ds1307);
 
 	ds1307->client	= client;
-	ds1307->type	= id->driver_data;
+	ds1307->type	= (int) idof->data;
 
 	if (!pdata && client->dev.of_node)
 		ds1307_trickle_of_init(client, chip);
@@ -1435,6 +1448,9 @@ read_rtc:
 	 */
 	tmp = ds1307->regs[DS1307_REG_SECS];
 	switch (ds1307->type) {
+	case maxim_1307:
+		dev_info(&client->dev, "I'm a Maxim ... \n");
+		/* fallthrough */
 	case ds_1307:
 	case m41t00:
 		/* clock halted?  turn it on, so clock can tick. */
@@ -1613,10 +1629,10 @@ static int ds1307_remove(struct i2c_client *client)
 static struct i2c_driver ds1307_driver = {
 	.driver = {
 		.name	= "rtc-ds1307",
+		.of_match_table = of_match_ptr(ds1307_dt_ids),
 	},
-	.probe		= ds1307_probe,
+	.probe_new	= ds1307_probe,
 	.remove		= ds1307_remove,
-	.id_table	= ds1307_id,
 };
 
 module_i2c_driver(ds1307_driver);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1421064 — Re: [TEST PATCH] rtc: convert ds1307 to interim probe_new

[Multipart message — attachments visible in raw view] — view raw

> As we match on only the device, not the manufacturer, I've changed your sketch
> so that the test maxim line is on a compatible with name ds9999 to make it
> unique. Otherwise, we would match to the dallas,ds1307 id type.

OK, so I assumed correctly that userspace instantiation wouldn't have
worked fully.

> If you would prefer that we support separate manufacturers, I can update the
> match function so that it attempts a full match first, followed by a stripped
> manufacturer match. I'm not certain if we have a need for that at the moment
> though, as the current drivers simply match on the device name:

I think we *need* that. We can't instantiate my previous example via
userspace otherwise. Also, stripping vendor names is what we want to get
rid of with this series, no?

> This patch also demonstrates a method to obtain the device ID from the new
> match system for drivers which would normally have expected this information to
> be passed in.

Thanks for the testing!

   Wolfram

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web