* [U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK" @ 2015-12-10 21:27 Troy Kisky 2015-12-11 14:59 ` Jagan Teki 0 siblings, 1 reply; 7+ messages in thread From: Troy Kisky @ 2015-12-10 21:27 UTC (permalink / raw) To: u-boot If GENMASK is REALLY desired, it should be GENMASK(23,0) But since GENMASK is obviously more confusing, let's just revert. This reverts commit bad490a24212c068c5b718b9189f47ea4075d078. Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> --- v2: add sign off/ reviewed by --- drivers/spi/fsl_qspi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index ed39114..dd7048a 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -24,7 +24,7 @@ DECLARE_GLOBAL_DATA_PTR; #define TX_BUFFER_SIZE 0x40 #endif -#define OFFSET_BITS_MASK GENMASK(24, 0) +#define OFFSET_BITS_MASK 0x00ffffff #define FLASH_STATUS_WEL 0x02 -- 2.5.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK" 2015-12-10 21:27 [U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK" Troy Kisky @ 2015-12-11 14:59 ` Jagan Teki 2015-12-11 15:24 ` Marek Vasut 0 siblings, 1 reply; 7+ messages in thread From: Jagan Teki @ 2015-12-11 14:59 UTC (permalink / raw) To: u-boot On 11 December 2015 at 02:57, Troy Kisky <troy.kisky@boundarydevices.com> wrote: > If GENMASK is REALLY desired, it should be GENMASK(23,0) > But since GENMASK is obviously more confusing, let's just revert. > > This reverts commit bad490a24212c068c5b718b9189f47ea4075d078. Sorry, just fix genmask why revert? because driver author has no complaint on this, please don't just say simply "more confusing" more over I usually prefer reverting bug fix patches. > > Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com> > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > > --- > v2: add sign off/ reviewed by > --- > drivers/spi/fsl_qspi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c > index ed39114..dd7048a 100644 > --- a/drivers/spi/fsl_qspi.c > +++ b/drivers/spi/fsl_qspi.c > @@ -24,7 +24,7 @@ DECLARE_GLOBAL_DATA_PTR; > #define TX_BUFFER_SIZE 0x40 > #endif > > -#define OFFSET_BITS_MASK GENMASK(24, 0) > +#define OFFSET_BITS_MASK 0x00ffffff > > #define FLASH_STATUS_WEL 0x02 > > -- > 2.5.0 thanks! -- Jagan. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK" 2015-12-11 14:59 ` Jagan Teki @ 2015-12-11 15:24 ` Marek Vasut 2015-12-11 15:28 ` Fabio Estevam 2015-12-14 11:21 ` Stefano Babic 0 siblings, 2 replies; 7+ messages in thread From: Marek Vasut @ 2015-12-11 15:24 UTC (permalink / raw) To: u-boot On Friday, December 11, 2015 at 03:59:14 PM, Jagan Teki wrote: > On 11 December 2015 at 02:57, Troy Kisky <troy.kisky@boundarydevices.com> wrote: > > If GENMASK is REALLY desired, it should be GENMASK(23,0) > > But since GENMASK is obviously more confusing, let's just revert. > > > > This reverts commit bad490a24212c068c5b718b9189f47ea4075d078. > > Sorry, just fix genmask why revert? because driver author has no > complaint on this, please don't just say simply "more confusing" more > over I usually prefer reverting bug fix patches. I also agree this GENMASK() crap is confusing. > > Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com> > > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> > > > > --- > > v2: add sign off/ reviewed by > > --- > > > > drivers/spi/fsl_qspi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c > > index ed39114..dd7048a 100644 > > --- a/drivers/spi/fsl_qspi.c > > +++ b/drivers/spi/fsl_qspi.c > > @@ -24,7 +24,7 @@ DECLARE_GLOBAL_DATA_PTR; > > > > #define TX_BUFFER_SIZE 0x40 > > #endif > > > > -#define OFFSET_BITS_MASK GENMASK(24, 0) > > +#define OFFSET_BITS_MASK 0x00ffffff > > > > #define FLASH_STATUS_WEL 0x02 > > > > -- > > 2.5.0 > > thanks! Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK" 2015-12-11 15:24 ` Marek Vasut @ 2015-12-11 15:28 ` Fabio Estevam 2015-12-11 15:41 ` Jagan Teki 2015-12-14 11:21 ` Stefano Babic 1 sibling, 1 reply; 7+ messages in thread From: Fabio Estevam @ 2015-12-11 15:28 UTC (permalink / raw) To: u-boot On Fri, Dec 11, 2015 at 1:24 PM, Marek Vasut <marex@denx.de> wrote: > On Friday, December 11, 2015 at 03:59:14 PM, Jagan Teki wrote: >> On 11 December 2015 at 02:57, Troy Kisky <troy.kisky@boundarydevices.com> > wrote: >> > If GENMASK is REALLY desired, it should be GENMASK(23,0) >> > But since GENMASK is obviously more confusing, let's just revert. >> > >> > This reverts commit bad490a24212c068c5b718b9189f47ea4075d078. >> >> Sorry, just fix genmask why revert? because driver author has no >> complaint on this, please don't just say simply "more confusing" more >> over I usually prefer reverting bug fix patches. > > I also agree this GENMASK() crap is confusing. Agreed. A simple proof that this is confusing is that it broke the fsl_qspi.c driver. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK" 2015-12-11 15:28 ` Fabio Estevam @ 2015-12-11 15:41 ` Jagan Teki 2015-12-11 18:54 ` Marek Vasut 0 siblings, 1 reply; 7+ messages in thread From: Jagan Teki @ 2015-12-11 15:41 UTC (permalink / raw) To: u-boot On 11 December 2015 at 20:58, Fabio Estevam <festevam@gmail.com> wrote: > On Fri, Dec 11, 2015 at 1:24 PM, Marek Vasut <marex@denx.de> wrote: >> On Friday, December 11, 2015 at 03:59:14 PM, Jagan Teki wrote: >>> On 11 December 2015 at 02:57, Troy Kisky <troy.kisky@boundarydevices.com> >> wrote: >>> > If GENMASK is REALLY desired, it should be GENMASK(23,0) >>> > But since GENMASK is obviously more confusing, let's just revert. >>> > >>> > This reverts commit bad490a24212c068c5b718b9189f47ea4075d078. >>> >>> Sorry, just fix genmask why revert? because driver author has no >>> complaint on this, please don't just say simply "more confusing" more >>> over I usually prefer reverting bug fix patches. >> >> I also agree this GENMASK() crap is confusing. > > Agreed. A simple proof that this is confusing is that it broke the > fsl_qspi.c driver. Probably your pointed at wrong h on genmask - but this wasn't be a proof of any global defined macro it's a mistake of using improper h value. If something went wrong with the patch then always ask and fix for proper thing and blaming global macro which used in other files is not a good thing. thanks! -- Jagan. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK" 2015-12-11 15:41 ` Jagan Teki @ 2015-12-11 18:54 ` Marek Vasut 0 siblings, 0 replies; 7+ messages in thread From: Marek Vasut @ 2015-12-11 18:54 UTC (permalink / raw) To: u-boot On Friday, December 11, 2015 at 04:41:50 PM, Jagan Teki wrote: > On 11 December 2015 at 20:58, Fabio Estevam <festevam@gmail.com> wrote: > > On Fri, Dec 11, 2015 at 1:24 PM, Marek Vasut <marex@denx.de> wrote: > >> On Friday, December 11, 2015 at 03:59:14 PM, Jagan Teki wrote: > >>> On 11 December 2015 at 02:57, Troy Kisky > >>> <troy.kisky@boundarydevices.com> > >> > >> wrote: > >>> > If GENMASK is REALLY desired, it should be GENMASK(23,0) > >>> > But since GENMASK is obviously more confusing, let's just revert. > >>> > > >>> > This reverts commit bad490a24212c068c5b718b9189f47ea4075d078. > >>> > >>> Sorry, just fix genmask why revert? because driver author has no > >>> complaint on this, please don't just say simply "more confusing" more > >>> over I usually prefer reverting bug fix patches. > >> > >> I also agree this GENMASK() crap is confusing. > > > > Agreed. A simple proof that this is confusing is that it broke the > > fsl_qspi.c driver. > > Probably your pointed at wrong h on genmask - but this wasn't be a > proof of any global defined macro it's a mistake of using improper h > value. Sorry, I do not understand this sentence at all. > If something went wrong with the patch then always ask and fix > for proper thing and blaming global macro which used in other files is > not a good thing. Usage of the macro itself is so confusing that even the conversion went wrong and introduced bugs. Clearly, using the macro is NOT an improvement. I also vote for the revert. Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK" 2015-12-11 15:24 ` Marek Vasut 2015-12-11 15:28 ` Fabio Estevam @ 2015-12-14 11:21 ` Stefano Babic 1 sibling, 0 replies; 7+ messages in thread From: Stefano Babic @ 2015-12-14 11:21 UTC (permalink / raw) To: u-boot Hi Jagan, Marek, Troy, On 11/12/2015 16:24, Marek Vasut wrote: > On Friday, December 11, 2015 at 03:59:14 PM, Jagan Teki wrote: >> On 11 December 2015 at 02:57, Troy Kisky <troy.kisky@boundarydevices.com> > wrote: >>> If GENMASK is REALLY desired, it should be GENMASK(23,0) >>> But since GENMASK is obviously more confusing, let's just revert. >>> >>> This reverts commit bad490a24212c068c5b718b9189f47ea4075d078. >> >> Sorry, just fix genmask why revert? because driver author has no >> complaint on this, please don't just say simply "more confusing" more >> over I usually prefer reverting bug fix patches. > > I also agree this GENMASK() crap is confusing. > IMHO it is easier for me to check directly the mask, as relying to a macro. But this is only my personal opinion. On the other side, the GENMASK macro is taken from the kernel and it is used by several SOCs, in U-Boot too. Maybe we like to see directly the mask, but several users like use it and I should accept it. Both solutions (fixing GENMASK or revert it) are fine with me. Today a new patch was sent : http://patchwork.ozlabs.org/patch/556361/ same issue, fixing the GENMASK(). Troy's patch was assigned to me, but it belongs to SPI's custodian, so I move it to Jagan. Best regards, Stefano Babic >>> Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com> >>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> >>> >>> --- >>> v2: add sign off/ reviewed by >>> --- >>> >>> drivers/spi/fsl_qspi.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c >>> index ed39114..dd7048a 100644 >>> --- a/drivers/spi/fsl_qspi.c >>> +++ b/drivers/spi/fsl_qspi.c >>> @@ -24,7 +24,7 @@ DECLARE_GLOBAL_DATA_PTR; >>> >>> #define TX_BUFFER_SIZE 0x40 >>> #endif >>> >>> -#define OFFSET_BITS_MASK GENMASK(24, 0) >>> +#define OFFSET_BITS_MASK 0x00ffffff >>> >>> #define FLASH_STATUS_WEL 0x02 >>> >>> -- >>> 2.5.0 >> >> thanks! > > Best regards, > Marek Vasut > -- ===================================================================== DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de ===================================================================== ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-12-14 11:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-12-10 21:27 [U-Boot] [PATCH V2 1/1] Revert "spi: fsl_qspi: Use GENMASK" Troy Kisky 2015-12-11 14:59 ` Jagan Teki 2015-12-11 15:24 ` Marek Vasut 2015-12-11 15:28 ` Fabio Estevam 2015-12-11 15:41 ` Jagan Teki 2015-12-11 18:54 ` Marek Vasut 2015-12-14 11:21 ` Stefano Babic
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox