[PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*

[PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*

[Ricardo Ribalda Delgado]

There are some situations where it is interesting to load/remove serdev
devices dynamically, like during board bring-up or when we are
developing a new driver or for devices that are neither described via
ACPI or device tree.

This implementation allows the creation of serdev devices via sysfs,
in a similar way as the i2c bus allows sysfs instantiation [1].

It also opens the door to create platform drivers for specific
platforms, such us notebooks or industrial computers, when their serial
devices are not described via DT or ACPI.

This patchset also supports automatic module load via udev/modprobe,
simplifying its use by the final user.

Currently, serdev does not manage ports that do not have a serdev device
defined at boot time. This implementation adds a new serdev module,
ttydev, that provides the same functionality of tty (it calls the
original one), but can be unloaded.

TL/DR:

When we want to create a new device, we just run:
root@qt5022:~# echo ttydev > /sys/bus/serial/devices/serial0/new_device

This will create a new device:
root@qt5022:~# ls /sys/bus/serial/devices/serial0-0/
driver	modalias  power  subsystem  tty  uevent

And load the required driver to use it:
root@qt5022:~# lsmod | grep serdev_ttydev
serdev_ttydev          16384  0

The device can be removed:
root@qt5022:~#
	echo serial0-0 > /sys/bus/serial/devices/serial0/delete_device

And now we can connect a new device:
root@qt5022:~# echo hci-ti > /sys/bus/serial/devices/serial0/new_device


Changelog v2:

New functionality:
- New functions: get/put controller add_probed_device
- Rave_sp: Match all the variants

Changes proposed by Andy Shevchenko <andy.shevchenko@gmail.com>
- Avoid strchr
- Terminators with no comma

[1] https://www.kernel.org/doc/Documentation/i2c/instantiating-devices

Ricardo Ribalda Delgado (24):
  serdev: Add id_table to serdev_device_driver
  Bluetooth: hci_bcm: Add serdev_id_table
  Bluetooth: hci_ll: Add serdev_id_table
  Bluetooth: hci_nokia: Add serdev_id_table
  serdev: Introduce modalias field
  serdev: Support bus matching with modalias field
  serdev: Allows dynamic creation of devices via sysfs
  serdev: Provide modalias attribute for modalias devices
  serdev: Provide modalias uevent for modalias devices
  file2alias: Support for serdev devices
  Bluetooth: hci_bcm: MODULE_DEVICE_TABLE(serdev)
  Bluetooth: hci_ll: MODULE_DEVICE_TABLE(serdev)
  Bluetooth: hci_nokia: MODULE_DEVICE_TABLE(serdev)
  mfd: rave-sp: MODULE_DEVICE_TABLE(serdev)
  net: qualcomm: MODULE_DEVICE_TABLE(serdev)
  serdev: ttyport: Move serport structure to its own header
  serdev: Mark controllers compatible with ttyport
  serdev: ttydev: Serdev driver that creates an standard TTY port
  serdev: Instantiate a ttydev serdev if acpi and of fails
  serdev: Make match_id accessible by drivers
  rave-sp: Support for variants on modalias drivers
  serdev: Replace IDA functions with IDR
  serdev: get/put controller
  serdev: serdev_controller_add_probed_device

 drivers/bluetooth/hci_bcm.c              |   8 +
 drivers/bluetooth/hci_ll.c               |  19 +++
 drivers/bluetooth/hci_nokia.c            |   8 +
 drivers/mfd/rave-sp.c                    |  25 ++-
 drivers/net/ethernet/qualcomm/qca_uart.c |   7 +
 drivers/tty/serdev/Kconfig               |  10 ++
 drivers/tty/serdev/Makefile              |   2 +
 drivers/tty/serdev/core.c                | 186 ++++++++++++++++++++---
 drivers/tty/serdev/serdev-ttydev.c       |  60 ++++++++
 drivers/tty/serdev/serdev-ttyport.c      |  10 +-
 drivers/tty/serdev/serport.h             |  16 ++
 include/linux/mod_devicetable.h          |  10 ++
 include/linux/serdev.h                   |  11 ++
 scripts/mod/devicetable-offsets.c        |   3 +
 scripts/mod/file2alias.c                 |  11 ++
 15 files changed, 360 insertions(+), 26 deletions(-)
 create mode 100644 drivers/tty/serdev/serdev-ttydev.c
 create mode 100644 drivers/tty/serdev/serport.h

-- 
2.17.1

[PATCH v2 01/24] serdev: Add id_table to serdev_device_driver

[Ricardo Ribalda Delgado]

Currently, serdev device driver can only be used with devices that are
nodes of a device tree, or are part of the ACPI table.

Id_table will be used for devices that are created at runtime or that
are not part of the device tree nor the ACPI table.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 include/linux/mod_devicetable.h | 10 ++++++++++
 include/linux/serdev.h          |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 7d361be2e24f..1877a4e43f1b 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -448,6 +448,16 @@ struct pci_epf_device_id {
 	kernel_ulong_t driver_data;
 };
 
+/* serdev */
+
+#define SERDEV_NAME_SIZE	32
+#define SERDEV_MODULE_PREFIX	"serdev:"
+
+struct serdev_device_id {
+	char name[SERDEV_NAME_SIZE];
+	kernel_ulong_t driver_data;	/* Data private to the driver */
+};
+
 /* spi */
 
 #define SPI_NAME_SIZE	32
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index f153b2c7f0cd..62f1b085a794 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -15,6 +15,7 @@
 
 #include <linux/types.h>
 #include <linux/device.h>
+#include <linux/mod_devicetable.h>
 #include <linux/termios.h>
 #include <linux/delay.h>
 
@@ -68,6 +69,7 @@ static inline struct serdev_device *to_serdev_device(struct device *d)
  * @remove:	unbinds this driver from the serdev device.
  */
 struct serdev_device_driver {
+	const struct serdev_device_id *id_table;
 	struct device_driver driver;
 	int	(*probe)(struct serdev_device *);
 	void	(*remove)(struct serdev_device *);
-- 
2.17.1

[PATCH v2 02/24] Bluetooth: hci_bcm: Add serdev_id_table

[Ricardo Ribalda Delgado]

Describe which hardware is supported by the current driver.

Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: linux-bluetooth@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/bluetooth/hci_bcm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 441f5e1deb11..f4d7846c06b8 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -1325,6 +1325,11 @@ static const struct of_device_id bcm_bluetooth_of_match[] = {
 MODULE_DEVICE_TABLE(of, bcm_bluetooth_of_match);
 #endif
 
+static const struct serdev_device_id bcm_serdev_id[] = {
+	{ "bcm43438-bt", },
+	{}
+};
+
 static struct serdev_device_driver bcm_serdev_driver = {
 	.probe = bcm_serdev_probe,
 	.remove = bcm_serdev_remove,
@@ -1334,6 +1339,7 @@ static struct serdev_device_driver bcm_serdev_driver = {
 		.acpi_match_table = ACPI_PTR(bcm_acpi_match),
 		.pm = &bcm_pm_ops,
 	},
+	.id_table = bcm_serdev_id,
 };
 
 int __init bcm_init(void)
-- 
2.17.1

[PATCH v2 03/24] Bluetooth: hci_ll: Add serdev_id_table

[Ricardo Ribalda Delgado]

Describe which hardware is supported by the current driver.

Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: linux-bluetooth@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/bluetooth/hci_ll.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
index 27e414b4e3a2..276fdf677df4 100644
--- a/drivers/bluetooth/hci_ll.c
+++ b/drivers/bluetooth/hci_ll.c
@@ -776,6 +776,23 @@ static const struct of_device_id hci_ti_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, hci_ti_of_match);
 
+static struct serdev_device_id hci_ti_id[] = {
+	{ "cc2560", },
+	{ "wl1271-st", },
+	{ "wl1273-st", },
+	{ "wl1281-st", },
+	{ "wl1283-st", },
+	{ "wl1285-st", },
+	{ "wl1801-st", },
+	{ "wl1805-st", },
+	{ "wl1807-st", },
+	{ "wl1831-st", },
+	{ "wl1835-st", },
+	{ "wl1837-st", },
+	{}
+};
+MODULE_DEVICE_TABLE(serdev, hci_ti_id);
+
 static struct serdev_device_driver hci_ti_drv = {
 	.driver		= {
 		.name	= "hci-ti",
@@ -783,6 +800,7 @@ static struct serdev_device_driver hci_ti_drv = {
 	},
 	.probe	= hci_ti_probe,
 	.remove	= hci_ti_remove,
+	.id_table = hci_ti_id,
 };
 #else
 #define ll_setup NULL
-- 
2.17.1

[PATCH v2 04/24] Bluetooth: hci_nokia: Add serdev_id_table

[Ricardo Ribalda Delgado]

Describe which hardware is supported by the current driver.

Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-bluetooth@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/bluetooth/hci_nokia.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c
index 3539fd03f47e..eb3d59894aef 100644
--- a/drivers/bluetooth/hci_nokia.c
+++ b/drivers/bluetooth/hci_nokia.c
@@ -801,6 +801,11 @@ static const struct of_device_id nokia_bluetooth_of_match[] = {
 MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
 #endif
 
+static struct serdev_device_id nokia_bluetooth_serdev_id[] = {
+	{ "hp4-bluetooth", },
+	{}
+};
+
 static struct serdev_device_driver nokia_bluetooth_serdev_driver = {
 	.probe = nokia_bluetooth_serdev_probe,
 	.remove = nokia_bluetooth_serdev_remove,
@@ -809,6 +814,7 @@ static struct serdev_device_driver nokia_bluetooth_serdev_driver = {
 		.pm = &nokia_bluetooth_pm_ops,
 		.of_match_table = of_match_ptr(nokia_bluetooth_of_match),
 	},
+	.id_table = nokia_bluetooth_serdev_id,
 };
 
 module_serdev_device_driver(nokia_bluetooth_serdev_driver);
-- 
2.17.1

[PATCH v2 05/24] serdev: Introduce modalias field

[Ricardo Ribalda Delgado]

Name of the driver to use with this device, or an alias for that name,
or an alias for the part.
Required for hardware that is neither an of_node nor part of the ACPI
table.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 include/linux/serdev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 62f1b085a794..bb3b9599c652 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -54,6 +54,7 @@ struct serdev_device {
 	const struct serdev_device_ops *ops;
 	struct completion write_comp;
 	struct mutex write_lock;
+	char modalias[SERDEV_NAME_SIZE];
 };
 
 static inline struct serdev_device *to_serdev_device(struct device *d)
-- 
2.17.1

[PATCH v2 06/24] serdev: Support bus matching with modalias field

[Ricardo Ribalda Delgado]

Match devices to drivers by their modalias when the ACPI and the OF
match fails.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/tty/serdev/core.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index df93b727e984..2c79f47fc0db 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -79,8 +79,23 @@ static const struct device_type serdev_ctrl_type = {
 	.release	= serdev_ctrl_release,
 };
 
+static int serdev_match_id(const struct serdev_device_id *id,
+			   const struct serdev_device *sdev)
+{
+	while (id->name[0]) {
+		if (!strcmp(sdev->modalias, id->name))
+			return 1;
+		id++;
+	}
+
+	return 0;
+}
+
 static int serdev_device_match(struct device *dev, struct device_driver *drv)
 {
+	const struct serdev_device *sdev = to_serdev_device(dev);
+	const struct serdev_device_driver *sdrv = to_serdev_device_driver(drv);
+
 	if (!is_serdev_device(dev))
 		return 0;
 
@@ -88,7 +103,13 @@ static int serdev_device_match(struct device *dev, struct device_driver *drv)
 	if (acpi_driver_match_device(dev, drv))
 		return 1;
 
-	return of_driver_match_device(dev, drv);
+	if (of_driver_match_device(dev, drv))
+		return 1;
+
+	if (sdrv->id_table)
+		return serdev_match_id(sdrv->id_table, sdev);
+
+	return strcmp(sdev->modalias, drv->name) == 0;
 }
 
 /**
-- 
2.17.1

[PATCH v2 07/24] serdev: Allows dynamic creation of devices via sysfs

[Ricardo Ribalda Delgado]

Allow creating and deleting devices via sysfs. Devices created will be
matched to serdev drivers via modalias (the string provided by the user)
and deleted via their name. Eg:

 # Create device
root@qt5022:~# echo ttydev > /sys/bus/serial/devices/serial0/new_device

 # Delete device
root@qt5022:~#
	echo serial0-0 > /sys/bus/serial/devices/serial0/delete_device

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/tty/serdev/core.c | 69 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 2c79f47fc0db..5df01d8cf307 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -75,7 +75,76 @@ static void serdev_ctrl_release(struct device *dev)
 	kfree(ctrl);
 }
 
+static ssize_t
+new_device_store(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct serdev_controller *ctrl = to_serdev_controller(dev);
+	struct serdev_device *serdev;
+	char *nline;
+	int len;
+	int err;
+
+	serdev = serdev_device_alloc(ctrl);
+	if (!serdev)
+		return -ENOMEM;
+
+	nline = strchr(buf, '\n');
+	if (nline)
+		len = nline - buf;
+	else
+		len = strlen(buf);
+	len = min(SERDEV_NAME_SIZE - 1, len);
+
+	strncpy(serdev->modalias, buf, len);
+	serdev->modalias[len] = '\0';
+
+	err = serdev_device_add(serdev);
+	if (err) {
+		serdev_device_put(serdev);
+		return err;
+	}
+
+	return count;
+}
+static DEVICE_ATTR_WO(new_device);
+
+static ssize_t
+delete_device_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct serdev_controller *ctrl = to_serdev_controller(dev);
+	struct serdev_device *serdev = ctrl->serdev;
+	char *nline;
+	int len;
+
+	nline = strchr(buf, '\n');
+	if (nline)
+		len = nline - buf;
+	else
+		len = strlen(buf);
+	len = min(SERDEV_NAME_SIZE - 1, len);
+
+	if (!ctrl->serdev ||
+	    strncmp(dev_name(&serdev->dev), buf, len))
+		return -ENODEV;
+
+	serdev_device_remove(serdev);
+
+	return count;
+}
+static DEVICE_ATTR_IGNORE_LOCKDEP(delete_device, 0200, NULL,
+				  delete_device_store);
+
+static struct attribute *serdev_ctrl_attrs[] = {
+	&dev_attr_new_device.attr,
+	&dev_attr_delete_device.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(serdev_ctrl);
+
 static const struct device_type serdev_ctrl_type = {
+	.groups		= serdev_ctrl_groups,
 	.release	= serdev_ctrl_release,
 };
 
-- 
2.17.1

[PATCH v2 08/24] serdev: Provide modalias attribute for modalias devices

[Ricardo Ribalda Delgado]

Create modalias sysfs attribute for modalias devices.
This is required by modprobe/udev to autoload the serdev driver.

Eg:
root@qt5022:~# cat /sys/bus/serial/devices/serial1-0/modalias
serdev:ttydev

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/tty/serdev/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 5df01d8cf307..b9bb0c0ee319 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -23,12 +23,17 @@ static ssize_t modalias_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
 	int len;
+	struct serdev_device *serdev = to_serdev_device(dev);
 
 	len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
 	if (len != -ENODEV)
 		return len;
 
-	return of_device_modalias(dev, buf, PAGE_SIZE);
+	len = of_device_modalias(dev, buf, PAGE_SIZE);
+	if (len != -ENODEV)
+		return len;
+
+	return sprintf(buf, "%s%s\n", SERDEV_MODULE_PREFIX, serdev->modalias);
 }
 static DEVICE_ATTR_RO(modalias);
 
-- 
2.17.1

[PATCH v2 09/24] serdev: Provide modalias uevent for modalias devices

[Ricardo Ribalda Delgado]

Create the sysfs uevent for modalias devices. This is required by newer
versions of udev for autoload modules.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/tty/serdev/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index b9bb0c0ee319..584cb994213a 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -46,6 +46,7 @@ ATTRIBUTE_GROUPS(serdev_device);
 static int serdev_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	int rc;
+	struct serdev_device *serdev = to_serdev_device(dev);
 
 	/* TODO: platform modalias */
 
@@ -53,7 +54,11 @@ static int serdev_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 	if (rc != -ENODEV)
 		return rc;
 
-	return of_device_uevent_modalias(dev, env);
+	if (rc != of_device_uevent_modalias(dev, env))
+		return rc;
+
+	return add_uevent_var(env, "MODALIAS=%s%s", SERDEV_MODULE_PREFIX,
+						    serdev->modalias);
 }
 
 static void serdev_device_release(struct device *dev)
-- 
2.17.1

[PATCH v2 10/24] file2alias: Support for serdev devices

[Ricardo Ribalda Delgado]

This patch allows file2alias to generate the proper module headers to
support serdev modalias drivers.

Eg:

root@qt5022:~# modinfo serdev:ttydev | grep alias
alias:          serdev:ttydev_serdev
alias:          serdev:ttydev

root@qt5022:~#
	cat /lib/modules/4.16.0-qtec-standard/modules.alias | grep serdev
alias serdev:ttydev_serdev serdev_ttydev
alias serdev:ttydev serdev_ttydev

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Philippe Ombredanne <pombredanne@nexb.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 scripts/mod/devicetable-offsets.c |  3 +++
 scripts/mod/file2alias.c          | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index 9fad6afe4c41..6082c41b7ad7 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -142,6 +142,9 @@ int main(void)
 	DEVID(i2c_device_id);
 	DEVID_FIELD(i2c_device_id, name);
 
+	DEVID(serdev_device_id);
+	DEVID_FIELD(serdev_device_id, name);
+
 	DEVID(spi_device_id);
 	DEVID_FIELD(spi_device_id, name);
 
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index b9beeaa4695b..dce6df3a159a 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -955,6 +955,17 @@ static int do_i2c_entry(const char *filename, void *symval,
 }
 ADD_TO_DEVTABLE("i2c", i2c_device_id, do_i2c_entry);
 
+/* Looks like: serdev:S */
+static int do_serdev_entry(const char *filename, void *symval,
+			   char *alias)
+{
+	DEF_FIELD_ADDR(symval, serdev_device_id, name);
+	sprintf(alias, SERDEV_MODULE_PREFIX "%s", *name);
+
+	return 1;
+}
+ADD_TO_DEVTABLE("serdev", serdev_device_id, do_serdev_entry);
+
 /* Looks like: spi:S */
 static int do_spi_entry(const char *filename, void *symval,
 			char *alias)
-- 
2.17.1

[PATCH v2 11/24] Bluetooth: hci_bcm: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Export serdev table to the module header, allowing module autoload via
udev/modprobe.

Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: linux-bluetooth@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/bluetooth/hci_bcm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index f4d7846c06b8..ff0fd3502a90 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -1327,8 +1327,10 @@ MODULE_DEVICE_TABLE(of, bcm_bluetooth_of_match);
 
 static const struct serdev_device_id bcm_serdev_id[] = {
 	{ "bcm43438-bt", },
+	{ "hci_uart_bcm", },
 	{}
 };
+MODULE_DEVICE_TABLE(serdev, bcm_serdev_id);
 
 static struct serdev_device_driver bcm_serdev_driver = {
 	.probe = bcm_serdev_probe,
-- 
2.17.1

[PATCH v2 12/24] Bluetooth: hci_ll: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Export serdev table to the module header, allowing module autoload via
udev/modprobe.

Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: linux-bluetooth@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/bluetooth/hci_ll.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
index 276fdf677df4..3fbe7045e857 100644
--- a/drivers/bluetooth/hci_ll.c
+++ b/drivers/bluetooth/hci_ll.c
@@ -789,6 +789,7 @@ static struct serdev_device_id hci_ti_id[] = {
 	{ "wl1831-st", },
 	{ "wl1835-st", },
 	{ "wl1837-st", },
+	{ "hci-ti", },
 	{}
 };
 MODULE_DEVICE_TABLE(serdev, hci_ti_id);
-- 
2.17.1

[PATCH v2 13/24] Bluetooth: hci_nokia: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Export serdev table to the module header, allowing module autoload via
udev/modprobe.

Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: linux-bluetooth@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/bluetooth/hci_nokia.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c
index eb3d59894aef..fef285608f10 100644
--- a/drivers/bluetooth/hci_nokia.c
+++ b/drivers/bluetooth/hci_nokia.c
@@ -803,8 +803,10 @@ MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
 
 static struct serdev_device_id nokia_bluetooth_serdev_id[] = {
 	{ "hp4-bluetooth", },
+	{ "nokia-bluetooth", },
 	{}
 };
+MODULE_DEVICE_TABLE(serdev, nokia_bluetooth_serdev_id);
 
 static struct serdev_device_driver nokia_bluetooth_serdev_driver = {
 	.probe = nokia_bluetooth_serdev_probe,
-- 
2.17.1

[PATCH v2 14/24] mfd: rave-sp: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Export serdev table to the module header, allowing module autoload via
udev/modprobe.

Cc: Lee Jones <lee.jones@linaro.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/mfd/rave-sp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
index 5c858e784a89..c0ecfbc16dca 100644
--- a/drivers/mfd/rave-sp.c
+++ b/drivers/mfd/rave-sp.c
@@ -694,12 +694,19 @@ static int rave_sp_probe(struct serdev_device *serdev)
 
 MODULE_DEVICE_TABLE(of, rave_sp_dt_ids);
 
+static struct serdev_device_id rave_sp_serdev_id[] = {
+	{ "rave-sp", },
+	{}
+};
+MODULE_DEVICE_TABLE(serdev, rave_sp_serdev_id);
+
 static struct serdev_device_driver rave_sp_drv = {
 	.probe			= rave_sp_probe,
 	.driver = {
 		.name		= "rave-sp",
 		.of_match_table	= rave_sp_dt_ids,
 	},
+	.id_table = rave_sp_serdev_id,
 };
 module_serdev_device_driver(rave_sp_drv);
 
-- 
2.17.1

[PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Export serdev table to the module header, allowing module autoload via
udev/modprobe.

Cc: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: netdev@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/net/ethernet/qualcomm/qca_uart.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/qca_uart.c b/drivers/net/ethernet/qualcomm/qca_uart.c
index db6068cd7a1f..6d2ac6cae63f 100644
--- a/drivers/net/ethernet/qualcomm/qca_uart.c
+++ b/drivers/net/ethernet/qualcomm/qca_uart.c
@@ -405,6 +405,12 @@ static void qca_uart_remove(struct serdev_device *serdev)
 	free_netdev(qca->net_dev);
 }
 
+static struct serdev_device_id qca_uart_serdev_id[] = {
+	{ QCAUART_DRV_NAME, },
+	{}
+};
+MODULE_DEVICE_TABLE(serdev, qca_uart_serdev_id);
+
 static struct serdev_device_driver qca_uart_driver = {
 	.probe = qca_uart_probe,
 	.remove = qca_uart_remove,
@@ -412,6 +418,7 @@ static struct serdev_device_driver qca_uart_driver = {
 		.name = QCAUART_DRV_NAME,
 		.of_match_table = of_match_ptr(qca_uart_of_match),
 	},
+	.id_table = qca_uart_serdev_id,
 };
 
 module_serdev_device_driver(qca_uart_driver);
-- 
2.17.1

[PATCH v2 16/24] serdev: ttyport: Move serport structure to its own header

[Ricardo Ribalda Delgado]

This way we can reuse this structure in other modules.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/tty/serdev/serdev-ttyport.c |  9 +--------
 drivers/tty/serdev/serport.h        | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 8 deletions(-)
 create mode 100644 drivers/tty/serdev/serport.h

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index fa1672993b4c..4acc5f41dc67 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -7,17 +7,10 @@
 #include <linux/tty.h>
 #include <linux/tty_driver.h>
 #include <linux/poll.h>
+#include "serport.h"
 
 #define SERPORT_ACTIVE		1
 
-struct serport {
-	struct tty_port *port;
-	struct tty_struct *tty;
-	struct tty_driver *tty_drv;
-	int tty_idx;
-	unsigned long flags;
-};
-
 /*
  * Callback functions from the tty port.
  */
diff --git a/drivers/tty/serdev/serport.h b/drivers/tty/serdev/serport.h
new file mode 100644
index 000000000000..15bc1ff6326e
--- /dev/null
+++ b/drivers/tty/serdev/serport.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2016-2017 Linaro Ltd., Rob Herring <robh@kernel.org>
+ */
+#ifndef _SERPORT_H
+#define _SERPORT_H
+
+struct serport {
+	struct tty_port *port;
+	struct tty_struct *tty;
+	struct tty_driver *tty_drv;
+	int tty_idx;
+	unsigned long flags;
+};
+
+#endif
-- 
2.17.1

[PATCH v2 17/24] serdev: Mark controllers compatible with ttyport

[Ricardo Ribalda Delgado]

This allows us to treat differently this controllers, by creating a tty
compatibility layer.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/tty/serdev/serdev-ttyport.c | 1 +
 include/linux/serdev.h              | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index 4acc5f41dc67..ae961260e4a4 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -276,6 +276,7 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 	serport->tty_drv = drv;
 
 	ctrl->ops = &ctrl_ops;
+	ctrl->is_ttyport = true;
 
 	old_ops = port->client_ops;
 	port->client_ops = &client_ops;
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index bb3b9599c652..07d63933bdb9 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -116,6 +116,7 @@ struct serdev_controller {
 	unsigned int		nr;
 	struct serdev_device	*serdev;
 	const struct serdev_controller_ops *ops;
+	bool			is_ttyport;
 };
 
 static inline struct serdev_controller *to_serdev_controller(struct device *d)
-- 
2.17.1

[PATCH v2 18/24] serdev: ttydev: Serdev driver that creates an standard TTY port

[Ricardo Ribalda Delgado]

Standard TTY port that can be loaded/unloaded via serdev sysfs. This
serdev driver can only be used by serdev controllers that are compatible
with ttyport.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/tty/serdev/Kconfig         | 10 +++++
 drivers/tty/serdev/Makefile        |  2 +
 drivers/tty/serdev/serdev-ttydev.c | 60 ++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)
 create mode 100644 drivers/tty/serdev/serdev-ttydev.c

diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
index 1dbc8352e027..848692caabd6 100644
--- a/drivers/tty/serdev/Kconfig
+++ b/drivers/tty/serdev/Kconfig
@@ -21,4 +21,14 @@ config SERIAL_DEV_CTRL_TTYPORT
 	depends on SERIAL_DEV_BUS != m
 	default y
 
+config SERIAL_DEV_CTRL_TTYDEV
+	tristate "TTY port dynamically loaded by the Serial Device Bus"
+	help
+	  Say Y here if you want to create a bridge driver between the Serial
+	  device bus and the TTY chardevice. This driver can be dynamically
+	  loaded/unloaded by the Serial Device Bus.
+
+	  If unsure, say N.
+	depends on SERIAL_DEV_CTRL_TTYPORT
+
 endif
diff --git a/drivers/tty/serdev/Makefile b/drivers/tty/serdev/Makefile
index 0cbdb9444d9d..5c807b77d12d 100644
--- a/drivers/tty/serdev/Makefile
+++ b/drivers/tty/serdev/Makefile
@@ -3,3 +3,5 @@ serdev-objs := core.o
 obj-$(CONFIG_SERIAL_DEV_BUS) += serdev.o
 
 obj-$(CONFIG_SERIAL_DEV_CTRL_TTYPORT) += serdev-ttyport.o
+
+obj-$(CONFIG_SERIAL_DEV_CTRL_TTYDEV) += serdev-ttydev.o
diff --git a/drivers/tty/serdev/serdev-ttydev.c b/drivers/tty/serdev/serdev-ttydev.c
new file mode 100644
index 000000000000..180035e101dc
--- /dev/null
+++ b/drivers/tty/serdev/serdev-ttydev.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Ricardo Ribalda <ricardo.ribalda@gmail.com>
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/serdev.h>
+#include <linux/tty.h>
+#include "serport.h"
+
+static int ttydev_serdev_probe(struct serdev_device *serdev)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+	struct serport *serport;
+	struct device *dev;
+
+	if (!ctrl->is_ttyport)
+		return -ENODEV;
+
+	serport = serdev_controller_get_drvdata(ctrl);
+
+	dev = tty_register_device_attr(serport->tty_drv, serport->tty_idx,
+					&serdev->dev, NULL, NULL);
+
+	return dev ? 0 : PTR_ERR(dev);
+}
+
+static void ttydev_serdev_remove(struct serdev_device *serdev)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+	struct serport *serport;
+
+	serport = serdev_controller_get_drvdata(ctrl);
+	tty_unregister_device(serport->tty_drv, serport->tty_idx);
+}
+
+static const struct serdev_device_id ttydev_serdev_id[] = {
+	{ "ttydev", },
+	{ "ttydev_serdev", },
+	{}
+};
+MODULE_DEVICE_TABLE(serdev, ttydev_serdev_id);
+
+static struct serdev_device_driver ttydev_serdev_driver = {
+	.probe = ttydev_serdev_probe,
+	.remove = ttydev_serdev_remove,
+	.driver = {
+		.name = "ttydev_serdev",
+	},
+	.id_table = ttydev_serdev_id,
+};
+
+module_serdev_device_driver(ttydev_serdev_driver);
+
+MODULE_AUTHOR("Ricardo Ribalda <ricardo.ribalda@gmail.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Serdev to ttydev module");
-- 
2.17.1

[PATCH v2 19/24] serdev: Instantiate a ttydev serdev if acpi and of fails

[Ricardo Ribalda Delgado]

If a serdev ttyport controller does not have an acpi nor an of child,
create a ttydev as a child of that controller.

Doing this allows the removal, addition and replacement of ttydev devices
at runtime.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/tty/serdev/core.c | 49 ++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 584cb994213a..587d2796b3d5 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -628,6 +628,26 @@ static inline int acpi_serdev_register_devices(struct serdev_controller *ctrl)
 }
 #endif /* CONFIG_ACPI */
 
+#if IS_ENABLED(CONFIG_SERIAL_DEV_CTRL_TTYDEV)
+static int serdev_controller_add_ttydev(struct serdev_controller *ctrl)
+{
+	struct serdev_device *serdev;
+	int err;
+
+	serdev = serdev_device_alloc(ctrl);
+	if (!serdev)
+		return -ENOMEM;
+
+	strcpy(serdev->modalias, "ttydev");
+
+	err = serdev_device_add(serdev);
+	if (err)
+		serdev_device_put(serdev);
+
+	return err;
+}
+#endif
+
 /**
  * serdev_controller_add() - Add an serdev controller
  * @ctrl:	controller to be registered.
@@ -637,7 +657,7 @@ static inline int acpi_serdev_register_devices(struct serdev_controller *ctrl)
  */
 int serdev_controller_add(struct serdev_controller *ctrl)
 {
-	int ret_of, ret_acpi, ret;
+	int ret_of, ret_acpi, ret, ret_tty = -ENODEV;
 
 	/* Can't register until after driver model init */
 	if (WARN_ON(!is_registered))
@@ -648,21 +668,28 @@ int serdev_controller_add(struct serdev_controller *ctrl)
 		return ret;
 
 	ret_of = of_serdev_register_devices(ctrl);
+	if (!ret_of)
+		goto out_dev_ok;
+
 	ret_acpi = acpi_serdev_register_devices(ctrl);
-	if (ret_of && ret_acpi) {
-		dev_dbg(&ctrl->dev, "no devices registered: of:%d acpi:%d\n",
-			ret_of, ret_acpi);
-		ret = -ENODEV;
-		goto out_dev_del;
-	}
+	if (!ret_acpi)
+		goto out_dev_ok;
+
+#if IS_ENABLED(CONFIG_SERIAL_DEV_CTRL_TTYDEV)
+	ret_tty = serdev_controller_add_ttydev(ctrl);
+	if (!ret_tty)
+		goto out_dev_ok;
+#endif
 
+	dev_dbg(&ctrl->dev, "no devices registered: of:%d acpi:%d tty:%d\n",
+		ret_of, ret_acpi, ret_tty);
+	device_del(&ctrl->dev);
+	return -ENODEV;
+
+out_dev_ok:
 	dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
 		ctrl->nr, &ctrl->dev);
 	return 0;
-
-out_dev_del:
-	device_del(&ctrl->dev);
-	return ret;
 };
 EXPORT_SYMBOL_GPL(serdev_controller_add);
 
-- 
2.17.1

[PATCH v2 20/24] serdev: Make match_id accessible by drivers

[Ricardo Ribalda Delgado]

Drivers that make use of the driver_data field require to transverse the
id_table. There is no reason to have one implementation per driver.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/tty/serdev/core.c | 12 +++++++-----
 include/linux/serdev.h    |  3 +++
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 587d2796b3d5..3084b6d10179 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -158,17 +158,19 @@ static const struct device_type serdev_ctrl_type = {
 	.release	= serdev_ctrl_release,
 };
 
-static int serdev_match_id(const struct serdev_device_id *id,
-			   const struct serdev_device *sdev)
+const struct serdev_device_id *serdev_match_id(
+			const struct serdev_device_id *id,
+			const struct serdev_device *sdev)
 {
 	while (id->name[0]) {
 		if (!strcmp(sdev->modalias, id->name))
-			return 1;
+			return id;
 		id++;
 	}
 
-	return 0;
+	return NULL;
 }
+EXPORT_SYMBOL_GPL(serdev_match_id);
 
 static int serdev_device_match(struct device *dev, struct device_driver *drv)
 {
@@ -186,7 +188,7 @@ static int serdev_device_match(struct device *dev, struct device_driver *drv)
 		return 1;
 
 	if (sdrv->id_table)
-		return serdev_match_id(sdrv->id_table, sdev);
+		return serdev_match_id(sdrv->id_table, sdev) != NULL;
 
 	return strcmp(sdev->modalias, drv->name) == 0;
 }
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 07d63933bdb9..bf282b3781b9 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -172,6 +172,9 @@ static inline void serdev_controller_put(struct serdev_controller *ctrl)
 		put_device(&ctrl->dev);
 }
 
