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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Kieran,

On Wed, Sep 23, 2020 at 11:41 AM Kieran Bingham
<kieran.bingham+renesas@xxxxxxxxxxxxxxxx> wrote:
> On 23/09/2020 08:42, Geert Uytterhoeven wrote:
> > On Tue, Sep 22, 2020 at 5:52 PM Kieran Bingham
> > <kieran.bingham+renesas@xxxxxxxxxxxxxxxx> wrote:
> >> Convert the bitfield definitions to use unsigned integers.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> >
> > 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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux