Linux Serial subsystem development
 help / color / mirror / Atom feed
* [PATCH v2 16/24] serdev: ttyport: Move serport structure to its own header
From: Ricardo Ribalda Delgado @ 2018-06-11 11:52 UTC (permalink / raw)
  To: linux-kernel, linux-serial
  Cc: Ricardo Ribalda Delgado, Rob Herring, Johan Hovold,
	Greg Kroah-Hartman, Jiri Slaby
In-Reply-To: <20180611115240.32606-1-ricardo.ribalda@gmail.com>

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

^ permalink raw reply related

* [PATCH v2 17/24] serdev: Mark controllers compatible with ttyport
From: Ricardo Ribalda Delgado @ 2018-06-11 11:52 UTC (permalink / raw)
  To: linux-kernel, linux-serial
  Cc: Ricardo Ribalda Delgado, Rob Herring, Johan Hovold,
	Greg Kroah-Hartman, Jiri Slaby
In-Reply-To: <20180611115240.32606-1-ricardo.ribalda@gmail.com>

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

^ permalink raw reply related

* [PATCH v2 18/24] serdev: ttydev: Serdev driver that creates an standard TTY port
From: Ricardo Ribalda Delgado @ 2018-06-11 11:52 UTC (permalink / raw)
  To: linux-kernel, linux-serial
  Cc: Ricardo Ribalda Delgado, Rob Herring, Johan Hovold,
	Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko
In-Reply-To: <20180611115240.32606-1-ricardo.ribalda@gmail.com>

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

^ permalink raw reply related

* [PATCH v2 19/24] serdev: Instantiate a ttydev serdev if acpi and of fails
From: Ricardo Ribalda Delgado @ 2018-06-11 11:52 UTC (permalink / raw)
  To: linux-kernel, linux-serial
  Cc: Ricardo Ribalda Delgado, Rob Herring, Johan Hovold,
	Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko
In-Reply-To: <20180611115240.32606-1-ricardo.ribalda@gmail.com>

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

^ permalink raw reply related

* [PATCH v2 20/24] serdev: Make match_id accessible by drivers
From: Ricardo Ribalda Delgado @ 2018-06-11 11:52 UTC (permalink / raw)
  To: linux-kernel, linux-serial
  Cc: Ricardo Ribalda Delgado, Rob Herring, Johan Hovold,
	Greg Kroah-Hartman, Jiri Slaby
In-Reply-To: <20180611115240.32606-1-ricardo.ribalda@gmail.com>

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

^ permalink raw reply related

* [PATCH v2 21/24] rave-sp: Support for variants on modalias drivers
From: Ricardo Ribalda Delgado @ 2018-06-11 11:52 UTC (permalink / raw)
  To: linux-kernel, linux-serial
  Cc: Ricardo Ribalda Delgado, Lee Jones, Rob Herring, Johan Hovold
In-Reply-To: <20180611115240.32606-1-ricardo.ribalda@gmail.com>

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

^ permalink raw reply related

* [PATCH v2 22/24] serdev: Replace IDA functions with IDR
From: Ricardo Ribalda Delgado @ 2018-06-11 11:52 UTC (permalink / raw)
  To: linux-kernel, linux-serial
  Cc: Ricardo Ribalda Delgado, Rob Herring, Johan Hovold,
	Greg Kroah-Hartman, Jiri Slaby
In-Reply-To: <20180611115240.32606-1-ricardo.ribalda@gmail.com>

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

^ permalink raw reply related

* [PATCH v2 23/24] serdev: get/put controller
From: Ricardo Ribalda Delgado @ 2018-06-11 11:52 UTC (permalink / raw)
  To: linux-kernel, linux-serial
  Cc: Ricardo Ribalda Delgado, Rob Herring, Johan Hovold,
	Greg Kroah-Hartman, Jiri Slaby
In-Reply-To: <20180611115240.32606-1-ricardo.ribalda@gmail.com>

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

^ permalink raw reply related

* [PATCH v2 24/24] serdev: serdev_controller_add_probed_device
From: Ricardo Ribalda Delgado @ 2018-06-11 11:52 UTC (permalink / raw)
  To: linux-kernel, linux-serial
  Cc: Ricardo Ribalda Delgado, Rob Herring, Johan Hovold,
	Greg Kroah-Hartman, Jiri Slaby
