Re: [Outreachy kernel] [PATCH v2] Staging: media: omap4iss: Use BIT() macro

[Stefano Brivio <sbrivio@redhat.com>]

Hi Sam,

On Sat, 04 Apr 2020 01:33:09 -0400
Sam Muhammed <jane.pnx9@gmail.com> wrote:

> On Wed, 2020-04-01 at 04:55 +0200, Stefano Brivio wrote:
> > On Tue, 31 Mar 2020 18:58:17 -0400
> > Sam Muhammed <jane.pnx9@gmail.com> wrote:
>
> [...]
>
> > > diff --git a/drivers/staging/media/omap4iss/iss_regs.h b/drivers/staging/media/omap4iss/iss_regs.h
> > > index 09a7375c89ac..85c6fefeb13a 100644
> > > --- a/drivers/staging/media/omap4iss/iss_regs.h
> > > +++ b/drivers/staging/media/omap4iss/iss_regs.h
> > > @@ -93,10 +93,10 @@
> > >  #define CSI2_SYSCONFIG					0x10
> > >  #define CSI2_SYSCONFIG_MSTANDBY_MODE_MASK		(3 << 12)
> > >  #define CSI2_SYSCONFIG_MSTANDBY_MODE_FORCE		(0 << 12)
> > > -#define CSI2_SYSCONFIG_MSTANDBY_MODE_NO			(1 << 12)
> > > +#define CSI2_SYSCONFIG_MSTANDBY_MODE_NO			BIT(12)
> > >  #define CSI2_SYSCONFIG_MSTANDBY_MODE_SMART		(2 << 12)  
> >   
> 
> Hi Stefano,
> honestly i'am confused about how is there a difference between BIT(x)
> and (1 << x)
> 
>  So i'am just going to interpret this hunk as i understand it and
> please correct me.
> 
> #define CSI2_SYSCONFIG_MSTANDBY_MODE_MASK		(3 << 12)
> 	shift binary 3 by 12: "write 3 starting at bit 12"
> 		0011 ---- ---- ----		
>
> #define CSI2_SYSCONFIG_MSTANDBY_MODE_FORCE		(0 << 12)
> 	"write 0 at bit 12"
> 		0000 ---- ---- ----
> 
> #define CSI2_SYSCONFIG_MSTANDBY_MODE_NO			(1 << 12)
> 	"shift binary 1 by 12": "write 1 at bit 12"
> 		0001 ---- ---- ----
> #define CSI2_SYSCONFIG_MSTANDBY_MODE_NO			BIT(12)
> 	"shift binary 1 by 12": _isn't this what BIT() does?_
> 		0001 ---- ---- ----

This is the key perhaps: it is what BIT() does. And it's not what you
should be... thinking of doing. Even though the result is clearly the
same.

BIT() is used to set a single bit. You need to write *some* bits, here.
That "some" is 2: you can write 0, 1, 2, 3, to this region. It's not 1.

Now, if you _write_ 1, you need to _set_ one bit, so coincidentally
BIT() works too.

Suppose you have a tray to make ice cubes. You use one, and put the
whole thing back in the freezer. Before you do that, it is your duty
towards the society to fill it completely with water.

After you use one ice cube, it might look like this:

  1    2    3    4
 .----.----.----.----.
 |xxxx|xxxx|xxxx|xxxx|
 |----|----|----|----|
 |xxxx|xxxx|    |xxxx|
 '----'----'----'----'
  5    6    7    8

so we have two ways to express what you have to do:

  a. fill hole number 7
  b. refill the whole thing

a. will do the job just like b., but b. is how you would most naturally
describe the operation. Especially because sometimes you take two ice
cubes, sometimes three.

> now i dont understand what you meant by "That doesn't make bit 12 any
> special", does that assume that iam wrong with what BIT() is defined
> for? 

Yes, in some sense. BIT() is used to operate on a single bit: the
difference is between:

  a. set bit 12
  b. write a number to bit region from 12 to 13

if the number from b. is 1, then a. is equivalent.

But right above and below this #define you have defines for 0 << 12,
2 << 12, 3 << 12. And 1 << 12 is not a special case.

