[PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Alexander Holler]

This makes it possible to define i2c-devices at the kernel command line
or as a module parameter for bus-drivers which want to offer such
an functionality.

Drivers which are using it will have the a parameter named
devices with format devname1@addr1,devname2@addr2,...
e.g. devices=ds1307@0x68,pcf8563@0x51

The devices will be probed using the standard probe mechanism,
the definition of up to 8 devices is allowed.

Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Till Harbaum <till-RcHadlBFbzVAfugRpC6u6w@public.gmane.org>
Signed-off-by: Alexander Holler <holler-SXC+2es9fhnfWeYVQQPykw@public.gmane.org>
---
 Documentation/i2c/instantiating-devices | 37 ++++++++++++++++++++++++++++++
 drivers/i2c/i2c-core.c                  | 40 +++++++++++++++++++++++++++++++++
 include/linux/i2c.h                     | 14 ++++++++++++
 3 files changed, 91 insertions(+)

diff --git a/Documentation/i2c/instantiating-devices b/Documentation/i2c/instantiating-devices
index abf6361..1dbfdf3 100644
--- a/Documentation/i2c/instantiating-devices
+++ b/Documentation/i2c/instantiating-devices
@@ -209,3 +209,40 @@ device driver individually, it is much more efficient, and also has the
 advantage that you do not have to reload the driver to change a setting.
 You can also instantiate the device before the driver is loaded or even
 available, and you don't need to know what driver the device needs.
+
+
+Method 5: By module parameter
+-----------------------------
+
+If you want to add the possibility for user-defined optional devices to
+your i2c-bus driver, do it like that in the source of the driver:
+
+(...)
+MODULE_PARAM_I2C_OPTIONAL_DEVICES(opt_devices);
+(...)
+
+static int i2c_my_bus_probe(...)
+{
+	(...)
+	i2c_add_adapter(&dev->adapter);
+	(...)
+
+	i2c_add_optional_devices(&dev->adapter, opt_devices);
+
+	return 0;
+}
+(...)
+
+The call to i2c_add_optional_devices() should only be done if and after
+the driver is registered. opt_devices will be used as a name for a
+static array of pointers to char (for usage with module_param_array_named()
+in the macro MODULE_PARAM_I2C_OPTIONAL_DEVICES).
+
+This interfaces offers users the possibility to define devices by using a
+module parameter. E.g.
+
+modprobe my_i2c_bus devices=ds1307@0x68,pcf8563@0x51
+
+or even at the kernel command line with
+
+my_i2c_bus.devices=ds1307@0x68,pcf8563@0x51
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a7edf98..7d84bc40 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2204,6 +2204,46 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
 }
 EXPORT_SYMBOL(i2c_smbus_xfer);
 
+void i2c_add_optional_devices(struct i2c_adapter *adapter, char **opt_devices)
+{
+	int i;
+	struct i2c_board_info i2c_info;
+	uint addr;
+	unsigned short i2c_addr[] = { 0, I2C_CLIENT_END };
+	char *at;
+
+	for (i = 0; opt_devices[i]; ++i) {
+		at = strchr(opt_devices[i], '@');
+		if (at == NULL) {
+			dev_warn(&adapter->dev,
+				"address need in device definition '%s'\n",
+				opt_devices[i]);
+			continue;
+		}
+		*at++ = 0;
+		if (kstrtouint(at, 0, &addr) || addr >= I2C_CLIENT_END) {
+			*--at = '@';
+			dev_warn(&adapter->dev,
+				"wrong address in slave definition '%s'\n",
+				opt_devices[i]);
+			continue;
+		}
+		memset(&i2c_info, 0, sizeof(struct i2c_board_info));
+		strlcpy(i2c_info.type, opt_devices[i], I2C_NAME_SIZE);
+		*--at = '@'; /* if someone uses opt_devices afterwards */
+		i2c_addr[0] = addr;
+		if (i2c_new_probed_device(adapter, &i2c_info, i2c_addr, NULL))
+			dev_info(&adapter->dev,
+				"device %s at address 0x%02x registered\n",
+				i2c_info.type, addr);
+		else
+			dev_warn(&adapter->dev,
+				"device %s at address 0x%02x not found\n",
+				i2c_info.type, addr);
+	}
+}
+EXPORT_SYMBOL(i2c_add_optional_devices);
+
 MODULE_AUTHOR("Simon G. Vogl <simon-nD9nYVNVf00W+b/DJNNodF6hYfS7NtTn@public.gmane.org>");
 MODULE_DESCRIPTION("I2C-Bus main module");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 800de22..bd0cbb1 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -489,6 +489,20 @@ static inline int i2c_adapter_id(struct i2c_adapter *adap)
 }
 
 /**
+ * Used for user-defined optional devices, see
+ * Documentation/i2c/instantiating-devices about how to use it.
+ */
+
+extern void i2c_add_optional_devices(struct i2c_adapter *adapter,
+				     char **opt_devices);
+
+#define MAX_OPTIONAL_I2C_DEVICES 8
+#define MODULE_PARAM_I2C_OPTIONAL_DEVICES(_opt_devices) \
+	static char *_opt_devices[MAX_OPTIONAL_I2C_DEVICES]; \
+	module_param_array_named(devices, _opt_devices, charp, NULL, 0); \
+	MODULE_PARM_DESC(devices, "devname1@adr1,... (e.g. ds1307@0x68)")
+
+/**
  * module_i2c_driver() - Helper macro for registering a I2C driver
  * @__i2c_driver: i2c_driver struct
  *
-- 
1.7.11.7

[PATCH 2/2] i2c: i2c-tiny-usb: Add parameter for optional user-defined devices.

[Alexander Holler]

Now there is an example about how to use this functionality.

Cc: Jean Delvare <khali@linux-fr.org>
Cc: Till Harbaum <till@harbaum.org>
Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 Documentation/i2c/instantiating-devices | 2 ++
 drivers/i2c/busses/i2c-tiny-usb.c       | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/Documentation/i2c/instantiating-devices b/Documentation/i2c/instantiating-devices
index 1dbfdf3..783a79f 100644
--- a/Documentation/i2c/instantiating-devices
+++ b/Documentation/i2c/instantiating-devices
@@ -246,3 +246,5 @@ modprobe my_i2c_bus devices=ds1307@0x68,pcf8563@0x51
 or even at the kernel command line with
 
 my_i2c_bus.devices=ds1307@0x68,pcf8563@0x51
+
+See the i2c-tiny-usb driver for an exmaple.
diff --git a/drivers/i2c/busses/i2c-tiny-usb.c b/drivers/i2c/busses/i2c-tiny-usb.c
index 0510636..9fd03c6 100644
--- a/drivers/i2c/busses/i2c-tiny-usb.c
+++ b/drivers/i2c/busses/i2c-tiny-usb.c
@@ -40,6 +40,8 @@ module_param(delay, ushort, 0);
 MODULE_PARM_DESC(delay, "bit delay in microseconds "
 		 "(default is 10us for 100kHz max)");
 
+MODULE_PARAM_I2C_OPTIONAL_DEVICES(opt_devices);
+
 static int usb_read(struct i2c_adapter *adapter, int cmd,
 		    int value, int index, void *data, int len);
 
@@ -236,6 +238,8 @@ static int i2c_tiny_usb_probe(struct usb_interface *interface,
 	/* inform user about successful attachment to i2c layer */
 	dev_info(&dev->adapter.dev, "connected i2c-tiny-usb device\n");
 
