diff mbox

[TEST] rtc: convert ds1307 to interim probe_new

Message ID 1465766027-485-1-git-send-email-kieran@bingham.xyz
State Not Applicable
Headers show

Commit Message

Kieran Bingham June 12, 2016, 9:13 p.m. UTC 
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:

Comments

kernel test robot June 12, 2016, 9:26 p.m. UTC | #1 
Hi,

[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on v4.7-rc3 next-20160609]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kieran-Bingham/rtc-convert-ds1307-to-interim-probe_new/20160613-051618
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All error/warnings (new ones prefixed by >>):

   drivers/rtc/rtc-ds1307.c: In function 'ds1307_probe':
>> drivers/rtc/rtc-ds1307.c:1262:9: error: implicit declaration of function 'i2c_of_match_device' [-Werror=implicit-function-declaration]
     idof = i2c_of_match_device(ds1307_dt_ids, client);
            ^
>> drivers/rtc/rtc-ds1307.c:1262:7: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     idof = i2c_of_match_device(ds1307_dt_ids, client);
          ^
>> drivers/rtc/rtc-ds1307.c:1269:16: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     chip = &chips[(int)idof->data];
                   ^
   drivers/rtc/rtc-ds1307.c:1274:17: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     ds1307->type = (int) idof->data;
                    ^
   drivers/rtc/rtc-ds1307.c: At top level:
>> drivers/rtc/rtc-ds1307.c:1634:2: error: unknown field 'probe_new' specified in initializer
     .probe_new = ds1307_probe,
     ^
>> drivers/rtc/rtc-ds1307.c:1634:15: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
     .probe_new = ds1307_probe,
                  ^
   drivers/rtc/rtc-ds1307.c:1634:15: note: (near initialization for 'ds1307_driver.id_table')
   cc1: some warnings being treated as errors

vim +/i2c_of_match_device +1262 drivers/rtc/rtc-ds1307.c

  1256	
  1257		ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
  1258		if (!ds1307)
  1259			return -ENOMEM;
  1260	
  1261		/* If we've got this far, this shouldn't be able to fail - but check anyway for now */
> 1262		idof = i2c_of_match_device(ds1307_dt_ids, client);
  1263		if (!idof) {
  1264			dev_err(&client->dev, "Probe failed to find an id entry\n");
  1265			return -ENODEV;
  1266		}
  1267	
  1268		/* Now we can set our chip entry */
> 1269		chip = &chips[(int)idof->data];
  1270	
  1271		i2c_set_clientdata(client, ds1307);
  1272	
  1273		ds1307->client	= client;
> 1274		ds1307->type	= (int) idof->data;
  1275	
  1276		if (!pdata && client->dev.of_node)
  1277			ds1307_trickle_of_init(client, chip);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot June 12, 2016, 9:26 p.m. UTC | #2 
Hi,

[auto build test WARNING on abelloni/rtc-next]
[also build test WARNING on v4.7-rc3 next-20160609]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kieran-Bingham/rtc-convert-ds1307-to-interim-probe_new/20160613-051618
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: tile-allmodconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 4.6.2
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=tile 

All warnings (new ones prefixed by >>):

   drivers/rtc/rtc-ds1307.c: In function 'ds1307_probe':
   drivers/rtc/rtc-ds1307.c:1262:2: error: implicit declaration of function 'i2c_of_match_device'
   drivers/rtc/rtc-ds1307.c:1262:7: warning: assignment makes pointer from integer without a cast [enabled by default]
   drivers/rtc/rtc-ds1307.c:1269:16: warning: cast from pointer to integer of different size
   drivers/rtc/rtc-ds1307.c:1274:17: warning: cast from pointer to integer of different size
   drivers/rtc/rtc-ds1307.c: At top level:
   drivers/rtc/rtc-ds1307.c:1634:2: error: unknown field 'probe_new' specified in initializer
>> drivers/rtc/rtc-ds1307.c:1634:2: warning: initialization from incompatible pointer type [enabled by default]
   drivers/rtc/rtc-ds1307.c:1634:2: warning: (near initialization for 'ds1307_driver.id_table') [enabled by default]
   cc1: some warnings being treated as errors

vim +1634 drivers/rtc/rtc-ds1307.c

  1618	
  1619	static int ds1307_remove(struct i2c_client *client)
  1620	{
  1621		struct ds1307 *ds1307 = i2c_get_clientdata(client);
  1622	
  1623		if (test_and_clear_bit(HAS_NVRAM, &ds1307->flags))
  1624			sysfs_remove_bin_file(&client->dev.kobj, ds1307->nvram);
  1625	
  1626		return 0;
  1627	}
  1628	
  1629	static struct i2c_driver ds1307_driver = {
  1630		.driver = {
  1631			.name	= "rtc-ds1307",
  1632			.of_match_table = of_match_ptr(ds1307_dt_ids),
  1633		},
> 1634		.probe_new	= ds1307_probe,
  1635		.remove		= ds1307_remove,
  1636	};
  1637	
  1638	module_i2c_driver(ds1307_driver);
  1639	
  1640	MODULE_DESCRIPTION("RTC driver for DS1307 and similar chips");
  1641	MODULE_LICENSE("GPL");

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Wolfram Sang June 13, 2016, 5:13 p.m. UTC | #3 
> 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
Kieran Bingham July 11, 2016, 9:13 a.m. UTC | #4 
Hi Wolfram,

On 13/06/16 18:13, Wolfram Sang wrote:
> 
>> 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?

Awww <multiple expletives removed>

I just re-read this - Has this series been blocked on me without me
realising?

/me cowers in a corner in shame...

I think the aim of this series was to simplify the code structures.

Making the vendor names usable, makes sense IMO, but I don't think that
was a goal of the original series.

I think the series was trying to make an improvement *without* changing
functionality / usage.

Re-introducing vendor names into the i2c matching would be a change in
scope from my view, but if it's required to get this series moving let
me know.

>> 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
>
diff mbox

Patch

=======-

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