+const struct serdev_device_id *serdev_match_id(
+		const struct serdev_device_id *id,
+		const struct serdev_device *sdev);
 struct serdev_device *serdev_device_alloc(struct serdev_controller *);
 int serdev_device_add(struct serdev_device *);
 void serdev_device_remove(struct serdev_device *);
-- 
2.17.1

[PATCH v2 21/24] rave-sp: Support for variants on modalias drivers

[Ricardo Ribalda Delgado]

Rave-sp behaves differently based on the device variant.

Cc: Lee Jones <lee.jones@linaro.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/mfd/rave-sp.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
index c0ecfbc16dca..d33cb9d914e8 100644
--- a/drivers/mfd/rave-sp.c
+++ b/drivers/mfd/rave-sp.c
@@ -654,14 +654,30 @@ static const struct serdev_device_ops rave_sp_serdev_device_ops = {
 	.write_wakeup = serdev_device_write_wakeup,
 };
 
+static struct serdev_device_id rave_sp_serdev_id[] = {
+	{ "rave-sp", (kernel_ulong_t) &rave_sp_legacy},
+	{ "rave-sp-niu", (kernel_ulong_t) &rave_sp_legacy},
+	{ "rave-sp-mezz", (kernel_ulong_t) &rave_sp_legacy},
+	{ "rave-sp-esb", (kernel_ulong_t) &rave_sp_legacy},
+	{ "rave-sp-rdu1", (kernel_ulong_t) &rave_sp_rdu1},
+	{ "rave-sp-rdu2", (kernel_ulong_t) &rave_sp_rdu2},
+	{}
+};
+MODULE_DEVICE_TABLE(serdev, rave_sp_serdev_id);
+
 static int rave_sp_probe(struct serdev_device *serdev)
 {
 	struct device *dev = &serdev->dev;
+	const struct serdev_device_id *id;
 	struct rave_sp *sp;
 	u32 baud;
 	int ret;
 
-	if (of_property_read_u32(dev->of_node, "current-speed", &baud)) {
+	if (!dev->of_node) {
+		/* Baudrate at zii,rave-sp.txt */
+		baud = 1000000;
+	} else if (of_property_read_u32(dev->of_node,
+					"current-speed", &baud)) {
 		dev_err(dev,
 			"'current-speed' is not specified in device node\n");
 		return -EINVAL;
@@ -675,6 +691,12 @@ static int rave_sp_probe(struct serdev_device *serdev)
 	dev_set_drvdata(dev, sp);
 
 	sp->variant = of_device_get_match_data(dev);
+	if (!sp->variant) {
+		id = serdev_match_id(rave_sp_serdev_id, serdev);
+		if (id)
+			sp->variant = (const struct rave_sp_variant *)
+							id->driver_data;
+	}
 	if (!sp->variant)
 		return -ENODEV;
 
@@ -694,12 +716,6 @@ static int rave_sp_probe(struct serdev_device *serdev)
 
 MODULE_DEVICE_TABLE(of, rave_sp_dt_ids);
 
-static struct serdev_device_id rave_sp_serdev_id[] = {
-	{ "rave-sp", },
-	{}
-};
-MODULE_DEVICE_TABLE(serdev, rave_sp_serdev_id);
-
 static struct serdev_device_driver rave_sp_drv = {
 	.probe			= rave_sp_probe,
 	.driver = {
-- 
2.17.1

[PATCH v2 22/24] serdev: Replace IDA functions with IDR

[Ricardo Ribalda Delgado]

IDR functions support associating an ID with a pointer. This is required
if we need to access the controllers based on their ID.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/tty/serdev/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 3084b6d10179..6b24b0a74fbf 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -17,7 +17,7 @@
 #include <linux/slab.h>
 
 static bool is_registered;
-static DEFINE_IDA(ctrl_ida);
+static DEFINE_IDR(ctrl_idr);
 
 static ssize_t modalias_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
@@ -81,7 +81,7 @@ static bool is_serdev_device(const struct device *dev)
 static void serdev_ctrl_release(struct device *dev)
 {
 	struct serdev_controller *ctrl = to_serdev_controller(dev);
-	ida_simple_remove(&ctrl_ida, ctrl->nr);
+	idr_remove(&ctrl_idr, ctrl->nr);
 	kfree(ctrl);
 }
 
@@ -500,7 +500,7 @@ struct serdev_controller *serdev_controller_alloc(struct device *parent,
 	if (!ctrl)
 		return NULL;
 
-	id = ida_simple_get(&ctrl_ida, 0, 0, GFP_KERNEL);
+	id = idr_alloc(&ctrl_idr, ctrl, 0, 0, GFP_KERNEL);
 	if (id < 0) {
 		dev_err(parent,
 			"unable to allocate serdev controller identifier.\n");
-- 
2.17.1

[PATCH v2 23/24] serdev: get/put controller

[Ricardo Ribalda Delgado]

Allow access serdev controllers by other drivers in a safe way.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/tty/serdev/core.c | 23 +++++++++++++++++++++++
 include/linux/serdev.h    |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 6b24b0a74fbf..06310110104a 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -724,6 +724,29 @@ void serdev_controller_remove(struct serdev_controller *ctrl)
 }
 EXPORT_SYMBOL_GPL(serdev_controller_remove);
 
+struct serdev_controller *serdev_get_controller(int nr)
+{
+	struct serdev_controller *ctrl;
+
+	ctrl = idr_find(&ctrl_idr, nr);
+	if (!ctrl)
+		return NULL;
+
+	get_device(&ctrl->dev);
+
+	return ctrl;
+}
+EXPORT_SYMBOL_GPL(serdev_get_controller);
+
+void serdev_put_controller(struct serdev_controller *ctrl)
+{
+	if (!ctrl)
+		return;
+
+	put_device(&ctrl->dev);
+}
+EXPORT_SYMBOL_GPL(serdev_put_controller);
+
 /**
  * serdev_driver_register() - Register client driver with serdev core
  * @sdrv:	client driver to be associated with client-device.
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index bf282b3781b9..1ef6e6503650 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -182,6 +182,8 @@ void serdev_device_remove(struct serdev_device *);
 struct serdev_controller *serdev_controller_alloc(struct device *, size_t);
 int serdev_controller_add(struct serdev_controller *);
 void serdev_controller_remove(struct serdev_controller *);
+void serdev_put_controller(struct serdev_controller *ctrl);
+struct serdev_controller *serdev_get_controller(int nr);
 
 static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl)
 {
-- 
2.17.1

[PATCH v2 24/24] serdev: serdev_controller_add_probed_device

[Ricardo Ribalda Delgado]

Support adding probed devices by "platform" drivers.

Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/tty/serdev/core.c | 10 +++++-----
 include/linux/serdev.h    |  2 ++
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 06310110104a..e56a955d4ea9 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -630,8 +630,8 @@ static inline int acpi_serdev_register_devices(struct serdev_controller *ctrl)
 }
 #endif /* CONFIG_ACPI */
 
-#if IS_ENABLED(CONFIG_SERIAL_DEV_CTRL_TTYDEV)
-static int serdev_controller_add_ttydev(struct serdev_controller *ctrl)
+int serdev_controller_add_probed_device(struct serdev_controller *ctrl,
+					const char *name)
 {
 	struct serdev_device *serdev;
 	int err;
@@ -640,7 +640,7 @@ static int serdev_controller_add_ttydev(struct serdev_controller *ctrl)
 	if (!serdev)
 		return -ENOMEM;
 
-	strcpy(serdev->modalias, "ttydev");
+	strcpy(serdev->modalias, name);
 
 	err = serdev_device_add(serdev);
 	if (err)
@@ -648,7 +648,7 @@ static int serdev_controller_add_ttydev(struct serdev_controller *ctrl)
 
 	return err;
 }
-#endif
+EXPORT_SYMBOL_GPL(serdev_controller_add_probed_device);
 
 /**
  * serdev_controller_add() - Add an serdev controller
@@ -678,7 +678,7 @@ int serdev_controller_add(struct serdev_controller *ctrl)
 		goto out_dev_ok;
 
 #if IS_ENABLED(CONFIG_SERIAL_DEV_CTRL_TTYDEV)
-	ret_tty = serdev_controller_add_ttydev(ctrl);
+	ret_tty = serdev_controller_add_probed_device(ctrl, "ttydev");
 	if (!ret_tty)
 		goto out_dev_ok;
 #endif
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 1ef6e6503650..93f534a21ca9 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -184,6 +184,8 @@ int serdev_controller_add(struct serdev_controller *);
 void serdev_controller_remove(struct serdev_controller *);
 void serdev_put_controller(struct serdev_controller *ctrl);
 struct serdev_controller *serdev_get_controller(int nr);
+int serdev_controller_add_probed_device(struct serdev_controller *ctrl,
+					const char *name);
 
 static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl)
 {
-- 
2.17.1

Re: [PATCH v2 07/24] serdev: Allows dynamic creation of devices via sysfs

[Andy Shevchenko]

On Mon, Jun 11, 2018 at 2:52 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> Allow creating and deleting devices via sysfs. Devices created will be
> matched to serdev drivers via modalias (the string provided by the user)
> and deleted via their name. Eg:

> +       nline = strchr(buf, '\n');

...

> +       if (nline)
> +               len = nline - buf;
> +       else

> +               len = strlen(buf);
> +       len = min(SERDEV_NAME_SIZE - 1, len);

If buf is guaranteed to have '\0', the strlen() is not needed.

I'm not sure about this entire dances with first line and so on.
When it's possible to get more, than two lines on input?

Would it be just as simple as strstrip() call followed by strscpy()?

> +       strncpy(serdev->modalias, buf, len);
> +       serdev->modalias[len] = '\0';

strspcy() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 20/24] serdev: Make match_id accessible by drivers

[Andy Shevchenko]

On Mon, Jun 11, 2018 at 2:52 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> Drivers that make use of the driver_data field require to transverse the
> id_table. There is no reason to have one implementation per driver.

> +const struct serdev_device_id *serdev_match_id(
> +                       const struct serdev_device_id *id,
> +                       const struct serdev_device *sdev)

I think slightly better would be

const strunct serdev_device *
serdev_match_id(const struct serdev_device_id *id,
                       const struct serdev_device *sdev)

>  {
>         while (id->name[0]) {
>                 if (!strcmp(sdev->modalias, id->name))
> -                       return 1;
> +                       return id;
>                 id++;
>         }
>
> -       return 0;
> +       return NULL;
>  }
> +EXPORT_SYMBOL_GPL(serdev_match_id);

Can we avoid ping-pong changes in the series? I mean to introduce this
helper in this form in the first place?


> -               return serdev_match_id(sdrv->id_table, sdev);
> +               return serdev_match_id(sdrv->id_table, sdev) != NULL;

return !!serdev_match_id(...);
?


> +const struct serdev_device_id *serdev_match_id(
> +               const struct serdev_device_id *id,
> +               const struct serdev_device *sdev);

Same as above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 21/24] rave-sp: Support for variants on modalias drivers

[Andy Shevchenko]

On Mon, Jun 11, 2018 at 2:52 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> Rave-sp behaves differently based on the device variant.


>         sp->variant = of_device_get_match_data(dev);
> +       if (!sp->variant) {
> +               id = serdev_match_id(rave_sp_serdev_id, serdev);

I think you may leave the ID table where it is in the code and use link.

> +               if (id)
> +                       sp->variant = (const struct rave_sp_variant *)
> +                                                       id->driver_data;
> +       }

Perhaps a helper like it's done for ACPI / OF cases?

[device_get_match_data() -> ]
  of_fwnode_device_get_match_data()
  acpi_fwnode_device_get_match_data()

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 04/24] Bluetooth: hci_nokia: Add serdev_id_table

[Marcel Holtmann]

Hi Ricardo,

> Describe which hardware is supported by the current driver.
> 
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: Johan Hedberg <johan.hedberg@gmail.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: linux-bluetooth@vger.kernel.org
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> drivers/bluetooth/hci_nokia.c | 6 ++++++
> 1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c
> index 3539fd03f47e..eb3d59894aef 100644
> --- a/drivers/bluetooth/hci_nokia.c
> +++ b/drivers/bluetooth/hci_nokia.c
> @@ -801,6 +801,11 @@ static const struct of_device_id nokia_bluetooth_of_match[] = {
> MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
> #endif
> 
> +static struct serdev_device_id nokia_bluetooth_serdev_id[] = {
> +	{ "hp4-bluetooth", },
> +	{}
> +};
> +
> static struct serdev_device_driver nokia_bluetooth_serdev_driver = {
> 	.probe = nokia_bluetooth_serdev_probe,
> 	.remove = nokia_bluetooth_serdev_remove,
> @@ -809,6 +814,7 @@ static struct serdev_device_driver nokia_bluetooth_serdev_driver = {
> 		.pm = &nokia_bluetooth_pm_ops,
> 		.of_match_table = of_match_ptr(nokia_bluetooth_of_match),
> 	},
> +	.id_table = nokia_bluetooth_serdev_id,
> };

as I said before, don’t bother with this driver. There is no need for this. Nothing is generic here, it is all specific for each of the 5 devices they shipped. Nokia is not building these kind of devices anymore and if they start changing their mind, then we add a patch for it listening this id.

Regards

Marcel

Re: [PATCH v2 11/24] Bluetooth: hci_bcm: MODULE_DEVICE_TABLE(serdev)

[Marcel Holtmann]

Hi Ricardo,

> Export serdev table to the module header, allowing module autoload via
> udev/modprobe.
> 
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: Johan Hedberg <johan.hedberg@gmail.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: linux-bluetooth@vger.kernel.org
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> drivers/bluetooth/hci_bcm.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index f4d7846c06b8..ff0fd3502a90 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -1327,8 +1327,10 @@ MODULE_DEVICE_TABLE(of, bcm_bluetooth_of_match);
> 
> static const struct serdev_device_id bcm_serdev_id[] = {
> 	{ "bcm43438-bt", },
> +	{ "hci_uart_bcm", },
> 	{}
> };
> +MODULE_DEVICE_TABLE(serdev, bcm_serdev_id);

so this one I can see real use of and is a good fix to finally clean up hci_bcm.c and remove platform support for the Edison hardware. However, I would really then first rename hci_uart_bcm into some Edison specific string since this is really just one outlier here. Everything else will have ACPI or DT support.

With that said, I do not understand why we need to duplicate the DT compatible strings in serdev_device_id. They will be enumerated fine via ACPI or DT in the first place. So if we want some new_id support, then wouldn’t an empty serdev_device_id table just be fine?

Regards

Marcel

Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Marcel Holtmann]

Hi Ricardo,

> Export serdev table to the module header, allowing module autoload via
> udev/modprobe.
> 
> Cc: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> drivers/net/ethernet/qualcomm/qca_uart.c | 7 +++++++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ethernet/qualcomm/qca_uart.c b/drivers/net/ethernet/qualcomm/qca_uart.c
> index db6068cd7a1f..6d2ac6cae63f 100644
> --- a/drivers/net/ethernet/qualcomm/qca_uart.c
> +++ b/drivers/net/ethernet/qualcomm/qca_uart.c
> @@ -405,6 +405,12 @@ static void qca_uart_remove(struct serdev_device *serdev)
> 	free_netdev(qca->net_dev);
> }
> 
> +static struct serdev_device_id qca_uart_serdev_id[] = {
> +	{ QCAUART_DRV_NAME, },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(serdev, qca_uart_serdev_id);
> +
> static struct serdev_device_driver qca_uart_driver = {
> 	.probe = qca_uart_probe,
> 	.remove = qca_uart_remove,
> @@ -412,6 +418,7 @@ static struct serdev_device_driver qca_uart_driver = {
> 		.name = QCAUART_DRV_NAME,
> 		.of_match_table = of_match_ptr(qca_uart_of_match),
> 	},
> +	.id_table = qca_uart_serdev_id,
> };

the commit message is misleading me. If I build something with ACPI or DT support, then modinfo will show all modalias information for ACPI and DT compatible strings. What else does udev/modprobe actually need? Is something broken with the modalias export?

Regards

Marcel

Re: [PATCH v2 07/24] serdev: Allows dynamic creation of devices via sysfs

[Ricardo Ribalda Delgado]

Hi Andy,

I cannot use strstrip() because the buffer is const. But I have
replaced strncpy with strscpy.

Thanks!

-- 
Ricardo Ribalda

Re: [PATCH v2 04/24] Bluetooth: hci_nokia: Add serdev_id_table

[Ricardo Ribalda Delgado]

HI Marcel

I have just removed it from my series

Sorry for the extra noise


-- 
Ricardo Ribalda

Re: [PATCH v2 20/24] serdev: Make match_id accessible by drivers

[Ricardo Ribalda Delgado]

Yes, I agree.

Fixed on serdev3 branch https://github.com/ribalda/linux/tree/serdev3

Will resend after I finish fixing all the comments

Thanks!


-- 
Ricardo Ribalda

Re: [PATCH v2 14/24] mfd: rave-sp: MODULE_DEVICE_TABLE(serdev)

[Marcel Holtmann]

Hi Ricardo,

> Export serdev table to the module header, allowing module autoload via
> udev/modprobe.
> 
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> drivers/mfd/rave-sp.c | 7 +++++++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
> index 5c858e784a89..c0ecfbc16dca 100644
> --- a/drivers/mfd/rave-sp.c
> +++ b/drivers/mfd/rave-sp.c
> @@ -694,12 +694,19 @@ static int rave_sp_probe(struct serdev_device *serdev)
> 
> MODULE_DEVICE_TABLE(of, rave_sp_dt_ids);
> 
> +static struct serdev_device_id rave_sp_serdev_id[] = {
> +	{ "rave-sp", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(serdev, rave_sp_serdev_id);
> +
> static struct serdev_device_driver rave_sp_drv = {
> 	.probe			= rave_sp_probe,
> 	.driver = {
> 		.name		= "rave-sp",
> 		.of_match_table	= rave_sp_dt_ids,
> 	},
> +	.id_table = rave_sp_serdev_id,
> };
> module_serdev_device_driver(rave_sp_drv);

so I would actually prefer that we not duplicate the .driver.name in the .id_table. This one for example is non-functional since all supported hardware needs a specific .data entry. It will fail here:

        sp->variant = of_device_get_match_data(dev);                             
        if (!sp->variant)                                                        
                return -ENODEV;

Maybe we focus first on getting the base support for new_device etc. merged and use the Edison Bluetooth platform driver support in hci_bcm.c so we can do a real cleanup there. And then later add broad new_device support. Some of these instances should be just fine with never getting it since they require to many per device quirks to make things functional. A blind search+replace is not going to work.

And things like device_get_match_data() should work as well even if the hardware is listed via serdev_device_id. Drivers that are solely DT centric will need a lot more work before dynamic adding of serdev devices can happen.

Regards

Marcel

Re: [PATCH v2 11/24] Bluetooth: hci_bcm: MODULE_DEVICE_TABLE(serdev)

[Andy Shevchenko]

On Mon, Jun 11, 2018 at 3:59 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Ricardo,
>
>> Export serdev table to the module header, allowing module autoload via
>> udev/modprobe.

>> static const struct serdev_device_id bcm_serdev_id[] = {
>>       { "bcm43438-bt", },
>> +     { "hci_uart_bcm", },
>>       {}
>> };
>> +MODULE_DEVICE_TABLE(serdev, bcm_serdev_id);
>
> so this one I can see real use of and is a good fix to finally clean up hci_bcm.c and remove platform support for the Edison hardware. However, I would really then first rename hci_uart_bcm into some Edison specific string since this is really just one outlier here.

Or other way around, hack
arch/x86/platform/intel-mid/device_libs/platform_bt.c to be compatible
with these changes (Dunno if it's possible).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 21/24] rave-sp: Support for variants on modalias drivers

[Marcel Holtmann]

Hi Andy,

>> Rave-sp behaves differently based on the device variant.
> 
> 
>>        sp->variant = of_device_get_match_data(dev);
>> +       if (!sp->variant) {
>> +               id = serdev_match_id(rave_sp_serdev_id, serdev);
> 
> I think you may leave the ID table where it is in the code and use link.
> 
>> +               if (id)
>> +                       sp->variant = (const struct rave_sp_variant *)
>> +                                                       id->driver_data;
>> +       }
> 
> Perhaps a helper like it's done for ACPI / OF cases?
> 
> [device_get_match_data() -> ]
>  of_fwnode_device_get_match_data()
>  acpi_fwnode_device_get_match_data()

something like that and frankly this is trying to hard. This driver is currently really DT specific and can be fixed up later. It is causing a lot of noise in this patch series. I would really urge to focus on the core changes get prominent drivers support. I think hci_bcm.c is a good example since a) we need to fix up Edison and b) new ACPI based tablets might be able to allow for easy testing.

Regards

Marcel

Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Hi Marcel,
On Mon, Jun 11, 2018 at 3:01 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
>
> the commit message is misleading me. If I build something with ACPI or DT support, then modinfo will show all modalias information for ACPI and DT compatible strings. What else does udev/modprobe actually need? Is something broken with the modalias export?

The main purpose is to autoload drivers for devices that have been
created via sysfs or another module.

Eg1: We have a serial port on a standard computer that has connected a
GPS module. Since it is something that is not in the ACPI nor the DT
table the user will run

echo serdev_gps > /sys/bus/serial/devices/serial0/new_device

Eg2 module: https://github.com/ribalda/linux/blob/415bb3f0076c2b846ebe5409589b8e1e3004f55a/drivers/tty/serdev/test_platform.c

Modprobe does not know what module to load for that device unless
there is a matching MODULE_DEVICE_TABLE
Today, we have the same functionality for i2c devices
https://www.kernel.org/doc/Documentation/i2c/instantiating-devices

I guess the commit message is really bad :), sorry about that. Any
suggestions to improve it?

Thanks!

>
> Regards
>
> Marcel
>


-- 
Ricardo Ribalda

Re: [PATCH v2 14/24] mfd: rave-sp: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Hi Marcel
On Mon, Jun 11, 2018 at 3:14 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Ricardo,
>
> > Export serdev table to the module header, allowing module autoload via
> > udev/modprobe.
> >
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Johan Hovold <johan@kernel.org>
> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > ---
> > drivers/mfd/rave-sp.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
> > index 5c858e784a89..c0ecfbc16dca 100644
> > --- a/drivers/mfd/rave-sp.c
> > +++ b/drivers/mfd/rave-sp.c
> > @@ -694,12 +694,19 @@ static int rave_sp_probe(struct serdev_device *serdev)
> >
> > MODULE_DEVICE_TABLE(of, rave_sp_dt_ids);
> >
> > +static struct serdev_device_id rave_sp_serdev_id[] = {
> > +     { "rave-sp", },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(serdev, rave_sp_serdev_id);
> > +
> > static struct serdev_device_driver rave_sp_drv = {
> >       .probe                  = rave_sp_probe,
> >       .driver = {
> >               .name           = "rave-sp",
> >               .of_match_table = rave_sp_dt_ids,
> >       },
> > +     .id_table = rave_sp_serdev_id,
> > };
> > module_serdev_device_driver(rave_sp_drv);
>
> so I would actually prefer that we not duplicate the .driver.name in the .id_table. This one for example is non-functional since all supported hardware needs a specific .data entry. It will fail here:
>
>         sp->variant = of_device_get_match_data(dev);
>         if (!sp->variant)
>                 return -ENODEV;
>
> Maybe we focus first on getting the base support for new_device etc. merged and use the Edison Bluetooth platform driver support in hci_bcm.c so we can do a real cleanup there. And then later add broad new_device support. Some of these instances should be just fine with never getting it since they require to many per device quirks to make things functional. A blind search+replace is not going to work.

I think that functionality is added on another patch from the series.
Let me merge both together so the driver is functional (and builds)
from any point of the tree. Let me merge it in
https://github.com/ribalda/linux/tree/serdev3 and resend after I fixed
all the other suggestions


Thanks

>
> And things like device_get_match_data() should work as well even if the hardware is listed via serdev_device_id. Drivers that are solely DT centric will need a lot more work before dynamic adding of serdev devices can happen.
>
> Regards
>
> Marcel
>


-- 
Ricardo Ribalda

Re: [PATCH v2 21/24] rave-sp: Support for variants on modalias drivers

[Ricardo Ribalda Delgado]

Hello


On Mon, Jun 11, 2018 at 3:38 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Andy,
>
> >> Rave-sp behaves differently based on the device variant.
> >
> >
> >>        sp->variant = of_device_get_match_data(dev);
> >> +       if (!sp->variant) {
> >> +               id = serdev_match_id(rave_sp_serdev_id, serdev);
> >
> > I think you may leave the ID table where it is in the code and use link.
> >
> >> +               if (id)
> >> +                       sp->variant = (const struct rave_sp_variant *)
> >> +                                                       id->driver_data;
> >> +       }
> >
> > Perhaps a helper like it's done for ACPI / OF cases?
> >
> > [device_get_match_data() -> ]
> >  of_fwnode_device_get_match_data()
> >  acpi_fwnode_device_get_match_data()
>
> something like that and frankly this is trying to hard. This driver is currently really DT specific and can be fixed up later. It is causing a lot of noise in this patch series. I would really urge to focus on the core changes get prominent drivers support. I think hci_bcm.c is a good example since a) we need to fix up Edison and b) new ACPI based tablets might be able to allow for easy testing.

I can try to implement something like andy proposes, but if it turns
out too complicated we can remove this driver from the series.

Thanks!

>
> Regards
>
> Marcel
>


-- 
Ricardo Ribalda

Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Marcel Holtmann]

Hi Marcel,

>> the commit message is misleading me. If I build something with ACPI or DT support, then modinfo will show all modalias information for ACPI and DT compatible strings. What else does udev/modprobe actually need? Is something broken with the modalias export?
> 
> The main purpose is to autoload drivers for devices that have been
> created via sysfs or another module.
> 
> Eg1: We have a serial port on a standard computer that has connected a
> GPS module. Since it is something that is not in the ACPI nor the DT
> table the user will run
> 
> echo serdev_gps > /sys/bus/serial/devices/serial0/new_device
> 
> Eg2 module: https://github.com/ribalda/linux/blob/415bb3f0076c2b846ebe5409589b8e1e3004f55a/drivers/tty/serdev/test_platform.c
> 
> Modprobe does not know what module to load for that device unless
> there is a matching MODULE_DEVICE_TABLE
> Today, we have the same functionality for i2c devices
> https://www.kernel.org/doc/Documentation/i2c/instantiating-devices

but why does this have to be the driver name? I would rather say, create a generic string that describes the hardware and then use that. I think you also want the special handling, as from USB for example, that lets you reference an existing .compatible or ACPI ID as reference so that the driver_data is copied.

Regards

Marcel

Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Hi Marcel,
On Mon, Jun 11, 2018 at 5:28 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Marcel,
>
> >> the commit message is misleading me. If I build something with ACPI or DT support, then modinfo will show all modalias information for ACPI and DT compatible strings. What else does udev/modprobe actually need? Is something broken with the modalias export?
> >
> > The main purpose is to autoload drivers for devices that have been
> > created via sysfs or another module.
> >
> > Eg1: We have a serial port on a standard computer that has connected a
> > GPS module. Since it is something that is not in the ACPI nor the DT
> > table the user will run
> >
> > echo serdev_gps > /sys/bus/serial/devices/serial0/new_device
> >
> > Eg2 module: https://github.com/ribalda/linux/blob/415bb3f0076c2b846ebe5409589b8e1e3004f55a/drivers/tty/serdev/test_platform.c
> >
> > Modprobe does not know what module to load for that device unless
> > there is a matching MODULE_DEVICE_TABLE
> > Today, we have the same functionality for i2c devices
> > https://www.kernel.org/doc/Documentation/i2c/instantiating-devices
>
> but why does this have to be the driver name? I would rather say, create a generic string that describes the hardware and then use that. I think you also want the special handling, as from USB for example, that lets you reference an existing .compatible or ACPI ID as reference so that the driver_data is copied.

We can choose any name, but if there are no special variants (like the
rave-sp driver) I would prefer using the module name. We can of course
use more names, like the part number of the the device, but it is very
convenient to also use the module name, and other subsystems also do
that.

If you want to add specific names for this device please let me know
and I will add them to the list. You know much more about this module
than myself.

Thanks!

>
> Regards
>
> Marcel
>


-- 
Ricardo Ribalda

Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Marcel Holtmann]

Hi Ricardo,

>>>> the commit message is misleading me. If I build something with ACPI or DT support, then modinfo will show all modalias information for ACPI and DT compatible strings. What else does udev/modprobe actually need? Is something broken with the modalias export?
>>> 
>>> The main purpose is to autoload drivers for devices that have been
>>> created via sysfs or another module.
>>> 
>>> Eg1: We have a serial port on a standard computer that has connected a
>>> GPS module. Since it is something that is not in the ACPI nor the DT
>>> table the user will run
>>> 
>>> echo serdev_gps > /sys/bus/serial/devices/serial0/new_device
>>> 
>>> Eg2 module: https://github.com/ribalda/linux/blob/415bb3f0076c2b846ebe5409589b8e1e3004f55a/drivers/tty/serdev/test_platform.c
>>> 
>>> Modprobe does not know what module to load for that device unless
>>> there is a matching MODULE_DEVICE_TABLE
>>> Today, we have the same functionality for i2c devices
>>> https://www.kernel.org/doc/Documentation/i2c/instantiating-devices
>> 
>> but why does this have to be the driver name? I would rather say, create a generic string that describes the hardware and then use that. I think you also want the special handling, as from USB for example, that lets you reference an existing .compatible or ACPI ID as reference so that the driver_data is copied.
> 
> We can choose any name, but if there are no special variants (like the
> rave-sp driver) I would prefer using the module name. We can of course
> use more names, like the part number of the the device, but it is very
> convenient to also use the module name, and other subsystems also do
> that.
> 
> If you want to add specific names for this device please let me know
> and I will add them to the list. You know much more about this module
> than myself.

if we want to use the driver name, then why not build this into the new_device handling itself instead of duplicating that name in each serdev_device_id table. What is the reference here? For example platform device do matching on driver name if I recall this correctly.

However what is most important is that device_get_match_data() actually works. There should be no difference between DT, ACPI or a dynamic device.

Regards

Marcel

Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Hi Marcel,
On Mon, Jun 11, 2018 at 5:52 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Ricardo,
>
> >>>> the commit message is misleading me. If I build something with ACPI or DT support, then modinfo will show all modalias information for ACPI and DT compatible strings. What else does udev/modprobe actually need? Is something broken with the modalias export?
> >>>
> >>> The main purpose is to autoload drivers for devices that have been
> >>> created via sysfs or another module.
> >>>
> >>> Eg1: We have a serial port on a standard computer that has connected a
> >>> GPS module. Since it is something that is not in the ACPI nor the DT
> >>> table the user will run
> >>>
> >>> echo serdev_gps > /sys/bus/serial/devices/serial0/new_device
> >>>
> >>> Eg2 module: https://github.com/ribalda/linux/blob/415bb3f0076c2b846ebe5409589b8e1e3004f55a/drivers/tty/serdev/test_platform.c
> >>>
> >>> Modprobe does not know what module to load for that device unless
> >>> there is a matching MODULE_DEVICE_TABLE
> >>> Today, we have the same functionality for i2c devices
> >>> https://www.kernel.org/doc/Documentation/i2c/instantiating-devices
> >>
> >> but why does this have to be the driver name? I would rather say, create a generic string that describes the hardware and then use that. I think you also want the special handling, as from USB for example, that lets you reference an existing .compatible or ACPI ID as reference so that the driver_data is copied.
> >
> > We can choose any name, but if there are no special variants (like the
> > rave-sp driver) I would prefer using the module name. We can of course
> > use more names, like the part number of the the device, but it is very
> > convenient to also use the module name, and other subsystems also do
> > that.
> >
> > If you want to add specific names for this device please let me know
> > and I will add them to the list. You know much more about this module
> > than myself.
>
> if we want to use the driver name, then why not build this into the new_device handling itself instead of duplicating that name in each serdev_device_id table. What is the reference here? For example platform device do matching on driver name if I recall this correctly.

We do the matching also over driver name at serdev_device_match():

if (sdrv->id_table)
return !!serdev_match_id(sdrv->id_table, sdev);

return strcmp(sdev->modalias, drv->name) == 0;

This is just for the module autoloading

>
> However what is most important is that device_get_match_data() actually works. There should be no difference between DT, ACPI or a dynamic device.

Agree, will take a look to this tomorrow morning.

>
> Regards
>
> Marcel
>


-- 
Ricardo Ribalda

Re: [PATCH v2 18/24] serdev: ttydev: Serdev driver that creates an standard TTY port

[Rob Herring]

On Mon, Jun 11, 2018 at 5:52 AM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
> Standard TTY port that can be loaded/unloaded via serdev sysfs. This
> serdev driver can only be used by serdev controllers that are compatible
> with ttyport.

I'm hesitant to expose a tty device on top of serdev to userspace that
we then have to support forever unless that's the only way tty devices
(for serial ports) are created.

I did a similar patch which just registered both serdev and a tty
allowing them to coexist. It suffered from warnings about open counts
though and felt hacky.

Rob

Re: [PATCH v2 18/24] serdev: ttydev: Serdev driver that creates an standard TTY port

[Ricardo Ribalda Delgado]

Hi Rob,
On Wed, Jun 13, 2018 at 3:20 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Jun 11, 2018 at 5:52 AM, Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
> > Standard TTY port that can be loaded/unloaded via serdev sysfs. This
> > serdev driver can only be used by serdev controllers that are compatible
> > with ttyport.
>
> I'm hesitant to expose a tty device on top of serdev to userspace that
> we then have to support forever unless that's the only way tty devices
> (for serial ports) are created.

My concern is that, with the current implementation, serdev does have
a tiny collection of usecases:

-It cannot be used for board bring up
-It cannot be used as a replacement of hciattach and friends

It can only be used on embedded devices or platforms where the
developer has control of the ACPI table.

This hack, allows its use in almost any scenario and I have been
happily using it for two weeks with no issue.
It is also a very simple solution that does not have the issues of
cdev/serdev coexistence that you mention.

I am not very convinced about all the ttydev being serdevs. Adding 1K
lines of code to people that do not plan to use serdev seems like a
high expense. If in the future we can get rid of all the hciattach
programs then we can redesign the port probing.

>
> I did a similar patch which just registered both serdev and a tty
> allowing them to coexist. It suffered from warnings about open counts
> though and felt hacky.
>
> Rob



-- 
Ricardo Ribalda

Re: [PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*

[Johan Hovold]

Hi Ricardo, 

On Mon, Jun 11, 2018 at 01:52:16PM +0200, Ricardo Ribalda Delgado wrote:
> There are some situations where it is interesting to load/remove serdev
> devices dynamically, like during board bring-up or when we are
> developing a new driver or for devices that are neither described via
> ACPI or device tree.

First of all, this would be more appropriately labeled an RFC as this is
far from being in a mergeable state. Besides some implementation issues,
we need to determine if this approach is at all viable.

Second, I wonder how you tested this given that this series breaks RX
(and write-wakeup signalling) for serial ports (patch 19/24)?

Third, and as Marcel already suggested, you need to limit your scope
here. Aim at ten patches or so, and use a representative serdev driver
as an example of the kind of driver updates that would be needed. It
also looks like some patches should be squashed (e.g. the ones
introducing new fields and the first one actually using them).

> This implementation allows the creation of serdev devices via sysfs,
> in a similar way as the i2c bus allows sysfs instantiation [1].

Note that this is a legacy interface and not necessarily something that
new interfaces should be modelled after.

Johan

Re: [PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*

[Ricardo Ribalda Delgado]

Hi Johan
On Thu, Jun 14, 2018 at 12:48 PM Johan Hovold <johan@kernel.org> wrote:
>
> Hi Ricardo,
>
> On Mon, Jun 11, 2018 at 01:52:16PM +0200, Ricardo Ribalda Delgado wrote:
> > There are some situations where it is interesting to load/remove serdev
> > devices dynamically, like during board bring-up or when we are
> > developing a new driver or for devices that are neither described via
> > ACPI or device tree.
>
> First of all, this would be more appropriately labeled an RFC as this is
> far from being in a mergeable state. Besides some implementation issues,
> we need to determine if this approach is at all viable.

From previous conversations with Greg it seemed that RFC labels was
something to avoid, but I do not mind reseding it as RFC on v3.

http://lists.kernelnewbies.org/pipermail/kernelnewbies/2018-March/018844.html

>
> Second, I wonder how you tested this given that this series breaks RX
> (and write-wakeup signalling) for serial ports (patch 19/24)?

I have a serdev device (led ctrls) that is dynamically enumerated with
something very similar to:

https://github.com/ribalda/linux/commit/415bb3f0076c2b846ebe5409589b8e1e3004f55a

and then I have a script that does adds and removes. For standard
serial port I was not testing the data path, just that the ttyS* was
enumerated fine.

But  yesterday I believe that we found the bug that you mentioned and
we have fixed it (check end of mail). I will patch the series and
resend after I get more feedback and also implement what Marcel
suggested.

WIP is at
https://github.com/ribalda/linux/tree/serdev3

Besides this bug, we have used the new driver for over a week now with
no issues.

>
> Third, and as Marcel already suggested, you need to limit your scope
> here. Aim at ten patches or so, and use a representative serdev driver
> as an example of the kind of driver updates that would be needed. It
> also looks like some patches should be squashed (e.g. the ones
> introducing new fields and the first one actually using them).

>
> > This implementation allows the creation of serdev devices via sysfs,
> > in a similar way as the i2c bus allows sysfs instantiation [1].
>
> Note that this is a legacy interface and not necessarily something that
> new interfaces should be modelled after.

I would not consider it legacy, it is the only way to use an i2c
module without writing your own driver and/or modifying ACPI/DT table.
Just google around for i2c linux...
Thanks!

>
> Johan
Author: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Date:   Thu Jun 14 11:30:27 2018 +0200

    serdev-ttydev: Restore/change ttyport client ops

diff --git a/drivers/tty/serdev/serdev-ttydev.c
b/drivers/tty/serdev/serdev-ttydev.c
index 180035e101dc..b151c9645a1d 100644
--- a/drivers/tty/serdev/serdev-ttydev.c
+++ b/drivers/tty/serdev/serdev-ttydev.c
@@ -16,14 +16,23 @@ static int ttydev_serdev_probe(struct serdev_device *serdev)
        struct serdev_controller *ctrl = serdev->ctrl;
        struct serport *serport;
        struct device *dev;
+       const struct tty_port_client_operations *serdev_ops;

        if (!ctrl->is_ttyport)
                return -ENODEV;

        serport = serdev_controller_get_drvdata(ctrl);

+       serdev_ops = serport->port->client_ops;
+
+       serport->port->client_ops = serport->tty_ops;
        dev = tty_register_device_attr(serport->tty_drv, serport->tty_idx,
-                                       &serdev->dev, NULL, NULL);
+                                      ctrl->dev.parent, NULL, NULL);
+
+       if (IS_ERR(dev))
+               serport->port->client_ops = serdev_ops;
+       else
+               serdev_device_set_drvdata(serdev, (void *)serdev_ops);

        return dev ? 0 : PTR_ERR(dev);
 }
@@ -35,6 +44,7 @@ static void ttydev_serdev_remove(struct serdev_device *serdev)

        serport = serdev_controller_get_drvdata(ctrl);
        tty_unregister_device(serport->tty_drv, serport->tty_idx);
+       serport->port->client_ops = serdev_device_get_drvdata(serdev);
 }



-- 
Ricardo Ribalda

Re: [PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*

[Johan Hovold]

On Thu, Jun 14, 2018 at 01:06:59PM +0200, Ricardo Ribalda Delgado wrote:
> Hi Johan
> On Thu, Jun 14, 2018 at 12:48 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > Hi Ricardo,
> >
> > On Mon, Jun 11, 2018 at 01:52:16PM +0200, Ricardo Ribalda Delgado wrote:
> > > There are some situations where it is interesting to load/remove serdev
> > > devices dynamically, like during board bring-up or when we are
> > > developing a new driver or for devices that are neither described via
> > > ACPI or device tree.
> >
> > First of all, this would be more appropriately labeled an RFC as this is
> > far from being in a mergeable state. Besides some implementation issues,
> > we need to determine if this approach is at all viable.
> 
> From previous conversations with Greg it seemed that RFC labels was
> something to avoid, but I do not mind reseding it as RFC on v3.
> 
> http://lists.kernelnewbies.org/pipermail/kernelnewbies/2018-March/018844.html

Yeah, Greg uses that to triage his insane workload, but RFCs still have
their use (and are still mentioned in submitting-patches.rst).

> > Second, I wonder how you tested this given that this series breaks RX
> > (and write-wakeup signalling) for serial ports (patch 19/24)?
> 
> I have a serdev device (led ctrls) that is dynamically enumerated with
> something very similar to:
> 
> https://github.com/ribalda/linux/commit/415bb3f0076c2b846ebe5409589b8e1e3004f55a
> 
> and then I have a script that does adds and removes. For standard
> serial port I was not testing the data path, just that the ttyS* was
> enumerated fine.

Well there's more to serial ports than just enumeration, you typically
want to send and receive data as well. ;)

> But  yesterday I believe that we found the bug that you mentioned and
> we have fixed it (check end of mail). I will patch the series and
> resend after I get more feedback and also implement what Marcel
> suggested.
> 
> WIP is at
> https://github.com/ribalda/linux/tree/serdev3
> 
> Besides this bug, we have used the new driver for over a week now with
> no issues.

Hmm... No issues when not testing the main functionality of serial
ports, you mean?

And there are more issues with the series which are less apparent than
the rx (and partial tx) regression.

> > Third, and as Marcel already suggested, you need to limit your scope
> > here. Aim at ten patches or so, and use a representative serdev driver
> > as an example of the kind of driver updates that would be needed. It
> > also looks like some patches should be squashed (e.g. the ones
> > introducing new fields and the first one actually using them).
> 
> >
> > > This implementation allows the creation of serdev devices via sysfs,
> > > in a similar way as the i2c bus allows sysfs instantiation [1].
> >
> > Note that this is a legacy interface and not necessarily something that
> > new interfaces should be modelled after.
> 
> I would not consider it legacy, it is the only way to use an i2c
> module without writing your own driver and/or modifying ACPI/DT table.

It's legacy as in old, and to be used for one-off hacks and such. But
sure, that is also what this series aims at even if that doesn't mean
you *have to* copy the interface.

> Author: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> Date:   Thu Jun 14 11:30:27 2018 +0200
> 
>     serdev-ttydev: Restore/change ttyport client ops

Yep, you found the source of the broken serial port rx/tx.

Johan

Re: [PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*

[Ricardo Ribalda Delgado]

Hi Johan,
On Thu, Jun 14, 2018 at 3:34 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Jun 14, 2018 at 01:06:59PM +0200, Ricardo Ribalda Delgado wrote:
> > Hi Johan
> > On Thu, Jun 14, 2018 at 12:48 PM Johan Hovold <johan@kernel.org> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > On Mon, Jun 11, 2018 at 01:52:16PM +0200, Ricardo Ribalda Delgado wrote:
> > > > There are some situations where it is interesting to load/remove serdev
> > > > devices dynamically, like during board bring-up or when we are
> > > > developing a new driver or for devices that are neither described via
> > > > ACPI or device tree.
> > >
> > > First of all, this would be more appropriately labeled an RFC as this is
> > > far from being in a mergeable state. Besides some implementation issues,
> > > we need to determine if this approach is at all viable.
> >
> > From previous conversations with Greg it seemed that RFC labels was
> > something to avoid, but I do not mind reseding it as RFC on v3.
> >
> > http://lists.kernelnewbies.org/pipermail/kernelnewbies/2018-March/018844.html
>
> Yeah, Greg uses that to triage his insane workload, but RFCs still have
> their use (and are still mentioned in submitting-patches.rst).
>
> > > Second, I wonder how you tested this given that this series breaks RX
> > > (and write-wakeup signalling) for serial ports (patch 19/24)?
> >
> > I have a serdev device (led ctrls) that is dynamically enumerated with
> > something very similar to:
> >
> > https://github.com/ribalda/linux/commit/415bb3f0076c2b846ebe5409589b8e1e3004f55a
> >
> > and then I have a script that does adds and removes. For standard
> > serial port I was not testing the data path, just that the ttyS* was
> > enumerated fine.
>
> Well there's more to serial ports than just enumeration, you typically
> want to send and receive data as well. ;)
>
> > But  yesterday I believe that we found the bug that you mentioned and
> > we have fixed it (check end of mail). I will patch the series and
> > resend after I get more feedback and also implement what Marcel
> > suggested.
> >
> > WIP is at
> > https://github.com/ribalda/linux/tree/serdev3
> >
> > Besides this bug, we have used the new driver for over a week now with
> > no issues.
>
> Hmm... No issues when not testing the main functionality of serial
> ports, you mean?
>
> And there are more issues with the series which are less apparent than
> the rx (and partial tx) regression.

Any hints about this? What else should I change on the series?

>
> > > Third, and as Marcel already suggested, you need to limit your scope
> > > here. Aim at ten patches or so, and use a representative serdev driver
> > > as an example of the kind of driver updates that would be needed. It
> > > also looks like some patches should be squashed (e.g. the ones
> > > introducing new fields and the first one actually using them).
> >
> > >
> > > > This implementation allows the creation of serdev devices via sysfs,
> > > > in a similar way as the i2c bus allows sysfs instantiation [1].
> > >
> > > Note that this is a legacy interface and not necessarily something that
> > > new interfaces should be modelled after.
> >
> > I would not consider it legacy, it is the only way to use an i2c
> > module without writing your own driver and/or modifying ACPI/DT table.
>
> It's legacy as in old, and to be used for one-off hacks and such. But
> sure, that is also what this series aims at even if that doesn't mean
> you *have to* copy the interface.

It is not only one-off hack. It is the ONLY way to use i2c devices
that are not enumerated.

The same way as today we do not have any way of using serdev on non
enumerated devices.

>
> > Author: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > Date:   Thu Jun 14 11:30:27 2018 +0200
> >
> >     serdev-ttydev: Restore/change ttyport client ops
>
> Yep, you found the source of the broken serial port rx/tx.
>
> Johan



--
Ricardo Ribalda

Re: [PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*

[Johan Hovold]

On Thu, Jun 14, 2018 at 04:06:18PM +0200, Ricardo Ribalda Delgado wrote:
> Hi Johan,
> On Thu, Jun 14, 2018 at 3:34 PM Johan Hovold <johan@kernel.org> wrote:

> > And there are more issues with the series which are less apparent than
> > the rx (and partial tx) regression.
> 
> Any hints about this? What else should I change on the series?

There are implementation issues and there's the more fundamental
question about whether your approach to this is the right one.

Like Rob, I'm not sure we want to have the device topology depend on a
kernel config symbol (serdev and your ttydev driver). We may need to
explore Rob's sibling-device idea further.

I also want to make sure that this can be used for discoverable buses
(e.g. the USB CEC device the I've used as an example before).

As for the current implementation there are both larger and smaller
issues, like for example:

 - the fact that your sysfs and lookup interface does not use any
   locking whatsoever and thus is susceptible to races

 - your ttyport driver currently breaks the sysfs interface for all
   serial (core) devices by ignoring the attribute groups

 - the ttyport driver is arguably a hack with layering issues (which
   admittedly may be hard to avoid given the retrofitting of serdev into
   the tty layer)

Again, I suggest you submit a subset of your series (aim at 10 patches
or so) as an RFC which can be used as a basis for further discussion. No
point in discussing every implementation detail if the underlying
approach needs to be revised.

> > It's legacy as in old, and to be used for one-off hacks and such. But
> > sure, that is also what this series aims at even if that doesn't mean
> > you *have to* copy the interface.
> 
> It is not only one-off hack. It is the ONLY way to use i2c devices
> that are not enumerated.
> 
> The same way as today we do not have any way of using serdev on non
> enumerated devices.

You're missing the point: none of that means you have to copy the
interface.

Johan

Re: [PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*

[Ricardo Ribalda Delgado]

Hi Johan,
On Thu, Jun 14, 2018 at 4:55 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Jun 14, 2018 at 04:06:18PM +0200, Ricardo Ribalda Delgado wrote:
> > Hi Johan,
> > On Thu, Jun 14, 2018 at 3:34 PM Johan Hovold <johan@kernel.org> wrote:
>
> > > And there are more issues with the series which are less apparent than
> > > the rx (and partial tx) regression.
> >
> > Any hints about this? What else should I change on the series?
>
> There are implementation issues and there's the more fundamental
> question about whether your approach to this is the right one.
>
> Like Rob, I'm not sure we want to have the device topology depend on a
> kernel config symbol (serdev and your ttydev driver). We may need to
> explore Rob's sibling-device idea further.

From my point of view, if the user enables serdev, then everything has
to be a serdev, because serdev does not provide the same functionality
as a core tty device I had to implement, serdev-ttydev.c. Which is
nothing more than a wrapper.

It is very hacky, but allows replacing the core tty device with another serdev.

>
> I also want to make sure that this can be used for discoverable buses
> (e.g. the USB CEC device the I've used as an example before).
>

I have tried your patch:

https://github.com/ribalda/linux/commit/5cb30b4ce6477132a23492c674d8b3dc81ecff86

the only issue is that the serdev device sometimes explotes (OOPS)
when the usb is unplugged :S.

And that might be quite tricy to solve

> As for the current implementation there are both larger and smaller
> issues, like for example:
>
>  - the fact that your sysfs and lookup interface does not use any
>    locking whatsoever and thus is susceptible to races

I thought that sysfs access where serialised. If that is not the case
yes, we need a lock.

>
>  - your ttyport driver currently breaks the sysfs interface for all
>    serial (core) devices by ignoring the attribute groups

Yep, you are right, I screwed up that one :).

>
>  - the ttyport driver is arguably a hack with layering issues (which
>    admittedly may be hard to avoid given the retrofitting of serdev into
>    the tty layer)
>
> Again, I suggest you submit a subset of your series (aim at 10 patches
> or so) as an RFC which can be used as a basis for further discussion. No
> point in discussing every implementation detail if the underlying
> approach needs to be revised.

Will do. Give me some time to give it a hand of paint.

Thanks for time reviewing my little moster

>
> > > It's legacy as in old, and to be used for one-off hacks and such. But
> > > sure, that is also what this series aims at even if that doesn't mean
> > > you *have to* copy the interface.
> >
> > It is not only one-off hack. It is the ONLY way to use i2c devices
> > that are not enumerated.
> >
> > The same way as today we do not have any way of using serdev on non
> > enumerated devices.
>
> You're missing the point: none of that means you have to copy the
> interface.
>
> Johan



-- 
Ricardo Ribalda

Re: [PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*

[Johan Hovold]

On Thu, Jun 14, 2018 at 05:20:40PM +0200, Ricardo Ribalda Delgado wrote:
> Hi Johan,
> On Thu, Jun 14, 2018 at 4:55 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Jun 14, 2018 at 04:06:18PM +0200, Ricardo Ribalda Delgado wrote:
> > > Hi Johan,
> > > On Thu, Jun 14, 2018 at 3:34 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > > > And there are more issues with the series which are less apparent than
> > > > the rx (and partial tx) regression.
> > >
> > > Any hints about this? What else should I change on the series?
> >
> > There are implementation issues and there's the more fundamental
> > question about whether your approach to this is the right one.
> >
> > Like Rob, I'm not sure we want to have the device topology depend on a
> > kernel config symbol (serdev and your ttydev driver). We may need to
> > explore Rob's sibling-device idea further.
> 
> From my point of view, if the user enables serdev, then everything has
> to be a serdev, because serdev does not provide the same functionality
> as a core tty device I had to implement, serdev-ttydev.c. Which is
> nothing more than a wrapper.
> 
> It is very hacky, but allows replacing the core tty device with
> another serdev.

Yes, and I'm a bit surprised (and impressed) that you got it to work
(mostly) so easily.

My point was that we probably don't want the tty devices to move around
in the device hierarchy depending on if serdev (and your ttydev driver)
is enabled or not.

> > I also want to make sure that this can be used for discoverable buses
> > (e.g. the USB CEC device the I've used as an example before).
> >
> 
> I have tried your patch:
> 
> https://github.com/ribalda/linux/commit/5cb30b4ce6477132a23492c674d8b3dc81ecff86
> 
> the only issue is that the serdev device sometimes explotes (OOPS)
> when the usb is unplugged :S.
> 
> And that might be quite tricy to solve

Yes, we all know serdev doesn't support hotplug, that's why it's not
enabled for usbserial.

But when/if we get that sorted, we may want to be able to reuse some of
the matching infrastructure that a sysfs interface would use also for
discoverable buses (e.g. passing a compatible string to serdev core).

Also note that the interface you're proposing suffers from similar
problems as hotplug in that serdev drivers must be prepared to handle
devices going away at anytime; be it through your delete_device
interface or from a sysfs driver unbind (i.e. already an issue today!).

This would be were the oops comes from.

> > As for the current implementation there are both larger and smaller
> > issues, like for example:
> >
> >  - the fact that your sysfs and lookup interface does not use any
> >    locking whatsoever and thus is susceptible to races
> 
> I thought that sysfs access where serialised. If that is not the case
> yes, we need a lock.

It's only serialised per attribute.

> >  - your ttyport driver currently breaks the sysfs interface for all
> >    serial (core) devices by ignoring the attribute groups
> 
> Yep, you are right, I screwed up that one :).

Easy to miss.

> >  - the ttyport driver is arguably a hack with layering issues (which
> >    admittedly may be hard to avoid given the retrofitting of serdev into
> >    the tty layer)
> >
> > Again, I suggest you submit a subset of your series (aim at 10 patches
> > or so) as an RFC which can be used as a basis for further discussion. No
> > point in discussing every implementation detail if the underlying
> > approach needs to be revised.
> 
> Will do. Give me some time to give it a hand of paint.
> 
> Thanks for time reviewing my little moster

You're welcome.

Johan