+	i2c_add_optional_devices(&dev->adapter, opt_devices);
+
 	return 0;
 
  error:
-- 
1.7.11.7

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Jean Delvare]

On Tue, 13 Nov 2012 19:06:07 +0100, Alexander Holler wrote:
> This makes it possible to define i2c-devices at the kernel command line
> or as a module parameter for bus-drivers which want to offer such
> an functionality.
> 
> Drivers which are using it will have the a parameter named
> devices with format devname1@addr1,devname2@addr2,...
> e.g. devices=ds1307@0x68,pcf8563@0x51

No, no, no. We did that 10 years ago, killed all the code 3 years ago
[1], let's not do the same mistake again, please. We have a sysfs
interface for instantiating clients dynamically from user-space, it's
way more powerful and flexible than your proposal. Just try plugging two
different i2c-tiny-usb adapters on the same system and see the new code
instantiate the wrong devices...

[1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=7f508118b1c1f9856a1c899a2bd4867a962b0225

> The devices will be probed using the standard probe mechanism,
> the definition of up to 8 devices is allowed.
> 
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Till Harbaum <till-RcHadlBFbzVAfugRpC6u6w@public.gmane.org>
> Signed-off-by: Alexander Holler <holler-SXC+2es9fhnfWeYVQQPykw@public.gmane.org>
> ---
>  Documentation/i2c/instantiating-devices | 37 ++++++++++++++++++++++++++++++
>  drivers/i2c/i2c-core.c                  | 40 +++++++++++++++++++++++++++++++++
>  include/linux/i2c.h                     | 14 ++++++++++++
>  3 files changed, 91 insertions(+)
> (...)

-- 
Jean Delvare

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Alexander Holler]

Am 13.11.2012 19:55, schrieb Jean Delvare:
> On Tue, 13 Nov 2012 19:06:07 +0100, Alexander Holler wrote:
>> This makes it possible to define i2c-devices at the kernel command line
>> or as a module parameter for bus-drivers which want to offer such
>> an functionality.
>>
>> Drivers which are using it will have the a parameter named
>> devices with format devname1@addr1,devname2@addr2,...
>> e.g. devices=ds1307@0x68,pcf8563@0x51
>
> No, no, no. We did that 10 years ago, killed all the code 3 years ago
> [1], let's not do the same mistake again, please. We have a sysfs
> interface for instantiating clients dynamically from user-space, it's
> way more powerful and flexible than your proposal. Just try plugging two

So how do I define a device, e.g. an RTC which I want to have alive 
before userland starts? Currently imho not possible.

> different i2c-tiny-usb adapters on the same system and see the new code
> instantiate the wrong devices...

I know about that, but it probes and you don't have to use that 
parameter at all. I think some people can think for their self, 
especially those how would use such a parameter.

