RE: [PATCH net-next v8 05/11] net: ethernet: oa_tc6: Support for hardware timestamp

[Selvamani Rajagopal]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Tuesday, October 6, 2026 5:49 AM
> Subject: Re: [PATCH net-next v8 05/11] net: ethernet: oa_tc6: Support for hardware timestamp

> 
> The indentation is a bit off. Generally the whole of BIT(...) would be
> on the next line.

Thanks. Fixing it.


> 
> The goto's here are a bit spaghetti code like. Since all out: does is
> return, maybe just use "return true" above.

Agree. Went little overboard in keeping one exit for the function.
Dropped the "drop" variable and used return true/false.

> 
> > }
> > + value = regs[0];
> 
> I assume there is a #define for register 0? It would be more readable
> to use the name.

Since it is used only in this function, I hesitated to add a name. But I realize the usage
isn't consistent. I will add a label for this offset and other offset for this array too.


> > + snprintf(info->name, sizeof(info->name), "%s",
> > + "OA TC6 PTP clock");
> 
> Are names meant to be unique? Have you tested this on a board with
> multiple devices?

Good question. I am changing to add "OA TC6 PTP <spi-device-name>". This way, the name will be
unique.

> OA_TC6_REG_CONFIG0 - OA_TC6_REG_CONFIG0?
> 
> > + status0 = regs[OA_TC6_REG_STATUS0 - OA_TC6_REG_CONFIG0];
> > + irqm = regs[OA_TC6_REG_INT_MASK0 - OA_TC6_REG_CONFIG0];
> 
> It then follow the pattern?

Will fix along with other instances. Thanks.

> 
> Andrew