Re: [PATCH v9 1/1] serial: core: Start managing serial controllers to enable runtime PM

[Tony Lindgren]

* 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