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

From: Ricardo Ribalda Delgado
Date: Thu Jun 14 2018 - 07:07:24 EST

Hi Johan
On Thu, Jun 14, 2018 at 12:48 PM Johan Hovold <johan@xxxxxxxxxx> 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.

> 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:

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

WIP is at

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...

> Johan
Author: Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx>
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
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