diff mbox series

[1/2] media: i2c: max9286: Use unsigned constants

Message ID 20200922155210.242536-2-kieran.bingham+renesas@ideasonboard.com (mailing list archive)
State Changes Requested
Delegated to: Kieran Bingham
Headers show
Series gmsl: Use unsigned constants | expand

Commit Message

Kieran Bingham Sept. 22, 2020, 3:52 p.m. UTC 
Convert the bitfield definitions to use unsigned integers.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 96 ++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

Comments

Geert Uytterhoeven Sept. 23, 2020, 7:42 a.m. UTC | #1 
Hi Kieran,

On Tue, Sep 22, 2020 at 5:52 PM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
> Convert the bitfield definitions to use unsigned integers.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Thanks for your patch!

> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -31,85 +31,85 @@
>  #include <media/v4l2-subdev.h>
>
>  /* Register 0x00 */
> -#define MAX9286_MSTLINKSEL_AUTO                (7 << 5)
> +#define MAX9286_MSTLINKSEL_AUTO                (7U << 5)

While using this format for multi-bit fields makes sense...

>  #define MAX9286_MSTLINKSEL(n)          ((n) << 5)
>  #define MAX9286_EN_VS_GEN              BIT(4)
> -#define MAX9286_LINKEN(n)              (1 << (n))
> +#define MAX9286_LINKEN(n)              (1U << (n))

... I think single-bit fields (more below) make better use of the BIT() macro.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Kieran Bingham Sept. 23, 2020, 9:41 a.m. UTC | #2 
Hi Geert,

On 23/09/2020 08:42, Geert Uytterhoeven wrote:
> Hi Kieran,
> 
> On Tue, Sep 22, 2020 at 5:52 PM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
>> Convert the bitfield definitions to use unsigned integers.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -31,85 +31,85 @@
>>  #include <media/v4l2-subdev.h>
>>
>>  /* Register 0x00 */
>> -#define MAX9286_MSTLINKSEL_AUTO                (7 << 5)
>> +#define MAX9286_MSTLINKSEL_AUTO                (7U << 5)
> 
> While using this format for multi-bit fields makes sense...
> 
>>  #define MAX9286_MSTLINKSEL(n)          ((n) << 5)
>>  #define MAX9286_EN_VS_GEN              BIT(4)
>> -#define MAX9286_LINKEN(n)              (1 << (n))
>> +#define MAX9286_LINKEN(n)              (1U << (n))
> 
> ... I think single-bit fields (more below) make better use of the BIT() macro.

Ooops, I missed that, indeed that certainly looks like a BIT.

I was really trying to make sure all the 'bit-field enum' type values
are consistent here, i.e.:

#define MAX9286_I2CSLVSH_1046NS_469NS	(3U << 5)
#define MAX9286_I2CSLVSH_938NS_352NS	(2U << 5)
#define MAX9286_I2CSLVSH_469NS_234NS	(1U << 5)
#define MAX9286_I2CSLVSH_352NS_117NS	(0U << 5)

I'll sift out the single bit fields that are more appropriate for BIT().

There is also the FIELD_PREP, FIELD_GET macros that could be used
instead from include/linux/bitfield.h which are new to me, and seem
interesting but I haven't worked out if it's worth converting the whole
driver to use that yet or not.

--
Kieran


> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>
Geert Uytterhoeven Sept. 23, 2020, 11:51 a.m. UTC | #3 
Hi Kieran,

On Wed, Sep 23, 2020 at 11:41 AM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
> On 23/09/2020 08:42, Geert Uytterhoeven wrote:
> > On Tue, Sep 22, 2020 at 5:52 PM Kieran Bingham
> > <kieran.bingham+renesas@ideasonboard.com> wrote:
> >> Convert the bitfield definitions to use unsigned integers.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >
> > Thanks for your patch!
> >
> >> --- a/drivers/media/i2c/max9286.c
> >> +++ b/drivers/media/i2c/max9286.c
> >> @@ -31,85 +31,85 @@
> >>  #include <media/v4l2-subdev.h>
> >>
> >>  /* Register 0x00 */
> >> -#define MAX9286_MSTLINKSEL_AUTO                (7 << 5)
> >> +#define MAX9286_MSTLINKSEL_AUTO                (7U << 5)
> >
> > While using this format for multi-bit fields makes sense...
> >
> >>  #define MAX9286_MSTLINKSEL(n)          ((n) << 5)
> >>  #define MAX9286_EN_VS_GEN              BIT(4)
> >> -#define MAX9286_LINKEN(n)              (1 << (n))
> >> +#define MAX9286_LINKEN(n)              (1U << (n))
> >
> > ... I think single-bit fields (more below) make better use of the BIT() macro.
>
> Ooops, I missed that, indeed that certainly looks like a BIT.
>
> I was really trying to make sure all the 'bit-field enum' type values
> are consistent here, i.e.:
>
> #define MAX9286_I2CSLVSH_1046NS_469NS   (3U << 5)
> #define MAX9286_I2CSLVSH_938NS_352NS    (2U << 5)
> #define MAX9286_I2CSLVSH_469NS_234NS    (1U << 5)
> #define MAX9286_I2CSLVSH_352NS_117NS    (0U << 5)
>
> I'll sift out the single bit fields that are more appropriate for BIT().
>
> There is also the FIELD_PREP, FIELD_GET macros that could be used
> instead from include/linux/bitfield.h which are new to me, and seem
> interesting but I haven't worked out if it's worth converting the whole
> driver to use that yet or not.

Personally, I'm not such a big fan of them.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 7c8212071df9..079d5d7cfc2d 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -31,85 +31,85 @@ 
 #include <media/v4l2-subdev.h>
 
 /* Register 0x00 */
-#define MAX9286_MSTLINKSEL_AUTO		(7 << 5)
+#define MAX9286_MSTLINKSEL_AUTO		(7U << 5)
 #define MAX9286_MSTLINKSEL(n)		((n) << 5)
 #define MAX9286_EN_VS_GEN		BIT(4)
-#define MAX9286_LINKEN(n)		(1 << (n))
+#define MAX9286_LINKEN(n)		(1U << (n))
 /* Register 0x01 */
-#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)
+#define MAX9286_FSYNCMODE_ECU		(3U << 6)
+#define MAX9286_FSYNCMODE_EXT		(2U << 6)
+#define MAX9286_FSYNCMODE_INT_OUT	(1U << 6)
+#define MAX9286_FSYNCMODE_INT_HIZ	(0U << 6)
 #define MAX9286_GPIEN			BIT(5)
 #define MAX9286_ENLMO_RSTFSYNC		BIT(2)
-#define MAX9286_FSYNCMETH_AUTO		(2 << 0)
-#define MAX9286_FSYNCMETH_SEMI_AUTO	(1 << 0)
-#define MAX9286_FSYNCMETH_MANUAL	(0 << 0)
+#define MAX9286_FSYNCMETH_AUTO		(2U << 0)
+#define MAX9286_FSYNCMETH_SEMI_AUTO	(1U << 0)
+#define MAX9286_FSYNCMETH_MANUAL	(0U << 0)
 #define MAX9286_REG_FSYNC_PERIOD_L	0x06
 #define MAX9286_REG_FSYNC_PERIOD_M	0x07
 #define MAX9286_REG_FSYNC_PERIOD_H	0x08
 /* Register 0x0a */
-#define MAX9286_FWDCCEN(n)		(1 << ((n) + 4))
-#define MAX9286_REVCCEN(n)		(1 << (n))
+#define MAX9286_FWDCCEN(n)		(1U << ((n) + 4))
+#define MAX9286_REVCCEN(n)		(1U << (n))
 /* Register 0x0c */
 #define MAX9286_HVEN			BIT(7)
-#define MAX9286_EDC_6BIT_HAMMING	(2 << 5)
-#define MAX9286_EDC_6BIT_CRC		(1 << 5)
-#define MAX9286_EDC_1BIT_PARITY		(0 << 5)
+#define MAX9286_EDC_6BIT_HAMMING	(2U << 5)
+#define MAX9286_EDC_6BIT_CRC		(1U << 5)
+#define MAX9286_EDC_1BIT_PARITY		(0U << 5)
 #define MAX9286_DESEL			BIT(4)
 #define MAX9286_INVVS			BIT(3)
 #define MAX9286_INVHS			BIT(2)
-#define MAX9286_HVSRC_D0		(2 << 0)
-#define MAX9286_HVSRC_D14		(1 << 0)
-#define MAX9286_HVSRC_D18		(0 << 0)
+#define MAX9286_HVSRC_D0		(2U << 0)
+#define MAX9286_HVSRC_D14		(1U << 0)
+#define MAX9286_HVSRC_D18		(0U << 0)
 /* Register 0x0f */
 #define MAX9286_0X0F_RESERVED		BIT(3)
 /* Register 0x12 */
 #define MAX9286_CSILANECNT(n)		(((n) - 1) << 6)
 #define MAX9286_CSIDBL			BIT(5)
 #define MAX9286_DBL			BIT(4)
