* Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> [230329 09:19]:
> On Thu, Mar 23, 2023 at 09:10:47AM +0200, Tony Lindgren wrote:
> > -obj-$(CONFIG_SERIAL_CORE) += serial_core.o
> > +obj-$(CONFIG_SERIAL_CORE) += serial_base.o serial_core.o serial_ctrl.o serial_port.o
>
> Why is this 3 new modules and not just all go into serial_base? What's
> going to auto-load the other modules you created here? Feels like this
> should all end up in the same .ko as they all depend on each other,
> right?
OK sure, I'll build them into serial_base. We now have uart_add_one_port()
and uart_remove_one_port() exported in serial_port so that ends up loading
the serial_base related modules.
> > +struct uart_port *serial_base_get_port(struct device *dev)
> > +{
> > + struct serial_base_device *sbd;
> > +
> > + if (!dev)
> > + return NULL;
> > +
> > + sbd = to_serial_base_device(dev);
> > +
> > + /* Check in case serial_core_add_one_port() happened to fail */
> > + if (!sbd->port->state) {
>
> This is odd, how can it fail and then this function be called after that
> failure?
On uart_add_one_port(), runtime PM resume function in serial_port gets
called before the port registration has completed. Sounds like I need
to recheck this, maybe we can just enable runtime PM for serial_port
after registration has completed.
> > +/*
> > + * Find a registered serial core controller device if one exists. Returns
> > + * the first device matching the ctrl_id. Caller must hold port_mutex.
> > + */
> > +static struct device *serial_core_ctrl_find(struct uart_driver *drv,
> > + struct device *phys_dev,
> > + int ctrl_id)
> > +{
> > + struct uart_state *state;
> > + int i;
> > +
> > + if (ctrl_id < 0)
> > + return NULL;
>
> Why is a negative number special here?
I think this can go, will check.
> > + dev = serial_base_device_add(port, "port", ctrl_dev);
>
> magic strings again :)
>
> Do you really just want two different "types" of devices on this bus,
> controllers and ports? If so, just do that, don't make the name magic
> here.
>
> Then you can have:
> serial_base_port_add()
> serial_base_ctrl_add()
>
> and one cleanup function will still work.
Yes two different types should do here, I'll take a look.
> Otherwise this looks good to me, thanks for doing all of this work.
OK great thanks,
Tony
