* [PATCH v2 1/3] Extcon (external connector): import Android's switch class and modify.
2011-12-14 10:28 [PATCH v2 0/3] introduce External Connector Class (extcon) MyungJoo Ham
@ 2011-12-14 10:28 ` MyungJoo Ham
2011-12-15 1:01 ` Greg KH
2011-12-14 10:28 ` [PATCH v2 2/3] Extcon: support notification based on the state changes MyungJoo Ham
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: MyungJoo Ham @ 2011-12-14 10:28 UTC (permalink / raw)
To: linux-kernel
Cc: Randy Dunlap, Mike Lockwood, Arve Hjønnevåg,
Kyungmin Park, Donggeun Kim, Greg KH, Arnd Bergmann, MyungJoo Ham,
Linus Walleij, Dmitry Torokhov, NeilBrown, Morten CHRISTIANSEN,
Mark Brown
External connector class (extcon) is based on and an extension of Android
kernel's switch class located at linux/drivers/switch/.
This patch provides the before-extension switch class moved to the
location where the extcon will be located (linux/drivers/extcon/).
The before-extension class, switch class of Android kernel, commits
imported are:
switch: switch class and GPIO drivers.
Author: Mike Lockwood <lockwood@android.com>
switch: gpio: Don't call request_irq with interrupts disabled
Author: Arve Hjønnevåg <arve@android.com>
switch: Use device_create instead of device_create_drvdata.
Author: Arve Hjønnevåg <arve@android.com>
switch_gpio: Add missing #include <linux/interrupt.h>
Author: Mike Lockwood <lockwood@android.com>
In this patch, upon the commits of Android kernel, we have added:
- Relocated and renamed for extcon.
- Comments, module name, and author information are updated
- Code clean for successing patches
- Bugfix: enabling write access without write functions
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
--
Changes from RFC
- Renamed to extcon (external connector) from multistate switch
- Added a seperated directory (drivers/extcon)
- Added kerneldoc comments
- Removed unused variables from extcon_gpio.c
- Added ABI Documentation.
---
Documentation/ABI/testing/sysfs-class-extcon | 26 ++++
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/extcon/Kconfig | 22 +++
drivers/extcon/Makefile | 6 +
drivers/extcon/extcon_class.c | 206 ++++++++++++++++++++++++++
drivers/extcon/extcon_gpio.c | 173 +++++++++++++++++++++
include/linux/extcon.h | 101 +++++++++++++
8 files changed, 537 insertions(+), 0 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-extcon
create mode 100644 drivers/extcon/Kconfig
create mode 100644 drivers/extcon/Makefile
create mode 100644 drivers/extcon/extcon_class.c
create mode 100644 drivers/extcon/extcon_gpio.c
create mode 100644 include/linux/extcon.h
diff --git a/Documentation/ABI/testing/sysfs-class-extcon b/Documentation/ABI/testing/sysfs-class-extcon
new file mode 100644
index 0000000..59a4b1c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-extcon
@@ -0,0 +1,26 @@
+What: /sys/class/extcon/.../
+Date: December 2011
+Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
+Description:
+ Provide a place in sysfs for the extcon objects.
+ This allows accessing extcon specific variables.
+ The name of extcon object denoted as ... is the name given
+ with extcon_dev_register.
+
+What: /sys/class/extcon/.../name
+Date: December 2011
+Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
+Description:
+ The /sys/class/extcon/.../name shows the name of the extcon
+ object. If the extcon object has an optional callback
+ "show_name" defined, the callback will provide the name with
+ this sysfs node.
+
+What: /sys/class/extcon/.../state
+Date: December 2011
+Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
+Description:
+ The /sys/class/extcon/.../state shows the cable attach/detach
+ information of the corresponding extcon object. If the extcon
+ objecct has an optional callback "show_state" defined, the
+ callback will provide the name with this sysfs node.
diff --git a/drivers/Kconfig b/drivers/Kconfig
index b5e6f24..da57c81 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -136,4 +136,6 @@ source "drivers/hv/Kconfig"
source "drivers/devfreq/Kconfig"
+source "drivers/extcon/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 1b31421..38ed177 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -132,3 +132,4 @@ obj-$(CONFIG_VIRT_DRIVERS) += virt/
obj-$(CONFIG_HYPERV) += hv/
obj-$(CONFIG_PM_DEVFREQ) += devfreq/
+obj-$(CONFIG_EXTCON) += extcon/
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
new file mode 100644
index 0000000..bedf945
--- /dev/null
+++ b/drivers/extcon/Kconfig
@@ -0,0 +1,22 @@
+menuconfig EXTCON
+ bool "External Connector Class (extcon) support"
+ help
+ Say Y here to enable external connector class (extcon) support.
+ This allows monitoring external connectors by userspace
+ via sysfs and uevent and supports external connectors with
+ multiple states; i.e., an extcon that may have multiple
+ cables attached. For example, an external connector of a device
+ may be used to connect an HDMI cable and a AC adaptor, and to
+ host USB ports. Many of 30-pin connectors including PDMI are
+ also good examples.
+
+if EXTCON
+
+config EXTCON_GPIO
+ tristate "GPIO extcon support"
+ depends on GENERIC_GPIO
+ help
+ Say Y here to enable GPIO based extcon support. Note that GPIO
+ extcon supports single state per extcon instance.
+
+endif # MULTISTATE_SWITCH
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
new file mode 100644
index 0000000..2c46d41
--- /dev/null
+++ b/drivers/extcon/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for external connector class (extcon) devices
+#
+
+obj-$(CONFIG_EXTCON) += extcon_class.o
+obj-$(CONFIG_EXTCON_GPIO) += extcon_gpio.o
diff --git a/drivers/extcon/extcon_class.c b/drivers/extcon/extcon_class.c
new file mode 100644
index 0000000..39fbe9f
--- /dev/null
+++ b/drivers/extcon/extcon_class.c
@@ -0,0 +1,206 @@
+/*
+ * drivers/extcon/extcon_class.c
+ *
+ * External connector (extcon) class driver
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * Author: Donggeun Kim <dg77.kim@samsung.com>
+ * Author: MyungJoo Ham <myungjoo.ham@samsung.com>
+ *
+ * based on android/drivers/switch/switch_class.c
+ * Copyright (C) 2008 Google, Inc.
+ * Author: Mike Lockwood <lockwood@android.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ *
+*/
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/err.h>
+#include <linux/extcon.h>
+
+struct class *extcon_class;
+static atomic_t device_count;
+
+static ssize_t state_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct extcon_dev *edev = (struct extcon_dev *)
+ dev_get_drvdata(dev);
+
+ if (edev->print_state) {
+ int ret = edev->print_state(edev, buf);
+
+ if (ret >= 0)
+ return ret;
+ /* Use default if failed */
+ }
+ return sprintf(buf, "%u\n", edev->state);
+}
+
+static ssize_t name_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct extcon_dev *edev = (struct extcon_dev *)
+ dev_get_drvdata(dev);
+
+ if (edev->print_name) {
+ int ret = edev->print_name(edev, buf);
+ if (ret >= 0)
+ return ret;
+ }
+ return sprintf(buf, "%s\n", edev->name);
+}
+
+static DEVICE_ATTR(state, S_IRUGO, state_show, NULL);
+static DEVICE_ATTR(name, S_IRUGO, name_show, NULL);
+
+/**
+ * extcon_set_state() - Set the cable attach states of the extcon device.
+ * @edev: the extcon device
+ * @state: new cable attach status for @edev
+ */
+void extcon_set_state(struct extcon_dev *edev, u32 state)
+{
+ char name_buf[120];
+ char state_buf[120];
+ char *prop_buf;
+ char *envp[3];
+ int env_offset = 0;
+ int length;
+
+ if (edev->state != state) {
+ edev->state = state;
+
+ prop_buf = (char *)get_zeroed_page(GFP_KERNEL);
+ if (prop_buf) {
+ length = name_show(edev->dev, NULL, prop_buf);
+ if (length > 0) {
+ if (prop_buf[length - 1] == '\n')
+ prop_buf[length - 1] = 0;
+ snprintf(name_buf, sizeof(name_buf),
+ "SWITCH_NAME=%s", prop_buf);
+ envp[env_offset++] = name_buf;
+ }
+ length = state_show(edev->dev, NULL, prop_buf);
+ if (length > 0) {
+ if (prop_buf[length - 1] == '\n')
+ prop_buf[length - 1] = 0;
+ snprintf(state_buf, sizeof(state_buf),
+ "SWITCH_STATE=%s", prop_buf);
+ envp[env_offset++] = state_buf;
+ }
+ envp[env_offset] = NULL;
+ kobject_uevent_env(&edev->dev->kobj, KOBJ_CHANGE, envp);
+ free_page((unsigned long)prop_buf);
+ } else {
+ printk(KERN_ERR "out of memory in extcon_set_state\n");
+ kobject_uevent(&edev->dev->kobj, KOBJ_CHANGE);
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(extcon_set_state);
+
+static int create_extcon_class(void)
+{
+ if (!extcon_class) {
+ extcon_class = class_create(THIS_MODULE, "extcon");
+ if (IS_ERR(extcon_class))
+ return PTR_ERR(extcon_class);
+ atomic_set(&device_count, 0);
+ }
+
+ return 0;
+}
+
+/**
+ * extcon_dev_register() - Register a new extcon device
+ * @edev : the new extcon device (should be allocated before calling)
+ *
+ * Among the members of edev struct, please set the "user initializing data"
+ * in any case and set the "optional callbacks" if required. However, please
+ * do not set the values of "internal data", which are initialized by
+ * this function.
+ */
+int extcon_dev_register(struct extcon_dev *edev)
+{
+ int ret;
+
+ if (!extcon_class) {
+ ret = create_extcon_class();
+ if (ret < 0)
+ return ret;
+ }
+
+ edev->index = atomic_inc_return(&device_count);
+ edev->dev = device_create(extcon_class, NULL,
+ MKDEV(0, edev->index), NULL, edev->name);
+ if (IS_ERR(edev->dev))
+ return PTR_ERR(edev->dev);
+
+ ret = device_create_file(edev->dev, &dev_attr_state);
+ if (ret < 0)
+ goto err_create_file_1;
+ ret = device_create_file(edev->dev, &dev_attr_name);
+ if (ret < 0)
+ goto err_create_file_2;
+
+ dev_set_drvdata(edev->dev, edev);
+ edev->state = 0;
+ return 0;
+
+err_create_file_2:
+ device_remove_file(edev->dev, &dev_attr_state);
+err_create_file_1:
+ device_destroy(extcon_class, MKDEV(0, edev->index));
+ printk(KERN_ERR "extcon: Failed to register driver %s\n", edev->name);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(extcon_dev_register);
+
+/**
+ * extcon_dev_unregister() - Unregister the extcon device.
+ * @edev: the extcon device instance to be unregitered.
+ *
+ * Note that this does not call kfree(edev) because edev was not allocated
+ * by this class.
+ */
+void extcon_dev_unregister(struct extcon_dev *edev)
+{
+ device_remove_file(edev->dev, &dev_attr_name);
+ device_remove_file(edev->dev, &dev_attr_state);
+ device_destroy(extcon_class, MKDEV(0, edev->index));
+ dev_set_drvdata(edev->dev, NULL);
+}
+EXPORT_SYMBOL_GPL(extcon_dev_unregister);
+
+static int __init extcon_class_init(void)
+{
+ return create_extcon_class();
+}
+
+static void __exit extcon_class_exit(void)
+{
+ class_destroy(extcon_class);
+}
+
+module_init(extcon_class_init);
+module_exit(extcon_class_exit);
+
+MODULE_AUTHOR("Mike Lockwood <lockwood@android.com>");
+MODULE_AUTHOR("Donggeun Kim <dg77.kim@samsung.com>");
+MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@samsung.com>");
+MODULE_DESCRIPTION("External connector (extcon) class driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/extcon/extcon_gpio.c b/drivers/extcon/extcon_gpio.c
new file mode 100644
index 0000000..88202de
--- /dev/null
+++ b/drivers/extcon/extcon_gpio.c
@@ -0,0 +1,173 @@
+/*
+ * drivers/extcon/extcon_gpio.c
+ *
+ * Single-state GPIO extcon driver based on extcon class
+ *
+ * Copyright (C) 2008 Google, Inc.
+ * Author: Mike Lockwood <lockwood@android.com>
+ *
+ * Modified by MyungJoo Ham <myungjoo.ham@samsung.com> to support extcon
+ * (originally switch class is supported)
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ *
+*/
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/extcon.h>
+#include <linux/workqueue.h>
+#include <linux/gpio.h>
+
+struct gpio_extcon_data {
+ struct extcon_dev edev;
+ unsigned gpio;
+ const char *state_on;
+ const char *state_off;
+ int irq;
+ struct work_struct work;
+};
+
+static void gpio_extcon_work(struct work_struct *work)
+{
+ int state;
+ struct gpio_extcon_data *data =
+ container_of(work, struct gpio_extcon_data, work);
+
+ state = gpio_get_value(data->gpio);
+ extcon_set_state(&data->edev, state);
+}
+
+static irqreturn_t gpio_irq_handler(int irq, void *dev_id)
+{
+ struct gpio_extcon_data *extcon_data =
+ (struct gpio_extcon_data *)dev_id;
+
+ schedule_work(&extcon_data->work);
+ return IRQ_HANDLED;
+}
+
+static ssize_t extcon_gpio_print_state(struct extcon_dev *edev, char *buf)
+{
+ struct gpio_extcon_data *extcon_data =
+ container_of(edev, struct gpio_extcon_data, edev);
+ const char *state;
+ if (extcon_get_state(edev))
+ state = extcon_data->state_on;
+ else
+ state = extcon_data->state_off;
+
+ if (state)
+ return sprintf(buf, "%s\n", state);
+ return -1;
+}
+
+static int gpio_extcon_probe(struct platform_device *pdev)
+{
+ struct gpio_extcon_platform_data *pdata = pdev->dev.platform_data;
+ struct gpio_extcon_data *extcon_data;
+ int ret = 0;
+
+ if (!pdata)
+ return -EBUSY;
+
+ extcon_data = kzalloc(sizeof(struct gpio_extcon_data), GFP_KERNEL);
+ if (!extcon_data)
+ return -ENOMEM;
+
+ extcon_data->edev.name = pdata->name;
+ extcon_data->gpio = pdata->gpio;
+ extcon_data->state_on = pdata->state_on;
+ extcon_data->state_off = pdata->state_off;
+ extcon_data->edev.print_state = extcon_gpio_print_state;
+
+ ret = extcon_dev_register(&extcon_data->edev);
+ if (ret < 0)
+ goto err_extcon_dev_register;
+
+ ret = gpio_request(extcon_data->gpio, pdev->name);
+ if (ret < 0)
+ goto err_request_gpio;
+
+ ret = gpio_direction_input(extcon_data->gpio);
+ if (ret < 0)
+ goto err_set_gpio_input;
+
+ INIT_WORK(&extcon_data->work, gpio_extcon_work);
+
+ extcon_data->irq = gpio_to_irq(extcon_data->gpio);
+ if (extcon_data->irq < 0) {
+ ret = extcon_data->irq;
+ goto err_detect_irq_num_failed;
+ }
+
+ ret = request_irq(extcon_data->irq, gpio_irq_handler,
+ IRQF_TRIGGER_LOW, pdev->name, extcon_data);
+ if (ret < 0)
+ goto err_request_irq;
+
+ /* Perform initial detection */
+ gpio_extcon_work(&extcon_data->work);
+
+ return 0;
+
+err_request_irq:
+err_detect_irq_num_failed:
+err_set_gpio_input:
+ gpio_free(extcon_data->gpio);
+err_request_gpio:
+ extcon_dev_unregister(&extcon_data->edev);
+err_extcon_dev_register:
+ kfree(extcon_data);
+
+ return ret;
+}
+
+static int __devexit gpio_extcon_remove(struct platform_device *pdev)
+{
+ struct gpio_extcon_data *extcon_data = platform_get_drvdata(pdev);
+
+ cancel_work_sync(&extcon_data->work);
+ gpio_free(extcon_data->gpio);
+ extcon_dev_unregister(&extcon_data->edev);
+ kfree(extcon_data);
+
+ return 0;
+}
+
+static struct platform_driver gpio_extcon_driver = {
+ .probe = gpio_extcon_probe,
+ .remove = __devexit_p(gpio_extcon_remove),
+ .driver = {
+ .name = "extcon-gpio",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init gpio_extcon_init(void)
+{
+ return platform_driver_register(&gpio_extcon_driver);
+}
+
+static void __exit gpio_extcon_exit(void)
+{
+ platform_driver_unregister(&gpio_extcon_driver);
+}
+
+module_init(gpio_extcon_init);
+module_exit(gpio_extcon_exit);
+
+MODULE_AUTHOR("Mike Lockwood <lockwood@android.com>");
+MODULE_DESCRIPTION("GPIO extcon driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
new file mode 100644
index 0000000..9c92bfb
--- /dev/null
+++ b/include/linux/extcon.h
@@ -0,0 +1,101 @@
+/*
+ * External connector (extcon) class driver
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * Author: Donggeun Kim <dg77.kim@samsung.com>
+ * Author: MyungJoo Ham <myungjoo.ham@samsung.com>
+ *
+ * based on switch class driver
+ * Copyright (C) 2008 Google, Inc.
+ * Author: Mike Lockwood <lockwood@android.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ *
+*/
+
+#ifndef __LINUX_EXTCON_H__
+#define __LINUX_EXTCON_H__
+
+/**
+ * struct extcon_dev - An extcon device represents one external connector.
+ * @name The name of this extcon device. Please provide one when
+ * registering an extcon_dev.
+ * @print_name An optional callback to override the method to print the
+ * name of the extcon device.
+ * @print_state An optional callback to override the method to print the
+ * status of the extcon device.
+ * @dev Device of this extcon. Do not provide at register-time.
+ * @state Attach/detach state of this extcon. Do not provide at
+ * register-time
+ * @index Device index. Do not provide at register-time.
+ *
+ * In most cases, users only need to provide "User initializing data" of
+ * this struct when registering an extcon. In some exceptional cases,
+ * optional callbacks may be needed. However, the values in "internal data"
+ * are overwritten by register function.
+ */
+struct extcon_dev {
+ /* --- User initializing data --- */
+ const char *name;
+
+ /* --- Optional callbacks to override class functions --- */
+ ssize_t (*print_name)(struct extcon_dev *edev, char *buf);
+ ssize_t (*print_state)(struct extcon_dev *edev, char *buf);
+
+ /* --- Internal data. Please do not set. --- */
+ struct device *dev;
+ u32 state;
+ int index;
+};
+
+/**
+ * struct gpio_extcon_platform_data - A simple GPIO-controlled extcon device.
+ * @name The name of this GPIO extcon device.
+ * @gpio Corresponding GPIO.
+ * @state_on print_state is overriden with state_on if attached. If Null,
+ * default method of extcon class is used.
+ * @state_off print_state is overriden with state_on if dettached. If Null,
+ * default method of extcon class is used.
+ */
+struct gpio_extcon_platform_data {
+ const char *name;
+ unsigned gpio;
+
+ /* if NULL, "0" or "1" will be printed */
+ const char *state_on;
+ const char *state_off;
+};
+
+#ifdef CONFIG_EXTCON
+extern int extcon_dev_register(struct extcon_dev *edev);
+extern void extcon_dev_unregister(struct extcon_dev *edev);
+
+static inline u32 extcon_get_state(struct extcon_dev *edev)
+{
+ return edev->state;
+}
+
+extern void extcon_set_state(struct extcon_dev *edev, u32 state);
+#else /* CONFIG_EXTCON */
+static inline int extcon_dev_register(struct extcon_dev *edev)
+{
+ return 0;
+}
+
+static inline void extcon_dev_unregister(struct extcon_dev *edev) { }
+
+static inline u32 extcon_get_state(struct extcon_dev *edev)
+{
+ return 0;
+}
+
+static inline void extcon_set_state(struct extcon_dev *edev, u32 state) { }
+#endif /* CONFIG_EXTCON */
+#endif /* __LINUX_EXTCON_H__ */
--
1.7.4.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/3] Extcon (external connector): import Android's switch class and modify.
2011-12-14 10:28 ` [PATCH v2 1/3] Extcon (external connector): import Android's switch class and modify MyungJoo Ham
@ 2011-12-15 1:01 ` Greg KH
2011-12-15 5:41 ` MyungJoo Ham
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2011-12-15 1:01 UTC (permalink / raw)
To: MyungJoo Ham
Cc: linux-kernel, Randy Dunlap, Mike Lockwood,
Arve Hjønnevåg, Kyungmin Park, Donggeun Kim,
Arnd Bergmann, MyungJoo Ham, Linus Walleij, Dmitry Torokhov,
NeilBrown, Morten CHRISTIANSEN, Mark Brown
On Wed, Dec 14, 2011 at 07:28:26PM +0900, MyungJoo Ham wrote:
> External connector class (extcon) is based on and an extension of Android
> kernel's switch class located at linux/drivers/switch/.
> This patch provides the before-extension switch class moved to the
> location where the extcon will be located (linux/drivers/extcon/).
>
> The before-extension class, switch class of Android kernel, commits
> imported are:
>
> switch: switch class and GPIO drivers.
> Author: Mike Lockwood <lockwood@android.com>
>
> switch: gpio: Don't call request_irq with interrupts disabled
> Author: Arve Hjønnevåg <arve@android.com>
>
> switch: Use device_create instead of device_create_drvdata.
> Author: Arve Hjønnevåg <arve@android.com>
>
> switch_gpio: Add missing #include <linux/interrupt.h>
> Author: Mike Lockwood <lockwood@android.com>
>
> In this patch, upon the commits of Android kernel, we have added:
> - Relocated and renamed for extcon.
> - Comments, module name, and author information are updated
> - Code clean for successing patches
> - Bugfix: enabling write access without write functions
Nice, but if we accept this, will someone also make the needed changes
to the Android userspace code to handle the user api changes that this
causes?
> --- /dev/null
> +++ b/drivers/extcon/Kconfig
> @@ -0,0 +1,22 @@
> +menuconfig EXTCON
> + bool "External Connector Class (extcon) support"
Why can't this be a module?
> + help
> + Say Y here to enable external connector class (extcon) support.
> + This allows monitoring external connectors by userspace
> + via sysfs and uevent and supports external connectors with
> + multiple states; i.e., an extcon that may have multiple
> + cables attached. For example, an external connector of a device
> + may be used to connect an HDMI cable and a AC adaptor, and to
> + host USB ports. Many of 30-pin connectors including PDMI are
> + also good examples.
> +
> +if EXTCON
> +
> +config EXTCON_GPIO
> + tristate "GPIO extcon support"
> + depends on GENERIC_GPIO
> + help
> + Say Y here to enable GPIO based extcon support. Note that GPIO
> + extcon supports single state per extcon instance.
> +
> +endif # MULTISTATE_SWITCH
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> new file mode 100644
> index 0000000..2c46d41
> --- /dev/null
> +++ b/drivers/extcon/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for external connector class (extcon) devices
> +#
> +
> +obj-$(CONFIG_EXTCON) += extcon_class.o
> +obj-$(CONFIG_EXTCON_GPIO) += extcon_gpio.o
> diff --git a/drivers/extcon/extcon_class.c b/drivers/extcon/extcon_class.c
> new file mode 100644
> index 0000000..39fbe9f
> --- /dev/null
> +++ b/drivers/extcon/extcon_class.c
> @@ -0,0 +1,206 @@
> +/*
> + * drivers/extcon/extcon_class.c
> + *
> + * External connector (extcon) class driver
> + *
> + * Copyright (C) 2011 Samsung Electronics
> + * Author: Donggeun Kim <dg77.kim@samsung.com>
> + * Author: MyungJoo Ham <myungjoo.ham@samsung.com>
> + *
> + * based on android/drivers/switch/switch_class.c
> + * Copyright (C) 2008 Google, Inc.
> + * Author: Mike Lockwood <lockwood@android.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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.
> + *
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/err.h>
> +#include <linux/extcon.h>
> +
> +struct class *extcon_class;
> +static atomic_t device_count;
this should use the idr subsystem, not just an atomic variable, as you
aren't properly handling devices being removed (which is something
overall this core code needs to properly handle.)
> +
> +static ssize_t state_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct extcon_dev *edev = (struct extcon_dev *)
> + dev_get_drvdata(dev);
> +
> + if (edev->print_state) {
> + int ret = edev->print_state(edev, buf);
> +
> + if (ret >= 0)
> + return ret;
> + /* Use default if failed */
> + }
> + return sprintf(buf, "%u\n", edev->state);
> +}
> +
> +static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct extcon_dev *edev = (struct extcon_dev *)
> + dev_get_drvdata(dev);
> +
> + if (edev->print_name) {
> + int ret = edev->print_name(edev, buf);
> + if (ret >= 0)
> + return ret;
> + }
> + return sprintf(buf, "%s\n", edev->name);
> +}
Why is the name somehow different from the device name in the first
place?
> +/**
> + * extcon_set_state() - Set the cable attach states of the extcon device.
> + * @edev: the extcon device
> + * @state: new cable attach status for @edev
> + */
> +void extcon_set_state(struct extcon_dev *edev, u32 state)
> +{
> + char name_buf[120];
> + char state_buf[120];
> + char *prop_buf;
> + char *envp[3];
> + int env_offset = 0;
> + int length;
> +
> + if (edev->state != state) {
> + edev->state = state;
> +
> + prop_buf = (char *)get_zeroed_page(GFP_KERNEL);
> + if (prop_buf) {
> + length = name_show(edev->dev, NULL, prop_buf);
Heh, nice hack, but wouldn't it just be simpler to call the
evdev->print_name() function directly here?
> + if (length > 0) {
> + if (prop_buf[length - 1] == '\n')
> + prop_buf[length - 1] = 0;
> + snprintf(name_buf, sizeof(name_buf),
> + "SWITCH_NAME=%s", prop_buf);
> + envp[env_offset++] = name_buf;
> + }
> + length = state_show(edev->dev, NULL, prop_buf);
Same here.
> + if (length > 0) {
> + if (prop_buf[length - 1] == '\n')
> + prop_buf[length - 1] = 0;
> + snprintf(state_buf, sizeof(state_buf),
> + "SWITCH_STATE=%s", prop_buf);
> + envp[env_offset++] = state_buf;
> + }
> + envp[env_offset] = NULL;
> + kobject_uevent_env(&edev->dev->kobj, KOBJ_CHANGE, envp);
> + free_page((unsigned long)prop_buf);
> + } else {
> + printk(KERN_ERR "out of memory in extcon_set_state\n");
dev_err()?
> + kobject_uevent(&edev->dev->kobj, KOBJ_CHANGE);
I really dislike using uevents, what is listening for them? Are you
hooked into udev's event chain in userspace to properly handle this? If
not, what is the point of sending them?
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(extcon_set_state);
> +
> +static int create_extcon_class(void)
> +{
> + if (!extcon_class) {
> + extcon_class = class_create(THIS_MODULE, "extcon");
> + if (IS_ERR(extcon_class))
> + return PTR_ERR(extcon_class);
> + atomic_set(&device_count, 0);
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * extcon_dev_register() - Register a new extcon device
> + * @edev : the new extcon device (should be allocated before calling)
> + *
> + * Among the members of edev struct, please set the "user initializing data"
> + * in any case and set the "optional callbacks" if required. However, please
> + * do not set the values of "internal data", which are initialized by
> + * this function.
> + */
> +int extcon_dev_register(struct extcon_dev *edev)
> +{
> + int ret;
> +
> + if (!extcon_class) {
> + ret = create_extcon_class();
> + if (ret < 0)
> + return ret;
> + }
> +
> + edev->index = atomic_inc_return(&device_count);
> + edev->dev = device_create(extcon_class, NULL,
> + MKDEV(0, edev->index), NULL, edev->name);
> + if (IS_ERR(edev->dev))
> + return PTR_ERR(edev->dev);
> +
> + ret = device_create_file(edev->dev, &dev_attr_state);
> + if (ret < 0)
> + goto err_create_file_1;
> + ret = device_create_file(edev->dev, &dev_attr_name);
> + if (ret < 0)
> + goto err_create_file_2;
Why not use default attributes for the class? That makes this code go
away, and solves the error handling issues as well.
> +
> + dev_set_drvdata(edev->dev, edev);
> + edev->state = 0;
> + return 0;
> +
> +err_create_file_2:
> + device_remove_file(edev->dev, &dev_attr_state);
> +err_create_file_1:
> + device_destroy(extcon_class, MKDEV(0, edev->index));
> + printk(KERN_ERR "extcon: Failed to register driver %s\n", edev->name);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(extcon_dev_register);
> +
> +/**
> + * extcon_dev_unregister() - Unregister the extcon device.
> + * @edev: the extcon device instance to be unregitered.
> + *
> + * Note that this does not call kfree(edev) because edev was not allocated
> + * by this class.
> + */
> +void extcon_dev_unregister(struct extcon_dev *edev)
> +{
> + device_remove_file(edev->dev, &dev_attr_name);
> + device_remove_file(edev->dev, &dev_attr_state);
> + device_destroy(extcon_class, MKDEV(0, edev->index));
> + dev_set_drvdata(edev->dev, NULL);
> +}
> +EXPORT_SYMBOL_GPL(extcon_dev_unregister);
As stated before, you forgot to decrement the count, but use the idr
subsystem and remember to decrement it here also.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/3] Extcon (external connector): import Android's switch class and modify.
2011-12-15 1:01 ` Greg KH
@ 2011-12-15 5:41 ` MyungJoo Ham
2011-12-15 7:18 ` Greg KH
0 siblings, 1 reply; 16+ messages in thread
From: MyungJoo Ham @ 2011-12-15 5:41 UTC (permalink / raw)
To: Greg KH
Cc: linux-kernel, Randy Dunlap, Mike Lockwood,
Arve Hjønnevåg, Kyungmin Park, Donggeun Kim,
Arnd Bergmann, Linus Walleij, Dmitry Torokhov, NeilBrown,
Morten CHRISTIANSEN, Mark Brown
On Thu, Dec 15, 2011 at 10:01 AM, Greg KH <gregkh@suse.de> wrote:
> On Wed, Dec 14, 2011 at 07:28:26PM +0900, MyungJoo Ham wrote:
>> External connector class (extcon) is based on and an extension of Android
>> kernel's switch class located at linux/drivers/switch/.
>> This patch provides the before-extension switch class moved to the
>> location where the extcon will be located (linux/drivers/extcon/).
>>
>> The before-extension class, switch class of Android kernel, commits
>> imported are:
>>
>> switch: switch class and GPIO drivers.
>> Author: Mike Lockwood <lockwood@android.com>
>>
>> switch: gpio: Don't call request_irq with interrupts disabled
>> Author: Arve Hjønnevåg <arve@android.com>
>>
>> switch: Use device_create instead of device_create_drvdata.
>> Author: Arve Hjønnevåg <arve@android.com>
>>
>> switch_gpio: Add missing #include <linux/interrupt.h>
>> Author: Mike Lockwood <lockwood@android.com>
>>
>> In this patch, upon the commits of Android kernel, we have added:
>> - Relocated and renamed for extcon.
>> - Comments, module name, and author information are updated
>> - Code clean for successing patches
>> - Bugfix: enabling write access without write functions
>
> Nice, but if we accept this, will someone also make the needed changes
> to the Android userspace code to handle the user api changes that this
> causes?
I have no idea about how Android will react to this as I have no
developmental experiences with Android.
However, from the perspective of general userspace, this modification
incurs path changes (/sys/class/switch/.... to /sys/class/extcon/...)
only.
>
>> --- /dev/null
>> +++ b/drivers/extcon/Kconfig
>> @@ -0,0 +1,22 @@
>> +menuconfig EXTCON
>> + bool "External Connector Class (extcon) support"
>
> Why can't this be a module?
>
I also think this can be a module as well. I'm wondering why I've
changed it to bool from RFC patchset's tristate. ;(
>> +
>> +struct class *extcon_class;
>> +static atomic_t device_count;
>
> this should use the idr subsystem, not just an atomic variable, as you
> aren't properly handling devices being removed (which is something
> overall this core code needs to properly handle.)
>
It seems that I need to rewrite extcon_dev_register/unregister
functions. I was simply reusing that part from android kernel;
however, this time, they do look ugly.
>
>> +
>> +static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct extcon_dev *edev = (struct extcon_dev *)
>> + dev_get_drvdata(dev);
>> +
>> + if (edev->print_state) {
>> + int ret = edev->print_state(edev, buf);
>> +
>> + if (ret >= 0)
>> + return ret;
>> + /* Use default if failed */
>> + }
>> + return sprintf(buf, "%u\n", edev->state);
>> +}
>> +
>> +static ssize_t name_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct extcon_dev *edev = (struct extcon_dev *)
>> + dev_get_drvdata(dev);
>> +
>> + if (edev->print_name) {
>> + int ret = edev->print_name(edev, buf);
>> + if (ret >= 0)
>> + return ret;
>> + }
>> + return sprintf(buf, "%s\n", edev->name);
>> +}
>
> Why is the name somehow different from the device name in the first
> place?
I'll let dev_get_name() return what "name_show" wants. I guess I'll
keep name_show sysfs entry to be compatible with Android kernel
(except for the path) and keep the optional "print_name" callback.
>
>> +/**
>> + * extcon_set_state() - Set the cable attach states of the extcon device.
>> + * @edev: the extcon device
>> + * @state: new cable attach status for @edev
>> + */
>> +void extcon_set_state(struct extcon_dev *edev, u32 state)
>> +{
>> + char name_buf[120];
>> + char state_buf[120];
>> + char *prop_buf;
>> + char *envp[3];
>> + int env_offset = 0;
>> + int length;
>> +
>> + if (edev->state != state) {
>> + edev->state = state;
>> +
>> + prop_buf = (char *)get_zeroed_page(GFP_KERNEL);
>> + if (prop_buf) {
>> + length = name_show(edev->dev, NULL, prop_buf);
>
> Heh, nice hack, but wouldn't it just be simpler to call the
> evdev->print_name() function directly here?
I didn't do so because print_name callback is optional.
>
>> + if (length > 0) {
>> + if (prop_buf[length - 1] == '\n')
>> + prop_buf[length - 1] = 0;
>> + snprintf(name_buf, sizeof(name_buf),
>> + "SWITCH_NAME=%s", prop_buf);
>> + envp[env_offset++] = name_buf;
>> + }
>> + length = state_show(edev->dev, NULL, prop_buf);
>
> Same here.
Same here. print_state is also optional. (and I expect many won't use
these optional callbacks at least including MAX8997, MAX77693, and
FSA9480)
>> + printk(KERN_ERR "out of memory in extcon_set_state\n");
>
> dev_err()?
Yes, that looks better.
>
>> + kobject_uevent(&edev->dev->kobj, KOBJ_CHANGE);
>
> I really dislike using uevents, what is listening for them? Are you
> hooked into udev's event chain in userspace to properly handle this? If
> not, what is the point of sending them?
It is to let userspace processes get notified for the events in extcon.
Do you think sysfs_notify() would be better for this purpose?
>
>> +
>> + ret = device_create_file(edev->dev, &dev_attr_state);
>> + if (ret < 0)
>> + goto err_create_file_1;
>> + ret = device_create_file(edev->dev, &dev_attr_name);
>> + if (ret < 0)
>> + goto err_create_file_2;
>
> Why not use default attributes for the class? That makes this code go
> away, and solves the error handling issues as well.
Thanks for the remind! I'll use extcon_class->dev_attrs.
>> + */
>> +void extcon_dev_unregister(struct extcon_dev *edev)
>> +{
>> + device_remove_file(edev->dev, &dev_attr_name);
>> + device_remove_file(edev->dev, &dev_attr_state);
>> + device_destroy(extcon_class, MKDEV(0, edev->index));
>> + dev_set_drvdata(edev->dev, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>
> As stated before, you forgot to decrement the count, but use the idr
> subsystem and remember to decrement it here also.
>
Sure, I'll revise unregister function.
Thank you so much.
Cheers!
MyungJoo
--
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/3] Extcon (external connector): import Android's switch class and modify.
2011-12-15 5:41 ` MyungJoo Ham
@ 2011-12-15 7:18 ` Greg KH
2011-12-16 5:38 ` MyungJoo Ham
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2011-12-15 7:18 UTC (permalink / raw)
To: myungjoo.ham
Cc: linux-kernel, Randy Dunlap, Mike Lockwood,
Arve Hjønnevåg, Kyungmin Park, Donggeun Kim,
Arnd Bergmann, Linus Walleij, Dmitry Torokhov, NeilBrown,
Morten CHRISTIANSEN, Mark Brown
On Thu, Dec 15, 2011 at 02:41:38PM +0900, MyungJoo Ham wrote:
> On Thu, Dec 15, 2011 at 10:01 AM, Greg KH <gregkh@suse.de> wrote:
> > On Wed, Dec 14, 2011 at 07:28:26PM +0900, MyungJoo Ham wrote:
> >> External connector class (extcon) is based on and an extension of Android
> >> kernel's switch class located at linux/drivers/switch/.
> >> This patch provides the before-extension switch class moved to the
> >> location where the extcon will be located (linux/drivers/extcon/).
> >>
> >> The before-extension class, switch class of Android kernel, commits
> >> imported are:
> >>
> >> switch: switch class and GPIO drivers.
> >> Author: Mike Lockwood <lockwood@android.com>
> >>
> >> switch: gpio: Don't call request_irq with interrupts disabled
> >> Author: Arve Hjønnevåg <arve@android.com>
> >>
> >> switch: Use device_create instead of device_create_drvdata.
> >> Author: Arve Hjønnevåg <arve@android.com>
> >>
> >> switch_gpio: Add missing #include <linux/interrupt.h>
> >> Author: Mike Lockwood <lockwood@android.com>
> >>
> >> In this patch, upon the commits of Android kernel, we have added:
> >> - Relocated and renamed for extcon.
> >> - Comments, module name, and author information are updated
> >> - Code clean for successing patches
> >> - Bugfix: enabling write access without write functions
> >
> > Nice, but if we accept this, will someone also make the needed changes
> > to the Android userspace code to handle the user api changes that this
> > causes?
>
> I have no idea about how Android will react to this as I have no
> developmental experiences with Android.
> However, from the perspective of general userspace, this modification
> incurs path changes (/sys/class/switch/.... to /sys/class/extcon/...)
> only.
Well, without such changes, any Android platform will still have to
include the switch code in their system, making this work a bit
pointless, right?
Please look into changing this in userspace, if for no other reason than
to test that this kernel code works properly with the Android userspace
needs as well.
> >> + kobject_uevent(&edev->dev->kobj, KOBJ_CHANGE);
> >
> > I really dislike using uevents, what is listening for them? Are you
> > hooked into udev's event chain in userspace to properly handle this? If
> > not, what is the point of sending them?
>
> It is to let userspace processes get notified for the events in extcon.
> Do you think sysfs_notify() would be better for this purpose?
No, I don't think it does what you think it does :)
What are you trying to accomplish here? And how would sysfs_notify()
accomplish that?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] Extcon (external connector): import Android's switch class and modify.
2011-12-15 7:18 ` Greg KH
@ 2011-12-16 5:38 ` MyungJoo Ham
2011-12-16 18:18 ` Greg KH
0 siblings, 1 reply; 16+ messages in thread
From: MyungJoo Ham @ 2011-12-16 5:38 UTC (permalink / raw)
To: Greg KH
Cc: linux-kernel, Randy Dunlap, Mike Lockwood,
Arve Hjønnevåg, Kyungmin Park, Donggeun Kim,
Arnd Bergmann, Linus Walleij, Dmitry Torokhov, NeilBrown,
Morten CHRISTIANSEN, Mark Brown, android-kernel
On Thu, Dec 15, 2011 at 4:18 PM, Greg KH <gregkh@suse.de> wrote:
> On Thu, Dec 15, 2011 at 02:41:38PM +0900, MyungJoo Ham wrote:
>> On Thu, Dec 15, 2011 at 10:01 AM, Greg KH <gregkh@suse.de> wrote:
>> >
>> > Nice, but if we accept this, will someone also make the needed changes
>> > to the Android userspace code to handle the user api changes that this
>> > causes?
>>
>> I have no idea about how Android will react to this as I have no
>> developmental experiences with Android.
>> However, from the perspective of general userspace, this modification
>> incurs path changes (/sys/class/switch/.... to /sys/class/extcon/...)
>> only.
>
> Well, without such changes, any Android platform will still have to
> include the switch code in their system, making this work a bit
> pointless, right?
>
> Please look into changing this in userspace, if for no other reason than
> to test that this kernel code works properly with the Android userspace
> needs as well.
1. Kernel source with Android patch has "switch" class drivers at
/drivers/switch, which are not compatible with "extcon" class.
2. Android userspace access /sys/class/switch/ nodes, which are
compatible with extcon nodes.
Not to interfere with Android kernel patches, not to be confused with
incompatible Android switch class and its drivers, and to be
compatible with Android userspace at the same time, we may put the
device drivers located at Linux/drivers/extcon/ and let the class uses
/sys/class/switch/*.
I remember there were big no-es on using the name "switch" for these
external connectors; however, would this userspace class name be fine?
Or would it be fine to make the class name customizable? (let the
device driver choose whether to be named "switch" or "extcon" or let
the class define its location based on kernel config).
In order to get some comments from Android kernel builders, I've added
android-kernel@googlegroups.com.
>
>> >> + kobject_uevent(&edev->dev->kobj, KOBJ_CHANGE);
>> >
>> > I really dislike using uevents, what is listening for them? Are you
>> > hooked into udev's event chain in userspace to properly handle this? If
>> > not, what is the point of sending them?
>>
>> It is to let userspace processes get notified for the events in extcon.
>> Do you think sysfs_notify() would be better for this purpose?
>
> No, I don't think it does what you think it does :)
>
> What are you trying to accomplish here? And how would sysfs_notify()
> accomplish that?
Android has been "observing" kobject uevents. It has been observing
the switch class kobject (&edev->dev->kobj) and that has not been
changed from Android kernel.
In Android, with "startObserving" method, one can let a function to be
called with incoming UEvent.
I haven't look into how "startObserving" is implemented. However, they
do observe KOBJ_CHANGE at userspace. And this is not limted to
Android.
Here, my intention is the same. It is to invoke a function in a user
process that has registered to be invoked for a UEvent. If we are
going to use other methods for this, we will also force userspace
processes to change their methods to get events (at least for switch
class and chargers) not from UEVENT.
Maybe the userspace is abusing UEVENT/KOBJ_CHANGE, though... that's
what userspace observes to get events from kernel often.
Cheers!
MyungJoo
--
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] Extcon (external connector): import Android's switch class and modify.
2011-12-16 5:38 ` MyungJoo Ham
@ 2011-12-16 18:18 ` Greg KH
0 siblings, 0 replies; 16+ messages in thread
From: Greg KH @ 2011-12-16 18:18 UTC (permalink / raw)
To: MyungJoo Ham
Cc: linux-kernel, Randy Dunlap, Mike Lockwood,
Arve Hjønnevåg, Kyungmin Park, Donggeun Kim,
Arnd Bergmann, Linus Walleij, Dmitry Torokhov, NeilBrown,
Morten CHRISTIANSEN, Mark Brown, android-kernel
On Fri, Dec 16, 2011 at 02:38:38PM +0900, MyungJoo Ham wrote:
> On Thu, Dec 15, 2011 at 4:18 PM, Greg KH <gregkh@suse.de> wrote:
> > On Thu, Dec 15, 2011 at 02:41:38PM +0900, MyungJoo Ham wrote:
> >> On Thu, Dec 15, 2011 at 10:01 AM, Greg KH <gregkh@suse.de> wrote:
> >> >
> >> > Nice, but if we accept this, will someone also make the needed changes
> >> > to the Android userspace code to handle the user api changes that this
> >> > causes?
> >>
> >> I have no idea about how Android will react to this as I have no
> >> developmental experiences with Android.
> >> However, from the perspective of general userspace, this modification
> >> incurs path changes (/sys/class/switch/.... to /sys/class/extcon/...)
> >> only.
> >
> > Well, without such changes, any Android platform will still have to
> > include the switch code in their system, making this work a bit
> > pointless, right?
> >
> > Please look into changing this in userspace, if for no other reason than
> > to test that this kernel code works properly with the Android userspace
> > needs as well.
>
> 1. Kernel source with Android patch has "switch" class drivers at
> /drivers/switch, which are not compatible with "extcon" class.
> 2. Android userspace access /sys/class/switch/ nodes, which are
> compatible with extcon nodes.
>
> Not to interfere with Android kernel patches, not to be confused with
> incompatible Android switch class and its drivers, and to be
> compatible with Android userspace at the same time, we may put the
> device drivers located at Linux/drivers/extcon/ and let the class uses
> /sys/class/switch/*.
No, that's really not going to help as you changed the way the sysfs
files worked, right?
> I remember there were big no-es on using the name "switch" for these
> external connectors; however, would this userspace class name be fine?
> Or would it be fine to make the class name customizable? (let the
> device driver choose whether to be named "switch" or "extcon" or let
> the class define its location based on kernel config).
>
> In order to get some comments from Android kernel builders, I've added
> android-kernel@googlegroups.com.
Why can't you submit patches for the userspace Android code to use this
new api instead of their sys/class/switch/ code? If functionally it's
the same, it should be ok to do this, right?
> >> >> + kobject_uevent(&edev->dev->kobj, KOBJ_CHANGE);
> >> >
> >> > I really dislike using uevents, what is listening for them? Are you
> >> > hooked into udev's event chain in userspace to properly handle this? If
> >> > not, what is the point of sending them?
> >>
> >> It is to let userspace processes get notified for the events in extcon.
> >> Do you think sysfs_notify() would be better for this purpose?
> >
> > No, I don't think it does what you think it does :)
> >
> > What are you trying to accomplish here? And how would sysfs_notify()
> > accomplish that?
>
> Android has been "observing" kobject uevents. It has been observing
> the switch class kobject (&edev->dev->kobj) and that has not been
> changed from Android kernel.
> In Android, with "startObserving" method, one can let a function to be
> called with incoming UEvent.
> I haven't look into how "startObserving" is implemented. However, they
> do observe KOBJ_CHANGE at userspace. And this is not limted to
> Android.
It would be good to figure out how "observing" actually works here.
> Here, my intention is the same. It is to invoke a function in a user
> process that has registered to be invoked for a UEvent. If we are
> going to use other methods for this, we will also force userspace
> processes to change their methods to get events (at least for switch
> class and chargers) not from UEVENT.
>
> Maybe the userspace is abusing UEVENT/KOBJ_CHANGE, though... that's
> what userspace observes to get events from kernel often.
I don't mind using the kobject change call, I just want to be very
careful about it as there aren't many users of it in the kernel at the
moment, and very little userspace code that I know of that takes
advantage of it.
If the Android framework is using it, that's fine, it's one reason to
use it, but actually determining this would be a good thing for you to
do, right?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/3] Extcon: support notification based on the state changes.
2011-12-14 10:28 [PATCH v2 0/3] introduce External Connector Class (extcon) MyungJoo Ham
2011-12-14 10:28 ` [PATCH v2 1/3] Extcon (external connector): import Android's switch class and modify MyungJoo Ham
@ 2011-12-14 10:28 ` MyungJoo Ham
2011-12-14 10:28 ` [PATCH v2 3/3] Extcon: support multiple states at a device MyungJoo Ham
2011-12-15 2:32 ` [PATCH v2 0/3] introduce External Connector Class (extcon) NeilBrown
3 siblings, 0 replies; 16+ messages in thread
From: MyungJoo Ham @ 2011-12-14 10:28 UTC (permalink / raw)
To: linux-kernel
Cc: Randy Dunlap, Mike Lockwood, Arve Hjønnevåg,
Kyungmin Park, Donggeun Kim, Greg KH, Arnd Bergmann, MyungJoo Ham,
Linus Walleij, Dmitry Torokhov, NeilBrown, Morten CHRISTIANSEN,
Mark Brown
From: Donggeun Kim <dg77.kim@samsung.com>
State changes of extcon devices have been notified via kobjet_uevent.
This patch adds notifier interfaces in order to allow device drivers to
get notified easily. Along with notifier interface,
extcon_get_extcon_dev() function is added so that device drivers may
discover a extcon_dev easily.
Signed-off-by: Donggeun Kim <dg77.kim@samsung.com>
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
--
Changes from RFC
- Renamed switch to extcon
- Bugfix: extcon_dev_unregister()
- Bugfix: "edev->dev" is "internal" data.
- Added kerneldoc comments.
- Reworded comments.
---
drivers/extcon/extcon_class.c | 67 +++++++++++++++++++++++++++++++++++++++++
include/linux/extcon.h | 39 ++++++++++++++++++++++++
2 files changed, 106 insertions(+), 0 deletions(-)
diff --git a/drivers/extcon/extcon_class.c b/drivers/extcon/extcon_class.c
index 39fbe9f..0223897 100644
--- a/drivers/extcon/extcon_class.c
+++ b/drivers/extcon/extcon_class.c
@@ -33,6 +33,9 @@
struct class *extcon_class;
static atomic_t device_count;
+static LIST_HEAD(extcon_dev_list);
+static DEFINE_MUTEX(extcon_dev_list_lock);
+
static ssize_t state_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
@@ -70,6 +73,9 @@ static DEVICE_ATTR(name, S_IRUGO, name_show, NULL);
* extcon_set_state() - Set the cable attach states of the extcon device.
* @edev: the extcon device
* @state: new cable attach status for @edev
+ *
+ * Note that notifier provides the which bits are changes in the state
+ * variable with "val" to the callback.
*/
void extcon_set_state(struct extcon_dev *edev, u32 state)
{
@@ -79,10 +85,14 @@ void extcon_set_state(struct extcon_dev *edev, u32 state)
char *envp[3];
int env_offset = 0;
int length;
+ u32 old_state = edev->state;
if (edev->state != state) {
edev->state = state;
+ raw_notifier_call_chain(&edev->nh, old_state ^ edev->state,
+ edev);
+
prop_buf = (char *)get_zeroed_page(GFP_KERNEL);
if (prop_buf) {
length = name_show(edev->dev, NULL, prop_buf);
@@ -112,6 +122,51 @@ void extcon_set_state(struct extcon_dev *edev, u32 state)
}
EXPORT_SYMBOL_GPL(extcon_set_state);
+/**
+ * extcon_get_extcon_dev() - Get the extcon device instance from the name
+ * @extcon_name: The extcon name provided with extcon_dev_register()
+ */
+struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
+{
+ struct extcon_dev *sd;
+
+ mutex_lock(&extcon_dev_list_lock);
+ list_for_each_entry(sd, &extcon_dev_list, entry) {
+ if (!strcmp(sd->name, extcon_name))
+ goto out;
+ }
+ sd = NULL;
+out:
+ mutex_unlock(&extcon_dev_list_lock);
+ return sd;
+}
+EXPORT_SYMBOL_GPL(extcon_get_extcon_dev);
+
+/**
+ * extcon_register_notifier() - Register a notifee to get notified by
+ * any attach status changes from the extcon.
+ * @edev: the extcon device.
+ * @nb: a notifier block to be registered.
+ */
+int extcon_register_notifier(struct extcon_dev *edev,
+ struct notifier_block *nb)
+{
+ return raw_notifier_chain_register(&edev->nh, nb);
+}
+EXPORT_SYMBOL_GPL(extcon_register_notifier);
+
+/**
+ * extcon_unregister_notifier() - Unregister a notifee from the extcon device.
+ * @edev: the extcon device.
+ * @nb: a registered notifier block to be unregistered.
+ */
+int extcon_unregister_notifier(struct extcon_dev *edev,
+ struct notifier_block *nb)
+{
+ return raw_notifier_chain_unregister(&edev->nh, nb);
+}
+EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
+
static int create_extcon_class(void)
{
if (!extcon_class) {
@@ -156,8 +211,15 @@ int extcon_dev_register(struct extcon_dev *edev)
if (ret < 0)
goto err_create_file_2;
+ RAW_INIT_NOTIFIER_HEAD(&edev->nh);
+
dev_set_drvdata(edev->dev, edev);
edev->state = 0;
+
+ mutex_lock(&extcon_dev_list_lock);
+ list_add(&edev->entry, &extcon_dev_list);
+ mutex_unlock(&extcon_dev_list_lock);
+
return 0;
err_create_file_2:
@@ -182,6 +244,11 @@ void extcon_dev_unregister(struct extcon_dev *edev)
device_remove_file(edev->dev, &dev_attr_name);
device_remove_file(edev->dev, &dev_attr_state);
device_destroy(extcon_class, MKDEV(0, edev->index));
+
+ mutex_lock(&extcon_dev_list_lock);
+ list_del(&edev->entry);
+ mutex_unlock(&extcon_dev_list_lock);
+
dev_set_drvdata(edev->dev, NULL);
}
EXPORT_SYMBOL_GPL(extcon_dev_unregister);
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index 9c92bfb..64b57a2 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -23,6 +23,7 @@
#ifndef __LINUX_EXTCON_H__
#define __LINUX_EXTCON_H__
+#include <linux/notifier.h>
/**
* struct extcon_dev - An extcon device represents one external connector.
* @name The name of this extcon device. Please provide one when
@@ -35,6 +36,9 @@
* @state Attach/detach state of this extcon. Do not provide at
* register-time
* @index Device index. Do not provide at register-time.
+ * @nh Notifier for the state change events from this extcon
+ * @entry To support list of extcon devices so that uses can search
+ * for extcon devices based on the extcon name.
*
* In most cases, users only need to provide "User initializing data" of
* this struct when registering an extcon. In some exceptional cases,
@@ -53,6 +57,8 @@ struct extcon_dev {
struct device *dev;
u32 state;
int index;
+ struct raw_notifier_head nh;
+ struct list_head entry;
};
/**
@@ -74,8 +80,15 @@ struct gpio_extcon_platform_data {
};
#ifdef CONFIG_EXTCON
+
+/*
+ * Following APIs are for notifiers or configurations.
+ * Notifiers are the external port and connection devices.
+ */
+
extern int extcon_dev_register(struct extcon_dev *edev);
extern void extcon_dev_unregister(struct extcon_dev *edev);
+extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
static inline u32 extcon_get_state(struct extcon_dev *edev)
{
@@ -83,6 +96,15 @@ static inline u32 extcon_get_state(struct extcon_dev *edev)
}
extern void extcon_set_state(struct extcon_dev *edev, u32 state);
+
+/*
+ * Following APIs are to monitor every action of a notifier.
+ * Registerer gets notified for every external port of a connection device.
+ */
+extern int extcon_register_notifier(struct extcon_dev *edev,
+ struct notifier_block *nb);
+extern int extcon_unregister_notifier(struct extcon_dev *edev,
+ struct notifier_block *nb);
#else /* CONFIG_EXTCON */
static inline int extcon_dev_register(struct extcon_dev *edev)
{
@@ -97,5 +119,22 @@ static inline u32 extcon_get_state(struct extcon_dev *edev)
}
static inline void extcon_set_state(struct extcon_dev *edev, u32 state) { }
+static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
+{
+ return NULL;
+}
+
+static inline int extcon_register_notifier(struct extcon_dev *edev,
+ struct notifier_block *nb)
+{
+ return 0;
+}
+
+static inline int extcon_unregister_notifier(struct extcon_dev *edev,
+ struct notifier_block *nb)
+{
+ return 0;
+}
+
#endif /* CONFIG_EXTCON */
#endif /* __LINUX_EXTCON_H__ */
--
1.7.4.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 3/3] Extcon: support multiple states at a device.
2011-12-14 10:28 [PATCH v2 0/3] introduce External Connector Class (extcon) MyungJoo Ham
2011-12-14 10:28 ` [PATCH v2 1/3] Extcon (external connector): import Android's switch class and modify MyungJoo Ham
2011-12-14 10:28 ` [PATCH v2 2/3] Extcon: support notification based on the state changes MyungJoo Ham
@ 2011-12-14 10:28 ` MyungJoo Ham
2011-12-15 2:32 ` [PATCH v2 0/3] introduce External Connector Class (extcon) NeilBrown
3 siblings, 0 replies; 16+ messages in thread
From: MyungJoo Ham @ 2011-12-14 10:28 UTC (permalink / raw)
To: linux-kernel
Cc: Randy Dunlap, Mike Lockwood, Arve Hjønnevåg,
Kyungmin Park, Donggeun Kim, Greg KH, Arnd Bergmann, MyungJoo Ham,
Linus Walleij, Dmitry Torokhov, NeilBrown, Morten CHRISTIANSEN,
Mark Brown
One switch device (e.g., MUIC(MAX8997, MAX77686, ...), and some 30-pin
devices) may have multiple cables attached. For example, one
30-pin port may inhabit a USB cable, an HDMI cable, and a mic.
Thus, one switch device requires multiple state bits each representing
a type of cable.
For such purpose, we use the 32bit state variable; thus, up to 32
different type of cables may be defined for a switch device. The list of
possible cables is defined by the array of cable names in the switch_dev
struct given to the class.
Signed-off-by: Donggeun Kim <dg77.kim@samsung.com>
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
--
Changes from RFC
- Switch is renamed to extcon
- Added kerneldoc comments
- Added APIs to support "standard" cable names
- Added helper APIs to support notifier block registration with cable
name.
- Regrouped function list in the header file.
---
drivers/extcon/extcon_class.c | 255 +++++++++++++++++++++++++++++++++++++++--
include/linux/extcon.h | 149 ++++++++++++++++++++++++
2 files changed, 395 insertions(+), 9 deletions(-)
diff --git a/drivers/extcon/extcon_class.c b/drivers/extcon/extcon_class.c
index 0223897..72f6e3e 100644
--- a/drivers/extcon/extcon_class.c
+++ b/drivers/extcon/extcon_class.c
@@ -30,6 +30,25 @@
#include <linux/err.h>
#include <linux/extcon.h>
+const char *extcon_cable_name[] = {
+ [EXTCON_USB] = "USB",
+ [EXTCON_USB_HOST] = "USB-Host",
+ [EXTCON_TA] = "TA",
+ [EXTCON_FAST_CHARGER] = "Fast-charger",
+ [EXTCON_SLOW_CHARGER] = "Slow-charger",
+ [EXTCON_CHARGE_DOWNSTREAM] = "Charge-downstream",
+ [EXTCON_HDMI] = "HDMI",
+ [EXTCON_MHL] = "MHL",
+ [EXTCON_DVI] = "DVI",
+ [EXTCON_VGA] = "VGA",
+ [EXTCON_DOCK] = "Dock",
+ [EXTCON_AUDIO_IN] = "Audio-in",
+ [EXTCON_AUDIO_OUT] = "Audio-out",
+ [EXTCON_VIDEO_IN] = "Video-in",
+ [EXTCON_VIDEO_OUT] = "Video-out",
+
+ NULL,
+};
struct class *extcon_class;
static atomic_t device_count;
@@ -39,6 +58,7 @@ static DEFINE_MUTEX(extcon_dev_list_lock);
static ssize_t state_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
+ int i, count = 0;
struct extcon_dev *edev = (struct extcon_dev *)
dev_get_drvdata(dev);
@@ -49,7 +69,22 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr,
return ret;
/* Use default if failed */
}
- return sprintf(buf, "%u\n", edev->state);
+
+ if (edev->max_supported == 0)
+ return sprintf(buf, "%u\n", edev->state);
+
+ for (i = 0; i < SUPPORTED_CABLE_MAX; i++) {
+ if (!edev->supported_cable[i])
+ break;
+ if (i > 0)
+ count += sprintf(buf + count, ", ");
+ count += sprintf(buf + count, "%s %d",
+ edev->supported_cable[i],
+ !!(edev->state & (1 << i)));
+ }
+ count += sprintf(buf + count, "\n");
+
+ return count;
}
static ssize_t name_show(struct device *dev, struct device_attribute *attr,
@@ -70,14 +105,16 @@ static DEVICE_ATTR(state, S_IRUGO, state_show, NULL);
static DEVICE_ATTR(name, S_IRUGO, name_show, NULL);
/**
- * extcon_set_state() - Set the cable attach states of the extcon device.
+ * extcon_update_state() - Update the cable attach states of the extcon device
+ * only for the masked bits.
* @edev: the extcon device
+ * @mask: the bit mask to designate updated bits.
* @state: new cable attach status for @edev
*
- * Note that notifier provides the which bits are changes in the state
- * variable with "val" to the callback.
+ * Note that the notifier provides which bits are changed in the state
+ * variable with the val parameter (second) to the callback.
*/
-void extcon_set_state(struct extcon_dev *edev, u32 state)
+void extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
{
char name_buf[120];
char state_buf[120];
@@ -85,10 +122,15 @@ void extcon_set_state(struct extcon_dev *edev, u32 state)
char *envp[3];
int env_offset = 0;
int length;
- u32 old_state = edev->state;
+ unsigned long flags;
+
+ spin_lock_irqsave(&edev->lock, flags);
- if (edev->state != state) {
- edev->state = state;
+ if (edev->state != ((edev->state & ~mask) | (state & mask))) {
+ u32 old_state = edev->state;
+
+ edev->state &= ~mask;
+ edev->state |= state & mask;
raw_notifier_call_chain(&edev->nh, old_state ^ edev->state,
edev);
@@ -112,17 +154,132 @@ void extcon_set_state(struct extcon_dev *edev, u32 state)
envp[env_offset++] = state_buf;
}
envp[env_offset] = NULL;
+ /* Unlock early before uevent */
+ spin_unlock_irqrestore(&edev->lock, flags);
+
kobject_uevent_env(&edev->dev->kobj, KOBJ_CHANGE, envp);
free_page((unsigned long)prop_buf);
} else {
+ /* Unlock early before uevent */
+ spin_unlock_irqrestore(&edev->lock, flags);
+
printk(KERN_ERR "out of memory in extcon_set_state\n");
kobject_uevent(&edev->dev->kobj, KOBJ_CHANGE);
}
+ } else {
+ /* No changes */
+ spin_unlock_irqrestore(&edev->lock, flags);
}
}
+EXPORT_SYMBOL_GPL(extcon_update_state);
+
+/**
+ * extcon_set_state() - Set the cable attach states of the extcon device.
+ * @edev: the extcon device
+ * @state: new cable attach status for @edev
+ *
+ * Note that notifier provides which bits are changed in the state
+ * variable with the val parameter (second) to the callback.
+ */
+void extcon_set_state(struct extcon_dev *edev, u32 state)
+{
+ extcon_update_state(edev, 0xffffffff, state);
+}
EXPORT_SYMBOL_GPL(extcon_set_state);
/**
+ * extcon_find_cable_index() - Get the cable index based on the cable name.
+ * @edev: the extcon device that has the cable.
+ * @cable_name: cable name to be searched.
+ *
+ * Note that accessing a cable state based on cable_index is faster than
+ * cable_name because using cable_name induces a loop with strncmp().
+ * Thus, when get/set_cable_state is repeatedly used, using cable_index
+ * is recommended.
+ */
+int extcon_find_cable_index(struct extcon_dev *edev, const char *cable_name)
+{
+ int i;
+
+ if (edev->supported_cable) {
+ for (i = 0; edev->supported_cable[i]; i++) {
+ if (!strncmp(edev->supported_cable[i],
+ cable_name, CABLE_NAME_MAX))
+ return i;
+ }
+ }
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(extcon_find_cable_index);
+
+/**
+ * extcon_get_cable_state_() - Get the status of a specific cable.
+ * @edev: the extcon device that has the cable.
+ * @index: cable index that can be retrieved by extcon_find_cable_index().
+ */
+int extcon_get_cable_state_(struct extcon_dev *edev, int index)
+{
+ if (index < 0 || (edev->max_supported && edev->max_supported <= index))
+ return -EINVAL;
+
+ return !!(edev->state & (1 << index));
+}
+EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
+
+/**
+ * extcon_get_cable_state() - Get the status of a specific cable.
+ * @edev: the extcon device that has the cable.
+ * @cable_name: cable name.
+ *
+ * Note that this is slower than extcon_get_cable_state_.
+ */
+int extcon_get_cable_state(struct extcon_dev *edev, const char *cable_name)
+{
+ return extcon_get_cable_state_(edev, extcon_find_cable_index
+ (edev, cable_name));
+}
+EXPORT_SYMBOL_GPL(extcon_get_cable_state);
+
+/**
+ * extcon_get_cable_state_() - Set the status of a specific cable.
+ * @edev: the extcon device that has the cable.
+ * @index: cable index that can be retrieved by extcon_find_cable_index().
+ * @cable_state: the new cable status. The default semantics is
+ * true: attached / false: detached.
+ */
+int extcon_set_cable_state_(struct extcon_dev *edev,
+ int index, bool cable_state)
+{
+ u32 state;
+
+ if (index < 0 || (edev->max_supported && edev->max_supported <= index))
+ return -EINVAL;
+
+ state = cable_state ? (1 << index) : 0;
+ extcon_update_state(edev, 1 << index, state);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
+
+/**
+ * extcon_get_cable_state() - Set the status of a specific cable.
+ * @edev: the extcon device that has the cable.
+ * @cable_name: cable name.
+ * @cable_state: the new cable status. The default semantics is
+ * true: attached / false: detached.
+ *
+ * Note that this is slower than extcon_set_cable_state_.
+ */
+int extcon_set_cable_state(struct extcon_dev *edev,
+ const char *cable_name, bool cable_state)
+{
+ return extcon_set_cable_state_(edev, extcon_find_cable_index
+ (edev, cable_name), cable_state);
+}
+EXPORT_SYMBOL_GPL(extcon_set_cable_state);
+
+/**
* extcon_get_extcon_dev() - Get the extcon device instance from the name
* @extcon_name: The extcon name provided with extcon_dev_register()
*/
@@ -142,6 +299,71 @@ out:
}
EXPORT_SYMBOL_GPL(extcon_get_extcon_dev);
+static int _call_per_cable(struct notifier_block *nb, unsigned long val,
+ void *ptr)
+{
+ struct extcon_specific_cable_nb *obj = container_of(nb,
+ struct extcon_specific_cable_nb, internal_nb);
+
+ if (val & (1 << obj->cable_index))
+ return obj->user_nb->notifier_call(nb, val, ptr);
+
+ return NOTIFY_OK;
+}
+
+/**
+ * extcon_register_interest() - Register a notifier for a state change of a
+ * specific cable, not a entier set of cables of a
+ * extcon device.
+ * @obj: an empty extcon_specific_cable_nb object to be returned.
+ * @extcon_name: the name of extcon device.
+ * @cable_name: the target cable name.
+ * @nb: the notifier block to get notified.
+ *
+ * Provide an empty extcon_specific_cable_nb. extcon_register_interest() sets
+ * the struct for you.
+ *
+ * extcon_register_interest is a helper function for those who want to get
+ * notification for a single specific cable's status change. If a user wants
+ * to get notification for any changes of all cables of a extcon device,
+ * he/she should use the general extcon_register_notifier().
+ */
+int extcon_register_interest(struct extcon_specific_cable_nb *obj,
+ const char *extcon_name, const char *cable_name,
+ struct notifier_block *nb)
+{
+ if (!obj || !extcon_name || !cable_name || !nb)
+ return -EINVAL;
+
+ obj->edev = extcon_get_extcon_dev(extcon_name);
+ if (!obj->edev)
+ return -ENODEV;
+
+ obj->cable_index = extcon_find_cable_index(obj->edev, cable_name);
+ if (obj->cable_index < 0)
+ return -ENODEV;
+
+ obj->user_nb = nb;
+
+ obj->internal_nb.notifier_call = _call_per_cable;
+
+ return raw_notifier_chain_register(&obj->edev->nh, &obj->internal_nb);
+}
+
+/**
+ * extcon_unregister_interest() - Unregister the notifier registered by
+ * extcon_register_interest().
+ * @obj: the extcon_specific_cable_nb object returned by
+ * extcon_register_interest().
+ */
+int extcon_unregister_interest(struct extcon_specific_cable_nb *obj)
+{
+ if (!obj)
+ return -EINVAL;
+
+ return raw_notifier_chain_unregister(&obj->edev->nh, &obj->internal_nb);
+}
+
/**
* extcon_register_notifier() - Register a notifee to get notified by
* any attach status changes from the extcon.
@@ -190,7 +412,7 @@ static int create_extcon_class(void)
*/
int extcon_dev_register(struct extcon_dev *edev)
{
- int ret;
+ int ret, index = 0;
if (!extcon_class) {
ret = create_extcon_class();
@@ -198,6 +420,21 @@ int extcon_dev_register(struct extcon_dev *edev)
return ret;
}
+ if (edev->supported_cable) {
+ /* Get size of array */
+ for (index = 0; edev->supported_cable[index]; index++)
+ ;
+ edev->max_supported = index;
+ } else {
+ edev->max_supported = 0;
+ }
+
+ if (index > SUPPORTED_CABLE_MAX) {
+ dev_err(edev->dev, "extcon: maximum number of supported cables exceeded.\n");
+ return -EINVAL;
+ }
+
+ spin_lock_init(&edev->lock);
edev->index = atomic_inc_return(&device_count);
edev->dev = device_create(extcon_class, NULL,
MKDEV(0, edev->index), NULL, edev->name);
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index 64b57a2..77be7b1 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -24,10 +24,54 @@
#define __LINUX_EXTCON_H__
#include <linux/notifier.h>
+
+#define SUPPORTED_CABLE_MAX 32
+#define CABLE_NAME_MAX 30
+
+/*
+ * The standard cable name is to help support general notifier
+ * and notifee device drivers to share the common names.
+ * Please use standard cable names unless your notifier device has
+ * a very unique and abnormal cable or
+ * the cable type is supposed to be used with only one unique
+ * pair of notifier/notifee devices.
+ *
+ * Please add any other "standard" cables used with extcon dev.
+ *
+ * You may add a dot and number to specify version or specification
+ * of the specific cable if it is required. (e.g., "Fast-charger.18"
+ * and "Fast-charger.10" for 1.8A and 1.0A chargers)
+ * However, the notifee and notifier should be able to handle such
+ * string and if the notifee can negotiate the protocol or idenify,
+ * you don't need such convention. This convention is helpful when
+ * notifier can distinguish but notifiee cannot.
+ */
+enum extcon_cable_name {
+ EXTCON_USB = 0,
+ EXTCON_USB_HOST,
+ EXTCON_TA, /* Travel Adaptor */
+ EXTCON_FAST_CHARGER,
+ EXTCON_SLOW_CHARGER,
+ EXTCON_CHARGE_DOWNSTREAM, /* Charging an external device */
+ EXTCON_HDMI,
+ EXTCON_MHL,
+ EXTCON_DVI,
+ EXTCON_VGA,
+ EXTCON_DOCK,
+ EXTCON_AUDIO_IN,
+ EXTCON_AUDIO_OUT,
+ EXTCON_VIDEO_IN,
+ EXTCON_VIDEO_OUT,
+};
+extern const char *extcon_cable_name[];
+
/**
* struct extcon_dev - An extcon device represents one external connector.
* @name The name of this extcon device. Please provide one when
* registering an extcon_dev.
+ * @supported_cable Array of supported cable name ending with NULL.
+ * If supported_cable is NULL, cable name related APIs
+ * are disabled.
* @print_name An optional callback to override the method to print the
* name of the extcon device.
* @print_state An optional callback to override the method to print the
@@ -39,6 +83,8 @@
* @nh Notifier for the state change events from this extcon
* @entry To support list of extcon devices so that uses can search
* for extcon devices based on the extcon name.
+ * @lock
+ * @max_supported Internal value to store the number of cables.
*
* In most cases, users only need to provide "User initializing data" of
* this struct when registering an extcon. In some exceptional cases,
@@ -48,6 +94,7 @@
struct extcon_dev {
/* --- User initializing data --- */
const char *name;
+ const char **supported_cable;
/* --- Optional callbacks to override class functions --- */
ssize_t (*print_name)(struct extcon_dev *edev, char *buf);
@@ -59,6 +106,8 @@ struct extcon_dev {
int index;
struct raw_notifier_head nh;
struct list_head entry;
+ spinlock_t lock; /* could be called by irq handler */
+ int max_supported;
};
/**
@@ -79,6 +128,21 @@ struct gpio_extcon_platform_data {
const char *state_off;
};
+/**
+ * struct extcon_specific_cable_nb - An internal data for
+ * extcon_register_interest().
+ * @internal_nb a notifier block bridging extcon notifier and cable notifier.
+ * @user_nb user provided notifier block for events from a specific cable.
+ * @cable_index the target cable.
+ * @edev the target extcon device.
+ */
+struct extcon_specific_cable_nb {
+ struct notifier_block internal_nb;
+ struct notifier_block *user_nb;
+ int cable_index;
+ struct extcon_dev *edev;
+};
+
#ifdef CONFIG_EXTCON
/*
@@ -90,16 +154,54 @@ extern int extcon_dev_register(struct extcon_dev *edev);
extern void extcon_dev_unregister(struct extcon_dev *edev);
extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
+/*
+ * get/set/update_state access the 32b encoded state value, which represents
+ * states of all possible cables of the multistate port. For example, if one
+ * calls extcon_set_states(edev, 0x7), it may mean that all the three cables
+ * are attached to the port.
+ */
static inline u32 extcon_get_state(struct extcon_dev *edev)
{
return edev->state;
}
extern void extcon_set_state(struct extcon_dev *edev, u32 state);
+extern void extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state);
+
+/*
+ * get/set_cable_state access each bit of the 32b encoded state value.
+ * They are used to access the status of each cable based on the cable_name
+ * or cable_index, which is retrived by extcon_find_cable_index
+ */
+extern int extcon_find_cable_index(struct extcon_dev *sdev,
+ const char *cable_name);
+extern int extcon_get_cable_state_(struct extcon_dev *edev, int cable_index);
+extern int extcon_set_cable_state_(struct extcon_dev *edev, int cable_index,
+ bool cable_state);
+
+extern int extcon_get_cable_state(struct extcon_dev *edev,
+ const char *cable_name);
+extern int extcon_set_cable_state(struct extcon_dev *edev,
+ const char *cable_name, bool cable_state);
+
+/*
+ * Following APIs are for notifiees (those who want to be notified)
+ * to register a callback for events from a specific cable of the extcon.
+ * Notifiees are the connected device drivers wanting to get notified by
+ * a specific external port of a connection device.
+ */
+extern int extcon_register_interest(struct extcon_specific_cable_nb *obj,
+ const char *extcon_name,
+ const char *cable_name,
+ struct notifier_block *nb);
+extern int extcon_unregister_interest(struct extcon_specific_cable_nb *nb);
/*
* Following APIs are to monitor every action of a notifier.
* Registerer gets notified for every external port of a connection device.
+ * Probably this could be used to debug an action of notifier; however,
+ * we do not recommend to use this at normal 'notifiee' device drivers who
+ * want to be notified by a specific external port of the notifier.
*/
extern int extcon_register_notifier(struct extcon_dev *edev,
struct notifier_block *nb);
@@ -119,6 +221,41 @@ static inline u32 extcon_get_state(struct extcon_dev *edev)
}
static inline void extcon_set_state(struct extcon_dev *edev, u32 state) { }
+
+static inline void extcon_update_state(struct extcon_dev *edev, u32 mask,
+ u32 state)
+{ }
+
+static inline int extcon_find_cable_index(struct extcon_dev *edev,
+ const char *cable_name)
+{
+ return 0;
+}
+
+static inline int extcon_get_cable_state_(struct extcon_dev *edev,
+ int cable_index)
+{
+ return 0;
+}
+
+static inline int extcon_set_cable_state_(struct extcon_dev *edev,
+ int cable_index, bool cable_state)
+{
+ return 0;
+}
+
+static inline int extcon_get_cable_state(struct extcon_dev *edev,
+ const char *cable_name)
+{
+ return 0;
+}
+
+static inline int extcon_set_cable_state(struct extcon_dev *edev,
+ const char *cable_name, int state)
+{
+ return 0;
+}
+
static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
{
return NULL;
@@ -136,5 +273,17 @@ static inline int extcon_unregister_notifier(struct extcon_dev *edev,
return 0;
}
+static inline int extcon_register_interest(struct extcon_specific_cable_nb *obj,
+ const char *extcon_name,
+ const char *cable_name,
+ struct notifier_block *nb)
+{
+ return 0;
+}
+
+static inline int extcon_unregister_interest(struct extcon_specific_cable_nb *)
+{
+ return 0;
+}
#endif /* CONFIG_EXTCON */
#endif /* __LINUX_EXTCON_H__ */
--
1.7.4.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 0/3] introduce External Connector Class (extcon)
2011-12-14 10:28 [PATCH v2 0/3] introduce External Connector Class (extcon) MyungJoo Ham
` (2 preceding siblings ...)
2011-12-14 10:28 ` [PATCH v2 3/3] Extcon: support multiple states at a device MyungJoo Ham
@ 2011-12-15 2:32 ` NeilBrown
2011-12-15 6:36 ` MyungJoo Ham
2011-12-15 6:51 ` Mark Brown
3 siblings, 2 replies; 16+ messages in thread
From: NeilBrown @ 2011-12-15 2:32 UTC (permalink / raw)
To: MyungJoo Ham
Cc: linux-kernel, Randy Dunlap, Mike Lockwood,
Arve Hjønnevåg, Kyungmin Park, Donggeun Kim, Greg KH,
Arnd Bergmann, MyungJoo Ham, Linus Walleij, Dmitry Torokhov,
Morten CHRISTIANSEN, Mark Brown
[-- Attachment #1: Type: text/plain, Size: 4233 bytes --]
On Wed, 14 Dec 2011 19:28:25 +0900 MyungJoo Ham <myungjoo.ham@samsung.com>
wrote:
> Note that previously, the patchset has been submitted as
> "Multistate Switch Class".
>
> For external connectors, which may have different types of cables attached
> (USB, TA, HDMI, Analog A/V, and others), we often have seperated device
> drivers that detect the state changes at the port and device drivers that
> do something according to the state changes.
>
> For example, when MAX8997-MUIC detects a Charger cable insertion, another
> device driver (such as MAX8903 charger, MAX8997 charger, Charger Manager,
> or board file) needs to set charger current limit accordingly and when
> MAX8997-MUIC detects a HDMI cable insertion, multimedia device drivers
> need to do some operations accordingly.
Hi,
a few comments...
1/ I still don't understand why this needs to a single device which can
report multiple cables, rather than simply having multiple devices each
of which reports a single cable at a time.
i.e. A physical 32 bit port might be represented as several 'extcon'
devices each of which reports a different function: USB, HDMI, Charger,
etc.
The fact that the devices are related can be made clear by the choice of
port names.
If these is a good reason, it needs to be included in the patch
somewhere, possibly in a file in Documentation, so that when someone comes
along to use this class in a year or two they can understand all the
consequences of any decision they make (and so that I stop asking now:-)
2/ When you have multiple cable options, the 'state' sysfs file looks
like e.g.
USB 0, HDMI 1, DVI 1
Having list values in sysfs files is generally frowned upon but sometimes
is necessary. However I'm not sure this is a good format to use.
There aren't a whole lot of examples to follow (because it is so frowned
upon) but one option is
USB [HDMI] [DVI]
where the selected option(s) are in square brackets.
Another might be
USB=0
HDMI=1
DVI=1
with newlines - just like in the 'uevent' file.
Also I think that some pairs of cables are mutually exclusive while others
aren't. This fact isn't made apparent in the 'state' file. Maybe it
should be.
Whatever format is used should be documented in the
Documentation/ABI file.
Of course this problem would go away if you didn't allow multiple
concurrent cables per port.
3/ The use of extcon_get_extcon_dev() requires that the port device be
registered before the device that wants to be notified by it. Ensuring
correct ordering of device discovery is (I believe) not always easy -
particularly with module loading.
Would it make sense to instead have one device register as a
switch-provider and the other device register as a switch-listener and as
soon as they both exist they get connected: a bit like how 'devices' and
'drivers' can be registered in any order.
Maybe the same device/driver infrastructure could be reused (an extcon
bus maybe?) but I'm not really familiar enough with it to say (Greg??).
What I do know is that I have repeatedly tripped over the fact that you
cannot use a GPIO line until the gpiochip has been registered and
sometimes the device that needs it is registered before the device which
provides it. I wouldn't like to see that happening here too.
Are there other examples of inter-device dependencies that can be used as
a model?
>
> This patchset supports the usage of notifier for passing such information
> between device drivers.
>
> Another issue is that at a single switch port, there might be multiple
> and heterogeneous cables attached at the same time. Besides the state
> (Attached or Detached) of each cable may alter independently.
>
> In order to address such issues, Android kernel's "Switch" class seems to
> be a good basis and we have implemented "Multistate Switch Class" based on
> it. The "Switch" class code of Android kernel is GPL as well.
Do you need to update this text to remove "Multistate Switch Class" ??
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 0/3] introduce External Connector Class (extcon)
2011-12-15 2:32 ` [PATCH v2 0/3] introduce External Connector Class (extcon) NeilBrown
@ 2011-12-15 6:36 ` MyungJoo Ham
2011-12-15 20:20 ` NeilBrown
2011-12-15 6:51 ` Mark Brown
1 sibling, 1 reply; 16+ messages in thread
From: MyungJoo Ham @ 2011-12-15 6:36 UTC (permalink / raw)
To: NeilBrown
Cc: linux-kernel, Randy Dunlap, Mike Lockwood,
Arve Hjønnevåg, Kyungmin Park, Donggeun Kim, Greg KH,
Arnd Bergmann, Linus Walleij, Dmitry Torokhov,
Morten CHRISTIANSEN, Mark Brown
On Thu, Dec 15, 2011 at 11:32 AM, NeilBrown <neilb@suse.de> wrote:
> On Wed, 14 Dec 2011 19:28:25 +0900 MyungJoo Ham <myungjoo.ham@samsung.com>
> wrote:
>
>> Note that previously, the patchset has been submitted as
>> "Multistate Switch Class".
>>
>> For external connectors, which may have different types of cables attached
>> (USB, TA, HDMI, Analog A/V, and others), we often have seperated device
>> drivers that detect the state changes at the port and device drivers that
>> do something according to the state changes.
>>
>> For example, when MAX8997-MUIC detects a Charger cable insertion, another
>> device driver (such as MAX8903 charger, MAX8997 charger, Charger Manager,
>> or board file) needs to set charger current limit accordingly and when
>> MAX8997-MUIC detects a HDMI cable insertion, multimedia device drivers
>> need to do some operations accordingly.
>
> Hi,
> a few comments...
>
> 1/ I still don't understand why this needs to a single device which can
> report multiple cables, rather than simply having multiple devices each
> of which reports a single cable at a time.
>
> i.e. A physical 32 bit port might be represented as several 'extcon'
> devices each of which reports a different function: USB, HDMI, Charger,
> etc.
>
Implementing what you've suggested would require another device driver
or framework to relay notifications from the physical 32 bit port to
several 'extcon' devices unless the physical 32 bit port has 32 IRQ
lines or addresses. If it is implemented in a device driver, we will
reimplement the same feature in every multi-port device drivers. If we
make another framework, we are simply seperating extcon framework into
two seperated frameworks.
It is because the "report" comes from a single device ("notifier":
implementing extcon device) and multiple cables and devices using the
cables need to listen to the "report" ("notifiee": registering
notifier block to the extcon device). With the FSA9480 USB Switch,
MAX8997 MUIC, or MAX77693 MUIC representing an external connector with
multiple cables possible, it is impossible to create one device to
handle events from one cable. We simply have only one GPIO for the
interrupt and one I2C slave address for them. Even when we implement
one device for one cable, we need something working as a "network
router" between the connector(WAN port) and cables (LAN ports), which
is "extcon" in this case.
If we implement an extcon-like device to handle each possible cable
state and notify devices according to every cable state, then, it
would like implementing extcon class code at each extcon-like device
driver. We would need to implement this method (registering cable
interest) in every extcon-like device (MUIC / USB Switch / ...)
> The fact that the devices are related can be made clear by the choice of
> port names.
>
> If these is a good reason, it needs to be included in the patch
> somewhere, possibly in a file in Documentation, so that when someone comes
> along to use this class in a year or two they can understand all the
> consequences of any decision they make (and so that I stop asking now:-)
With the current patchset,
extcon_register_interest(null_obj, "max8997-muic.0", "HDMI", nb_callback);
registers the notifier-block callback for a specific cable "HDMI" of
max8997-muic.0
Do you mean to include a usage description like above in
Documentation? If so, I'll make one under linux/Documentation/extcon/
later.
>
> 2/ When you have multiple cable options, the 'state' sysfs file looks
> like e.g.
> USB 0, HDMI 1, DVI 1
>
> Having list values in sysfs files is generally frowned upon but sometimes
> is necessary. However I'm not sure this is a good format to use.
> There aren't a whole lot of examples to follow (because it is so frowned
> upon) but one option is
>
> USB [HDMI] [DVI]
>
> where the selected option(s) are in square brackets.
> Another might be
>
> USB=0
> HDMI=1
> DVI=1
>
> with newlines - just like in the 'uevent' file.
Yes. This looks much better. Thanks.
>
> Also I think that some pairs of cables are mutually exclusive while others
> aren't. This fact isn't made apparent in the 'state' file. Maybe it
> should be.
>
I think as long as the notifier (extcon class device) is the only one
who is supposed to have the right to write and userspace cannot write
state value, I don't think this is required. Such relation should be
dealt in the notifier device and all the others who are "readers" do
not need to know such detail.
> Whatever format is used should be documented in the
> Documentation/ABI file.
Yes, I will add that one.
>
> Of course this problem would go away if you didn't allow multiple
> concurrent cables per port.
>
> 3/ The use of extcon_get_extcon_dev() requires that the port device be
> registered before the device that wants to be notified by it. Ensuring
> correct ordering of device discovery is (I believe) not always easy -
> particularly with module loading.
>
> Would it make sense to instead have one device register as a
> switch-provider and the other device register as a switch-listener and as
> soon as they both exist they get connected: a bit like how 'devices' and
> 'drivers' can be registered in any order.
> Maybe the same device/driver infrastructure could be reused (an extcon
> bus maybe?) but I'm not really familiar enough with it to say (Greg??).
>
> What I do know is that I have repeatedly tripped over the fact that you
> cannot use a GPIO line until the gpiochip has been registered and
> sometimes the device that needs it is registered before the device which
> provides it. I wouldn't like to see that happening here too.
>
> Are there other examples of inter-device dependencies that can be used as
> a model?
Yes, this has been sometimes a problem for me as well.
What if let "extcon_register_interest" returns <0 for errors, "1" if
successfully registered, and "0" if the extcon name does not exists
(probably will be "probed" later?) and register the requested interest
later when the corresponding extcon is probed?
Then, I'll need to add "bool effective" in the struct
extcon_specific_cable_nb so that the notifee (caller of
extcon_register_interest()) my know if the interest is effectively
registered or still pending.
Also, I may need to add an option to extcon_register_interest stating
whether it is ok to be "pending" if extcon_name does not exist or is
should return error if extcon_name does not exist.
Anyway, it seems that extcon_register_interest is the only one that
needs to care this inter-device dependencies because
extcon_register/unregister_interest are the only ones supposed to be
called by notifee (depending devices).
>
>
>
>>
>> This patchset supports the usage of notifier for passing such information
>> between device drivers.
>>
>> Another issue is that at a single switch port, there might be multiple
>> and heterogeneous cables attached at the same time. Besides the state
>> (Attached or Detached) of each cable may alter independently.
>>
>> In order to address such issues, Android kernel's "Switch" class seems to
>> be a good basis and we have implemented "Multistate Switch Class" based on
>> it. The "Switch" class code of Android kernel is GPL as well.
>
> Do you need to update this text to remove "Multistate Switch Class" ??
>
Yes, I do.
Thank you for your comments.
Cheers!
MyungJoo
--
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 0/3] introduce External Connector Class (extcon)
2011-12-15 6:36 ` MyungJoo Ham
@ 2011-12-15 20:20 ` NeilBrown
0 siblings, 0 replies; 16+ messages in thread
From: NeilBrown @ 2011-12-15 20:20 UTC (permalink / raw)
To: myungjoo.ham
Cc: myungjoo.ham, linux-kernel, Randy Dunlap, Mike Lockwood,
Arve Hjønnevåg, Kyungmin Park, Donggeun Kim, Greg KH,
Arnd Bergmann, Linus Walleij, Dmitry Torokhov,
Morten CHRISTIANSEN, Mark Brown
[-- Attachment #1: Type: text/plain, Size: 9957 bytes --]
On Thu, 15 Dec 2011 15:36:15 +0900 MyungJoo Ham <myungjoo.ham@samsung.com>
wrote:
> On Thu, Dec 15, 2011 at 11:32 AM, NeilBrown <neilb@suse.de> wrote:
> > On Wed, 14 Dec 2011 19:28:25 +0900 MyungJoo Ham <myungjoo.ham@samsung.com>
> > wrote:
> >
> >> Note that previously, the patchset has been submitted as
> >> "Multistate Switch Class".
> >>
> >> For external connectors, which may have different types of cables attached
> >> (USB, TA, HDMI, Analog A/V, and others), we often have seperated device
> >> drivers that detect the state changes at the port and device drivers that
> >> do something according to the state changes.
> >>
> >> For example, when MAX8997-MUIC detects a Charger cable insertion, another
> >> device driver (such as MAX8903 charger, MAX8997 charger, Charger Manager,
> >> or board file) needs to set charger current limit accordingly and when
> >> MAX8997-MUIC detects a HDMI cable insertion, multimedia device drivers
> >> need to do some operations accordingly.
> >
> > Hi,
> > a few comments...
> >
> > 1/ I still don't understand why this needs to a single device which can
> > report multiple cables, rather than simply having multiple devices each
> > of which reports a single cable at a time.
> >
> > i.e. A physical 32 bit port might be represented as several 'extcon'
> > devices each of which reports a different function: USB, HDMI, Charger,
> > etc.
> >
>
> Implementing what you've suggested would require another device driver
> or framework to relay notifications from the physical 32 bit port to
> several 'extcon' devices unless the physical 32 bit port has 32 IRQ
> lines or addresses. If it is implemented in a device driver, we will
> reimplement the same feature in every multi-port device drivers. If we
> make another framework, we are simply seperating extcon framework into
> two seperated frameworks.
>
> It is because the "report" comes from a single device ("notifier":
> implementing extcon device) and multiple cables and devices using the
> cables need to listen to the "report" ("notifiee": registering
> notifier block to the extcon device). With the FSA9480 USB Switch,
> MAX8997 MUIC, or MAX77693 MUIC representing an external connector with
> multiple cables possible, it is impossible to create one device to
> handle events from one cable. We simply have only one GPIO for the
> interrupt and one I2C slave address for them. Even when we implement
> one device for one cable, we need something working as a "network
> router" between the connector(WAN port) and cables (LAN ports), which
> is "extcon" in this case.
>
> If we implement an extcon-like device to handle each possible cable
> state and notify devices according to every cable state, then, it
> would like implementing extcon class code at each extcon-like device
> driver. We would need to implement this method (registering cable
> interest) in every extcon-like device (MUIC / USB Switch / ...)
I don't think this is nearly as complicated as you seem to be presenting it.
It is perfectly normal for a device to register multiple sub-devices.
Possibly of the same class, possibly different. And then to manage them all.
So in your case you would have an i2c device which is the whole port and it
would register a collection of extcon devices and send notifications through
them as needed.
Some other port might be implemented by an spi device and it would register a
different set of extcon devices. The only code overlap between the two would
be a simple 'for' loop.
Consider for example a typical 'led' driver, say leds-adp5520.c
It manages multiple LEDs. But that doesn't mean we need a multi-led class.
We just have a 'led' class and adp5520_led_probe has a for loop to register
all the different led devices that it needs.
It holds these in an array and can do something to them whenever it needs.
(It only needs to do something to everything when shutting down, so it isn't
a perfect example but it will have to do).
So your one device driver would register a set of extcon devices, keep them
in an array and whenever something changes it would use a for loop to walk
the array and send a notification on each device.
Client ("notifiee") devices would register just with the particular
function(s) they are interested in.
>
> > The fact that the devices are related can be made clear by the choice of
> > port names.
> >
> > If these is a good reason, it needs to be included in the patch
> > somewhere, possibly in a file in Documentation, so that when someone comes
> > along to use this class in a year or two they can understand all the
> > consequences of any decision they make (and so that I stop asking now:-)
>
> With the current patchset,
>
> extcon_register_interest(null_obj, "max8997-muic.0", "HDMI", nb_callback);
>
> registers the notifier-block callback for a specific cable "HDMI" of
> max8997-muic.0
>
> Do you mean to include a usage description like above in
> Documentation? If so, I'll make one under linux/Documentation/extcon/
> later.
I'm not exactly sure what I meant... Just that the justification for your
design choices weren't obvious, so I felt they needed to be documented
somehow. I'm still hoping to change your design choices :-)
>
> >
> > 2/ When you have multiple cable options, the 'state' sysfs file looks
> > like e.g.
> > USB 0, HDMI 1, DVI 1
> >
> > Having list values in sysfs files is generally frowned upon but sometimes
> > is necessary. However I'm not sure this is a good format to use.
> > There aren't a whole lot of examples to follow (because it is so frowned
> > upon) but one option is
> >
> > USB [HDMI] [DVI]
> >
> > where the selected option(s) are in square brackets.
> > Another might be
> >
> > USB=0
> > HDMI=1
> > DVI=1
> >
> > with newlines - just like in the 'uevent' file.
>
> Yes. This looks much better. Thanks.
>
> >
> > Also I think that some pairs of cables are mutually exclusive while others
> > aren't. This fact isn't made apparent in the 'state' file. Maybe it
> > should be.
> >
>
> I think as long as the notifier (extcon class device) is the only one
> who is supposed to have the right to write and userspace cannot write
> state value, I don't think this is required. Such relation should be
> dealt in the notifier device and all the others who are "readers" do
> not need to know such detail.
It is always best to provide complete information - you never know when it
might be handy.
Suppose I wanted to write a little widget that displays an image on my
display of what sort of cable was attached. Knowing that some were mutually
exclusive would allow for a more meaningful display.
>
> > Whatever format is used should be documented in the
> > Documentation/ABI file.
>
> Yes, I will add that one.
>
> >
> > Of course this problem would go away if you didn't allow multiple
> > concurrent cables per port.
> >
> > 3/ The use of extcon_get_extcon_dev() requires that the port device be
> > registered before the device that wants to be notified by it. Ensuring
> > correct ordering of device discovery is (I believe) not always easy -
> > particularly with module loading.
> >
> > Would it make sense to instead have one device register as a
> > switch-provider and the other device register as a switch-listener and as
> > soon as they both exist they get connected: a bit like how 'devices' and
> > 'drivers' can be registered in any order.
> > Maybe the same device/driver infrastructure could be reused (an extcon
> > bus maybe?) but I'm not really familiar enough with it to say (Greg??).
> >
> > What I do know is that I have repeatedly tripped over the fact that you
> > cannot use a GPIO line until the gpiochip has been registered and
> > sometimes the device that needs it is registered before the device which
> > provides it. I wouldn't like to see that happening here too.
> >
> > Are there other examples of inter-device dependencies that can be used as
> > a model?
>
> Yes, this has been sometimes a problem for me as well.
>
> What if let "extcon_register_interest" returns <0 for errors, "1" if
> successfully registered, and "0" if the extcon name does not exists
> (probably will be "probed" later?) and register the requested interest
> later when the corresponding extcon is probed?
>
> Then, I'll need to add "bool effective" in the struct
> extcon_specific_cable_nb so that the notifee (caller of
> extcon_register_interest()) my know if the interest is effectively
> registered or still pending.
>
> Also, I may need to add an option to extcon_register_interest stating
> whether it is ok to be "pending" if extcon_name does not exist or is
> should return error if extcon_name does not exist.
>
> Anyway, it seems that extcon_register_interest is the only one that
> needs to care this inter-device dependencies because
> extcon_register/unregister_interest are the only ones supposed to be
> called by notifee (depending devices).
I think that extcon_register_interest should never fail (except maybe with
-ENOMEM).
The "interest" can be registered even if the port doesn't exist yet.
Every time an extcon device is registered, the extcon module looks through
the list of pending registrations and completes any connection that matches.
When the connection is completed I suspect it would be appropriate to send a
notification with the initial state - that is the only way the client would
find out that the registration was complete.
It might be good to eventually make a generic library for this so that other
resource providers can support async registration.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] introduce External Connector Class (extcon)
2011-12-15 2:32 ` [PATCH v2 0/3] introduce External Connector Class (extcon) NeilBrown
2011-12-15 6:36 ` MyungJoo Ham
@ 2011-12-15 6:51 ` Mark Brown
2011-12-18 7:15 ` NeilBrown
1 sibling, 1 reply; 16+ messages in thread
From: Mark Brown @ 2011-12-15 6:51 UTC (permalink / raw)
To: NeilBrown
Cc: MyungJoo Ham, linux-kernel, Randy Dunlap, Mike Lockwood,
Arve Hjønnevåg, Kyungmin Park, Donggeun Kim, Greg KH,
Arnd Bergmann, MyungJoo Ham, Linus Walleij, Dmitry Torokhov,
Morten CHRISTIANSEN
On Thu, Dec 15, 2011 at 01:32:29PM +1100, NeilBrown wrote:
j> 3/ The use of extcon_get_extcon_dev() requires that the port device be
> registered before the device that wants to be notified by it. Ensuring
> correct ordering of device discovery is (I believe) not always easy -
> particularly with module loading.
This is a massive problem throughout the kernel
> Would it make sense to instead have one device register as a
> switch-provider and the other device register as a switch-listener and as
> soon as they both exist they get connected: a bit like how 'devices' and
> 'drivers' can be registered in any order.
> Maybe the same device/driver infrastructure could be reused (an extcon
> bus maybe?) but I'm not really familiar enough with it to say (Greg??).
Grant has a proposal for this which revolves around devices trying to
acquire their resources and returning a "please retry" error code if
they don't have all their dependencies. Half the problem here is that
coming up with the dependency graph (and finding ways to talk about
devices that haven't been enumerated yet) is a very hard problem.
> Are there other examples of inter-device dependencies that can be used as
> a model?
The current solution is to fiddle with initcall order so that drivers
for devices that other devices depend on enumerate first.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] introduce External Connector Class (extcon)
2011-12-15 6:51 ` Mark Brown
@ 2011-12-18 7:15 ` NeilBrown
2011-12-20 1:01 ` Mark Brown
0 siblings, 1 reply; 16+ messages in thread
From: NeilBrown @ 2011-12-18 7:15 UTC (permalink / raw)
To: Mark Brown
Cc: MyungJoo Ham, linux-kernel, Randy Dunlap, Mike Lockwood,
Arve Hjønnevåg, Kyungmin Park, Donggeun Kim, Greg KH,
Arnd Bergmann, MyungJoo Ham, Linus Walleij, Dmitry Torokhov,
Morten CHRISTIANSEN
[-- Attachment #1: Type: text/plain, Size: 2948 bytes --]
On Thu, 15 Dec 2011 14:51:55 +0800 Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Dec 15, 2011 at 01:32:29PM +1100, NeilBrown wrote:
>
> j> 3/ The use of extcon_get_extcon_dev() requires that the port device be
> > registered before the device that wants to be notified by it. Ensuring
> > correct ordering of device discovery is (I believe) not always easy -
> > particularly with module loading.
>
> This is a massive problem throughout the kernel
So it seems. I just tripped over it with regulators. When one is supplied by
another they need to be probed in the right order.
Sounds like we need to find a general solution.
>
> > Would it make sense to instead have one device register as a
> > switch-provider and the other device register as a switch-listener and as
> > soon as they both exist they get connected: a bit like how 'devices' and
> > 'drivers' can be registered in any order.
> > Maybe the same device/driver infrastructure could be reused (an extcon
> > bus maybe?) but I'm not really familiar enough with it to say (Greg??).
>
> Grant has a proposal for this which revolves around devices trying to
> acquire their resources and returning a "please retry" error code if
> they don't have all their dependencies. Half the problem here is that
> coming up with the dependency graph (and finding ways to talk about
> devices that haven't been enumerated yet) is a very hard problem.
I can see how an -EAGAIN might work ... and it would exercise a lot of error
paths that otherwise never get called - I wonder if that is a good thing :-)
It doesn't feel quite like the right solution though. Deciding when to retry,
and where to queue the device/driver pairs might be messy.
A possibility I have been thinking about is to multithread do_initcalls() and
have the various request functions (gpio_request, regulator_get, request_irq,
etc) optionally block if the resource isn't available.
We wouldn't create a new thread for every initcall, just when all current
threads are blocked. And threads that have to block die once they finish
their initcall, so there is only ever one that is allowed to move on to the
'next' initcall.
Do we need to talk about devices that haven't been enumerated yet? or was
that just if we wanted to create an explicit dependency graph?
We would need names for resources that haven't been registered yet but that
seems fairly with with simple string names assigned in the board file (or
eventually the device-tree file I guess).
I might try hacking something together and see how badly it breaks.
>
> > Are there other examples of inter-device dependencies that can be used as
> > a model?
>
> The current solution is to fiddle with initcall order so that drivers
> for devices that other devices depend on enumerate first.
:-( I was afraid of that.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] introduce External Connector Class (extcon)
2011-12-18 7:15 ` NeilBrown
@ 2011-12-20 1:01 ` Mark Brown
2011-12-20 5:58 ` NeilBrown
0 siblings, 1 reply; 16+ messages in thread
From: Mark Brown @ 2011-12-20 1:01 UTC (permalink / raw)
To: NeilBrown
Cc: MyungJoo Ham, linux-kernel, Randy Dunlap, Mike Lockwood,
Arve Hjønnevåg, Kyungmin Park, Donggeun Kim, Greg KH,
Arnd Bergmann, MyungJoo Ham, Linus Walleij, Dmitry Torokhov,
Morten CHRISTIANSEN
On Sun, Dec 18, 2011 at 06:15:50PM +1100, NeilBrown wrote:
> On Thu, 15 Dec 2011 14:51:55 +0800 Mark Brown
> > Grant has a proposal for this which revolves around devices trying to
> > acquire their resources and returning a "please retry" error code if
> > they don't have all their dependencies. Half the problem here is that
> A possibility I have been thinking about is to multithread do_initcalls() and
> have the various request functions (gpio_request, regulator_get, request_irq,
> etc) optionally block if the resource isn't available.
That seems to be logically the same in terms of what it actually does
but introduces concurrency which wasn't there before which means that
things could get reordered for random reasons. That seems like it'd not
be great for robustness.
> Do we need to talk about devices that haven't been enumerated yet? or was
> that just if we wanted to create an explicit dependency graph?
You need to know about devices that aren't enumerated yet.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] introduce External Connector Class (extcon)
2011-12-20 1:01 ` Mark Brown
@ 2011-12-20 5:58 ` NeilBrown
0 siblings, 0 replies; 16+ messages in thread
From: NeilBrown @ 2011-12-20 5:58 UTC (permalink / raw)
To: Mark Brown
Cc: MyungJoo Ham, linux-kernel, Randy Dunlap, Mike Lockwood,
Arve Hjønnevåg, Kyungmin Park, Donggeun Kim, Greg KH,
Arnd Bergmann, MyungJoo Ham, Linus Walleij, Dmitry Torokhov,
Morten CHRISTIANSEN
[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]
On Tue, 20 Dec 2011 01:01:09 +0000 Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sun, Dec 18, 2011 at 06:15:50PM +1100, NeilBrown wrote:
> > On Thu, 15 Dec 2011 14:51:55 +0800 Mark Brown
>
> > > Grant has a proposal for this which revolves around devices trying to
> > > acquire their resources and returning a "please retry" error code if
> > > they don't have all their dependencies. Half the problem here is that
>
> > A possibility I have been thinking about is to multithread do_initcalls() and
> > have the various request functions (gpio_request, regulator_get, request_irq,
> > etc) optionally block if the resource isn't available.
>
> That seems to be logically the same in terms of what it actually does
> but introduces concurrency which wasn't there before which means that
> things could get reordered for random reasons. That seems like it'd not
> be great for robustness.
Logically similar..
One important difference is that (if I understand it correctly), the "please
retry" error need to be propagated up a lot further than it currently is so
that would be quite intrusive.
While my approach does introduce concurrency which wasn't there before, any
driver that can be built as a module must allow for concurrency in the init
function so I suspect (or hope) people normally make that code thread-safe.
I have some code that almost works (will post when it does work). The only
real issue I have found (apart from personal incompetence) is that we really
need to know up-front what resources are going to be available so we know
whether to wait or to fail (waiting until 'everything is finished' won't work
as there could be several things still waiting at the end and we don't know if
one is waiting for another, of if they are waiting for things that won't
appear).
I imagine that ultimately we can depend on a device-tree file to list every
resource (gpio, irq, regulator) that is relevant here and can write the code
to wait-or-fail accordingly.
>
> > Do we need to talk about devices that haven't been enumerated yet? or was
> > that just if we wanted to create an explicit dependency graph?
>
> You need to know about devices that aren't enumerated yet.
I'm still not quite sure what you mean. In my (limited) experience you need
to know about resources, not devices, and they have names.
And there is some precedent in regulator supply lists for naming devices
before they are enumerated. So I'm not seeing the problem that you are
seeing.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread