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.

quoted

./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.

quoted

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.

quoted

CHECK: Lines should not end with a '('
#1372: FILE: drivers/media/i2c/max9286.c:1166:
+			ret = v4l2_fwnode_endpoint_parse(

Full code block:

quoted

                        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