In-Reply-To: <20180611115240.32606-1-ricardo.ribalda@gmail.com>

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

^ permalink raw reply related

* Re: [PATCH v2 07/24] serdev: Allows dynamic creation of devices via sysfs
From: Andy Shevchenko @ 2018-06-11 12:39 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: Linux Kernel Mailing List, open list:SERIAL DRIVERS, Rob Herring,
	Johan Hovold, Greg Kroah-Hartman, Jiri Slaby
In-Reply-To: <20180611115240.32606-8-ricardo.ribalda@gmail.com>

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

^ permalink raw reply

* Re: [PATCH v2 20/24] serdev: Make match_id accessible by drivers
From: Andy Shevchenko @ 2018-06-11 12:47 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: Linux Kernel Mailing List, open list:SERIAL DRIVERS, Rob Herring,
	Johan Hovold, Greg Kroah-Hartman, Jiri Slaby
In-Reply-To: <20180611115240.32606-21-ricardo.ribalda@gmail.com>

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

^ permalink raw reply

* Re: [PATCH v2 21/24] rave-sp: Support for variants on modalias drivers
From: Andy Shevchenko @ 2018-06-11 12:54 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: Linux Kernel Mailing List, open list:SERIAL DRIVERS, Lee Jones,
	Rob Herring, Johan Hovold
In-Reply-To: <20180611115240.32606-22-ricardo.ribalda@gmail.com>

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

^ permalink raw reply

* Re: [PATCH v2 04/24] Bluetooth: hci_nokia: Add serdev_id_table
From: Marcel Holtmann @ 2018-06-11 12:56 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: LKML, linux-serial, Johan Hedberg, Rob Herring, Johan Hovold,
	Andy Shevchenko, linux-bluetooth
In-Reply-To: <20180611115240.32606-5-ricardo.ribalda@gmail.com>

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

^ permalink raw reply

* Re: [PATCH v2 11/24] Bluetooth: hci_bcm: MODULE_DEVICE_TABLE(serdev)
From: Marcel Holtmann @ 2018-06-11 12:59 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: linux-kernel, linux-serial, Johan Hedberg, Rob Herring,
	Johan Hovold, linux-bluetooth
In-Reply-To: <20180611115240.32606-12-ricardo.ribalda@gmail.com>

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

^ permalink raw reply

* Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)
From: Marcel Holtmann @ 2018-06-11 13:01 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: LKML, linux-serial, Lino Sanfilippo, David S. Miller,
	Stefan Wahren, Rob Herring, Johan Hovold, netdev
In-Reply-To: <20180611115240.32606-16-ricardo.ribalda@gmail.com>

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

^ permalink raw reply

* Re: [PATCH v2 07/24] serdev: Allows dynamic creation of devices via sysfs
From: Ricardo Ribalda Delgado @ 2018-06-11 13:03 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: LKML, open list:SERIAL DRIVERS, Rob Herring, Johan Hovold,
	Greg Kroah-Hartman, Jiri Slaby
In-Reply-To: <CAHp75Vf-V7BVU_SiyBp18D0cM_nOO=EAxfgzUaa4-hngh9PeMw@mail.gmail.com>

Hi Andy,

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

Thanks!

-- 
Ricardo Ribalda

^ permalink raw reply

* Re: [PATCH v2 04/24] Bluetooth: hci_nokia: Add serdev_id_table
From: Ricardo Ribalda Delgado @ 2018-06-11 13:04 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: LKML, open list:SERIAL DRIVERS, Johan Hedberg, Rob Herring,
	Johan Hovold, Andy Shevchenko, linux-bluetooth
In-Reply-To: <94360989-9AA9-422D-9CA3-AA631731BD2D@holtmann.org>

HI Marcel

I have just removed it from my series

Sorry for the extra noise


-- 
Ricardo Ribalda

^ permalink raw reply

* Re: [PATCH v2 20/24] serdev: Make match_id accessible by drivers
From: Ricardo Ribalda Delgado @ 2018-06-11 13:10 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: LKML, open list:SERIAL DRIVERS, Rob Herring, Johan Hovold,
	Greg Kroah-Hartman, Jiri Slaby
