Hi Andrew, On 27/10/23 1:16 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> Still if you feel like using "write" instead of "wnr" and "protect" >> instead of "prote", I will change them in the next revision. > > There is some value in using names from the standard, if they are > actually good names. But i guess most developers don't have a copy of > the standard by there side. > > You actually wrote in the patch: > > +/* Control header */ > +#define CTRL_HDR_DNC BIT(31) /* Data-Not-Control */ > +#define CTRL_HDR_HDRB BIT(30) /* Received Header Bad */ > +#define CTRL_HDR_WNR BIT(29) /* Write-Not-Read */ > +#define CTRL_HDR_AID BIT(28) /* Address Increment Disable */ > +#define CTRL_HDR_MMS GENMASK(27, 24) /* Memory Map Selector */ > > The comments suggest you also don't think the names are particularly > good, otherwise you would not of added comments. > > But if you instead had: > > /* Control header */ > #define CTRL_HDR_DATA_NOT_CTRL BIT(31) > #define CTRL_HDR_HDR_RX_BAD BIT(30) > #define CTRL_HDR_WRITE BIT(29) > #define CTRL_HDR_ADDR_INC_DISABLE BIT(28) > #define CTRL_HDR_MEM_MAP_SELECTOR GENMASK(27, 24) > > the names are probably sufficient that comments are not needed. And > is should be easy for somebody to map these back to the names used in > the standard. > > This also to some extent comes into the comment about coding style, a > function does one thing, is short, etc. Short functions tend to have > less indentation, meaning you can use longer names. And longer names > are more readable, making the function easier to understand, so it > does that one thing well. Ok, thanks for the explanation. I will do it in the next revision. Best Regards, Parthiban V > > Andrew >
