| Message ID | 20200612144713.502006-1-kieran.bingham+renesas@ideasonboard.com |
|---|---|
| Headers | show |
| Series | MAX9286 GMSL Support (+RDACM20) | expand |
Hi Kieran, On Thu, Jul 16, 2020 at 10:02:24AM +0100, Kieran Bingham wrote: > Hi Sakari, > > This is the output of checkpatch --strict on this driver. Sorry for not > detailing this in the commit or cover letter. No worries. > > > ./patches/gmsl/v10/v10-0001-dt-bindings-media-i2c-Add-bindings-for-Maxim-Int.patch has style problems, please review. > > -------------------------------------------------------------- > > ./patches/gmsl/v10/v10-0002-media-i2c-Add-MAX9286-driver.patch > > -------------------------------------------------------------- > > CHECK: Prefer using the BIT macro > > #246: FILE: drivers/media/i2c/max9286.c:40: > > +#define MAX9286_FSYNCMODE_INT_OUT (1 << 6) > > > > CHECK: Prefer using the BIT macro > > #251: FILE: drivers/media/i2c/max9286.c:45: > > +#define MAX9286_FSYNCMETH_SEMI_AUTO (1 << 0) > > > > CHECK: Prefer using the BIT macro > > #262: FILE: drivers/media/i2c/max9286.c:56: > > +#define MAX9286_EDC_6BIT_CRC (1 << 5) > > > > CHECK: Prefer using the BIT macro > > #268: FILE: drivers/media/i2c/max9286.c:62: > > +#define MAX9286_HVSRC_D14 (1 << 0) > > > > CHECK: Prefer using the BIT macro > > #286: FILE: drivers/media/i2c/max9286.c:80: > > +#define MAX9286_DATATYPE_RGB565 (1 << 0) > > > > CHECK: Prefer using the BIT macro > > #304: FILE: drivers/media/i2c/max9286.c:98: > > +#define MAX9286_I2CSLVSH_469NS_234NS (1 << 5) > > > > CHECK: Prefer using the BIT macro > > #312: FILE: drivers/media/i2c/max9286.c:106: > > +#define MAX9286_I2CMSTBT_28KBPS (1 << 2) > > > > CHECK: Prefer using the BIT macro > > #316: FILE: drivers/media/i2c/max9286.c:110: > > +#define MAX9286_I2CSLVTO_256US (1 << 0) > > None of those are appropriate to use the BIT() macro, as they are all > entries of a specific field with a shift, such as: > > #define MAX9286_FSYNCMODE_ECU (3 << 6) > #define MAX9286_FSYNCMODE_EXT (2 << 6) > #define MAX9286_FSYNCMODE_INT_OUT (1 << 6) > #define MAX9286_FSYNCMODE_INT_HIZ (0 << 6) > > Checkpatch is only picking up on the "1 << x" variant of each entry. Ideally you should use "1U << x" everywhere. If you happen to have a register with 31st bit signifying something, mayhem would follow. So the practice is to make all such definitions unsigned. > > > > CHECK: Macro argument reuse 'source' - possible side-effects? > > #399: FILE: drivers/media/i2c/max9286.c:193: > > +#define for_each_source(priv, source) \ > > + for ((source) = NULL; ((source) = next_source((priv), (source))); ) > > This warns against possible side effects, but the 're-use' effects are > desired ;-) > > If you'd prefer this macro to be re-written please let me know. Works for me. Some warnigns are just not useful. I bet quite a few macros elsewhere in the kernel would trigger this. > > > > CHECK: Lines should not end with a '(' > > #1372: FILE: drivers/media/i2c/max9286.c:1166: > > + ret = v4l2_fwnode_endpoint_parse( > > Full code block: > > > ret = v4l2_fwnode_endpoint_parse( > > of_fwnode_handle(node), &vep); > > if (ret) { > > of_node_put(node); > > return ret; > > } > > That one is awkward, and I chose to keep it as a lesser evil. > Of course now that we can officially go up to 120 chars, I could move > this line up. > > If you'd like this to be moved to a single line now we can go over 80 > chars, please confirm. I don't mind that. Mauro, any thoughts on this? -- Kind regards, Sakari Ailus