> [1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=7f508118b1c1f9856a1c899a2bd4867a962b0225

I think my patch looks nicier.

Alexander

[PATCH] i2c: i2c-tiny-usb: Add parameter for optional i2c-devices.

[Alexander Holler]

Make it possible to define i2c-devices at the kernel command line
or as a module parameter.

Format is devname1@addr1,devname2@addr2,...

Example for the kernel command line:

   i2c-tiny-usb.devices=ds1307@0x68,pcf8563@0x51

The devices will be probed using the standard probe mechanism,
the definition of up to 8 devices is allowed.

Cc: Till Harbaum <till@harbaum.org>
Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 drivers/i2c/busses/i2c-tiny-usb.c | 40 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tiny-usb.c b/drivers/i2c/busses/i2c-tiny-usb.c
index 0510636..2096092 100644
--- a/drivers/i2c/busses/i2c-tiny-usb.c
+++ b/drivers/i2c/busses/i2c-tiny-usb.c
@@ -40,6 +40,11 @@ module_param(delay, ushort, 0);
 MODULE_PARM_DESC(delay, "bit delay in microseconds "
 		 "(default is 10us for 100kHz max)");
 
+#define MAX_OPTIONAL_I2C_DEVICES 8
+static char *opt_devices[MAX_OPTIONAL_I2C_DEVICES];
+module_param_array_named(devices, opt_devices, charp, NULL, 0);
+MODULE_PARM_DESC(devices, "devname1@adr1,devname2@adr2,... (e.g. ds1307@0x68)");
+
 static int usb_read(struct i2c_adapter *adapter, int cmd,
 		    int value, int index, void *data, int len);
 
@@ -190,6 +195,11 @@ static int i2c_tiny_usb_probe(struct usb_interface *interface,
 	struct i2c_tiny_usb *dev;
 	int retval = -ENOMEM;
 	u16 version;
+	unsigned i;
+	struct i2c_board_info i2c_info;
+	uint addr;
+	unsigned short i2c_addr[] = { 0, I2C_CLIENT_END };
+	char *at;
 
 	dev_dbg(&interface->dev, "probing usb device\n");
 
@@ -236,6 +246,36 @@ static int i2c_tiny_usb_probe(struct usb_interface *interface,
 	/* inform user about successful attachment to i2c layer */
 	dev_info(&dev->adapter.dev, "connected i2c-tiny-usb device\n");
 
+	for (i = 0; opt_devices[i]; ++i) {
+		at = strchr(opt_devices[i], '@');
+		if (at == NULL) {
+			dev_warn(&dev->adapter.dev,
+				"address needed in device definition '%s'\n",
+				opt_devices[i]);
+			continue;
+		}
+		*at++ = 0;
+		if (kstrtouint(at, 0, &addr) || addr >= I2C_CLIENT_END) {
+			*--at = '@';
+			dev_warn(&dev->adapter.dev,
+				"wrong address in slave definition '%s'\n",
+				opt_devices[i]);
+			continue;
+		}
+		memset(&i2c_info, 0, sizeof(struct i2c_board_info));
+		strlcpy(i2c_info.type, opt_devices[i], I2C_NAME_SIZE);
+		i2c_addr[0] = addr;
+		if (i2c_new_probed_device(&dev->adapter, &i2c_info, i2c_addr,
+			NULL))
+			dev_info(&dev->adapter.dev,
+				"device %s at address 0x%02x registered\n",
+				i2c_info.type, addr);
+		else
+			dev_warn(&dev->adapter.dev,
+				"device %s at address 0x%02x not found\n",
+				i2c_info.type, addr);
+	}
+
 	return 0;
 
  error:
-- 
1.7.11.7

Re: [PATCH] i2c: i2c-tiny-usb: Add parameter for optional i2c-devices.

[Alan Cox]

On Tue, 13 Nov 2012 21:52:20 +0100
Alexander Holler <holler-SXC+2es9fhnfWeYVQQPykw@public.gmane.org> wrote:

> Make it possible to define i2c-devices at the kernel command line
> or as a module parameter.
> 
> Format is devname1@addr1,devname2@addr2,...

Why not do this with your initrd, you can deal with your rtc
before mounting the real root fs.

Alan

Re: [PATCH] i2c: i2c-tiny-usb: Add parameter for optional i2c-devices.

[Alexander Holler]

Am 13.11.2012 23:42, schrieb Alan Cox:
> On Tue, 13 Nov 2012 21:52:20 +0100
> Alexander Holler <holler-SXC+2es9fhnfWeYVQQPykw@public.gmane.org> wrote:
>
>> Make it possible to define i2c-devices at the kernel command line
>> or as a module parameter.
>>
>> Format is devname1@addr1,devname2@addr2,...
>
> Why not do this with your initrd, you can deal with your rtc
> before mounting the real root fs.

I almost never use an initrd. Most of the time it isn't necessary and is 
just a waste of time to build, maintain and use it.

Thanks to git, I even find it it easier to maintain my own set of 
patches for the kernel (which I do since several years) and I find it 
easier to explain such others too, than to explain someone about how to 
build, maintain and use an initrd.

Regards,

Alexander

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Jean Delvare]

Hi Alexander,

On Tue, 13 Nov 2012 21:23:04 +0100, Alexander Holler wrote:
> Am 13.11.2012 19:55, schrieb Jean Delvare:
> > On Tue, 13 Nov 2012 19:06:07 +0100, Alexander Holler wrote:
> >> This makes it possible to define i2c-devices at the kernel command line
> >> or as a module parameter for bus-drivers which want to offer such
> >> an functionality.
> >>
> >> Drivers which are using it will have the a parameter named
> >> devices with format devname1@addr1,devname2@addr2,...
> >> e.g. devices=ds1307@0x68,pcf8563@0x51
> >
> > No, no, no. We did that 10 years ago, killed all the code 3 years ago
> > [1], let's not do the same mistake again, please. We have a sysfs
> > interface for instantiating clients dynamically from user-space, it's
> > way more powerful and flexible than your proposal. Just try plugging two
> 
> So how do I define a device, e.g. an RTC which I want to have alive 
> before userland starts? Currently imho not possible.

If your system relies on a an RTC clock chip hanging off an USB-to-I2C
adapter, I'd say your design is very wrong to start with. I can only
suppose (hope!) you are doing that during some development phase and
the final hardware design is totally different.

If you need I2C devices to be instantiated early in the boot process,
this would typically be done by platform code. This is going to be a
little difficult with i2c-tiny-usb as it was definitely not meant to
host system-critical chips. But you can probably get it to work if you
try hard enough (in particular by allowing i2c-tiny-usb to claim
specific i2c bus number(s).) If you want your development system to
mimic the final hardware as closely as possible, this is the best
approach anyway.

> > different i2c-tiny-usb adapters on the same system and see the new code
> > instantiate the wrong devices...
> 
> I know about that, but it probes

It probes in the sense "check if a device is present", not in the sense
"check if the device there is really what the user told me." So very
easy to get wrong. Plus there is no universal probe method on I2C,
i2c_new_probed_device() uses a default heuristic which may not only
fail to detect a device's presence, but may even heavily confuse the
device in question. Usage of i2c_new_probed_device() should be limited
to very specific cases.

> and you don't have to use that parameter at all.

This has never been a good reason to accept every piece of code into
the kernel.

> I think some people can think for their self, 
> especially those how would use such a parameter.

This isn't the point. At least this is not my point. My point is, if we
let every developer add its own custom way of instantiating I2C devices
to fit their specific need of the day (and this is just one example
amongst others), we will end up with too many ways to achieve roughly
the same, and in the long run the duplicate code will diverge, bugs
will crop in, and we'll end up with an unmaintainable mess.

> > [1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=7f508118b1c1f9856a1c899a2bd4867a962b0225
> 
> I think my patch looks nicier.

Certainly, that's not terribly difficult. The code we got rid of at
that time was horrid :( And the commit above was only the tip of the
Halloween iceberg.

I am not questioning the quality of your code, I did not even look at
it. I'm questioning the pertinence of adding yet another way to
instantiate i2c devices when we already have 4 which made everybody
else happy for the past 3 years AFAIK.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Alexander Holler]

Am 13.11.2012 22:08, schrieb Jean Delvare:


>>> No, no, no. We did that 10 years ago, killed all the code 3 years ago

Btw. I wonder where all you kernel devs have learned that "No, no, no". 
Do you all attend the same rhetoric courses at some conferences? ;)

>>> [1], let's not do the same mistake again, please. We have a sysfs
>>> interface for instantiating clients dynamically from user-space, it's
>>> way more powerful and flexible than your proposal. Just try plugging two
>>
>> So how do I define a device, e.g. an RTC which I want to have alive
>> before userland starts? Currently imho not possible.
>
> If your system relies on a an RTC clock chip hanging off an USB-to-I2C
> adapter, I'd say your design is very wrong to start with. I can only
> suppose (hope!) you are doing that during some development phase and
> the final hardware design is totally different.
 >
> If you need I2C devices to be instantiated early in the boot process,
> this would typically be done by platform code. This is going to be a
> little difficult with i2c-tiny-usb as it was definitely not meant to
> host system-critical chips. But you can probably get it to work if you
> try hard enough (in particular by allowing i2c-tiny-usb to claim
> specific i2c bus number(s).) If you want your development system to
> mimic the final hardware as closely as possible, this is the best
> approach anyway.

I never would develop some hardware meant for usage with Linux which 
doesn't have a RTC. But there are several such boards available. E.g. 
that currently so much hyped Rasberry PI. So there are certainly some 
people which have a usage for such a parameter (including me).

Maybe you could forward your suggestion to those hw-devs which are 
responsible for linux-hw without an RTC. I don't know such poeple. ;)

> It probes in the sense "check if a device is present", not in the sense
> "check if the device there is really what the user told me." So very
> easy to get wrong. Plus there is no universal probe method on I2C,
> i2c_new_probed_device() uses a default heuristic which may not only
> fail to detect a device's presence, but may even heavily confuse the
> device in question. Usage of i2c_new_probed_device() should be limited
> to very specific cases.

I know about that too. But I prefer such a probe instead of doing it 
without an probe. Just try what happens if you add e.g. an pcf8563 (or 
ds1307) which is not available. The driver doesn't care and you will 
find an /dev/rtcN afterwards in your system. So probing is imho better 
than not.

> I am not questioning the quality of your code, I did not even look at
> it. I'm questioning the pertinence of adding yet another way to
> instantiate i2c devices when we already have 4 which made everybody
> else happy for the past 3 years AFAIK.

As said, currently there is no way to do that whithout either patching 
the kernel or working in userspace. And a RTC is just an example for a 
device you really want before userspace starts (but imho a very good).

Regards,

Alexander

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Jean Delvare]

On Tue, 13 Nov 2012 22:24:50 +0100, Alexander Holler wrote:
> Am 13.11.2012 22:08, schrieb Jean Delvare:
> > It probes in the sense "check if a device is present", not in the sense
> > "check if the device there is really what the user told me." So very
> > easy to get wrong. Plus there is no universal probe method on I2C,
> > i2c_new_probed_device() uses a default heuristic which may not only
> > fail to detect a device's presence, but may even heavily confuse the
> > device in question. Usage of i2c_new_probed_device() should be limited
> > to very specific cases.
> 
> I know about that too. But I prefer such a probe instead of doing it 
> without an probe. Just try what happens if you add e.g. an pcf8563 (or 
> ds1307) which is not available. The driver doesn't care and you will 
> find an /dev/rtcN afterwards in your system. So probing is imho better 
> than not.

Question is, what will you do the day someone wants to instantiate a
device for which the default probing mechanism doesn't work?

Plus you don't address the main issues. Your syntax gives you no way to
support two i2c-tiny-usb adapters with different chips at a specific
address. The sysfs interface supports such a setup. Also instantiating
the wrong devices is worse than instating a device that doesn't exist
at all. So the use of i2c_new_probed_device() here will randomly help
in a limited number of cases and randomly be problematic in others.
Hard to justify...

> > I am not questioning the quality of your code, I did not even look at
> > it. I'm questioning the pertinence of adding yet another way to
> > instantiate i2c devices when we already have 4 which made everybody
> > else happy for the past 3 years AFAIK.
> 
> As said, currently there is no way to do that whithout either patching 
> the kernel or working in userspace. And a RTC is just an example for a 
> device you really want before userspace starts (but imho a very good).

I am not familiar with RTC constraints. What is so important about it
that it can't wait for user-space? It'll have to wait for the USB and
I2C stacks to initialize anyway, so it won't be available at the very
early stages of the boot.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Alexander Holler]

