On 18. 10. 23, 23:58, Frank Li wrote:

Add tty over I3C slave function driver.

Many of the master review comments apply also here. Please fix here too. 
More below.

...

+static void i3c_slave_tty_rx_complete(struct i3c_request *req)
+{
+	struct ttyi3c_port *port = req->context;
+
+	if (req->status == I3C_REQUEST_CANCEL) {
+		i3c_slave_ctrl_free_request(req);
+		return;
+	}
+
+	for (int i = 0; i < req->actual; i++)
+		tty_insert_flip_char(&port->port, *(u8 *)(req->buf + i), 0);

Maybe I miss something obvious, but req->buf is void *. So why not 
simple tty_insert_flip_string()?

+	sport->buffer = (void *)get_zeroed_page(GFP_KERNEL);
+	if (!sport->buffer)
+		return -ENOMEM;
+
+	sport->xmit.buf = (void *)get_zeroed_page(GFP_KERNEL);

tty_port_alloc_xmit_buf()

+static int i3c_tty_probe(struct i3c_slave_func *func)
+{
+	struct device *dev = &func->dev;
+	struct ttyi3c_port *port;
+
+	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->i3cdev = func;
+	dev_set_drvdata(&func->dev, port);
+
+	port->workqueue = alloc_workqueue("%s", 0, 0, dev_name(&func->dev));

Another wq? You'd have to have a strong reason for these. Drop that.

+	if (!port->workqueue)
+		return -ENOMEM;
+
+	INIT_WORK(&port->work, i3c_slave_tty_i3c_work);
+
+	return 0;
+}
+
+static void  i3c_tty_remove(struct i3c_slave_func *func)
+{
+	struct ttyi3c_port *port;
+
+	port = dev_get_drvdata(&func->dev);

That can be on one line.

+
+	destroy_workqueue(port->workqueue);
+}

+static int i3c_open(struct tty_struct *tty, struct file *filp)
+{
+	struct ttyi3c_port *sport = tty->driver_data;
+	int ret;
+
+	if (!i3c_slave_ctrl_get_addr(sport->i3cdev->ctrl)) {
+		dev_info(&sport->i3cdev->dev, "No slave addr assigned, try hotjoin");

Should this be a debug print instead?

+		ret = i3c_slave_ctrl_hotjoin(sport->i3cdev->ctrl);
+		if (ret) {
+			dev_err(&sport->i3cdev->dev, "Hotjoin failure, check connection");
+			return ret;
+		}
+	}
+
+	return tty_port_open(&sport->port, tty, filp);

regards,
-- 
js
suse labs