Messages in this thread | | | Date | Thu, 19 Oct 2023 09:21:38 +0200 | Subject | Re: [PATCH 4/5] i3c: slave: func: add tty driver | From | Jiri Slaby <> |
| |
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.
> --- /dev/null > +++ b/drivers/i3c/func/tty.c > @@ -0,0 +1,548 @@ ... > +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
| |