Am 13.11.2012 22:42, schrieb Jean Delvare:
> On Tue, 13 Nov 2012 22:24:50 +0100, Alexander Holler wrote:
>> Am 13.11.2012 22:08, schrieb Jean Delvare:
>>> It probes in the sense "check if a device is present", not in the sense
>>> "check if the device there is really what the user told me." So very
>>> easy to get wrong. Plus there is no universal probe method on I2C,
>>> i2c_new_probed_device() uses a default heuristic which may not only
>>> fail to detect a device's presence, but may even heavily confuse the
>>> device in question. Usage of i2c_new_probed_device() should be limited
>>> to very specific cases.
>>
>> I know about that too. But I prefer such a probe instead of doing it
>> without an probe. Just try what happens if you add e.g. an pcf8563 (or
>> ds1307) which is not available. The driver doesn't care and you will
>> find an /dev/rtcN afterwards in your system. So probing is imho better
>> than not.
>
> Question is, what will you do the day someone wants to instantiate a
> device for which the default probing mechanism doesn't work?

Do you solve all problems you and others might have in the future 
already now?

> Plus you don't address the main issues. Your syntax gives you no way to
> support two i2c-tiny-usb adapters with different chips at a specific
> address. The sysfs interface supports such a setup. Also instantiating

It isn't possible to do such, because the only ID available for 
i2c-busses is given them at runtime. So people have to live with that 
imho artificially problem, if they use my patch. I don't have any other 
solution until the numbering is predictable. But I assume you already 
know all that, otherwise you wouldn't have mentioned it.