In-Reply-To: <CAHp75VcKxZdp=Rmkt=K+yHq2aFAfSsD+SrKvPsSCCVLmLgc1pA@mail.gmail.com>

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

^ permalink raw reply

* Re: [PATCH v2 14/24] mfd: rave-sp: MODULE_DEVICE_TABLE(serdev)
From: Marcel Holtmann @ 2018-06-11 13:14 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado
  Cc: linux-kernel, linux-serial, Lee Jones, Rob Herring, Johan Hovold
In-Reply-To: <20180611115240.32606-15-ricardo.ribalda@gmail.com>

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

^ permalink raw reply

* Re: [PATCH v2 11/24] Bluetooth: hci_bcm: MODULE_DEVICE_TABLE(serdev)
From: Andy Shevchenko @ 2018-06-11 13:31 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Ricardo Ribalda Delgado, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS, Johan Hedberg, Rob Herring,
	Johan Hovold, linux-bluetooth
In-Reply-To: <D4622EB7-BB18-4931-BF14-34995F7EBBA8@holtmann.org>

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

^ permalink raw reply

* Re: [PATCH v2 21/24] rave-sp: Support for variants on modalias drivers
From: Marcel Holtmann @ 2018-06-11 13:38 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Ricardo Ribalda Delgado, Linux Kernel Mailing List,
	open list:SERIAL DRIVERS, Lee Jones, Rob Herring, Johan Hovold
In-Reply-To: <CAHp75VeeF4eNjyhhBxocRXboN3fRY=RjrT6QQ72=QJ-s8gvSfA@mail.gmail.com>

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

^ permalink raw reply

* Re: [PATCH v5 3/6] mfd: at91-usart: added mfd driver for usart
From: Enric Balletbo Serra @ 2018-06-11 14:55 UTC (permalink / raw)
  To: radu.pirea
  Cc: Mark Brown, Nicolas Ferre, Alexandre Belloni, Lee Jones,
	richard.genoud, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
	linux-spi, Linux ARM, linux-kernel, devicetree@vger.kernel.org,
	linux-serial
In-Reply-To: <20180604165943.31381-4-radu.pirea@microchip.com>

Hi Radu,

Some few feedback in a hope to help you.

Missatge de Radu Pirea <radu.pirea@microchip.com> del dia dl., 4 de
juny 2018 a les 19:03:
>
> This mfd driver is just a wrapper over atmel_serial driver and
> spi-at91-usart driver. Selection of one of the drivers is based on a
> property from device tree. If the property is not specified, the default
> driver is atmel_serial.
>
> Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/mfd/Kconfig      |  9 ++++++
>  drivers/mfd/Makefile     |  1 +
>  drivers/mfd/at91-usart.c | 67 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+)
>  create mode 100644 drivers/mfd/at91-usart.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b860eb5aa194..a886672b960d 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -99,6 +99,15 @@ config MFD_AAT2870_CORE
>           additional drivers must be enabled in order to use the
>           functionality of the device.
>
> +config MFD_AT91_USART
> +       tristate "AT91 USART Driver"
> +       select MFD_CORE
> +       help
> +         Select this to get support for AT91 USART IP. This is a wrapper
> +         over at91-usart-serial driver and usart-spi-driver. Only one function
> +         can be used at a time. The choice is done at boot time by the probe
> +         function of this MFD driver according to a device tree property.
> +
>  config MFD_ATMEL_FLEXCOM
>         tristate "Atmel Flexcom (Flexible Serial Communication Unit)"
>         select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index d9d2cf0d32ef..db1332aa96db 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -185,6 +185,7 @@ obj-$(CONFIG_MFD_SPMI_PMIC) += qcom-spmi-pmic.o
>  obj-$(CONFIG_TPS65911_COMPARATOR)      += tps65911-comparator.o
>  obj-$(CONFIG_MFD_TPS65090)     += tps65090.o
>  obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o
> +obj-$(CONFIG_MFD_AT91_USART)   += at91-usart.o
>  obj-$(CONFIG_MFD_ATMEL_FLEXCOM)        += atmel-flexcom.o
>  obj-$(CONFIG_MFD_ATMEL_HLCDC)  += atmel-hlcdc.o
>  obj-$(CONFIG_MFD_ATMEL_SMC)    += atmel-smc.o
> diff --git a/drivers/mfd/at91-usart.c b/drivers/mfd/at91-usart.c
> new file mode 100644
> index 000000000000..262e7affba22
> --- /dev/null
> +++ b/drivers/mfd/at91-usart.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0+