-#define MAX9286_DATATYPE_USER_8BIT	(11 << 0)
-#define MAX9286_DATATYPE_USER_YUV_12BIT	(10 << 0)
-#define MAX9286_DATATYPE_USER_24BIT	(9 << 0)
-#define MAX9286_DATATYPE_RAW14		(8 << 0)
-#define MAX9286_DATATYPE_RAW11		(7 << 0)
-#define MAX9286_DATATYPE_RAW10		(6 << 0)
-#define MAX9286_DATATYPE_RAW8		(5 << 0)
-#define MAX9286_DATATYPE_YUV422_10BIT	(4 << 0)
-#define MAX9286_DATATYPE_YUV422_8BIT	(3 << 0)
-#define MAX9286_DATATYPE_RGB555		(2 << 0)
-#define MAX9286_DATATYPE_RGB565		(1 << 0)
-#define MAX9286_DATATYPE_RGB888		(0 << 0)
+#define MAX9286_DATATYPE_USER_8BIT	(11U << 0)
+#define MAX9286_DATATYPE_USER_YUV_12BIT	(10U << 0)
+#define MAX9286_DATATYPE_USER_24BIT	(9U << 0)
+#define MAX9286_DATATYPE_RAW14		(8U << 0)
+#define MAX9286_DATATYPE_RAW11		(7U << 0)
+#define MAX9286_DATATYPE_RAW10		(6U << 0)
+#define MAX9286_DATATYPE_RAW8		(5U << 0)
+#define MAX9286_DATATYPE_YUV422_10BIT	(4U << 0)
+#define MAX9286_DATATYPE_YUV422_8BIT	(3U << 0)
+#define MAX9286_DATATYPE_RGB555		(2U << 0)
+#define MAX9286_DATATYPE_RGB565		(1U << 0)
+#define MAX9286_DATATYPE_RGB888		(0U << 0)
 /* Register 0x15 */
 #define MAX9286_VC(n)			((n) << 5)
 #define MAX9286_VCTYPE			BIT(4)
 #define MAX9286_CSIOUTEN		BIT(3)
-#define MAX9286_0X15_RESV		(3 << 0)
+#define MAX9286_0X15_RESV		(3U << 0)
 /* Register 0x1b */
-#define MAX9286_SWITCHIN(n)		(1 << ((n) + 4))
-#define MAX9286_ENEQ(n)			(1 << (n))
+#define MAX9286_SWITCHIN(n)		(1U << ((n) + 4))
+#define MAX9286_ENEQ(n)			(1U << (n))
 /* Register 0x27 */
 #define MAX9286_LOCKED			BIT(7)
 /* Register 0x31 */
 #define MAX9286_FSYNC_LOCKED		BIT(6)
 /* Register 0x34 */
 #define MAX9286_I2CLOCACK		BIT(7)
-#define MAX9286_I2CSLVSH_1046NS_469NS	(3 << 5)
-#define MAX9286_I2CSLVSH_938NS_352NS	(2 << 5)
-#define MAX9286_I2CSLVSH_469NS_234NS	(1 << 5)
-#define MAX9286_I2CSLVSH_352NS_117NS	(0 << 5)
-#define MAX9286_I2CMSTBT_837KBPS	(7 << 2)
-#define MAX9286_I2CMSTBT_533KBPS	(6 << 2)
-#define MAX9286_I2CMSTBT_339KBPS	(5 << 2)
-#define MAX9286_I2CMSTBT_173KBPS	(4 << 2)
-#define MAX9286_I2CMSTBT_105KBPS	(3 << 2)
-#define MAX9286_I2CMSTBT_84KBPS		(2 << 2)
-#define MAX9286_I2CMSTBT_28KBPS		(1 << 2)
-#define MAX9286_I2CMSTBT_8KBPS		(0 << 2)
-#define MAX9286_I2CSLVTO_NONE		(3 << 0)
-#define MAX9286_I2CSLVTO_1024US		(2 << 0)
-#define MAX9286_I2CSLVTO_256US		(1 << 0)
-#define MAX9286_I2CSLVTO_64US		(0 << 0)
+#define MAX9286_I2CSLVSH_1046NS_469NS	(3U << 5)
+#define MAX9286_I2CSLVSH_938NS_352NS	(2U << 5)
+#define MAX9286_I2CSLVSH_469NS_234NS	(1U << 5)
+#define MAX9286_I2CSLVSH_352NS_117NS	(0U << 5)
+#define MAX9286_I2CMSTBT_837KBPS	(7U << 2)
+#define MAX9286_I2CMSTBT_533KBPS	(6U << 2)
+#define MAX9286_I2CMSTBT_339KBPS	(5U << 2)
+#define MAX9286_I2CMSTBT_173KBPS	(4U << 2)
+#define MAX9286_I2CMSTBT_105KBPS	(3U << 2)
+#define MAX9286_I2CMSTBT_84KBPS		(2U << 2)
+#define MAX9286_I2CMSTBT_28KBPS		(1U << 2)
+#define MAX9286_I2CMSTBT_8KBPS		(0U << 2)
+#define MAX9286_I2CSLVTO_NONE		(3U << 0)
+#define MAX9286_I2CSLVTO_1024US		(2U << 0)
+#define MAX9286_I2CSLVTO_256US		(1U << 0)
+#define MAX9286_I2CSLVTO_64US		(0U << 0)
 /* Register 0x3b */
 #define MAX9286_REV_TRF(n)		((n) << 4)
 #define MAX9286_REV_AMP(n)		((((n) - 30) / 10) << 1) /* in mV */