> the wrong devices is worse than instating a device that doesn't exist
> at all. So the use of i2c_new_probed_device() here will randomly help
> in a limited number of cases and randomly be problematic in others.
> Hard to justify...

So would you be satisfied if I would make the syntax more complicated by 
adding something which would allow them to define if the devices gets 
probed? E.g. dev1@addr1,dev2@addr2.noprobe,... ?

> I am not familiar with RTC constraints. What is so important about it
> that it can't wait for user-space? It'll have to wait for the USB and
> I2C stacks to initialize anyway, so it won't be available at the very
> early stages of the boot.

Try playing with a system which does have the wrong time. There is so 
much stuff which depends on the correct time, it is just a pain if the 
time is wrong or even the same across multiple system starts. And even 
if you know that, you might e.g. forget it and will use git to send some 
email and patches with erroneous times. But that is just an example.

And as I said, there might some other devices you might want or need in 
the future before usespace starts, so it solves a problem as the one 
above which I didn't have solved. ;)

Alexander

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Jean Delvare]

On Wed, 14 Nov 2012 00:38:52 +0100, Alexander Holler wrote:
> Am 13.11.2012 22:42, schrieb Jean Delvare:
> > Question is, what will you do the day someone wants to instantiate a
> > device for which the default probing mechanism doesn't work?
> 
> Do you solve all problems you and others might have in the future 
> already now?

Not all. But at least I try to avoid artificial limitations when
writing new code and to anticipate misbehavior in use cases other than
my own. I also try to learn from our past collective mistakes (starting
with my own.)

For clarity: it doesn't matter if new code doesn't cover all possible
present and future use cases, but it should not allow users to screw up
their system, and it should be possible to extend it to other
foreseeable use cases without breaking compatibility or making the code
unmaintainable or the interface ugly and confusing.

> > Plus you don't address the main issues. Your syntax gives you no way to
> > support two i2c-tiny-usb adapters with different chips at a specific
> > address. The sysfs interface supports such a setup. Also instantiating
> 
> It isn't possible to do such, because the only ID available for 
> i2c-busses is given them at runtime. So people have to live with that 
> imho artificially problem, if they use my patch. I don't have any other 
> solution until the numbering is predictable. But I assume you already 
> know all that, otherwise you wouldn't have mentioned it.

The problem is inherent to external, hot-plug I2C adapters. You can't
predict their bus number, and actually you can't even always predict
their name. In the case of usb-tiny-i2c, you may be able to look-up the
bus number from the adapter name, but you won't be able to always
differentiate between two adapters, if they are connected to paired USB
ports.

This is a design issue with the i2c-tiny-usb hardware in the first
place. If it is needed to differentiate between adapters, their would
need to be a locally unique ID in every adapter, which the kernel can
query. I suppose this was not deemed necessary at design time, as most
people will only connect one such adapter at a time to their system.

So I would agree that the problem I pointed out probably doesn't exist
in practice. Still, the limitation should at least be documented.

> > the wrong devices is worse than instating a device that doesn't exist
> > at all. So the use of i2c_new_probed_device() here will randomly help
> > in a limited number of cases and randomly be problematic in others.
> > Hard to justify...
> 
> So would you be satisfied if I would make the syntax more complicated by 
> adding something which would allow them to define if the devices gets 
> probed? E.g. dev1@addr1,dev2-fltQwxZSWAxmOS8nLLos6g@public.gmane.org,... ?

No. I would be more satisfied (although still not thinking your code
should make it into the kernel - but I am no longer the one to decide)
if you did use i2c_new_device() instead of i2c_new_probed_device(). You
said yourself that this module parameter was for users who knew what
they were doing.

