[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <7c87e50e-771c-4cf4-bb28-38e13b0542fd@linux.dev>
Date: Fri, 16 Aug 2024 19:48:02 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Jiri Slaby <jirislaby@...nel.org>, Vadim Fedorenko <vadfed@...a.com>,
Jakub Kicinski <kuba@...nel.org>, Jonathan Lemon <jonathan.lemon@...il.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net v3 1/2] ptp: ocp: adjust sysfs entries to expose tty
information
On 16/08/2024 06:26, Jiri Slaby wrote:
> On 15. 08. 24, 14:59, Vadim Fedorenko wrote:
>> Starting v6.8 the serial port subsystem changed the hierarchy of devices
>> and symlinks are not working anymore. Previous discussion made it clear
>> that the idea of symlinks for tty devices was wrong by design.
>
> Care to link it here?
Sure, with the next version.
>
>> Implement
>> additional attributes to expose the information. Fixes tag points to the
>> commit which introduced the change.
> ...
>> --- a/drivers/ptp/ptp_ocp.c
>> +++ b/drivers/ptp/ptp_ocp.c
>> @@ -316,6 +316,15 @@ struct ptp_ocp_serial_port {
>> #define OCP_SERIAL_LEN 6
>> #define OCP_SMA_NUM 4
>> +enum {
>> + PORT_GNSS,
>> + PORT_GNSS2,
>> + PORT_MAC, /* miniature atomic clock */
>> + PORT_NMEA,
>> +
>> + PORT_NUM_MAX
>> +};
>> +
>
> The conversion to the array needs to go to a separate patch, apparently.
I'm not sure here. I'm trying to fix the regression introduced back in
6.8. The conversion to the array itself doesn't solve the issue, it's
pure net-next material. But the simple fix was NACKed previously, that's
why I had to introduce such a big change. I would like to keep these
changes all together in one patch.
>
>> +static ssize_t
>> +ptp_ocp_tty_show(struct device *dev, struct device_attribute *attr,
>> char *buf)
>> +{
>> + struct dev_ext_attribute *ea = to_ext_attr(attr);
>> + struct ptp_ocp *bp = dev_get_drvdata(dev);
>> + struct ptp_ocp_serial_port *port;
>> +
>> + return sysfs_emit(buf, "ttyS%d", bp->port[(uintptr_t)ea->var].line);
>
> uintptr_t is unusual as Greg points out. It is a correct type per C99 to
> cast from/to pointers, but we usually do "unsigned long". (int wouldn't
> work as it has a different size (on 64bit).)
>
> But looking at the code, uintptr_t is used all over. So perhaps use that
> to be consistent?
so, now I'm lost. I'm ok with both options, just let me know which one
will have no objections to be merged...
>
>> @@ -3960,16 +4017,16 @@ ptp_ocp_summary_show(struct seq_file *s, void
>> *data)
>> bp = dev_get_drvdata(dev);
>> seq_printf(s, "%7s: /dev/ptp%d\n", "PTP", ptp_clock_index(bp-
>> >ptp));
>> - if (bp->gnss_port.line != -1)
>> + if (bp->port[PORT_GNSS].line != -1)
>> seq_printf(s, "%7s: /dev/ttyS%d\n", "GNSS1",
>> - bp->gnss_port.line);
>> - if (bp->gnss2_port.line != -1)
>> + bp->port[PORT_GNSS].line);
>> + if (bp->port[PORT_GNSS2].line != -1)
>> seq_printf(s, "%7s: /dev/ttyS%d\n", "GNSS2",
>> - bp->gnss2_port.line);
>> - if (bp->mac_port.line != -1)
>> - seq_printf(s, "%7s: /dev/ttyS%d\n", "MAC", bp->mac_port.line);
>> - if (bp->nmea_port.line != -1)
>> - seq_printf(s, "%7s: /dev/ttyS%d\n", "NMEA", bp->nmea_port.line);
>> + bp->port[PORT_GNSS2].line);
>> + if (bp->port[PORT_MAC].line != -1)
>> + seq_printf(s, "%7s: /dev/ttyS%d\n", "MAC", bp-
>> >port[PORT_MAC].line);
>> + if (bp->port[PORT_NMEA].line != -1)
>> + seq_printf(s, "%7s: /dev/ttyS%d\n", "NMEA", bp-
>> >port[PORT_NMEA].line);
>
> Perhaps you can introduce some to_name() function (mapping enum -> const
> char *)? Can this code be then a three-line for loop?
yeah, I can do it, for sure.
>
>> @@ -4419,20 +4460,21 @@ ptp_ocp_info(struct ptp_ocp *bp)
>> ptp_ocp_phc_info(bp);
>> - ptp_ocp_serial_info(dev, "GNSS", bp->gnss_port.line,
>> - bp->gnss_port.baud);
>> - ptp_ocp_serial_info(dev, "GNSS2", bp->gnss2_port.line,
>> - bp->gnss2_port.baud);
>> - ptp_ocp_serial_info(dev, "MAC", bp->mac_port.line, bp-
>> >mac_port.baud);
>> - if (bp->nmea_out && bp->nmea_port.line != -1) {
>> - bp->nmea_port.baud = -1;
>> + ptp_ocp_serial_info(dev, "GNSS", bp->port[PORT_GNSS].line,
>> + bp->port[PORT_GNSS].baud);
>> + ptp_ocp_serial_info(dev, "GNSS2", bp->port[PORT_GNSS2].line,
>> + bp->port[PORT_GNSS2].baud);
>> + ptp_ocp_serial_info(dev, "MAC", bp->port[PORT_MAC].line,
>> + bp->port[PORT_MAC].baud);
>> + if (bp->nmea_out && bp->port[PORT_NMEA].line != -1) {
>> + bp->port[PORT_NMEA].baud = -1;
>> reg = ioread32(&bp->nmea_out->uart_baud);
>> if (reg < ARRAY_SIZE(nmea_baud))
>> - bp->nmea_port.baud = nmea_baud[reg];
>> + bp->port[PORT_NMEA].baud = nmea_baud[reg];
>> - ptp_ocp_serial_info(dev, "NMEA", bp->nmea_port.line,
>> - bp->nmea_port.baud);
>> + ptp_ocp_serial_info(dev, "NMEA", bp->port[PORT_NMEA].line,
>> + bp->port[PORT_NMEA].baud);
>
> Maybe even here with if (iterator == PORT_NMEA)?
>
Should be ok, will make it in the next revision.
Thanks,
Vadim
Powered by blists - more mailing lists