This means GPL-2.0 or later ...

> +/*
> + * Driver for AT91 USART
> + *
> + * Copyright (C) 2018 Microchip Technology
> + *
> + * Author: Radu Pirea <radu.pirea@microchip.com>
> + *
> + */
> +
> +#include <dt-bindings/mfd/at91-usart.h>
> +
> +#include <linux/module.h>
> +#include <linux/mfd/core.h>
> +#include <linux/property.h>
> +
> +static struct mfd_cell at91_usart_spi_subdev = {
> +               .name = "at91_usart_spi",
> +               .of_compatible = "microchip,at91sam9g45-usart-spi",
> +       };
> +
> +static struct mfd_cell at91_usart_serial_subdev = {
> +               .name = "atmel_usart_serial",
> +               .of_compatible = "atmel,at91rm9200-usart-serial",
> +       };
> +
> +static int at91_usart_mode_probe(struct platform_device *pdev)
> +{
> +       struct mfd_cell cell;
> +       u32 opmode;
> +       int err;
> +
> +       err = device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> +

The returned value is not checked (and probably has no sense to
check), so I think you can just remove the err variable.

> +       switch (opmode) {
> +       case AT91_USART_MODE_SPI:
> +               cell = at91_usart_spi_subdev;
> +               break;
> +       case AT91_USART_MODE_SERIAL:
> +       default:
> +               cell = at91_usart_serial_subdev;
> +       }
> +
> +       return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO, &cell, 1,
> +                             NULL, 0, NULL);
> +}
> +
> +static const struct of_device_id at91_usart_mode_of_match[] = {
> +       { .compatible = "atmel,at91rm9200-usart" },
> +       { .compatible = "atmel,at91sam9260-usart" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, at91_flexcom_of_match);
> +
> +static struct platform_driver at91_usart_mfd = {
> +       .probe  = at91_usart_mode_probe,
> +       .driver = {
> +               .name           = "at91_usart_mode",
> +               .of_match_table = at91_usart_mode_of_match,
> +       },
> +};
> +
> +module_platform_driver(at91_usart_mfd);
> +
> +MODULE_AUTHOR("Radu Pirea <radu.pirea@microchip.com>");
> +MODULE_DESCRIPTION("AT91 USART MFD driver");
> +MODULE_LICENSE("GPL v2");

And this means GPL-2.0-only, so there is a mismatch. The SPDX
identifier should be GPL-2.0 or the module license should be GPL.

> --
> 2.17.1
>
Cheers,
  Enric

^ permalink raw reply

* Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)
From: Ricardo Ribalda Delgado @ 2018-06-11 15:09 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: LKML, open list:SERIAL DRIVERS, Lino Sanfilippo, David Miller,
	Stefan Wahren, Rob Herring, Johan Hovold, netdev
In-Reply-To: <CCB7FF79-F89A-4421-A6C0-E0663BEE6065@holtmann.org>

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

^ permalink raw reply

* Re: [PATCH v2 14/24] mfd: rave-sp: MODULE_DEVICE_TABLE(serdev)
From: Ricardo Ribalda Delgado @ 2018-06-11 15:18 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: LKML, open list:SERIAL DRIVERS, Lee Jones, Rob Herring,
	Johan Hovold
In-Reply-To: <EBCCCD1F-0F23-4599-8A78-32BBF2DAF746@holtmann.org>

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

^ permalink raw reply

* Re: [PATCH v2 21/24] rave-sp: Support for variants on modalias drivers
From: Ricardo Ribalda Delgado @ 2018-06-11 15:21 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Andy Shevchenko, LKML, open list:SERIAL DRIVERS, Lee Jones,
	Rob Herring, Johan Hovold
In-Reply-To: <8912BE88-4AA3-44ED-9828-57666CB31522@holtmann.org>

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

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox