From mboxrd@z Thu Jan 1 00:00:00 1970 X-GM-THRID: 6810510356795883520 X-Received: by 2002:ac2:50ce:: with SMTP id h14mr8864121lfm.76.1586028753844; Sat, 04 Apr 2020 12:32:33 -0700 (PDT) X-BeenThere: outreachy-kernel@googlegroups.com Received: by 2002:a2e:8e69:: with SMTP id t9ls1719216ljk.4.gmail; Sat, 04 Apr 2020 12:32:32 -0700 (PDT) X-Google-Smtp-Source: APiQypLFUc2lbto+6hv6yiAjrRTe0Rh7NvNLS/SQWrj7tH9pXSNdqwJhgSLad1ONJR6Xdvys9s6n X-Received: by 2002:a2e:a362:: with SMTP id i2mr8081999ljn.52.1586028752098; Sat, 04 Apr 2020 12:32:32 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1586028752; cv=none; d=google.com; s=arc-20160816; b=WR/ANzITc9BspQO4UutzOsize5fJYhawx0UmKQXZgSlboj47RCkXCI+ILK+lGXaxgE 4IGj0Fk8HeOZQrjhKwC3nAbHMxxvZH0vTzUSGMaeWUySE8phk1kxmKO3aMRmjqqcJISA z2+lq2ZRumuH71G86an/UVAmX73PKfQPZAtm/QEjDGhrC2XM/wCg3OvBVakZobA6uc/Z SDARZW06KqjpQsv/0Y3Rcbm6g3BOH8ia73W85ov+VbqsU/hCBwJu4jfvh8stZpUzKrzd SjDr5Ebz5PSSYKqWxqRE0Epibx6853AMH9u271AqqMgnGCNV4sz5hnGJNvpNjxpApXKk 3JQA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:user-agent:references :in-reply-to:date:cc:to:from:subject:message-id:dkim-signature; bh=kBhwuHxM58agM6qFRT0L6iYATRzK4JLE7myNN/8YPdQ=; b=siTQRXFIWprV48DUcozf32fZOipmp3rEq2x6OdcC/0qi/OeBs2lO1OukqfOcxw2FdH iZri0WvIuColwetW3Kbh20ZuihmV1pwtCbHYBsZVlEmKyujGAVTBzn+Bc4PUygfkyj+5 mxaawS10xOQy162P8L61crB5ExsiaiUuwHsqXIemnCH1s3TBL7GFZh8Vaz5cxnd9UxN1 djbgtNIQgGA0MFiFx+n/0BDrBraFSbTuEzyoJNvWhcVNXX2YQCwoilY7xX0n141ROKyH /WyLnUleCH0ClAkHNdziRpBA30sSoto5tpYdsLgQDDlRLjS5KMbjQTdIJ0jb/iM1gmMm elBg== ARC-Authentication-Results: i=1; gmr-mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=RgnOyv7w; spf=pass (google.com: domain of jane.pnx9@gmail.com designates 2a00:1450:4864:20::441 as permitted sender) smtp.mailfrom=jane.pnx9@gmail.com; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com. [2a00:1450:4864:20::441]) by gmr-mx.google.com with ESMTPS id s22si686852ljp.0.2020.04.04.12.32.32 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 04 Apr 2020 12:32:32 -0700 (PDT) Received-SPF: pass (google.com: domain of jane.pnx9@gmail.com designates 2a00:1450:4864:20::441 as permitted sender) client-ip=2a00:1450:4864:20::441; Authentication-Results: gmr-mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=RgnOyv7w; spf=pass (google.com: domain of jane.pnx9@gmail.com designates 2a00:1450:4864:20::441 as permitted sender) smtp.mailfrom=jane.pnx9@gmail.com; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: by mail-wr1-x441.google.com with SMTP id v5so1694554wrp.12 for ; Sat, 04 Apr 2020 12:32:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=message-id:subject:from:to:cc:date:in-reply-to:references :user-agent:mime-version:content-transfer-encoding; bh=kBhwuHxM58agM6qFRT0L6iYATRzK4JLE7myNN/8YPdQ=; b=RgnOyv7wmVGF6P5s/nPd8qzXQetskAPlhJxwazshFXGKujWPtT+aNkjlq9oYlzzwUV wI+/vImPWwCNGpym55/7IeOMvECk444qMw3LLj6Soc4ICPc7NqXu65xnLXLVy22ICuYt JRKukg0s0RND4c5NPMOTjGXNCtr4CrTFqELro183PXXowiLdgKAy8AK71Nn9gWt6a/2P l2+594GjRtaeRyBrHqOI+EmG/rK5Lz4MSyjL3sRnEJrVH3Hz46D3Ny9dXZ9/5eLF6QpS fW/EzHv31aw62RISii7ihZ6Qhi6n3ufV//RQEw8v6vRO7biOyajpMKGUDlsoMU11msaR gU9w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=kBhwuHxM58agM6qFRT0L6iYATRzK4JLE7myNN/8YPdQ=; b=ud3VMbbKJdc5rkAHOq/nrjr9d64FquKdSHj4NMEA78bfedov5tx+s6stjOaBexuOJ9 1fel7T76+UppIydqm8QRxBZ/mm0RquqldjH7Nh+/IbcQt5x//iFicYqdlMIez0izUFyg eS/hrxhrrCFu9pZBbupslE0qQ2CGCK1t/+JDI+Xaht3DTHPlrCsIVWP6A+pwOU7cvfz0 FQl+9kfQcjM+94oyeeShbtTz/v7nmdOTZEDeFeKcTc22qStP+9iKWQZ9lZMoIzii9leD YoECKz8SKl7N8kGsTNBnzMytpVn9fqPIoFunuRvp44VodqGswsM4HWjyCwFmFGaAZ/SL 2kag== X-Gm-Message-State: AGi0PublEgXXjPdKw8gB3h4GMksX019jAHwWPAEc88cngna8Fg3M1Vss RzLQGxA4yCCWGMfhDNQLfykei5lOHnU= X-Received: by 2002:adf:b1c1:: with SMTP id r1mr14588098wra.337.1586028751422; Sat, 04 Apr 2020 12:32:31 -0700 (PDT) Return-Path: Received: from debian ([197.34.214.63]) by smtp.gmail.com with ESMTPSA id d28sm6202735wrb.31.2020.04.04.12.32.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 04 Apr 2020 12:32:30 -0700 (PDT) Message-ID: Subject: Re: [Outreachy kernel] [PATCH v2] Staging: media: omap4iss: Use BIT() macro From: Sam Muhammed To: Stefano Brivio Cc: Laurent Pinchart , Mauro Carvalho Chehab , Greg Kroah-Hartman , outreachy-kernel@googlegroups.com Date: Sat, 04 Apr 2020 15:32:23 -0400 In-Reply-To: <20200404141125.3b04dd96@elisabeth> References: <20200331225817.14834-1-jane.pnx9@gmail.com> <20200401045531.5de93729@elisabeth> <20200404141125.3b04dd96@elisabeth> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.30.5-1.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit On Sat, 2020-04-04 at 14:11 +0200, Stefano Brivio wrote: > Hi Sam, > > On Sat, 04 Apr 2020 01:33:09 -0400 > Sam Muhammed wrote: > > > On Wed, 2020-04-01 at 04:55 +0200, Stefano Brivio wrote: > > > On Tue, 31 Mar 2020 18:58:17 -0400 > > > Sam Muhammed 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< > NOT > > shift binary 1 by 2 > > ++++ > > Yes, it's also about this. But from your example above: > SetBit(register,bit) => (register|=(1< > 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< > 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) > I guess that is a great example: So if the operations are on a single bit being toggled on, BIT() is welcome to be used. But when the operations involve a region of bits being toggled on/off i should stick to the typical way of bit manipulation even if there happens to be an operation that requires only one bit to be set. ++++ My thoughts were: _quoting your example_ #define RED_ON BIT(0) #define GREEN_ON BIT(1) => 0001 #define BLUE_ON BIT(2) => 0010 This does set bit 2, but affected the rest "so it was a _lame_ way of saying that there is a region of bits being manipulated though only one of them is set" :D ++++ so when it came to: #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) *my brain* --> yes, makes sense #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? "my brain" --> why b3 is not 0 now? shouldn't BIT(2) == 1*2^2 == 4 == 0100 ?? So i saw it like: lets mask b2 to be set regardless of the rest so if b3 was set, it will still be set next to setting b2. so the meaning of BIT(2) got twisted up. That why i got lost between these two: SetBit(register,bit) => (register|=(1< 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? > Thank You, I guess i got it right this time? have i?? Sam