Re: [PATCH v2 net] ptp: ocp: adjust serial port symlink creation

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 10 May 2024 11:04:05 +0000 Vadim Fedorenko wrote:
> The commit b286f4e87e32 ("serial: core: Move tty and serdev to be children
> of serial core port device") changed the hierarchy of serial port devices
> and device_find_child_by_name cannot find ttyS* devices because they are
> no longer directly attached. Add some logic to restore symlinks creation
> to the driver for OCP TimeCard.
> 
> Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be children of serial core port device")
> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> ---
> v2:
>  add serial/8250 maintainers
> ---
>  drivers/ptp/ptp_ocp.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index ee2ced88ab34..50b7cb9db3be 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -25,6 +25,8 @@
>  #include <linux/crc16.h>
>  #include <linux/dpll.h>
>  
> +#include "../tty/serial/8250/8250.h"

Hi Greg, Jiri, does this look reasonable to you?
The cross tree include raises an obvious red flag.

Should serial / u8250 provide a more official API?
Can we use device_for_each_child() to deal with the extra
layer in the hierarchy?

>  #define PCI_VENDOR_ID_FACEBOOK			0x1d9b
>  #define PCI_DEVICE_ID_FACEBOOK_TIMECARD		0x0400
>  
> @@ -4330,11 +4332,9 @@ ptp_ocp_symlink(struct ptp_ocp *bp, struct device *child, const char *link)
>  }
>  
>  static void
> -ptp_ocp_link_child(struct ptp_ocp *bp, const char *name, const char *link)
> +ptp_ocp_link_child(struct ptp_ocp *bp, struct device *dev, const char *name, const char *link)
>  {
> -	struct device *dev, *child;
> -
> -	dev = &bp->pdev->dev;
> +	struct device *child;
>  
>  	child = device_find_child_by_name(dev, name);
>  	if (!child) {
> @@ -4349,27 +4349,39 @@ ptp_ocp_link_child(struct ptp_ocp *bp, const char *name, const char *link)
>  static int
>  ptp_ocp_complete(struct ptp_ocp *bp)
>  {
> +	struct device *dev, *port_dev;
> +	struct uart_8250_port *port;
>  	struct pps_device *pps;
>  	char buf[32];
>  
> +	dev = &bp->pdev->dev;
> +
>  	if (bp->gnss_port.line != -1) {
> +		port = serial8250_get_port(bp->gnss_port.line);
> +		port_dev = (struct device *)port->port.port_dev;
>  		sprintf(buf, "ttyS%d", bp->gnss_port.line);
> -		ptp_ocp_link_child(bp, buf, "ttyGNSS");
> +		ptp_ocp_link_child(bp, port_dev, buf, "ttyGNSS");
>  	}
>  	if (bp->gnss2_port.line != -1) {
> +		port = serial8250_get_port(bp->gnss2_port.line);
> +		port_dev = (struct device *)port->port.port_dev;
>  		sprintf(buf, "ttyS%d", bp->gnss2_port.line);
> -		ptp_ocp_link_child(bp, buf, "ttyGNSS2");
> +		ptp_ocp_link_child(bp, port_dev, buf, "ttyGNSS2");
>  	}
>  	if (bp->mac_port.line != -1) {
> +		port = serial8250_get_port(bp->mac_port.line);
> +		port_dev = (struct device *)port->port.port_dev;
>  		sprintf(buf, "ttyS%d", bp->mac_port.line);
> -		ptp_ocp_link_child(bp, buf, "ttyMAC");
> +		ptp_ocp_link_child(bp, port_dev, buf, "ttyMAC");
>  	}
>  	if (bp->nmea_port.line != -1) {
> +		port = serial8250_get_port(bp->nmea_port.line);
> +		port_dev = (struct device *)port->port.port_dev;
>  		sprintf(buf, "ttyS%d", bp->nmea_port.line);
> -		ptp_ocp_link_child(bp, buf, "ttyNMEA");
> +		ptp_ocp_link_child(bp, port_dev, buf, "ttyNMEA");
>  	}
>  	sprintf(buf, "ptp%d", ptp_clock_index(bp->ptp));
> -	ptp_ocp_link_child(bp, buf, "ptp");
> +	ptp_ocp_link_child(bp, dev, buf, "ptp");
>  
>  	pps = pps_lookup_dev(bp->ptp);
>  	if (pps)