Spun another version of this patch. Thanks Coretin for your feedback.
I've incorporated your suggestions.
v2 : * Added MODULE_DEVICE_TABLE for the dmi table.
* Made __init and __initdata position consistent across this file.
* Made prefix static const char *
* Made strncmp expressions more explicit
* Added an additional match for DMI_SYS_VENDOR for Samsung Lumpy.
Benson
This adds the chromeos_laptop driver. It supports
the Cypress APA SMBUS touchpad as well as the isl29018 i2c ambient
light sensor on the Samsung Series 5 550 Chromebook.
Signed-off-by: Benson Leung <[email protected]>
Reviewed-by: Olof Johansson <[email protected]>
---
Version history :
v2 : * Added MODULE_DEVICE_TABLE for the dmi table.
* Made __init and __initdata position consistent across this file.
* Made prefix static const char *
* Made strncmp expressions more explicit
* Added an additional match for DMI_SYS_VENDOR for Samsung Lumpy.
v1 : Initial
---
drivers/platform/x86/Kconfig | 11 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/chromeos_laptop.c | 205 ++++++++++++++++++++++++++++++++
3 files changed, 217 insertions(+), 0 deletions(-)
create mode 100644 drivers/platform/x86/chromeos_laptop.c
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index c86bae8..12b8594 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -79,6 +79,17 @@ config ASUS_LAPTOP
If you have an ACPI-compatible ASUS laptop, say Y or M here.
+config CHROMEOS_LAPTOP
+ tristate "Chrome OS Laptop"
+ depends on I2C
+ depends on DMI
+ ---help---
+ This driver instantiates i2c and smbus devices such as
+ light sensors and touchpads.
+
+ If you have a supported Chromebook, choose Y or M here.
+ The module will be called chromeos_laptop.
+
config DELL_LAPTOP
tristate "Dell Laptop Extras (EXPERIMENTAL)"
depends on X86
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index bf7e4f9..ace2b38 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -50,3 +50,4 @@ obj-$(CONFIG_INTEL_MID_POWER_BUTTON) += intel_mid_powerbtn.o
obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o
obj-$(CONFIG_SAMSUNG_Q10) += samsung-q10.o
obj-$(CONFIG_APPLE_GMUX) += apple-gmux.o
+obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
diff --git a/drivers/platform/x86/chromeos_laptop.c b/drivers/platform/x86/chromeos_laptop.c
new file mode 100644
index 0000000..a8329c3
--- /dev/null
+++ b/drivers/platform/x86/chromeos_laptop.c
@@ -0,0 +1,205 @@
+/*
+ * chromeos_laptop.c - Driver to instantiate Chromebook i2c/smbus devices.
+ *
+ * Author : Benson Leung <[email protected]>
+ *
+ * Copyright (C) 2012 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+#include <linux/dmi.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+
+#define CYAPA_TP_I2C_ADDR 0x67
+#define ISL_ALS_I2C_ADDR 0x44
+
+static struct i2c_client *als;
+static struct i2c_client *tp;
+
+const char *i2c_adapter_names[] = {
+ "SMBus I801 adapter",
+};
+
+/* Keep this enum consistent with i2c_adapter_names */
+enum i2c_adapter_type {
+ I2C_ADAPTER_SMBUS = 0,
+};
+
+static struct i2c_board_info __initdata cyapa_device = {
+ I2C_BOARD_INFO("cyapa", CYAPA_TP_I2C_ADDR),
+ .flags = I2C_CLIENT_WAKE,
+};
+
+static struct i2c_board_info __initdata isl_als_device = {
+ I2C_BOARD_INFO("isl29018", ISL_ALS_I2C_ADDR),
+};
+
+static struct i2c_client __init *__add_probed_i2c_device(
+ const char *name,
+ int bus,
+ struct i2c_board_info *info,
+ const unsigned short *addrs)
+{
+ const struct dmi_device *dmi_dev;
+ const struct dmi_dev_onboard *dev_data;
+ struct i2c_adapter *adapter;
+ struct i2c_client *client;
+
+ if (bus < 0)
+ return NULL;
+ /*
+ * If a name is specified, look for irq platform information stashed
+ * in DMI_DEV_TYPE_DEV_ONBOARD by the Chrome OS custom system firmware.
+ */
+ if (name) {
+ dmi_dev = dmi_find_device(DMI_DEV_TYPE_DEV_ONBOARD, name, NULL);
+ if (!dmi_dev) {
+ pr_err("%s failed to dmi find device %s.\n",
+ __func__,
+ name);
+ return NULL;
+ }
+ dev_data = (struct dmi_dev_onboard *)dmi_dev->device_data;
+ if (!dev_data) {
+ pr_err("%s failed to get data from dmi for %s.\n",
+ __func__, name);
+ return NULL;
+ }
+ info->irq = dev_data->instance;
+ }
+
+ adapter = i2c_get_adapter(bus);
+ if (!adapter) {
+ pr_err("%s failed to get i2c adapter %d.\n", __func__, bus);
+ return NULL;
+ }
+
+ /* add the i2c device */
+ client = i2c_new_probed_device(adapter, info, addrs, NULL);
+ if (!client)
+ pr_err("%s failed to register device %d-%02x\n",
+ __func__, bus, info->addr);
+ else
+ pr_debug("%s added i2c device %d-%02x\n",
+ __func__, bus, info->addr);
+
+ i2c_put_adapter(adapter);
+ return client;
+}
+
+static int __init __find_i2c_adap(struct device *dev, void *data)
+{
+ const char *name = data;
+ static const char *prefix = "i2c-";
+ struct i2c_adapter *adapter;
+ if (strncmp(dev_name(dev), prefix, strlen(prefix)) != 0)
+ return 0;
+ adapter = to_i2c_adapter(dev);
+ return (strncmp(adapter->name, name, strlen(name)) == 0);
+}
+
+static int __init find_i2c_adapter_num(enum i2c_adapter_type type)
+{
+ struct device *dev = NULL;
+ struct i2c_adapter *adapter;
+ const char *name = i2c_adapter_names[type];
+ /* find the adapter by name */
+ dev = bus_find_device(&i2c_bus_type, NULL, (void *)name,
+ __find_i2c_adap);
+ if (!dev) {
+ pr_err("%s: i2c adapter %s not found on system.\n", __func__,
+ name);
+ return -ENODEV;
+ }
+ adapter = to_i2c_adapter(dev);
+ return adapter->nr;
+}
+
+/*
+ * Probes for a device at a single address, the one provided by
+ * info->addr.
+ * Returns NULL if no device found.
+ */
+static struct i2c_client __init *add_smbus_device(const char *name,
+ struct i2c_board_info *info)
+{
+ const unsigned short addr_list[] = { info->addr, I2C_CLIENT_END };
+ return __add_probed_i2c_device(name,
+ find_i2c_adapter_num(I2C_ADAPTER_SMBUS),
+ info,
+ addr_list);
+}
+
+static int __init setup_lumpy_tp(const struct dmi_system_id *id)
+{
+ /* add cyapa touchpad on smbus */
+ tp = add_smbus_device("trackpad", &cyapa_device);
+ return 0;
+}
+
+static int __init setup_isl29018_als(const struct dmi_system_id *id)
+{
+ /* add isl29018 light sensor */
+ als = add_smbus_device("lightsensor", &isl_als_device);
+ return 0;
+}
+
+static struct dmi_system_id __initdata chromeos_laptop_dmi_table[] = {
+ {
+ .ident = "Samsung Series 5 550 - Touchpad",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Lumpy"),
+ },
+ .callback = setup_lumpy_tp,
+ },
+ {
+ .ident = "Samsung Series 5 550 - Light Sensor",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Lumpy"),
+ },
+ .callback = setup_isl29018_als,
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(dmi, chromeos_laptop_dmi_table);
+
+static int __init chromeos_laptop_init(void)
+{
+ if (!dmi_check_system(chromeos_laptop_dmi_table)) {
+ pr_debug("%s unsupported system.\n", __func__);
+ return -ENODEV;
+ }
+ return 0;
+}
+
+static void __exit chromeos_laptop_exit(void)
+{
+ if (als)
+ i2c_unregister_device(als);
+ if (tp)
+ i2c_unregister_device(tp);
+}
+
+module_init(chromeos_laptop_init);
+module_exit(chromeos_laptop_exit);
+
+MODULE_DESCRIPTION("Chrome OS Laptop driver");
+MODULE_AUTHOR("Benson Leung <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.7.7.3
On Thu, Oct 25, 2012 at 10:21 PM, Benson Leung <[email protected]> wrote:
> This adds the chromeos_laptop driver. It supports
> the Cypress APA SMBUS touchpad as well as the isl29018 i2c ambient
> light sensor on the Samsung Series 5 550 Chromebook.
>
> Signed-off-by: Benson Leung <[email protected]>
> Reviewed-by: Olof Johansson <[email protected]>
> ---
> Version history :
> v2 : * Added MODULE_DEVICE_TABLE for the dmi table.
> * Made __init and __initdata position consistent across this file.
> * Made prefix static const char *
> * Made strncmp expressions more explicit
> * Added an additional match for DMI_SYS_VENDOR for Samsung Lumpy.
> v1 : Initial
> ---
> drivers/platform/x86/Kconfig | 11 ++
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/chromeos_laptop.c | 205 ++++++++++++++++++++++++++++++++
> 3 files changed, 217 insertions(+), 0 deletions(-)
> create mode 100644 drivers/platform/x86/chromeos_laptop.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index c86bae8..12b8594 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -79,6 +79,17 @@ config ASUS_LAPTOP
>
> If you have an ACPI-compatible ASUS laptop, say Y or M here.
>
> +config CHROMEOS_LAPTOP
> + tristate "Chrome OS Laptop"
> + depends on I2C
> + depends on DMI
> + ---help---
> + This driver instantiates i2c and smbus devices such as
> + light sensors and touchpads.
> +
> + If you have a supported Chromebook, choose Y or M here.
> + The module will be called chromeos_laptop.
> +
> config DELL_LAPTOP
> tristate "Dell Laptop Extras (EXPERIMENTAL)"
> depends on X86
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index bf7e4f9..ace2b38 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -50,3 +50,4 @@ obj-$(CONFIG_INTEL_MID_POWER_BUTTON) += intel_mid_powerbtn.o
> obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o
> obj-$(CONFIG_SAMSUNG_Q10) += samsung-q10.o
> obj-$(CONFIG_APPLE_GMUX) += apple-gmux.o
> +obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
> diff --git a/drivers/platform/x86/chromeos_laptop.c b/drivers/platform/x86/chromeos_laptop.c
> new file mode 100644
> index 0000000..a8329c3
> --- /dev/null
> +++ b/drivers/platform/x86/chromeos_laptop.c
> @@ -0,0 +1,205 @@
> +/*
> + * chromeos_laptop.c - Driver to instantiate Chromebook i2c/smbus devices.
> + *
> + * Author : Benson Leung <[email protected]>
> + *
> + * Copyright (C) 2012 Google, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#include <linux/dmi.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +
> +#define CYAPA_TP_I2C_ADDR 0x67
> +#define ISL_ALS_I2C_ADDR 0x44
> +
> +static struct i2c_client *als;
> +static struct i2c_client *tp;
> +
> +const char *i2c_adapter_names[] = {
> + "SMBus I801 adapter",
> +};
> +
> +/* Keep this enum consistent with i2c_adapter_names */
> +enum i2c_adapter_type {
> + I2C_ADAPTER_SMBUS = 0,
> +};
> +
> +static struct i2c_board_info __initdata cyapa_device = {
> + I2C_BOARD_INFO("cyapa", CYAPA_TP_I2C_ADDR),
> + .flags = I2C_CLIENT_WAKE,
> +};
> +
> +static struct i2c_board_info __initdata isl_als_device = {
> + I2C_BOARD_INFO("isl29018", ISL_ALS_I2C_ADDR),
> +};
> +
> +static struct i2c_client __init *__add_probed_i2c_device(
> + const char *name,
> + int bus,
> + struct i2c_board_info *info,
> + const unsigned short *addrs)
> +{
> + const struct dmi_device *dmi_dev;
> + const struct dmi_dev_onboard *dev_data;
> + struct i2c_adapter *adapter;
> + struct i2c_client *client;
> +
> + if (bus < 0)
> + return NULL;
> + /*
> + * If a name is specified, look for irq platform information stashed
> + * in DMI_DEV_TYPE_DEV_ONBOARD by the Chrome OS custom system firmware.
> + */
> + if (name) {
> + dmi_dev = dmi_find_device(DMI_DEV_TYPE_DEV_ONBOARD, name, NULL);
> + if (!dmi_dev) {
> + pr_err("%s failed to dmi find device %s.\n",
> + __func__,
> + name);
> + return NULL;
> + }
> + dev_data = (struct dmi_dev_onboard *)dmi_dev->device_data;
> + if (!dev_data) {
> + pr_err("%s failed to get data from dmi for %s.\n",
> + __func__, name);
> + return NULL;
> + }
> + info->irq = dev_data->instance;
> + }
> +
> + adapter = i2c_get_adapter(bus);
> + if (!adapter) {
> + pr_err("%s failed to get i2c adapter %d.\n", __func__, bus);
> + return NULL;
> + }
> +
> + /* add the i2c device */
> + client = i2c_new_probed_device(adapter, info, addrs, NULL);
> + if (!client)
> + pr_err("%s failed to register device %d-%02x\n",
> + __func__, bus, info->addr);
> + else
> + pr_debug("%s added i2c device %d-%02x\n",
> + __func__, bus, info->addr);
> +
> + i2c_put_adapter(adapter);
> + return client;
> +}
> +
> +static int __init __find_i2c_adap(struct device *dev, void *data)
> +{
> + const char *name = data;
> + static const char *prefix = "i2c-";
> + struct i2c_adapter *adapter;
> + if (strncmp(dev_name(dev), prefix, strlen(prefix)) != 0)
> + return 0;
> + adapter = to_i2c_adapter(dev);
> + return (strncmp(adapter->name, name, strlen(name)) == 0);
> +}
> +
> +static int __init find_i2c_adapter_num(enum i2c_adapter_type type)
> +{
> + struct device *dev = NULL;
> + struct i2c_adapter *adapter;
> + const char *name = i2c_adapter_names[type];
> + /* find the adapter by name */
> + dev = bus_find_device(&i2c_bus_type, NULL, (void *)name,
> + __find_i2c_adap);
> + if (!dev) {
> + pr_err("%s: i2c adapter %s not found on system.\n", __func__,
> + name);
> + return -ENODEV;
> + }
> + adapter = to_i2c_adapter(dev);
> + return adapter->nr;
> +}
> +
> +/*
> + * Probes for a device at a single address, the one provided by
> + * info->addr.
> + * Returns NULL if no device found.
> + */
> +static struct i2c_client __init *add_smbus_device(const char *name,
> + struct i2c_board_info *info)
> +{
> + const unsigned short addr_list[] = { info->addr, I2C_CLIENT_END };
> + return __add_probed_i2c_device(name,
> + find_i2c_adapter_num(I2C_ADAPTER_SMBUS),
> + info,
> + addr_list);
> +}
> +
> +static int __init setup_lumpy_tp(const struct dmi_system_id *id)
> +{
> + /* add cyapa touchpad on smbus */
> + tp = add_smbus_device("trackpad", &cyapa_device);
> + return 0;
> +}
> +
> +static int __init setup_isl29018_als(const struct dmi_system_id *id)
> +{
> + /* add isl29018 light sensor */
> + als = add_smbus_device("lightsensor", &isl_als_device);
> + return 0;
> +}
> +
> +static struct dmi_system_id __initdata chromeos_laptop_dmi_table[] = {
> + {
> + .ident = "Samsung Series 5 550 - Touchpad",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Lumpy"),
> + },
> + .callback = setup_lumpy_tp,
> + },
> + {
> + .ident = "Samsung Series 5 550 - Light Sensor",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Lumpy"),
> + },
> + .callback = setup_isl29018_als,
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(dmi, chromeos_laptop_dmi_table);
> +
> +static int __init chromeos_laptop_init(void)
> +{
> + if (!dmi_check_system(chromeos_laptop_dmi_table)) {
> + pr_debug("%s unsupported system.\n", __func__);
> + return -ENODEV;
> + }
> + return 0;
> +}
> +
> +static void __exit chromeos_laptop_exit(void)
> +{
> + if (als)
> + i2c_unregister_device(als);
> + if (tp)
> + i2c_unregister_device(tp);
> +}
> +
> +module_init(chromeos_laptop_init);
> +module_exit(chromeos_laptop_exit);
> +
> +MODULE_DESCRIPTION("Chrome OS Laptop driver");
> +MODULE_AUTHOR("Benson Leung <[email protected]>");
> +MODULE_LICENSE("GPL");
> --
> 1.7.7.3
Looks better, but I'm curious, what is the final purpose of this driver ?
What ABI will be exposed, who will use it ?
If it is going to be bigger, it may be a good idea to convert it to a
real platform driver (platform_drivers/platform_device stuff).
Thanks,
--
Corentin Chary
http://xf.iksaif.net
On Fri, Oct 26, 2012 at 10:30 AM, Corentin Chary
<[email protected]> wrote:
> Looks better, but I'm curious, what is the final purpose of this driver ?
> What ABI will be exposed, who will use it ?
>
> If it is going to be bigger, it may be a good idea to convert it to a
> real platform driver (platform_drivers/platform_device stuff).
It's not a driver per se. It's platform glue that, based on the DMI
table, registers platform and i2c devices (at this time only i2c
devices).
Unfortunately there's no way to do this nicely from userspace after
boot, since there's limits to how much data you can provide with the
simpler userspace-driven i2c probing protocol.
So, there's no user-facing ABI on this, and no one is expected to use
it from userspace. It's just there to make sure that the un-probably
devices on this kind of hardware gets bound to drivers properly.
If it's converted to a platform_driver, how do you expect that to
probe, where would the platform_device be registered?
-Olof
On Fri, Nov 2, 2012 at 12:45 PM, Olof Johansson <[email protected]> wrote:
> On Fri, Oct 26, 2012 at 10:30 AM, Corentin Chary
> <[email protected]> wrote:
>
>> Looks better, but I'm curious, what is the final purpose of this driver ?
>> What ABI will be exposed, who will use it ?
>>
>> If it is going to be bigger, it may be a good idea to convert it to a
>> real platform driver (platform_drivers/platform_device stuff).
>
> It's not a driver per se. It's platform glue that, based on the DMI
> table, registers platform and i2c devices (at this time only i2c
> devices).
>
> Unfortunately there's no way to do this nicely from userspace after
> boot, since there's limits to how much data you can provide with the
> simpler userspace-driven i2c probing protocol.
>
> So, there's no user-facing ABI on this, and no one is expected to use
> it from userspace. It's just there to make sure that the un-probably
> devices on this kind of hardware gets bound to drivers properly.
>
> If it's converted to a platform_driver, how do you expect that to
> probe, where would the platform_device be registered?
I guess I would check dmi in the module init method, and then use the
probe callback of platform_create_bundle to do more probing if
necessary.
--
Corentin Chary
http://xf.iksaif.net
On Fri, Nov 2, 2012 at 2:03 PM, Corentin Chary <[email protected]> wrote:
> On Fri, Nov 2, 2012 at 12:45 PM, Olof Johansson <[email protected]> wrote:
>> On Fri, Oct 26, 2012 at 10:30 AM, Corentin Chary
>> <[email protected]> wrote:
>>
>>> Looks better, but I'm curious, what is the final purpose of this driver ?
>>> What ABI will be exposed, who will use it ?
>>>
>>> If it is going to be bigger, it may be a good idea to convert it to a
>>> real platform driver (platform_drivers/platform_device stuff).
>>
>> It's not a driver per se. It's platform glue that, based on the DMI
>> table, registers platform and i2c devices (at this time only i2c
>> devices).
>>
>> Unfortunately there's no way to do this nicely from userspace after
>> boot, since there's limits to how much data you can provide with the
>> simpler userspace-driven i2c probing protocol.
>>
>> So, there's no user-facing ABI on this, and no one is expected to use
>> it from userspace. It's just there to make sure that the un-probably
>> devices on this kind of hardware gets bound to drivers properly.
>>
>> If it's converted to a platform_driver, how do you expect that to
>> probe, where would the platform_device be registered?
>
> I guess I would check dmi in the module init method, and then use the
> probe callback of platform_create_bundle to do more probing if
> necessary.
Maybe I'm dense but I don't see how that could possibly be better than
what the code does today. It would just add more overhead and clutter
by creating a unnecessary dummy device/driver setup just to, in the
end, register the same i2c devices.
-Olof
On Fri, Nov 2, 2012 at 1:09 PM, Olof Johansson <[email protected]> wrote:
> On Fri, Nov 2, 2012 at 2:03 PM, Corentin Chary <[email protected]> wrote:
>> On Fri, Nov 2, 2012 at 12:45 PM, Olof Johansson <[email protected]> wrote:
>>> On Fri, Oct 26, 2012 at 10:30 AM, Corentin Chary
>>> <[email protected]> wrote:
>>>
>>>> Looks better, but I'm curious, what is the final purpose of this driver ?
>>>> What ABI will be exposed, who will use it ?
>>>>
>>>> If it is going to be bigger, it may be a good idea to convert it to a
>>>> real platform driver (platform_drivers/platform_device stuff).
>>>
>>> It's not a driver per se. It's platform glue that, based on the DMI
>>> table, registers platform and i2c devices (at this time only i2c
>>> devices).
>>>
>>> Unfortunately there's no way to do this nicely from userspace after
>>> boot, since there's limits to how much data you can provide with the
>>> simpler userspace-driven i2c probing protocol.
>>>
>>> So, there's no user-facing ABI on this, and no one is expected to use
>>> it from userspace. It's just there to make sure that the un-probably
>>> devices on this kind of hardware gets bound to drivers properly.
>>>
>>> If it's converted to a platform_driver, how do you expect that to
>>> probe, where would the platform_device be registered?
>>
>> I guess I would check dmi in the module init method, and then use the
>> probe callback of platform_create_bundle to do more probing if
>> necessary.
>
> Maybe I'm dense but I don't see how that could possibly be better than
> what the code does today. It would just add more overhead and clutter
> by creating a unnecessary dummy device/driver setup just to, in the
> end, register the same i2c devices.
>
Well that was the point of "If it is going to be bigger".
Of course, as long as it only register those i2c devices, it doesn't
really matter.
--
Corentin Chary
http://xf.iksaif.net
Hi Corentin,
Thanks for your feedback on this. Indeed, what Olof is saying is
correct. In its current form, this driver exists to register i2c
devices on our laptops. We have one piece of information (the irq,
specifically) that cannot be added to the i2c_board_info when using
user space probing.
To answer your question about where this driver is going in the
future, you can take a look at the current version of the driver in
the chromiumos kernel:
https://gerrit.chromium.org/gitweb/?p=chromiumos/third_party/kernel.git;a=blob;f=drivers/platform/x86/chromeos_laptop.c;h=f948f61d864a5265f6e144833358512f68b7e467;hb=chromeos-3.4
I added more i2c devices that are registered the same way for a number
of other chromeos systems, but there is one platform device (a
keyboard backlight) that is added using
platform_device_register_simple.
Thanks again for reviewing,
Benson
On Fri, Nov 2, 2012 at 6:32 AM, Corentin Chary <[email protected]> wrote:
> On Fri, Nov 2, 2012 at 1:09 PM, Olof Johansson <[email protected]> wrote:
>> On Fri, Nov 2, 2012 at 2:03 PM, Corentin Chary <[email protected]> wrote:
>>> On Fri, Nov 2, 2012 at 12:45 PM, Olof Johansson <[email protected]> wrote:
>>>> On Fri, Oct 26, 2012 at 10:30 AM, Corentin Chary
>>>> <[email protected]> wrote:
>>>>
>>>>> Looks better, but I'm curious, what is the final purpose of this driver ?
>>>>> What ABI will be exposed, who will use it ?
>>>>>
>>>>> If it is going to be bigger, it may be a good idea to convert it to a
>>>>> real platform driver (platform_drivers/platform_device stuff).
>>>>
>>>> It's not a driver per se. It's platform glue that, based on the DMI
>>>> table, registers platform and i2c devices (at this time only i2c
>>>> devices).
>>>>
>>>> Unfortunately there's no way to do this nicely from userspace after
>>>> boot, since there's limits to how much data you can provide with the
>>>> simpler userspace-driven i2c probing protocol.
>>>>
>>>> So, there's no user-facing ABI on this, and no one is expected to use
>>>> it from userspace. It's just there to make sure that the un-probably
>>>> devices on this kind of hardware gets bound to drivers properly.
>>>>
>>>> If it's converted to a platform_driver, how do you expect that to
>>>> probe, where would the platform_device be registered?
>>>
>>> I guess I would check dmi in the module init method, and then use the
>>> probe callback of platform_create_bundle to do more probing if
>>> necessary.
>>
>> Maybe I'm dense but I don't see how that could possibly be better than
>> what the code does today. It would just add more overhead and clutter
>> by creating a unnecessary dummy device/driver setup just to, in the
>> end, register the same i2c devices.
>>
>
> Well that was the point of "If it is going to be bigger".
> Of course, as long as it only register those i2c devices, it doesn't
> really matter.
>
> --
> Corentin Chary
> http://xf.iksaif.net
--
Benson Leung
Software Engineer, Chrom* OS
[email protected]