Re: [RFC PATCH net-next 1/6] net: ethernet: implement OPEN Alliance control transaction interface

[<Parthiban.Veerasooran@microchip.com>]

Hi Andrew,

On 19/09/23 8:43 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>>>> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
>>>> +                                 u8 len, bool wnr, u8 *buf, bool ctrl_prot)
>>>> +{
>>>> +     u32 hdr;
>>>> +
>>>> +     /* Prepare the control header with the required details */
>>>> +     hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
>>>> +           FIELD_PREP(CTRL_HDR_WNR, wnr) |
>>>> +           FIELD_PREP(CTRL_HDR_AID, 0) |
>>>> +           FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
>>>> +           FIELD_PREP(CTRL_HDR_ADDR, addr) |
>>>> +           FIELD_PREP(CTRL_HDR_LEN, len - 1);
>>>> +     hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
>>>> +     *(u32 *)buf = cpu_to_be32(hdr);
>>>> +
>>>> +     if (wnr) {
>>>
>>> What does wnr mean? Maybe give it a more meaningful name, unless it is
>>> actually something in the standard. Kerneldoc would also help.
>> Ah, it is "write not read". Shall I name it as "write_not_read" ?
> 
> You might want to describe the high level concept as well in this
> file. What i _think_ this is about is that SPI is sort of a full
> duplex bus. While you are sending data to the SPI device, the device
> could also be sending a data to the CPU? And 'write not read' here
> means ignore what we receive from the device?
Ah ok, I think there is a misunderstanding here. This is related to OPEN 
Alliance protocol. Control transactions consist of one or more control 
commands. Control commands are used by the SPI host to read and write 
registers within the MAC-PHY. Each control commands are composed of a 
32-bit control command header followed by register data. WNR (write not 
read) bit in the control command header indicates if data is to be 
written to registers (when set) or read from registers (when clear). so 
basically it indicates the type of the control command on the registers.
> 
>> Ok, as per OA spec, up to 128 consecutive registers read or write can be
>> possible. So the maximum possible size would be 1032. As you suggested
>> will allocate this size of memory in the startup.
> 
> Yes, 1032 bytes it not huge, so allocate it once and keep it for the
> lifetime of the device.
Sure, will do that.
> 
>>>> +void oa_tc6_deinit(struct oa_tc6 *tc6)
>>>> +{
>>>> +     kfree(tc6);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(oa_tc6_deinit);
>>>
>>> Maybe consider a devm_ API to make the MAC driver simpler.
>> Sorry I don't get your point. Could you please explain bit more?
> 
> At least at this stage in the patch series, all you are doing is
> allocating memory. You add more code later, which might invalidate my
> point. But if all you are doing is allocating memory, you could use
> devm_kmalloc(). The driver core will then take care of releasing the
> memory when the driver is unloaded, or probe fails. That makes cleanup
> simpler and memory leaks less likely. There are a lot of devm_
> helpers, see if you can use them.
Sure, noted.

Best Regards,
Parthiban V
> 
>          Andrew
>