> > I am not familiar with RTC constraints. What is so important about it
> > that it can't wait for user-space? It'll have to wait for the USB and
> > I2C stacks to initialize anyway, so it won't be available at the very
> > early stages of the boot.
> 
> Try playing with a system which does have the wrong time. There is so 
> much stuff which depends on the correct time, it is just a pain if the 
> time is wrong or even the same across multiple system starts. And even 
> if you know that, you might e.g. forget it and will use git to send some 
> email and patches with erroneous times. But that is just an example.

I have already experienced this and yes, it is a pain. But I would
think that a system designed without a RTC in the first place has
another way of getting its time correct at boot time, for example NTP.
As I understand it the RTC chip is only there to set the system time at
boot, right, the actual timekeeping during run-time is still done by the
CPU?

> And as I said, there might some other devices you might want or need in 
> the future before usespace starts, so it solves a problem as the one 
> above which I didn't have solved. ;)

Or maybe this will never happen.

-- 
Jean Delvare

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Alexander Holler]

Hello,

Am 14.11.2012 10:40, schrieb Jean Delvare:

>> It isn't possible to do such, because the only ID available for
>> i2c-busses is given them at runtime. So people have to live with that
>> imho artificially problem, if they use my patch. I don't have any other
>> solution until the numbering is predictable. But I assume you already
>> know all that, otherwise you wouldn't have mentioned it.
>
> The problem is inherent to external, hot-plug I2C adapters. You can't
> predict their bus number, and actually you can't even always predict
> their name. In the case of usb-tiny-i2c, you may be able to look-up the
> bus number from the adapter name, but you won't be able to always
> differentiate between two adapters, if they are connected to paired USB
> ports.
>
> This is a design issue with the i2c-tiny-usb hardware in the first
> place. If it is needed to differentiate between adapters, their would
> need to be a locally unique ID in every adapter, which the kernel can
> query. I suppose this was not deemed necessary at design time, as most
> people will only connect one such adapter at a time to their system.

Actually many of the available USB devices (e.g. many usb-serials) don't 
offer a unique ID by themself. That isn't just a problem of the 
i2c-tiny-usb. But in contrast to other solutions, it shouldn't be very 
hard to give those adapters a unique ID. As the whole SUB solution is 
just software (and open source), one likely just would to write some 
small piece of additional code.

> I have already experienced this and yes, it is a pain. But I would
> think that a system designed without a RTC in the first place has
> another way of getting its time correct at boot time, for example NTP.
> As I understand it the RTC chip is only there to set the system time at
> boot, right, the actual timekeeping during run-time is still done by the
> CPU?

Whatever those people which want to us it decide. If I didn't want to 
help other people by offering them some small documentation about how to 
build such theirself, I wouldn't have taken the usual and almost 
unavoidable pain trying feed some silly patches into the kernel. ;)

Anyway, maybe Till Harbaum will like that solution and won't get blocked 
by you. And maybe in some years we will see how many other bus-drivers 
have adopted the same solution. In fact the in-driver solution was my 
first one and I've thought others might be interested too, so I've moved 
the few lines from the driver itself into the i2c-core before I sent the 
patches. Unfortunately a waste of time.

Regards,

Alexander

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[till-RcHadlBFbzVAfugRpC6u6w]

Hi,

i have seen i2c chips going nuts because some probing actually affected the chips state. So i fully agree with Jean here.

I2C just isn't meant to be used for hot plugging. And so isn't the i2c-tiny-usb. It's more a hacking and testing device and is e.g. very convenient to test i2c client drivers or to test some new i2c hardware. But i have never had a need for this before user land was available. And once it is you can really do any magic you want using e.g. udev and sysfs.

Also if you really need some chip to be available at boot time, then usb isn't for you. Usb can take pretty long to enumerate etc and you can never be sure when exactly a device shows up on the usb bus. You'd thus additionally need some means of blocking the entire boot process if you want to enforce that. E.g. the kernel can wait for boot disks to appear for exactly this reason. But it wouldn't make much sense to delay the boot for less cruicial things. Boot time is a critical thing and only the most important things are supposed to have a negative impact on that.

If you wan't an i2c device to be available at boot time, then you might consider to connect it to some non-volatile i2c bus. I am pretty sure the raspberry pi has one.

Regards,
  Till

