Re: [PATCH net v3 1/2] ptp: ocp: adjust sysfs entries to expose tty information

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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