> #define CSI2_SYSCONFIG_MSTANDBY_MODE_SMART		(2 << 12)
> 	"shift binary 2 by 12": write 2 at bit 12
> 		0010 ---- ---- ----
>  
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> for your previous explanation: _quoting_ :
> 
> 1. #define ISS_CTRL                                        0x80
>         "we have a register at 0x80"
>         https://en.wikipedia.org/wiki/Hardware_register
> 
> 2. how big is it? Datasheet tells you, but before you even look for it:
>    #define ISS_CTRL_CLK_DIV_MASK                          (3 << 4)
>         "at least 7 bits"
> 
>    #define ISS_CLKCTRL                                     0x84
>    #define ISS_CLKCTRL_VPORT2_CLK                          BIT(30)
>         "probably they are all the same size, might be 32 bits"
> 
> 3. picture it:
>         |_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ b3 _|_ b2 _|_ b1 _|_ b0 _|
>    (might be bigger, not too important)
> 
> 4. #define ISS_CTRL_INPUT_SEL_MASK                      (3 << 2)
>         "these bits define the input selection [whatever it is]":
> 
>         |_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ b3 _|_ b2 _|_ b1 _|_ b0 _|
>                                        ^^     ^^  | << shift by 2
>                                        2^1 + 2^0 = 3
>                                                    ^----(3 << 2)
> 
> 
> 5. #define ISS_CTRL_INPUT_SEL_CSI2A                     (0 << 2)
>         "to select input CSI2A [whatever it is]", we need:
>         |_ b7 _|_ b6 _|_ b5 _|_ b4 _|_  0 _|_  0 _|_ b1 _|_ b0 _|
>                                        ^^     ^^  | << shift by 2
>                                         0  +  0   = 0
>                                                     ^----(0 << 2)
> 
> 
>    #define ISS_CTRL_INPUT_SEL_CSI2B                     (1 << 2)
>         "to select input CSI2B [whatever it is]", we need:
>         |_ b7 _|_ b6 _|_ b5 _|_ b4 _|_  0 _|_  1 _|_ b1 _|_ b0 _|
>                                        ^^     ^^  | << shift by 2
>                                         0  + 2^0  = 1
>                                                     ^----(1 << 2)
> 
> 
> Now, that's how the current #defines work. I think they are
> actually intuitive (you could use the GENMASK() macro, but I think
> the difference is not that big).
> 
> With your change:
> 
> 5. #define ISS_CTRL_INPUT_SEL_CSI2A                     (0 << 2)
>         "to select input CSI2A [whatever it is]", we need:
>         |_ b7 _|_ b6 _|_ b5 _|_ b4 _|_  0 _|_  0 _|_ b1 _|_ b0 _|
>                                        ^^     ^^  | << shift by 2
>                                         0  +  0  =  0
>                                                     ^----(0 << 2)
> 
> 
>    #define ISS_CTRL_INPUT_SEL_CSI2B                     BIT(2)
>         "to select input CSI2B [whatever it is]", we need:
>         |_ b7 _|_ b6 _|_ b5 _|_ b4 _|_ b3 _|_  1 _|_ b1 _|_ b0 _|
>                                                ^
>                                                ' this bit set.
>                 What, why? How is this related with the rest?
> ++++
> now right at this point i got an overlapped definition of BIT()
> this expansion/explanation of BIT(2) in my mind got translated to:
> 
>      SetBit(register,bit) => (register|=(1<<bit))
> NOT
>      shift binary 1 by 2
> ++++

Yes, it's also about this. But from your example above:
      SetBit(register,bit) => (register|=(1<<bit))

you need to distinguish when you're operating on a bit *array* or a
generic group of bit regions. That is, it's:
      SetBit(register,bit) => (register|=(n<<bit))

where 'n' happens to be 1, sometimes. It's not always '1' by design.

Let's take a driver operating a lamp that has three LEDs inside: it
might work like this:

#define CTRL_REGISTER	0x5	/* a generic 8-bit register */
#define RED_ON		BIT(0)
#define GREEN_ON	BIT(1)
#define BLUE_ON		BIT(2)

those are single bits, and consistently so, so using BIT() is fine.

Now, the lamp can be on, off, blink slow or fast. In the same register,
you have 2 bits (starting from bit 4) controlling that: 0 means "off", 1
means "blink slow", 2 means "blink fast", 3 means "on":

#define MODE_OFF	(0 << 4)
#define MODE_SLOW	(1 << 4)
#define MODE_FAST	(2 << 4)
#define MODE_ON		(3 << 4)

this is a natural way of describing things. Suppose you do, instead:

#define MODE_OFF	(0 << 4)
#define MODE_SLOW	BIT(4)
#define MODE_FAST	(2 << 4)
#define MODE_ON		(3 << 4)

the compiler won't judge you, but I will. Why is the "slow" mode
special? This makes the "slow" mode look like it's a binary option, but
it's not.

> Basically i forgot about this patch and i'am more into clearing this
> confusion because i'am definitely missing something thats very basic
> and probably have a wrong understanding of something.

Thanks for sticking with this, I appreciate that you're trying to
figure out what I'm saying. Is it a bit clearer now?

-- 
Stefano