-- 
Dr. Till Harbaum <till-RcHadlBFbzVAfugRpC6u6w@public.gmane.org>
----- Ursprüngliche Mitteilung -----
> Hello,
> 
> Am 14.11.2012 10:40, schrieb Jean Delvare:
> 
> > > It isn't possible to do such, because the only ID available for
> > > i2c-busses is given them at runtime. So people have to live with that
> > > imho artificially problem, if they use my patch. I don't have any
> > > other solution until the numbering is predictable. But I assume you
> > > already know all that, otherwise you wouldn't have mentioned it.
> > 
> > The problem is inherent to external, hot-plug I2C adapters. You can't
> > predict their bus number, and actually you can't even always predict
> > their name. In the case of usb-tiny-i2c, you may be able to look-up the
> > bus number from the adapter name, but you won't be able to always
> > differentiate between two adapters, if they are connected to paired USB
> > ports.
> > 
> > This is a design issue with the i2c-tiny-usb hardware in the first
> > place. If it is needed to differentiate between adapters, their would
> > need to be a locally unique ID in every adapter, which the kernel can
> > query. I suppose this was not deemed necessary at design time, as most
> > people will only connect one such adapter at a time to their system.
> 
> Actually many of the available USB devices (e.g. many usb-serials) don't 
> offer a unique ID by themself. That isn't just a problem of the 
> i2c-tiny-usb. But in contrast to other solutions, it shouldn't be very 
> hard to give those adapters a unique ID. As the whole SUB solution is 
> just software (and open source), one likely just would to write some 
> small piece of additional code.
> 
> > I have already experienced this and yes, it is a pain. But I would
> > think that a system designed without a RTC in the first place has
> > another way of getting its time correct at boot time, for example NTP.
> > As I understand it the RTC chip is only there to set the system time at
> > boot, right, the actual timekeeping during run-time is still done by
> > the CPU?
> 
> Whatever those people which want to us it decide. If I didn't want to 
> help other people by offering them some small documentation about how to 
> build such theirself, I wouldn't have taken the usual and almost 
> unavoidable pain trying feed some silly patches into the kernel. ;)
> 
> Anyway, maybe Till Harbaum will like that solution and won't get blocked 
> by you. And maybe in some years we will see how many other bus-drivers 
> have adopted the same solution. In fact the in-driver solution was my 
> first one and I've thought others might be interested too, so I've moved 
> the few lines from the driver itself into the i2c-core before I sent the 
> patches. Unfortunately a waste of time.
> 
> Regards,
> 
> Alexander
> 

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Alexander Holler]

Am 14.11.2012 20:22, schrieb till-RcHadlBFbzVAfugRpC6u6w@public.gmane.org:
> Hi,
>
> i have seen i2c chips going nuts because some probing actually affected the chips state. So i fully agree with Jean here.

I'm fully aware of that.

> I2C just isn't meant to be used for hot plugging. And so isn't the i2c-tiny-usb. It's more a hacking and testing device and is e.g. very convenient to test i2c client drivers or to test some new i2c hardware. But i have never had a need for this before user land was available. And once it is you can really do any magic you want using e.g. udev and sysfs.

So I conclude you don't accept my patch too.

> Also if you really need some chip to be available at boot time, then usb isn't for you. Usb can take pretty long to enumerate etc and you can never be sure when exactly a device shows up on the usb bus. You'd thus additionally need some means of blocking the entire boot process if you want to enforce that. E.g. the kernel can wait for boot disks to appear for exactly this reason. But it wouldn't make much sense to delay the boot for less cruicial things. Boot time is a critical thing and only the most important things are supposed to have a negative impact on that.

Some people are booting from USB, including the small server this mail 
goes through. I wonder why you consider the boot time a critical thing. 
This device isn't meant for some industrial (embedded) application. And 
most people are booting only seldom their desktop (not to speak about 
servers).


> If you wan't an i2c device to be available at boot time, then you might consider to connect it to some non-volatile i2c bus. I am pretty sure the raspberry pi has one.

I don't have a Rasberry Pi and I don't need one.

Sorry, but think all of you have the impression I'm a dumb kid, playing 
with some silly hardware. How does that come? I might be even older than 
you. ;)

Anyway, if someone of you is curious why I've undergone all the pain 
trying to submit a simple patch, here you can find a first draft:

http://ahsoftware.de/usb-rtc/

Regards,

Alexander

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Alexander Holler]

Hello,

Am 15.11.2012 12:04, schrieb Till Harbaum:

> There's actually one thing you can do: You device doesn't seem to expose
> the i2c bus, anyway. What you have is an rtc connected via usb. So why not
> move all the i2c intelligence into the device? Do pure usb-rtc's exist?
> Could you perhaps even make your device compatible to one of these?
> Then a driver for this would imho have good chances to find their way into
> the kernel.

Sorry, but I'm satisfied with what I've done and I didn't do it just to 
get "something" into the kernel. I don't need my patches to become part 
of in the kernel, I can handle them by myself. And my free resources to 
submit patches just became exhausted (again). Maybe in some weeks or 
month ..., I don't know.

Regards,

Alexander

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Alexander Holler]

Hello,

after the needed break in the discussion (to calm down), I've decided to make a last try. I don't want to make a thesis about USB RTCs, nor do I want to write a whole new driver (and afterwards trying to get such into the kernel, not to speak about the then necessary explicit device/vendor ID).

On Wed, Nov 14, 2012 at 08:22:34PM +0100, till-RcHadlBFbzVAfugRpC6u6w@public.gmane.org wrote:

> i have seen i2c chips going nuts because some probing actually affected the chips state. So i fully agree with Jean here.

I've now changed the device registration from using i2c_new_probed_device() to i2c_new_device(),
even if that would register non-existent RTCs.

> I2C just isn't meant to be used for hot plugging. And so isn't the i2c-tiny-usb. It's more a hacking and testing device and is e.g. very convenient to test i2c client drivers or to test some new i2c hardware. But i have never had a need for this before user land was available. And once it is you can really do any magic you want using e.g. udev and sysfs.

As you've written yourself, i2c-tiny-usb is more a hacking and testing device, so there's hopefully no real argument left against including the few lines to enable devices through module options or the kernel command line. Someone could like to test such too with the "hacking and testing device".

Regards,

Alexander


>From 1aa6bbd0a87ce39f9889f835d12127226ffa9403 Mon Sep 17 00:00:00 2001
From: Alexander Holler <holler-SXC+2es9fhnfWeYVQQPykw@public.gmane.org>
Date: Tue, 13 Nov 2012 16:28:07 +0100
Subject: [PATCH] i2c: i2c-tiny-usb: Add parameter for optional i2c-devices.

Make it possible to define i2c-devices at the kernel command line
or as a module parameter.

Format is devname1@addr1,devname2@addr2,...

