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

[Parthiban.Veerasooran]

Hi Andrew,

Thanks for reviewing my patches.

On 24/10/23 2:58 am, 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 prote)
> 
> One of the comments i made last time was that wnr is not obvious. I
> assume it means write-not-read. So why not just write? Since it a
> boolean, i assume war is never needed, so !wnr cal always be
> considered rnw.
> 
> And prote could well be protect, the two extra characters make it a
> lot more obvious. Or better still.
Just to be aligned with the naming in the OPEN Alliance specification 
document, used the same name as it is. wnr and prote are taken from the 
specification document. Please refer the screenshots attached here from 
the specification document.

Still if you feel like using "write" instead of "wnr" and "protect" 
instead of "prote", I will change them in the next revision.
> 
>> +{
>> +     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));
>> +     *(__be32 *)buf = cpu_to_be32(hdr);
>> +
>> +     if (wnr) {
>> +             for (u8 i = 0; i < len; i++) {
>> +                     u16 pos;
>> +
>> +                     if (!prote) {
> 
> nitpick, but please use positive logic. Do the protected case first.
Sure, will do it in the next revision.

Best Regards,
Parthiban V
> 
>           Andrew
>

Attachment: wnr.png
Description: wnr.png

Attachment: prote.png
Description: prote.png