[U-Boot] [PATCH v5 17/23] spi: cadence_qspi_apb: Use GENMASK

Sun Oct 25 00:25:27 CEST 2015

On Sun, Oct 25, 2015 at 12:13:14AM +0200, Marek Vasut wrote:
> On Saturday, October 24, 2015 at 11:51:39 PM, Tom Rini wrote:
> > On Sat, Oct 24, 2015 at 02:41:41PM +0200, Marek Vasut wrote:
> > > On Saturday, October 24, 2015 at 05:39:04 AM, Jagan Teki wrote:
> > > > Replace numeric mask hexcodes with GENMASK macro
> > > > in cadence_qspi_apb
> > > > 
> > > > Cc: Fabio Estevam <festevam at gmail.com>
> > > > Cc: Stefan Roese <sr at denx.de>
> > > > Cc: Marek Vasut <marex at denx.de>
> > > > Cc: Tom Rini <trini at konsulko.com>
> > > > Acked-by: Vikas Manocha <vikas.manocha at st.com>
> > > > Signed-off-by: Jagan Teki <jteki at openedev.com>
> > > > ---
> > > > 
> > > >  drivers/spi/cadence_qspi_apb.c | 46
> > > > 
> > > > +++++++++++++++++++++--------------------- 1 file changed, 23
> > > > insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/drivers/spi/cadence_qspi_apb.c
> > > > b/drivers/spi/cadence_qspi_apb.c index 7786dd6..662d3bb 100644
> > > > --- a/drivers/spi/cadence_qspi_apb.c
> > > > +++ b/drivers/spi/cadence_qspi_apb.c
> > > > @@ -44,7 +44,7 @@
> > > > 
> > > >  #define CQSPI_INST_TYPE_QUAD			(2)
> > > >  
> > > >  #define CQSPI_STIG_DATA_LEN_MAX			(8)
> > > > 
> > > > -#define CQSPI_INDIRECTTRIGGER_ADDR_MASK		(0xFFFFF)
> > > > +#define CQSPI_INDIRECTTRIGGER_ADDR_MASK		GENMASK(19, 0)
> > > > 
> > > >  #define CQSPI_DUMMY_CLKS_PER_BYTE		(8)
> > > >  #define CQSPI_DUMMY_BYTES_MAX			(4)
> > > > 
> > > > @@ -65,8 +65,8 @@
> > > > 
> > > >  #define	CQSPI_REG_CONFIG_CHIPSELECT_LSB		10
> > > >  #define	CQSPI_REG_CONFIG_BAUD_LSB		19
> > > >  #define	CQSPI_REG_CONFIG_IDLE_LSB		31
> > > > 
> > > > -#define	CQSPI_REG_CONFIG_CHIPSELECT_MASK	0xF
> > > > -#define	CQSPI_REG_CONFIG_BAUD_MASK		0xF
> > > > +#define	CQSPI_REG_CONFIG_CHIPSELECT_MASK	GENMASK(3, 0)
> > > > +#define	CQSPI_REG_CONFIG_BAUD_MASK		GENMASK(3, 0)
> > > > 
> > > >  #define	CQSPI_REG_RD_INSTR			0x04
> > > >  #define	CQSPI_REG_RD_INSTR_OPCODE_LSB		0
> > > > 
> > > > @@ -75,10 +75,10 @@
> > > > 
> > > >  #define	CQSPI_REG_RD_INSTR_TYPE_DATA_LSB	16
> > > >  #define	CQSPI_REG_RD_INSTR_MODE_EN_LSB		20
> > > >  #define	CQSPI_REG_RD_INSTR_DUMMY_LSB		24
> > > > 
> > > > -#define	CQSPI_REG_RD_INSTR_TYPE_INSTR_MASK	0x3
> > > > -#define	CQSPI_REG_RD_INSTR_TYPE_ADDR_MASK	0x3
> > > > -#define	CQSPI_REG_RD_INSTR_TYPE_DATA_MASK	0x3
> > > > -#define	CQSPI_REG_RD_INSTR_DUMMY_MASK		0x1F
> > > > +#define	CQSPI_REG_RD_INSTR_TYPE_INSTR_MASK	GENMASK(1, 0)
> > > > +#define	CQSPI_REG_RD_INSTR_TYPE_ADDR_MASK	GENMASK(1, 0)
> > > > +#define	CQSPI_REG_RD_INSTR_TYPE_DATA_MASK	GENMASK(1, 0)
> > > > +#define	CQSPI_REG_RD_INSTR_DUMMY_MASK		GENMASK(4, 0)
> > > > 
> > > >  #define	CQSPI_REG_WR_INSTR			0x08
> > > >  #define	CQSPI_REG_WR_INSTR_OPCODE_LSB		0
> > > > 
> > > > @@ -88,23 +88,23 @@
> > > > 
> > > >  #define	CQSPI_REG_DELAY_TCHSH_LSB		8
> > > >  #define	CQSPI_REG_DELAY_TSD2D_LSB		16
> > > >  #define	CQSPI_REG_DELAY_TSHSL_LSB		24
> > > > 
> > > > -#define	CQSPI_REG_DELAY_TSLCH_MASK		0xFF
> > > > -#define	CQSPI_REG_DELAY_TCHSH_MASK		0xFF
> > > > -#define	CQSPI_REG_DELAY_TSD2D_MASK		0xFF
> > > > -#define	CQSPI_REG_DELAY_TSHSL_MASK		0xFF
> > > > +#define	CQSPI_REG_DELAY_TSLCH_MASK		GENMASK(7, 0)
> > > > +#define	CQSPI_REG_DELAY_TCHSH_MASK		GENMASK(7, 0)
> > > > +#define	CQSPI_REG_DELAY_TSD2D_MASK		GENMASK(7, 0)
> > > > +#define	CQSPI_REG_DELAY_TSHSL_MASK		GENMASK(7, 0)
> > > > 
> > > >  #define	CQSPI_READLCAPTURE			0x10
> > > >  #define	CQSPI_READLCAPTURE_BYPASS_LSB		0
> > > >  #define	CQSPI_READLCAPTURE_DELAY_LSB		1
> > > > 
> > > > -#define	CQSPI_READLCAPTURE_DELAY_MASK		0xF
> > > > +#define	CQSPI_READLCAPTURE_DELAY_MASK		GENMASK(3, 0)
> > > > 
> > > >  #define	CQSPI_REG_SIZE				0x14
> > > >  #define	CQSPI_REG_SIZE_ADDRESS_LSB		0
> > > >  #define	CQSPI_REG_SIZE_PAGE_LSB			4
> > > >  #define	CQSPI_REG_SIZE_BLOCK_LSB		16
> > > > 
> > > > -#define	CQSPI_REG_SIZE_ADDRESS_MASK		0xF
> > > > -#define	CQSPI_REG_SIZE_PAGE_MASK		0xFFF
> > > > -#define	CQSPI_REG_SIZE_BLOCK_MASK		0x3F
> > > > +#define	CQSPI_REG_SIZE_ADDRESS_MASK		GENMASK(3, 0)
> > > > +#define	CQSPI_REG_SIZE_PAGE_MASK		GENMASK(11, 0)
> > > > +#define	CQSPI_REG_SIZE_BLOCK_MASK		GENMASK(5, 0)
> > > > 
> > > >  #define	CQSPI_REG_SRAMPARTITION			0x18
> > > >  #define	CQSPI_REG_INDIRECTTRIGGER		0x1C
> > > > 
> > > > @@ -115,8 +115,8 @@
> > > > 
> > > >  #define	CQSPI_REG_SDRAMLEVEL			0x2C
> > > >  #define	CQSPI_REG_SDRAMLEVEL_RD_LSB		0
> > > >  #define	CQSPI_REG_SDRAMLEVEL_WR_LSB		16
> > > > 
> > > > -#define	CQSPI_REG_SDRAMLEVEL_RD_MASK		0xFFFF
> > > > -#define	CQSPI_REG_SDRAMLEVEL_WR_MASK		0xFFFF
> > > > +#define	CQSPI_REG_SDRAMLEVEL_RD_MASK		GENMASK(15, 0)
> > > > +#define	CQSPI_REG_SDRAMLEVEL_WR_MASK		GENMASK(15, 0)
> > > > 
> > > >  #define	CQSPI_REG_IRQSTATUS			0x40
> > > >  #define	CQSPI_REG_IRQMASK			0x44
> > > > 
> > > > @@ -142,11 +142,11 @@
> > > > 
> > > >  #define	CQSPI_REG_CMDCTRL_RD_BYTES_LSB		20
> > > >  #define	CQSPI_REG_CMDCTRL_RD_EN_LSB		23
> > > >  #define	CQSPI_REG_CMDCTRL_OPCODE_LSB		24
> > > > 
> > > > -#define	CQSPI_REG_CMDCTRL_DUMMY_MASK		0x1F
> > > > -#define	CQSPI_REG_CMDCTRL_WR_BYTES_MASK		0x7
> > > > -#define	CQSPI_REG_CMDCTRL_ADD_BYTES_MASK	0x3
> > > > -#define	CQSPI_REG_CMDCTRL_RD_BYTES_MASK		0x7
> > > > -#define	CQSPI_REG_CMDCTRL_OPCODE_MASK		0xFF
> > > > +#define	CQSPI_REG_CMDCTRL_DUMMY_MASK		GENMASK(4, 0)
> > > > +#define	CQSPI_REG_CMDCTRL_WR_BYTES_MASK		GENMASK(2, 0)
> > > > +#define	CQSPI_REG_CMDCTRL_ADD_BYTES_MASK	GENMASK(1, 0)
> > > > +#define	CQSPI_REG_CMDCTRL_RD_BYTES_MASK		GENMASK(2, 0)
> > > > +#define	CQSPI_REG_CMDCTRL_OPCODE_MASK		GENMASK(7, 0)
> > > > 
> > > >  #define	CQSPI_REG_INDIRECTWR			0x70
> > > >  #define	CQSPI_REG_INDIRECTWR_START_MASK		BIT(0)
> > > > 
> > > > @@ -463,7 +463,7 @@ void cadence_qspi_apb_chipselect(void *reg_base,
> > > > 
> > > >  		 * CS2 to 4b'1011
> > > >  		 * CS3 to 4b'0111
> > > >  		 */
> > > > 
> > > > -		chip_select = 0xF & ~(1 << chip_select);
> > > > +		chip_select = GENMASK(3, 0) & ~(1 << chip_select);
> > > 
> > > Again, this only makes things more cryptic for no good reason. NAK
> > 
> > Personal preference.
> 
> Yeah, sorry. They still didn't install CPP into my brain.

True.  But it's called GENMASK not BVTYKS.  Now, I'm not saying I never
checked that a macro was doing what it said it was doing, but that's
what reviewing the earlier parts of the patch are for.  Of course I'm
the person that pulls out bc and verifies hex-to-binary when fiddling
with bitfields so I'm biased here.

> > So as I asked before, who is mainly mucking around
> > in these drivers?
> 
> I guess Chin would be the one who's mostly plumbing in Cadence recently, 
> followed by Stefan Roese.

OK.  So, if Chin or Stefan doesn't like it, that's a good reason to NAK
it.  And the same goes for anyone else and the drivers they own and muck
around in.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151024/bbe6b88a/attachment.sig>