Example for the kernel command line:

   i2c-tiny-usb.devices=ds1307@0x68,pcf8563@0x51

The definition of up to 8 devices is allowed.

Cc: Till Harbaum <till-RcHadlBFbzVAfugRpC6u6w@public.gmane.org>
Signed-off-by: Alexander Holler <holler-SXC+2es9fhnfWeYVQQPykw@public.gmane.org>
---
 drivers/i2c/busses/i2c-tiny-usb.c |   44 +++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tiny-usb.c b/drivers/i2c/busses/i2c-tiny-usb.c
index 0510636..a3fc711 100644
--- a/drivers/i2c/busses/i2c-tiny-usb.c
+++ b/drivers/i2c/busses/i2c-tiny-usb.c
@@ -40,6 +40,11 @@ module_param(delay, ushort, 0);
 MODULE_PARM_DESC(delay, "bit delay in microseconds "
 		 "(default is 10us for 100kHz max)");
 
+#define MAX_OPTIONAL_I2C_DEVICES 8
+static char *opt_devices[MAX_OPTIONAL_I2C_DEVICES];
+module_param_array_named(devices, opt_devices, charp, NULL, 0);
+MODULE_PARM_DESC(devices, "devname1@adr1,devname2@adr2,... (e.g. ds1307@0x68)");
+
 static int usb_read(struct i2c_adapter *adapter, int cmd,
 		    int value, int index, void *data, int len);
 
@@ -184,6 +189,42 @@ static void i2c_tiny_usb_free(struct i2c_tiny_usb *dev)
 	kfree(dev);
 }
 
+static void i2c_add_optional_devices(struct i2c_adapter *adapter)
+{
+	int i;
+	struct i2c_board_info i2c_info;
+	uint addr;
+	const char *at;
+
+	for (i = 0; opt_devices[i]; ++i) {
+		at = strchr(opt_devices[i], '@');
+		if (at++ == NULL) {
+			dev_warn(&adapter->dev,
+				"address needed in device definition '%s'\n",
+				opt_devices[i]);
+			continue;
+		}
+		if (kstrtouint(at, 0, &addr) || addr >= I2C_CLIENT_END) {
+			dev_warn(&adapter->dev,
+				"wrong address in device definition '%s'\n",
+				opt_devices[i]);
+			continue;
+		}
+		memset(&i2c_info, 0, sizeof(struct i2c_board_info));
+		strlcpy(i2c_info.type, opt_devices[i],
+			min(I2C_NAME_SIZE, (int)(at-opt_devices[i])));
+		i2c_info.addr = addr;
+		if (i2c_new_device(adapter, &i2c_info) != NULL)
+			dev_info(&adapter->dev,
+				"device %s at address 0x%02x registered\n",
+				i2c_info.type, addr);
+		else
+			dev_warn(&adapter->dev,
+				"device %s at address 0x%02x not found\n",
+				i2c_info.type, addr);
+	}
+}
+
 static int i2c_tiny_usb_probe(struct usb_interface *interface,
 			      const struct usb_device_id *id)
 {
@@ -236,6 +277,9 @@ static int i2c_tiny_usb_probe(struct usb_interface *interface,
 	/* inform user about successful attachment to i2c layer */
 	dev_info(&dev->adapter.dev, "connected i2c-tiny-usb device\n");
 
+	/* add optional devices */
+	i2c_add_optional_devices(&dev->adapter);
+
 	return 0;
 
  error:
-- 
1.7.8.6

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Alexander Holler]

Am 14.11.2012 10:40, schrieb Jean Delvare:

> Or maybe this will never happen.

Which is exactly what your friendly greeting at the start of this thread
should have hinted to me. I'll take it as an advice for the future.

Alexander

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Alexander Holler]

Hello,

Am 13.11.2012 22:42, schrieb Jean Delvare:

> Plus you don't address the main issues. Your syntax gives you no way to
> support two i2c-tiny-usb adapters with different chips at a specific
> address. The sysfs interface supports such a setup. Also instantiating
> the wrong devices is worse than instating a device that doesn't exist
> at all. So the use of i2c_new_probed_device() here will randomly help
> in a limited number of cases and randomly be problematic in others.
> Hard to justify...

As you seem to have a solution for multiple devices of the same type by
using sysfs, how to do you decide which one to use? You might be able to
probe just one, but my simple mind currently doesn't come up with a
solution which device one has to probe. Just because I'm curious... ;)

Regards,

Alexander

Re: [PATCH 1/2] i2c: Add possibility for user-defined (i2c-)devices for bus-drivers.

[Alexander Holler]

Am 14.11.2012 03:47, schrieb Alexander Holler:
> Hello,
>
> Am 13.11.2012 22:42, schrieb Jean Delvare:
>
>> Plus you don't address the main issues. Your syntax gives you no way to
>> support two i2c-tiny-usb adapters with different chips at a specific
>> address. The sysfs interface supports such a setup. Also instantiating
>> the wrong devices is worse than instating a device that doesn't exist
>> at all. So the use of i2c_new_probed_device() here will randomly help
>> in a limited number of cases and randomly be problematic in others.
>> Hard to justify...
>
> As you seem to have a solution for multiple devices of the same type by
> using sysfs, how to do you decide which one to use? You might be able to
> probe just one, but my simple mind currently doesn't come up with a
> solution which device one has to probe. Just because I'm curious... ;)

And while we are at artificial problems, I've just seen a small problem 
in my patch which might occur if someone really uses two of those 
devices. I've already fixed it here, but until one of you gives me the 
ok for inclusion into the mainline, I don't want to waste more of your 
valuable time with posting a corrected patch for something no one of you 
wants.

Regards,

Alexander