Re: [PATCH 2/9] thunderbolt: Add USB4 port devices

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi,

On Wed, May 19, 2021 at 06:40:05PM +0300, Mika Westerberg wrote:
> > I noticed that in the next patch you add acpi_bus_type for these
> > ports, but is that really necessary? Why not just:
> > 
> > int usb4_port_fwnode_match(struct tb_port *port, struct fwnode_handle *fwnode)
> > {
> >         if (is_acpi_device_node(fwnode))
> >                 return acpi_device_adr(to_acpi_device_node(fwnode)) == port->port;
> > 
> >         return 0;
> > }
> 
> I have to say I might be missing some new additions to fwnode front but
> the acpi_bus_type is used to match the ACPI nodes to usb4_ports and also
> to routers. I see that this one may work for the former but not sure
> about the latter.

Yeah, I noticed that afterwards.

> I guess we could do similar for routers too in switch.c.

Forget about it.

> However, I would like to keep ACPI specific code in acpi.c if possible
> but if this is the preferred way then no problem doing what you say :)

Well, that's actually the thing. The matching is the only ACPI (or any
firmware type) specific thing that we need when assigning the firmware
nodes to the device. Note also that the above function works already
even with CONFIG_ACPI=n. That's why I though that there is no reason
not to just make this firmware node type agnostic from the start.

But since your acpi_bus_type handles also the routers, and also since
the node hierarchy with the separate UFP and DFP ports and everything
did not look straightforward, it's probable easier to just use that.

So sorry again for the noise.

> > > +/**
> > > + * usb4_port_device_add() - Add USB4 port device
> > > + * @port: Lane 0 adapter port to add the USB4 port
> > > + *
> > > + * Creates and registers a USB4 port device for @port. Returns the new
> > > + * USB4 port device pointer or ERR_PTR() in case of error.
> > > + */
> > > +struct usb4_port *usb4_port_device_add(struct tb_port *port)
> > > +{
> > 
> >         struct fwnode_handle *child;
> > 
> > > +	struct usb4_port *usb4;
> > > +	int ret;
> > > +
> > > +	usb4 = kzalloc(sizeof(*usb4), GFP_KERNEL);
> > > +	if (!usb4)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	usb4->port = port;
> > > +	usb4->dev.type = &usb4_port_device_type;
> > > +	usb4->dev.parent = &port->sw->dev;
> > > +	dev_set_name(&usb4->dev, "usb4_port%d", port->port);
> > 
> > and then here something like this (feel free to improve this part):
> > 
> >         device_for_each_child_node(&port->sw->dev, child) {
> >                 if (usb4_port_fwnode_match(port, child)) {
> >                         usb4->dev.fwnode = child;
> >                         break;
> >                 }
> >         }
> > 
> > Or maybe I'm missing something?
> > 
> > > +	ret = device_register(&usb4->dev);
> > > +	if (ret) {
> > > +		put_device(&usb4->dev);
> > > +		return ERR_PTR(ret);
> > > +	}
> > > +
> > > +	pm_runtime_no_callbacks(&usb4->dev);
> > > +	pm_runtime_set_active(&usb4->dev);
> > > +	pm_runtime_enable(&usb4->dev);
> > > +	pm_runtime_set_autosuspend_delay(&usb4->dev, TB_AUTOSUSPEND_DELAY);
> > > +	pm_runtime_mark_last_busy(&usb4->dev);
> > > +	pm_runtime_use_autosuspend(&usb4->dev);
> > > +
> > > +	return usb4;
> > > +}

thanks,

-- 
heikki