[v4,01/39] ARM: OMAP2+: gpmc: driver conversion

Convert GPMC code to driver. Boards using GPMC should provide driver
with type of configuration, timing, CS. Platform devices would then be
created for each connected peripheral (details also to be passed by
board so that it reaches respective driver). And GPMC driver would
populate memory resource details for the connected peripheral driver.
Boards should inform gpmc driver with platform data destined for
peripheral driver. gpmc driver will provide the same information to
peripheral driver.

A peripheral connected to GPMC can have multiple address spaces using
different chip select. Hence GPMC driver has been provided capability
to create platform device for peripheral using mutiple CS. The
peripheral that made it necessary was tusb6010.

Interrupts of GPMC are presented to drivers of connected peripherals
as resource. A fictitious interrupt controller chip handles these
interrupts at GPMC hardware level. Clients can use normal interrupt
APIs. Platform information of peripheral passed to GPMC driver should
indicate interrupts to be used via flags.

Driver is capable of configuring waitpin, waitpin details has to be
provided per CS. Wait pin has been considered as exclusive resource
as multiple peripherals should not using the same pin, at the same
it is valid for mutiple CS to use same waitpin provided they are
a part of single peripheral (eg. tusb6010)

An exported symbol for reconfiguring GPMC settings has been provided.
OneNAND is the one that neccessitated this.

Acquiring CS# for NAND is done on a few boards. It means, depending
on bootloader to embed this information. Probably CS# being used can
be set in the Kernel, and acquiring it can be removed. If ever this
capbility is needed, GPMC driver has to be made aware of handling it.

Modifications has been made keeping in mind that the driver would
have to move to driver folder. This explains requirement of clk_prd
field; even though clk_prd variable is not necessary as
gpmc_get_fclk_period is present in the same file as of now, this will
help in moving the driver easily to drivers folder.

Code related to GPMC clock may have to continue live in platform
folders as input clock is beyond the control of GPMC and calculating
timing for the peripheral may need other helpers. This explains
presence of 'gpmc_cs_calc_divider' along with 'gpmc_calc_divider',
both doing same work, latter meant to go with driver, former for
calculation in platform code.

Thanks to Vaibhav Hiremath & Jonathan Hunter on their various good
suggestions which resulted in improving the code.

Cc: Vaibhav Hiremath <hvaibhav@ti.com>
Cc: Jon Hunter <jon-hunter@ti.com>
Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 arch/arm/mach-omap2/gpmc.c             |  877 ++++++++++++++++++++++++++++----
 arch/arm/plat-omap/include/plat/gpmc.h |   93 +++-
 2 files changed, 872 insertions(+), 98 deletions(-)

Hi Afzal,

Looks good! Some minor comments ...

On 05/01/2012 07:19 AM, Afzal Mohammed wrote:
> Convert GPMC code to driver. Boards using GPMC should provide driver
> with type of configuration, timing, CS. Platform devices would then be
> created for each connected peripheral (details also to be passed by
> board so that it reaches respective driver). And GPMC driver would
> populate memory resource details for the connected peripheral driver.
> Boards should inform gpmc driver with platform data destined for
> peripheral driver. gpmc driver will provide the same information to
> peripheral driver.
> 
> A peripheral connected to GPMC can have multiple address spaces using
> different chip select. Hence GPMC driver has been provided capability
> to create platform device for peripheral using mutiple CS. The
> peripheral that made it necessary was tusb6010.
> 
> Interrupts of GPMC are presented to drivers of connected peripherals
> as resource. A fictitious interrupt controller chip handles these
> interrupts at GPMC hardware level. Clients can use normal interrupt
> APIs. Platform information of peripheral passed to GPMC driver should
> indicate interrupts to be used via flags.
> 
> Driver is capable of configuring waitpin, waitpin details has to be
> provided per CS. Wait pin has been considered as exclusive resource
> as multiple peripherals should not using the same pin, at the same
> it is valid for mutiple CS to use same waitpin provided they are
> a part of single peripheral (eg. tusb6010)
> 
> An exported symbol for reconfiguring GPMC settings has been provided.
> OneNAND is the one that neccessitated this.
> 
> Acquiring CS# for NAND is done on a few boards. It means, depending
> on bootloader to embed this information. Probably CS# being used can
> be set in the Kernel, and acquiring it can be removed. If ever this
> capbility is needed, GPMC driver has to be made aware of handling it.
> 
> Modifications has been made keeping in mind that the driver would
> have to move to driver folder. This explains requirement of clk_prd
> field; even though clk_prd variable is not necessary as
> gpmc_get_fclk_period is present in the same file as of now, this will
> help in moving the driver easily to drivers folder.
> 
> Code related to GPMC clock may have to continue live in platform
> folders as input clock is beyond the control of GPMC and calculating
> timing for the peripheral may need other helpers. This explains
> presence of 'gpmc_cs_calc_divider' along with 'gpmc_calc_divider',
> both doing same work, latter meant to go with driver, former for
> calculation in platform code.
> 
> Thanks to Vaibhav Hiremath & Jonathan Hunter on their various good
> suggestions which resulted in improving the code.
> 
> Cc: Vaibhav Hiremath <hvaibhav@ti.com>
> Cc: Jon Hunter <jon-hunter@ti.com>
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> ---
>  arch/arm/mach-omap2/gpmc.c             |  877 ++++++++++++++++++++++++++++----
>  arch/arm/plat-omap/include/plat/gpmc.h |   93 +++-
>  2 files changed, 872 insertions(+), 98 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 580e684..12916f3 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -14,8 +14,11 @@
>   */
>  #undef DEBUG
>  
> +#include <linux/platform_device.h>
> +
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
> +#include <linux/slab.h>
>  #include <linux/init.h>
>  #include <linux/err.h>
>  #include <linux/clk.h>
> @@ -53,6 +56,45 @@
>  #define GPMC_CS0_OFFSET		0x60
>  #define GPMC_CS_SIZE		0x30
>  
> +/* GPMC register bits */
> +#define	GPMC_CONFIG1_TIMEPARAGRANULARITY	BIT(4)
> +#define	GPMC_CONFIG1_DEVICETYPE_NAND		GPMC_CONFIG1_DEVICETYPE(0x2)
> +#define	GPMC_CONFIG1_WAIT_PIN_SEL_MASK		GPMC_CONFIG1_WAIT_PIN_SEL(0x3)
> +#define	GPMC_CONFIG1_WAIT_MON_TIME(val)		((val & 0x3) << 18)
> +#define	GPMC_CONFIG1_WRITEMULTIPLE		BIT(28)
> +#define	GPMC_CONFIG1_READMULTIPLE		BIT(30)
> +#define	GPMC_CONFIG1_WRAPBURST			BIT(31)
> +#define	GPMC_CONFIG_WAITPIN_POLARITY_SHIFT	0x8
> +#define	GPMC_CONFIG1_WAITPIN_MONITOR_TIME(val)	((val & 0x3) << 18)
> +#define	GPMC_CONFIG1_WAITPIN_MONITOR_TIME_1	\
> +				GPMC_CONFIG1_WAITPIN_MONITOR_TIME(0x1)
> +#define	GPMC_CONFIG1_WAITPIN_MONITOR_TIME_2	\
> +				GPMC_CONFIG1_WAITPIN_MONITOR_TIME(0x2)
> +#define	GPMC_CONFIG1_CLOCKACTIVATION_TIME(val)	((val & 0x3) << 25)
> +#define	GPMC_CONFIG1_CLOCKACTIVATION_TIME_1	\
> +				GPMC_CONFIG1_CLOCKACTIVATION_TIME(0x1)
> +#define	GPMC_CONFIG1_CLOCKACTIVATION_TIME_2	\
> +				GPMC_CONFIG1_CLOCKACTIVATION_TIME(0x2)
> +
> +#define	GPMC_CONFIG2_CSEXTRADELAY		BIT(7)
> +
> +#define	GPMC_CONFIG3_ADVEXTRADELAY		BIT(7)
> +
> +#define	GPMC_CONFIG4_OEEXTRADELAY		BIT(7)
> +#define	GPMC_CONFIG4_WEEXTRADELAY		BIT(23)
> +
> +#define	GPMC_CONFIG6_CYCLE2CYCLEDIFFCSEN	BIT(6)
> +#define	GPMC_CONFIG6_CYCLE2CYCLESAMECSEN	BIT(7)
> +
> +#define	GPMC_IRQ_BIT_FIFOEVENT		BIT(0)
> +#define	GPMC_IRQ_BIT_TERMINALCOUNT	BIT(1)
> +
> +#define	GPMC_WAITPIN_IDX0			0x0
> +#define	GPMC_WAITPIN_IDX1			0x1
> +#define	GPMC_WAITPIN_IDX2			0x2
> +#define	GPMC_WAITPIN_IDX3			0x3
> +#define	GPMC_NR_WAITPIN				0x4

How about an enum here? Also OMAP4 only have 3 wait pins and so the
number is device dependent.

>  #define GPMC_MEM_START		0x00000000
>  #define GPMC_MEM_END		0x3FFFFFFF
>  #define BOOT_ROM_SPACE		0x100000	/* 1MB */
> @@ -64,6 +106,55 @@
>  #define ENABLE_PREFETCH		(0x1 << 7)
>  #define DMA_MPU_MODE		2
>  
> +#define	DRIVER_NAME	"omap-gpmc"
> +
> +#define	GPMC_NR_IRQ		2

Why has this been changed to 2? It was 6 in the previous version. As we
discussed before the number of IRQs should be detected based upon GPMC
IP version.

> +
> +#define	HIGH			1
> +#define	LOW			-1
> +
> +struct gpmc_device {
> +	char			*name;
> +	int			id;
> +	void			*pdata;
> +	unsigned		pdata_size;
> +	struct resource		*per_res;
> +	unsigned		per_res_cnt;
> +	struct resource		*gpmc_res;
> +	unsigned		gpmc_res_cnt;
> +	bool			have_waitpin;
> +	unsigned		waitpin;
> +	int			waitpin_polarity;

I think that it make more sense to have the wait-pin information part of
the gpmc_cs_data structure for the following reasons ...

1. If a device can use multiple CS, then it could use multiple wait signals.
2. Eventually, all board information needs to move to device tree. By
having it in the pdata it will be easier to migrate to device tree.

It may be nice to have "have_waitpin" be an integer too, that indicates
if wait is used for both read and write or just one or the other.

Also, any reason why waitpin_polarity is an int? I see you define LOW as
-1, but I not sure why LOW cannot be 0 as 0 is programmed into the
register for an active low wait signal.

> +};
> +
> +struct gpmc_irq	{
> +	unsigned		irq;
> +	u32			bitmask;
> +};
> +
> +struct gpmc {
> +	struct device		*dev;
> +	unsigned long		clk_prd;
> +	void __iomem		*io_base;
> +	unsigned long		phys_base;
> +	u32			memsize;
> +	unsigned		cs_map;
> +	int			ecc_used;
> +	spinlock_t		mem_lock;
> +	struct resource		mem_root;
> +	struct resource		cs_mem[GPMC_CS_NUM];
> +	/* XXX: Or allocate dynamically ? */
> +	struct gpmc_device	device[GPMC_CS_NUM];
> +	unsigned		num_device;
> +	unsigned		master_irq;
> +	unsigned		irq_start;
> +	struct gpmc_irq		irq[GPMC_NR_IRQ];
> +	struct irq_chip		irq_chip;
> +	bool			wp;
> +	unsigned		waitpin_map;
> +	unsigned		revision;
> +};
> +
>  /* Structure to save gpmc cs context */
>  struct gpmc_cs_config {
>  	u32 config1;
> @@ -91,58 +182,50 @@ struct omap3_gpmc_regs {
>  	struct gpmc_cs_config cs_context[GPMC_CS_NUM];
>  };
>  
> -static struct resource	gpmc_mem_root;
> -static struct resource	gpmc_cs_mem[GPMC_CS_NUM];
> -static DEFINE_SPINLOCK(gpmc_mem_lock);
> -static unsigned int gpmc_cs_map;	/* flag for cs which are initialized */
> -static int gpmc_ecc_used = -EINVAL;	/* cs using ecc engine */
> -
> -static void __iomem *gpmc_base;
> -
>  static struct clk *gpmc_l3_clk;
>  
> -static irqreturn_t gpmc_handle_irq(int irq, void *dev);
> +static struct gpmc *gpmc;
>  
>  static void gpmc_write_reg(int idx, u32 val)
>  {
> -	__raw_writel(val, gpmc_base + idx);
> +	writel(val, gpmc->io_base + idx);
>  }
>  
>  static u32 gpmc_read_reg(int idx)
>  {
> -	return __raw_readl(gpmc_base + idx);
> +	return readl(gpmc->io_base + idx);
>  }
>  
>  static void gpmc_cs_write_byte(int cs, int idx, u8 val)
>  {
>  	void __iomem *reg_addr;
>  
> -	reg_addr = gpmc_base + GPMC_CS0_OFFSET + (cs * GPMC_CS_SIZE) + idx;
> -	__raw_writeb(val, reg_addr);
> +	reg_addr = gpmc->io_base + GPMC_CS0_OFFSET + (cs * GPMC_CS_SIZE) + idx;
> +	writeb(val, reg_addr);
>  }
>  
>  static u8 gpmc_cs_read_byte(int cs, int idx)
>  {
>  	void __iomem *reg_addr;
>  
> -	reg_addr = gpmc_base + GPMC_CS0_OFFSET + (cs * GPMC_CS_SIZE) + idx;
> -	return __raw_readb(reg_addr);
> +	reg_addr = gpmc->io_base + GPMC_CS0_OFFSET + (cs * GPMC_CS_SIZE) + idx;
> +	return readb(reg_addr);
>  }
>  
>  void gpmc_cs_write_reg(int cs, int idx, u32 val)
>  {
>  	void __iomem *reg_addr;
>  
> -	reg_addr = gpmc_base + GPMC_CS0_OFFSET + (cs * GPMC_CS_SIZE) + idx;
> -	__raw_writel(val, reg_addr);
> +	reg_addr = gpmc->io_base + GPMC_CS0_OFFSET + (cs * GPMC_CS_SIZE) + idx;
> +	writel(val, reg_addr);
>  }
>  
>  u32 gpmc_cs_read_reg(int cs, int idx)
>  {
>  	void __iomem *reg_addr;
>  
> -	reg_addr = gpmc_base + GPMC_CS0_OFFSET + (cs * GPMC_CS_SIZE) + idx;
> -	return __raw_readl(reg_addr);
> +	reg_addr = gpmc->io_base + GPMC_CS0_OFFSET + (cs * GPMC_CS_SIZE) + idx;
> +	return readl(reg_addr);
>  }
>  
>  /* TODO: Add support for gpmc_fck to clock framework and use it */
> @@ -207,7 +290,8 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  	if (time == 0)
>  		ticks = 0;
>  	else
> -		ticks = gpmc_ns_to_ticks(time);
> +		ticks = (time * 1000 + gpmc->clk_prd - 1) / gpmc->clk_prd;
> +
>  	nr_bits = end_bit - st_bit + 1;
>  	if (ticks >= 1 << nr_bits) {
>  #ifdef DEBUG
> @@ -222,7 +306,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  #ifdef DEBUG
>  	printk(KERN_INFO
>  		"GPMC CS%d: %-10s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
> -	       cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
> +	       cs, name, ticks, gpmc->clk_prd * ticks / 1000,
>  			(l >> st_bit) & mask, time);
>  #endif
>  	l &= ~(mask << st_bit);
> @@ -243,6 +327,21 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  		return -1
>  #endif
>  
> +int gpmc_calc_divider(unsigned int sync_clk)
> +{
> +	int div;
> +	u32 l;
> +
> +	l = sync_clk + (gpmc->clk_prd - 1);
> +	div = l / gpmc->clk_prd;
> +	if (div > 4)
> +		return -1;
> +	if (div <= 0)
> +		div = 1;
> +
> +	return div;
> +}
> +
>  int gpmc_cs_calc_divider(int cs, unsigned int sync_clk)
>  {
>  	int div;
> @@ -258,12 +357,53 @@ int gpmc_cs_calc_divider(int cs, unsigned int sync_clk)
>  	return div;
>  }
>  
> +static void gpmc_cs_onoff_timings(int cs, const struct gpmc_onoff_timings *p)
> +{
> +	u32 l;
> +
> +	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG2);
> +	if (p->cs_extra_delay)
> +		l |= GPMC_CONFIG2_CSEXTRADELAY;
> +	else
> +		l &= ~GPMC_CONFIG2_CSEXTRADELAY;
> +	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG2, l);
> +
> +	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG3);
> +	if (p->adv_extra_delay)
> +		l |= GPMC_CONFIG3_ADVEXTRADELAY;
> +	else
> +		l &= ~GPMC_CONFIG3_ADVEXTRADELAY;
> +	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG3, l);
> +
> +	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG4);
> +	if (p->oe_extra_delay)
> +		l |= GPMC_CONFIG4_OEEXTRADELAY;
> +	else
> +		l &= ~GPMC_CONFIG4_OEEXTRADELAY;
> +	if (p->we_extra_delay)
> +		l |= GPMC_CONFIG4_WEEXTRADELAY;
> +	else
> +		l &= ~GPMC_CONFIG4_WEEXTRADELAY;
> +	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG4, l);
> +
> +	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG6);
> +	if (p->cycle2cyclesamecsen)
> +		l |= GPMC_CONFIG6_CYCLE2CYCLESAMECSEN;
> +	else
> +		l &= ~GPMC_CONFIG6_CYCLE2CYCLESAMECSEN;
> +	if (p->cycle2cyclediffcsen)
> +		l |= GPMC_CONFIG6_CYCLE2CYCLEDIFFCSEN;
> +	else
> +		l &= ~GPMC_CONFIG6_CYCLE2CYCLEDIFFCSEN;
> +	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG6, l);
> +}
> +
>  int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
>  {
>  	int div;
>  	u32 l;
>  
> -	div = gpmc_cs_calc_divider(cs, t->sync_clk);
> +	div = gpmc_calc_divider(t->sync_clk);
>  	if (div < 0)
>  		return -1;
>  
> @@ -286,7 +426,10 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
>  
>  	GPMC_SET_ONE(GPMC_CS_CONFIG5, 24, 27, page_burst_access);
>  
> -	if (cpu_is_omap34xx()) {
> +	GPMC_SET_ONE(GPMC_CS_CONFIG6, 8, 11, cycle2cycle_delay);
> +	GPMC_SET_ONE(GPMC_CS_CONFIG6, 0, 3, busturnaround);
> +
> +	if (gpmc->revision >= 5) {
>  		GPMC_SET_ONE(GPMC_CS_CONFIG6, 16, 19, wr_data_mux_bus);
>  		GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
>  	}
> @@ -298,13 +441,15 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
>  	if (l & (GPMC_CONFIG1_READTYPE_SYNC | GPMC_CONFIG1_WRITETYPE_SYNC)) {
>  #ifdef DEBUG
>  		printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
> -				cs, (div * gpmc_get_fclk_period()) / 1000, div);
> +				cs, (div * gpmc->clk_prd) / 1000, div);
>  #endif
>  		l &= ~0x03;
>  		l |= (div - 1);
>  		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
>  	}
>  
> +	gpmc_cs_onoff_timings(cs, &t->control);
> +
>  	return 0;
>  }
>  
> @@ -332,7 +477,7 @@ static void gpmc_cs_disable_mem(int cs)
>  	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG7, l);
>  }
>  
> -static void gpmc_cs_get_memconf(int cs, u32 *base, u32 *size)
> +static __devinit void gpmc_cs_get_memconf(int cs, u32 *base, u32 *size)
>  {
>  	u32 l;
>  	u32 mask;
> @@ -351,23 +496,23 @@ static int gpmc_cs_mem_enabled(int cs)
>  	return l & GPMC_CONFIG7_CSVALID;
>  }
>  
> -int gpmc_cs_set_reserved(int cs, int reserved)
> +static int gpmc_cs_set_reserved(int cs, int reserved)
>  {
>  	if (cs > GPMC_CS_NUM)
>  		return -ENODEV;
>  
> -	gpmc_cs_map &= ~(1 << cs);
> -	gpmc_cs_map |= (reserved ? 1 : 0) << cs;
> +	gpmc->cs_map &= ~(1 << cs);
> +	gpmc->cs_map |= (reserved ? 1 : 0) << cs;
>  
>  	return 0;
>  }
>  
> -int gpmc_cs_reserved(int cs)
> +static int gpmc_cs_reserved(int cs)
>  {
>  	if (cs > GPMC_CS_NUM)
>  		return -ENODEV;
>  
> -	return gpmc_cs_map & (1 << cs);
> +	return gpmc->cs_map & (1 << cs);
>  }
>  
>  static unsigned long gpmc_mem_align(unsigned long size)
> @@ -384,24 +529,25 @@ static unsigned long gpmc_mem_align(unsigned long size)
>  	return size;
>  }
>  
> -static int gpmc_cs_insert_mem(int cs, unsigned long base, unsigned long size)
> +static __devinit
> +int gpmc_cs_insert_mem(int cs, unsigned long base, unsigned long size)
>  {
> -	struct resource	*res = &gpmc_cs_mem[cs];
> +	struct resource	*res = &gpmc->cs_mem[cs];
>  	int r;
>  
>  	size = gpmc_mem_align(size);
> -	spin_lock(&gpmc_mem_lock);
> +	spin_lock(&gpmc->mem_lock);
>  	res->start = base;
>  	res->end = base + size - 1;
> -	r = request_resource(&gpmc_mem_root, res);
> -	spin_unlock(&gpmc_mem_lock);
> +	r = request_resource(&gpmc->mem_root, res);
> +	spin_unlock(&gpmc->mem_lock);
>  
>  	return r;
>  }
>  
>  int gpmc_cs_request(int cs, unsigned long size, unsigned long *base)
>  {
> -	struct resource *res = &gpmc_cs_mem[cs];
> +	struct resource *res = &gpmc->cs_mem[cs];
>  	int r = -1;
>  
>  	if (cs > GPMC_CS_NUM)
> @@ -411,7 +557,7 @@ int gpmc_cs_request(int cs, unsigned long size, unsigned long *base)
>  	if (size > (1 << GPMC_SECTION_SHIFT))
>  		return -ENOMEM;
>  
> -	spin_lock(&gpmc_mem_lock);
> +	spin_lock(&gpmc->mem_lock);
>  	if (gpmc_cs_reserved(cs)) {
>  		r = -EBUSY;
>  		goto out;
> @@ -419,7 +565,7 @@ int gpmc_cs_request(int cs, unsigned long size, unsigned long *base)
>  	if (gpmc_cs_mem_enabled(cs))
>  		r = adjust_resource(res, res->start & ~(size - 1), size);
>  	if (r < 0)
> -		r = allocate_resource(&gpmc_mem_root, res, size, 0, ~0,
> +		r = allocate_resource(&gpmc->mem_root, res, size, 0, ~0,
>  				      size, NULL, NULL);
>  	if (r < 0)
>  		goto out;
> @@ -428,24 +574,24 @@ int gpmc_cs_request(int cs, unsigned long size, unsigned long *base)
>  	*base = res->start;
>  	gpmc_cs_set_reserved(cs, 1);
>  out:
> -	spin_unlock(&gpmc_mem_lock);
> +	spin_unlock(&gpmc->mem_lock);
>  	return r;
>  }
>  EXPORT_SYMBOL(gpmc_cs_request);
>  
>  void gpmc_cs_free(int cs)
>  {
> -	spin_lock(&gpmc_mem_lock);
> +	spin_lock(&gpmc->mem_lock);
>  	if (cs >= GPMC_CS_NUM || cs < 0 || !gpmc_cs_reserved(cs)) {
>  		printk(KERN_ERR "Trying to free non-reserved GPMC CS%d\n", cs);
>  		BUG();
> -		spin_unlock(&gpmc_mem_lock);
> +		spin_unlock(&gpmc->mem_lock);
>  		return;
>  	}
>  	gpmc_cs_disable_mem(cs);
> -	release_resource(&gpmc_cs_mem[cs]);
> +	release_resource(&gpmc->cs_mem[cs]);
>  	gpmc_cs_set_reserved(cs, 0);
> -	spin_unlock(&gpmc_mem_lock);
> +	spin_unlock(&gpmc->mem_lock);
>  }
>  EXPORT_SYMBOL(gpmc_cs_free);
>  
> @@ -668,7 +814,7 @@ int gpmc_prefetch_reset(int cs)
>  }
>  EXPORT_SYMBOL(gpmc_prefetch_reset);
>  
> -static void __init gpmc_mem_init(void)
> +static __devinit void gpmc_mem_init(void)
>  {
>  	int cs;
>  	unsigned long boot_rom_space = 0;
> @@ -680,8 +826,8 @@ static void __init gpmc_mem_init(void)
>  	/* In apollon the CS0 is mapped as 0x0000 0000 */
>  	if (machine_is_omap_apollon())
>  		boot_rom_space = 0;
> -	gpmc_mem_root.start = GPMC_MEM_START + boot_rom_space;
> -	gpmc_mem_root.end = GPMC_MEM_END;
> +	gpmc->mem_root.start = GPMC_MEM_START + boot_rom_space;
> +	gpmc->mem_root.end = GPMC_MEM_END;
>  
>  	/* Reserve all regions that has been set up by bootloader */
>  	for (cs = 0; cs < GPMC_CS_NUM; cs++) {
> @@ -697,26 +843,15 @@ static void __init gpmc_mem_init(void)
>  
>  static int __init gpmc_init(void)
>  {
> -	u32 l, irq;
> -	int cs, ret = -EINVAL;
> -	int gpmc_irq;
> +	int ret = -EINVAL;
>  	char *ck = NULL;
>  
>  	if (cpu_is_omap24xx()) {
>  		ck = "core_l3_ck";
> -		if (cpu_is_omap2420())
> -			l = OMAP2420_GPMC_BASE;
> -		else
> -			l = OMAP34XX_GPMC_BASE;
> -		gpmc_irq = INT_34XX_GPMC_IRQ;
>  	} else if (cpu_is_omap34xx()) {
>  		ck = "gpmc_fck";
> -		l = OMAP34XX_GPMC_BASE;
> -		gpmc_irq = INT_34XX_GPMC_IRQ;
>  	} else if (cpu_is_omap44xx()) {
>  		ck = "gpmc_ck";
> -		l = OMAP44XX_GPMC_BASE;
> -		gpmc_irq = OMAP44XX_IRQ_GPMC;
>  	}
>  
>  	if (WARN_ON(!ck))
> @@ -728,53 +863,607 @@ static int __init gpmc_init(void)
>  		BUG();
>  	}
>  
> -	gpmc_base = ioremap(l, SZ_4K);
> -	if (!gpmc_base) {
> -		clk_put(gpmc_l3_clk);
> -		printk(KERN_ERR "Could not get GPMC register memory\n");
> -		BUG();
> +	clk_enable(gpmc_l3_clk);
> +
> +	return 0;
> +}
> +postcore_initcall(gpmc_init);
> +
> +static inline int gpmc_waitpin_is_reserved(struct gpmc *gpmc, unsigned waitpin)
> +{
> +	return gpmc->waitpin_map & (0x1 << waitpin);
> +}
> +
> +static inline void gpmc_reserve_waitpin(struct gpmc *gpmc, unsigned waitpin)
> +{
> +	gpmc->waitpin_map &= ~(0x1 << waitpin);
> +	gpmc->waitpin_map |= (0x1 << waitpin);
> +}
> +
> +static int gpmc_waitpin_request(struct gpmc *gpmc, unsigned waitpin)
> +{
> +	if (!(waitpin < GPMC_NR_WAITPIN))
> +		return -ENODEV;
> +
> +	if (gpmc_waitpin_is_reserved(gpmc, waitpin))
> +		return -EBUSY;
> +	else
> +		gpmc_reserve_waitpin(gpmc, waitpin);
> +
> +	return 0;
> +}
> +
> +static int gpmc_setup_waitpin(struct gpmc *gpmc, struct gpmc_device *gd)
> +{
> +	int ret;
> +
> +	if (!gd->have_waitpin)
> +		return 0;
> +
> +	ret = gpmc_waitpin_request(gpmc, gd->waitpin);
> +	if (IS_ERR_VALUE(ret)) {
> +		dev_err(gpmc->dev, "waitpin %u reserved\n", gd->waitpin);
> +		return ret;
> +	} else if (gd->waitpin_polarity) {
> +		u32 l = gpmc_read_reg(GPMC_CONFIG);
> +		u32 shift = gd->waitpin + GPMC_CONFIG_WAITPIN_POLARITY_SHIFT;
> +
> +		if (gd->waitpin_polarity == HIGH)
> +			l |= 1 << shift;
> +		else
> +			l &= ~(1 << shift);
> +
> +		gpmc_write_reg(GPMC_CONFIG, l);
>  	}
> +	return 0;
> +}
>  
> -	clk_enable(gpmc_l3_clk);
> +static int gpmc_setup_cs_waitpin(struct gpmc *gpmc, struct gpmc_device *gd,
> +						unsigned cs, unsigned conf)
> +{
> +	u32 l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> +	unsigned idx = ~0x0;
> +	int polarity = 0;
>  
> -	l = gpmc_read_reg(GPMC_REVISION);
> -	printk(KERN_INFO "GPMC revision %d.%d\n", (l >> 4) & 0x0f, l & 0x0f);
> -	/* Set smart idle mode and automatic L3 clock gating */
> -	l = gpmc_read_reg(GPMC_SYSCONFIG);
> -	l &= 0x03 << 3;
> -	l |= (0x02 << 3) | (1 << 0);
> -	gpmc_write_reg(GPMC_SYSCONFIG, l);
> -	gpmc_mem_init();
> +	switch (conf & GPMC_WAITPIN_MASK) {
> +	case GPMC_WAITPIN_0:
> +		idx =  GPMC_WAITPIN_IDX0;
> +		break;
> +	case GPMC_WAITPIN_1:
> +		idx =  GPMC_WAITPIN_IDX1;
> +		break;
> +	case GPMC_WAITPIN_2:
> +		idx =  GPMC_WAITPIN_IDX2;
> +		break;
> +	case GPMC_WAITPIN_3:
> +		idx =  GPMC_WAITPIN_IDX3;
> +		break;
> +	/* no waitpin */
> +	case 0:
> +		break;
> +	default:
> +		dev_err(gpmc->dev, "multiple waitpins selected on CS:%u\n", cs);
> +		return -EINVAL;
> +		break;
> +	}
>  
> -	/* initalize the irq_chained */
> -	irq = OMAP_GPMC_IRQ_BASE;
> -	for (cs = 0; cs < GPMC_CS_NUM; cs++) {
> -		irq_set_chip_and_handler(irq, &dummy_irq_chip,
> -						handle_simple_irq);
> -		set_irq_flags(irq, IRQF_VALID);
> -		irq++;
> +	switch (conf & GPMC_WAITPIN_POLARITY_MASK) {
> +	case GPMC_WAITPIN_ACTIVE_LOW:
> +		polarity = LOW;
> +		break;
> +	case GPMC_WAITPIN_ACTIVE_HIGH:
> +		polarity = HIGH;
> +		break;
> +	/* no waitpin */
> +	case 0:
> +		break;
> +	default:
> +		dev_err(gpmc->dev, "waitpin polarity set to low & high\n");
> +		return -EINVAL;
> +		break;
>  	}
>  
> -	ret = request_irq(gpmc_irq, gpmc_handle_irq, IRQF_SHARED, "gpmc", NULL);
> -	if (ret)
> -		pr_err("gpmc: irq-%d could not claim: err %d\n",
> -						gpmc_irq, ret);
> -	return ret;
> +	if (idx != ~0x0) {
> +		if (gd->have_waitpin) {
> +			if (gd->waitpin != idx ||
> +					gd->waitpin_polarity != polarity) {
> +				dev_err(gpmc->dev, "error: conflict: waitpin %u with polarity %d on device %s.%d\n",
> +					gd->waitpin, gd->waitpin_polarity,
> +					gd->name, gd->id);
> +				return -EBUSY;
> +			}
> +		} else {
> +			gd->have_waitpin = true;
> +			gd->waitpin = idx;
> +			gd->waitpin_polarity = polarity;
> +		}

I am not sure about the above code. What happens if a device has
multiple CS signals and uses multiple wait signals?

I am also not sure why gpmc_setup_cs_waitpin and gpmc_setup_waitpin
cannot be combined. I think that it would be a fair assumption to make
that anyone using the waitpin does so inconjunction with the CS (I think
that they would have too).

However, if there is a reason for splitting it up this way please
explain why.

> +
> +		l &= ~GPMC_CONFIG1_WAIT_PIN_SEL_MASK;
> +		l |= GPMC_CONFIG1_WAIT_PIN_SEL(idx);
> +		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
> +	} else if (polarity) {
> +		dev_err(gpmc->dev, "error: waitpin polarity specified with out wait pin number on device %s.%d\n",
> +							gd->name, gd->id);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void gpmc_setup_cs_config(struct gpmc *gpmc, unsigned cs, unsigned conf)
> +{
> +	u32 l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> +
> +	l &= ~(GPMC_CONFIG1_TIMEPARAGRANULARITY |
> +		GPMC_CONFIG1_MUXADDDATA |
> +		GPMC_CONFIG1_DEVICETYPE(~0) |
> +		GPMC_CONFIG1_DEVICESIZE(~0) |
> +		GPMC_CONFIG1_WAIT_WRITE_MON |
> +		GPMC_CONFIG1_WAIT_READ_MON |
> +		GPMC_CONFIG1_PAGE_LEN(~0) |
> +		GPMC_CONFIG1_WRITETYPE_SYNC |
> +		GPMC_CONFIG1_WRITEMULTIPLE |
> +		GPMC_CONFIG1_READTYPE_SYNC |
> +		GPMC_CONFIG1_READMULTIPLE |
> +		GPMC_CONFIG1_WRAPBURST |
> +		GPMC_CONFIG1_WAITPIN_MONITOR_TIME(~0) |
> +		GPMC_CONFIG1_CLOCKACTIVATION_TIME(~0));
> +
> +	if (conf & GPMC_TIMEPARAGRANULARITY)
> +		l |= GPMC_CONFIG1_TIMEPARAGRANULARITY;
> +	if (conf & GPMC_MUXADDDATA)
> +		l |= GPMC_CONFIG1_MUXADDDATA;
> +	if (conf & GPMC_DEVICETYPE_NAND)
> +		l |= GPMC_CONFIG1_DEVICETYPE_NAND;
> +	if (conf & GPMC_DEVICESIZE_16)
> +		l |= GPMC_CONFIG1_DEVICESIZE_16;
> +	if (conf & GPMC_WAIT_WRITE_MON)
> +		l |= GPMC_CONFIG1_WAIT_WRITE_MON;
> +	if (conf & GPMC_WAIT_READ_MON)
> +		l |= GPMC_CONFIG1_WAIT_READ_MON;
> +	if (conf & GPMC_PAGE_LEN_16)
> +		l |= GPMC_CONFIG1_PAGE_LEN_16;
> +	else if (conf & GPMC_PAGE_LEN_8)
> +		l |= GPMC_CONFIG1_PAGE_LEN_8;
> +	if (conf & GPMC_WRITETYPE_SYNC)
> +		l |= GPMC_CONFIG1_WRITETYPE_SYNC;
> +	if (conf & GPMC_WRITEMULTIPLE)
> +		l |= GPMC_CONFIG1_WRITEMULTIPLE;
> +	if (conf & GPMC_READTYPE_SYNC)
> +		l |= GPMC_CONFIG1_READTYPE_SYNC;
> +	if (conf & GPMC_READMULTIPLE)
> +		l |= GPMC_CONFIG1_READMULTIPLE;
> +	if (conf & GPMC_WRAPBURST)
> +		l |= GPMC_CONFIG1_WRAPBURST;
> +	if (conf & GPMC_WAITPIN_MONITOR_TIME_1)
> +		l |= GPMC_CONFIG1_WAITPIN_MONITOR_TIME_1;
> +	else if (conf & GPMC_WAITPIN_MONITOR_TIME_2)
> +		l |= GPMC_CONFIG1_WAITPIN_MONITOR_TIME_2;
> +	if (conf & GPMC_CLOCKACTIVATION_TIME_1)
> +		l |= GPMC_CONFIG1_CLOCKACTIVATION_TIME_1;
> +	else if (conf & GPMC_CLOCKACTIVATION_TIME_2)
> +		l |= GPMC_CONFIG1_CLOCKACTIVATION_TIME_2;
> +
> +	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
> +
> +	if (conf & GPMC_WRITEPROTECT)
> +		gpmc->wp = true;
> +}
> +
> +static int gpmc_setup_cs_nonres(struct gpmc *gpmc,
> +			struct gpmc_device *gd, struct gpmc_cs_data *cs)

The name of this function is not 100% clear to me. What do you mean by
"nonres"?

> +{
> +	int ret;
> +
> +	/* some boards rely on bootloader for configuration */
> +	if (cs->have_config) {
> +		gpmc_setup_cs_config(gpmc, cs->cs, cs->config);
> +		ret = gpmc_setup_cs_waitpin(gpmc, gd, cs->cs, cs->config);
> +		if (IS_ERR_VALUE(ret)) {
> +			dev_err(gpmc->dev, "error: waitpin on CS %d\n", cs->cs);
> +			return ret;
> +		}
> +	} else
> +		dev_warn(gpmc->dev, "config not present for CS: %d\n", cs->cs);
> +
> +	if (cs->timing) {
> +		ret = gpmc_cs_set_timings(cs->cs, cs->timing);
> +		if (IS_ERR_VALUE(ret)) {
> +			dev_err(gpmc->dev, "error: timing on CS: %d\n", cs->cs);
> +			return ret;
> +		}
> +	} else
> +		dev_warn(gpmc->dev, "timing not present for CS: %u\n", cs->cs);
> +
> +	return 0;
> +}
> +
> +static int gpmc_match_device(struct gpmc *gpmc, char *name, int id)
> +{
> +	int i;
> +	struct gpmc_device *gd;
> +
> +	for (i = 0, gd = gpmc->device; i < gpmc->num_device; i++, gd++)
> +		if (!strcmp(gd->name, name) && gd->id == id)
> +			return i;
> +
> +	return -ENOENT;
> +}
> +
> +int gpmc_cs_reconfigure(char *name, int id, struct gpmc_cs_data *cs)

What scenario is this function used in? May be worth adding a comment
about the function.

> +{
> +	int i;
> +
> +	i = gpmc_match_device(gpmc, name, id);
> +	if (IS_ERR_VALUE(i)) {
> +		dev_err(gpmc->dev, "no device %s.%d to configure\n", name, id);
> +		return i;
> +	}
> +
> +	i = gpmc_setup_cs_nonres(gpmc, gpmc->device + i, cs);
> +	if (IS_ERR_VALUE(i)) {
> +		dev_err(gpmc->dev, "error: configure device %s.%d\n", name, id);
> +		return i;
> +	}
> +
> +	return gpmc_setup_waitpin(gpmc, gpmc->device + i);
> +
> +}
> +EXPORT_SYMBOL_GPL(gpmc_cs_reconfigure);
> +
> +static inline unsigned int gpmc_bit_to_irq(unsigned bitmask)
> +{
> +	if (bitmask & GPMC_IRQ_BIT_FIFOEVENT)
> +		return GPMC_IRQ_FIFOEVENTENABLE;
> +	else if (bitmask & GPMC_IRQ_BIT_TERMINALCOUNT)
> +		return GPMC_IRQ_COUNT_EVENT;
> +	else
> +		return 0;
> +}
> +
> +static __devinit int gpmc_setup_cs_irq(struct gpmc *gpmc,
> +			struct gpmc_cs_data *cs, struct resource *res)
> +{
> +	int i, n;
> +
> +	for (i = 0, n = 0; i < GPMC_NR_IRQ; i++)
> +		if (gpmc_bit_to_irq(gpmc->irq[i].bitmask) & cs->irq_config) {
> +			res[n].start = res[n].end = gpmc->irq[i].irq;
> +			res[n].flags = IORESOURCE_IRQ;
> +			dev_info(gpmc->dev, "irq %u on CS %d\n",
> +						res[n].start, cs->cs);
> +			n++;
> +		}
> +
> +	return n;
> +}
> +
> +static __devinit int gpmc_setup_cs_mem(struct gpmc *gpmc,
> +			struct gpmc_cs_data *cs, struct resource *res)
> +{
> +	unsigned long start;
> +	int ret;
> +
> +	ret = gpmc_cs_request(cs->cs, cs->mem_size, &start);
> +	if (IS_ERR_VALUE(ret)) {
> +		dev_err(gpmc->dev, "error: gpmc request on CS: %d\n", cs->cs);
> +		return ret;
> +	}
> +
> +	res->start = start + cs->mem_offset;
> +	res->end = res->start + cs->mem_size - 1;
> +	res->flags = IORESOURCE_MEM;
> +
> +	dev_info(gpmc->dev, "memory 0x%x-0x%x on CS %d\n", res->start,
> +							res->end, cs->cs);
> +
> +	return 1;
> +}
> +
> +static __devinit int gpmc_setup_cs(struct gpmc *gpmc, struct gpmc_device *gd,
> +			struct gpmc_cs_data *cs, struct resource *res)
> +{
> +	int num, ret;
> +
> +	num = gpmc_setup_cs_mem(gpmc, cs, res);
> +	if (IS_ERR_VALUE(num))
> +		return num;
> +
> +	ret = gpmc_setup_cs_nonres(gpmc, gd, cs);
> +	if (IS_ERR_VALUE(ret))
> +		return ret;
> +
> +	num += gpmc_setup_cs_irq(gpmc, cs, res + num);
> +
> +	return num;
> +}
> +
> +static __devinit int gpmc_setup_device(struct gpmc *gpmc,
> +		struct gpmc_device *gd, struct gpmc_device_pdata *gdp)
> +{
> +	int i, n, ret;
> +	struct gpmc_cs_data *cs;
> +
> +	for (i = 0, n = gdp->num_cs, cs = gdp->cs_data;
> +				i < gdp->num_cs; i++, cs++)
> +		n += hweight32(cs->irq_config);

As you know I am not a fan of these overloaded for-loops as I find them
hard to read ;-)

Why not ...

n = gdp->num_cs;
cs = gdp->cs_data;

/* Calculate total number of irqs used by device */	
for (i = 0, i < gdp->num_cs; i++)
	n += hweight32(cs[i].irq_flags & GPMC_IRQ_MASK);

> +	gd->gpmc_res = devm_kzalloc(gpmc->dev, sizeof(*gd->gpmc_res) * n,
> +								GFP_KERNEL);
> +	if (gd->gpmc_res == NULL) {
> +		dev_err(gpmc->dev, "error: memory allocation\n");
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0, cs = gdp->cs_data, gd->gpmc_res_cnt = 0;
> +			i < gdp->num_cs; cs++, i++) {

By the way, if you make the above change, you can also remove "cs =
gdp->cs_data" from this for-loop as it is already initialised.

> +		ret = gpmc_setup_cs(gpmc, gd, cs,
> +					gd->gpmc_res + gd->gpmc_res_cnt);
> +		if (IS_ERR_VALUE(ret) ||
> +				IS_ERR_VALUE(gpmc_setup_waitpin(gpmc, gd))) {

May be consider moving gpmc_setup_waitpin into gpmc_setup_cs as it makes
sense that it is part of the cs setup.

> +			dev_err(gpmc->dev, "error: setup for %s\n", gdp->name);
> +			devm_kfree(gpmc->dev, gd->gpmc_res);
> +			gd->gpmc_res = NULL;
> +			gd->gpmc_res_cnt = 0;
> +			return -EINVAL;
> +		} else
> +			gd->gpmc_res_cnt += ret;
> +	}
> +
> +	gd->name = gdp->name;
> +	gd->id = gdp->id;
> +	gd->pdata = gdp->pdata;
> +	gd->pdata_size = gdp->pdata_size;
> +	gd->per_res = gdp->per_res;
> +	gd->per_res_cnt = gdp->per_res_cnt;
> +
> +	return 0;
> +}
> +
> +static __devinit
> +struct platform_device *gpmc_create_device(struct gpmc *gpmc,
> +					struct gpmc_device *p)
> +{
> +	int num = p->per_res_cnt + p->gpmc_res_cnt;
> +	struct resource *res;
> +	struct platform_device *pdev;
> +
> +	res = devm_kzalloc(gpmc->dev, sizeof(struct resource) * num,
> +								GFP_KERNEL);
> +	if (!res) {
> +		dev_err(gpmc->dev, "error: allocating memory\n");
> +		return NULL;
> +	}
> +
> +	memcpy((char *)res, (const char *) p->gpmc_res,
> +				sizeof(struct resource) * p->gpmc_res_cnt);
> +	memcpy((char *)(res + p->gpmc_res_cnt), (const char *)p->per_res,
> +				sizeof(struct resource) * p->per_res_cnt);
> +
> +	pdev = platform_device_register_resndata(gpmc->dev, p->name, p->id,
> +					res, num, p->pdata, p->pdata_size);
> +
> +	devm_kfree(gpmc->dev, res);
> +
> +	return pdev;
>  }
> -postcore_initcall(gpmc_init);
>  
>  static irqreturn_t gpmc_handle_irq(int irq, void *dev)
>  {
> -	u8 cs;
> +	int i;
> +	u32 regval;
> +	struct gpmc *gpmc = dev;
> +
> +	regval = gpmc_read_reg(GPMC_IRQSTATUS);
>  
> -	/* check cs to invoke the irq */
> -	cs = ((gpmc_read_reg(GPMC_PREFETCH_CONFIG1)) >> CS_NUM_SHIFT) & 0x7;
> -	if (OMAP_GPMC_IRQ_BASE+cs <= OMAP_GPMC_IRQ_END)
> -		generic_handle_irq(OMAP_GPMC_IRQ_BASE+cs);
> +	for (i = 0; i < GPMC_NR_IRQ; i++)
> +		if (regval & gpmc->irq[i].bitmask)
> +			generic_handle_irq(gpmc->irq[i].irq);
> +
> +	gpmc_write_reg(GPMC_IRQSTATUS, regval);
>  
>  	return IRQ_HANDLED;
>  }
>  
> +static int gpmc_irq_endis(struct gpmc *gpmc, unsigned irq, bool endis)
> +{
> +	int i;
> +	u32 regval;
> +
> +	for (i = 0; i < GPMC_NR_IRQ; i++)
> +		if (irq == gpmc->irq[i].irq) {
> +			regval = gpmc_read_reg(GPMC_IRQENABLE);
> +			if (endis)
> +				regval |= gpmc->irq[i].bitmask;
> +			else
> +				regval &= ~gpmc->irq[i].bitmask;
> +			gpmc_write_reg(GPMC_IRQENABLE, regval);
> +			break;
> +		}
> +
> +	return 0;
> +}
> +
> +static void gpmc_irq_disable(struct irq_data *p)
> +{
> +	gpmc_irq_endis(irq_data_get_irq_chip_data(p), p->irq, false);
> +}
> +
> +static void gpmc_irq_enable(struct irq_data *p)
> +{
> +	gpmc_irq_endis(irq_data_get_irq_chip_data(p), p->irq, true);
> +}
> +
> +static void gpmc_irq_noop(struct irq_data *data) { }
> +
> +static unsigned int gpmc_irq_noop_ret(struct irq_data *data) { return 0; }
> +
> +static __devinit int gpmc_setup_irq(struct gpmc *gpmc)
> +{
> +	int i;
> +	u32 regval;
> +
> +	if (!gpmc->master_irq)
> +		return -EINVAL;
> +
> +	gpmc->irq_start = irq_alloc_descs(-1, 0, GPMC_NR_IRQ, 0);
> +	if (IS_ERR_VALUE(gpmc->irq_start)) {
> +		dev_err(gpmc->dev, "irq_alloc_descs failed\n");
> +		return gpmc->irq_start;
> +	}
> +
> +	gpmc->irq_chip.name = "gpmc";
> +	gpmc->irq_chip.irq_startup = gpmc_irq_noop_ret;
> +	gpmc->irq_chip.irq_enable = gpmc_irq_enable;
> +	gpmc->irq_chip.irq_disable = gpmc_irq_disable;
> +	gpmc->irq_chip.irq_shutdown = gpmc_irq_noop;
> +	gpmc->irq_chip.irq_ack = gpmc_irq_noop;
> +	gpmc->irq_chip.irq_mask = gpmc_irq_noop;
> +	gpmc->irq_chip.irq_unmask = gpmc_irq_noop;
> +
> +	gpmc->irq[0].bitmask = GPMC_IRQ_BIT_FIFOEVENT;
> +	gpmc->irq[1].bitmask = GPMC_IRQ_BIT_TERMINALCOUNT;
> +
> +	for (i = 0; i < GPMC_NR_IRQ; i++) {
> +		gpmc->irq[i].irq = gpmc->irq_start + i;
> +		irq_set_chip_and_handler(gpmc->irq[i].irq,
> +					&gpmc->irq_chip, handle_simple_irq);
> +		irq_set_chip_data(gpmc->irq[i].irq, gpmc);
> +		set_irq_flags(gpmc->irq[i].irq, IRQF_VALID | IRQF_NOAUTOEN);
> +	}
> +
> +	/* Disable interrupts */
> +	gpmc_write_reg(GPMC_IRQENABLE, 0);
> +
> +	/* clear interrupts */
> +	regval = gpmc_read_reg(GPMC_IRQSTATUS);
> +	gpmc_write_reg(GPMC_IRQSTATUS, regval);
> +
> +	return request_irq(gpmc->master_irq, gpmc_handle_irq, IRQF_SHARED,
> +							"gpmc", gpmc);
> +}
> +
> +static __devinit void gpmc_setup_writeprotect(struct gpmc *gpmc)
> +{
> +	u32 l;
> +
> +	l = gpmc_read_reg(GPMC_CONFIG);
> +	if (gpmc->wp == true) {
> +		l &= ~GPMC_CONFIG_WRITEPROTECT;
> +		dev_info(gpmc->dev, "write protect enabled\n");
> +	} else
> +		l |= GPMC_CONFIG_WRITEPROTECT;
> +	gpmc_write_reg(GPMC_CONFIG, l);
> +}
> +
> +static __devinit int gpmc_probe(struct platform_device *pdev)
> +{
> +	u32 l;
> +	int i;
> +	int ret = 0;
> +	struct resource *res = NULL;
> +	struct gpmc_pdata *gp = dev_get_platdata(&pdev->dev);
> +	struct gpmc_device_pdata **gdq = NULL;
> +	struct gpmc_device *gd = NULL;
> +
> +	gpmc = devm_kzalloc(&pdev->dev, sizeof(struct gpmc), GFP_KERNEL);
> +	if (gpmc == NULL)
> +		return -ENOMEM;
> +
> +	gpmc->dev = &pdev->dev;
> +	gpmc->clk_prd = gp->clk_prd;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (res == NULL)
> +		return -ENOENT;
> +
> +	gpmc->phys_base = res->start;
> +	gpmc->memsize = resource_size(res);
> +
> +	gpmc->io_base = devm_request_and_ioremap(gpmc->dev, res);
> +	if (!gpmc->io_base)
> +		return -EADDRNOTAVAIL;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (res == NULL)
> +		dev_warn(gpmc->dev, "Failed to get resource: irq\n");
> +	else
> +		gpmc->master_irq = res->start;
> +
> +	if (IS_ERR_VALUE(gpmc_setup_irq(gpmc)))
> +		dev_warn(gpmc->dev, "gpmc_setup_irq failed\n");
> +
> +	gpmc->ecc_used = -EINVAL;
> +	spin_lock_init(&gpmc->mem_lock);
> +	platform_set_drvdata(pdev, gpmc);
> +
> +	l = gpmc_read_reg(GPMC_REVISION);
> +	gpmc->revision = (l >> 4) & 0xf;
> +	dev_info(gpmc->dev, "GPMC revision %u.%u\n", gpmc->revision, l & 0x0f);
> +
> +	gpmc_mem_init();
> +
> +	for (i = 0, gdq = gp->device_pdata, gd = gpmc->device;
> +			(i < gp->num_device) && (*gdq); i++, gdq++) {

You have num_devices now and so do you really need the "&& (*gdq)"?
Seems redundant.

> +		ret = gpmc_setup_device(gpmc, gd, *gdq);

gdp[i]

> +		if (IS_ERR_VALUE(ret))
> +			dev_err(gpmc->dev, "gpmc setup on %s failed\n",
> +								(*gdq)->name);
> +		else
> +			gd++;
> +	}
> +	gpmc->num_device = gd - gpmc->device;
> +
> +	gpmc_setup_writeprotect(gpmc);

Write protect is a pin and there is only one. Like the waitpins and CS
signals this needs to be associated with a device. It would make sense
that this is associated with the cs data.

> +
> +	for (i = 0, gd = gpmc->device; i < gpmc->num_device; i++, gd++)
> +		if (IS_ERR(gpmc_create_device(gpmc, gd)))
> +			dev_err(gpmc->dev, "device creation on %s failed\n",
> +								gd->name);
> +
> +	return 0;
> +}
> +
> +static __devexit int gpmc_free_irq(struct gpmc *gpmc)
> +{
> +	int i;
> +
> +	if (gpmc->master_irq)
> +		free_irq(gpmc->master_irq, gpmc);
> +
> +	for (i = 0; i < GPMC_NR_IRQ; i++) {
> +		irq_set_handler(gpmc->irq[i].irq, NULL);
> +		irq_set_chip(gpmc->irq[i].irq, &no_irq_chip);
> +		irq_set_chip_data(gpmc->irq[i].irq, NULL);
> +		irq_modify_status(gpmc->irq[i].irq, 0, 0);
> +	}
> +
> +	irq_free_descs(gpmc->irq_start, GPMC_NR_IRQ);
> +
> +	return 0;
> +}
> +
> +static __devexit int gpmc_remove(struct platform_device *pdev)
> +{
> +	struct gpmc *gpmc = platform_get_drvdata(pdev);
> +
> +	platform_set_drvdata(pdev, NULL);
> +	gpmc_free_irq(gpmc);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver gpmc_driver = {
> +	.probe		= gpmc_probe,
> +	.remove		= __devexit_p(gpmc_remove),
> +	.driver		= {
> +		.name	= DRIVER_NAME,
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +module_platform_driver(gpmc_driver);
> +
>  #ifdef CONFIG_ARCH_OMAP3
>  static struct omap3_gpmc_regs gpmc_context;
>  
> @@ -854,10 +1543,10 @@ int gpmc_enable_hwecc(int cs, int mode, int dev_width, int ecc_size)
>  	unsigned int val;
>  
>  	/* check if ecc module is in used */
> -	if (gpmc_ecc_used != -EINVAL)
> +	if (gpmc->ecc_used != -EINVAL)
>  		return -EINVAL;
>  
> -	gpmc_ecc_used = cs;
> +	gpmc->ecc_used = cs;
>  
>  	/* clear ecc and enable bits */
>  	val = ((0x00000001<<8) | 0x00000001);
> @@ -905,7 +1594,7 @@ int gpmc_calculate_ecc(int cs, const u_char *dat, u_char *ecc_code)
>  {
>  	unsigned int val = 0x0;
>  
> -	if (gpmc_ecc_used != cs)
> +	if (gpmc->ecc_used != cs)
>  		return -EINVAL;
>  
>  	/* read ecc result */
> @@ -915,7 +1604,7 @@ int gpmc_calculate_ecc(int cs, const u_char *dat, u_char *ecc_code)
>  	/* P2048o, P1024o, P512o, P256o, P2048e, P1024e, P512e, P256e */
>  	*ecc_code++ = ((val >> 8) & 0x0f) | ((val >> 20) & 0xf0);
>  
> -	gpmc_ecc_used = -EINVAL;
> +	gpmc->ecc_used = -EINVAL;
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(gpmc_calculate_ecc);
> diff --git a/arch/arm/plat-omap/include/plat/gpmc.h b/arch/arm/plat-omap/include/plat/gpmc.h
> index 1527929..2eedd99 100644
> --- a/arch/arm/plat-omap/include/plat/gpmc.h
> +++ b/arch/arm/plat-omap/include/plat/gpmc.h
> @@ -11,6 +11,44 @@
>  #ifndef __OMAP2_GPMC_H
>  #define __OMAP2_GPMC_H
>  
> +/* configuration flags */
> +#define	GPMC_WRITEPROTECT		BIT(0)
> +#define	GPMC_TIMEPARAGRANULARITY	BIT(1)
> +#define	GPMC_MUXADDDATA			BIT(2)
> +#define	GPMC_DEVICETYPE_NOR		BIT(3)
> +#define	GPMC_DEVICETYPE_NAND		BIT(4)
> +#define	GPMC_DEVICESIZE_8		BIT(5)
> +#define	GPMC_DEVICESIZE_16		BIT(6)
> +#define	GPMC_WAIT_WRITE_MON		BIT(7)
> +#define	GPMC_WAIT_READ_MON		BIT(8)
> +#define	GPMC_PAGE_LEN_4			BIT(9)
> +#define	GPMC_PAGE_LEN_8			BIT(10)
> +#define	GPMC_PAGE_LEN_16		BIT(11)
> +#define	GPMC_CLOCKACTIVATIONTIME_0	BIT(12)
> +#define	GPMC_CLOCKACTIVATIONTIME_1	BIT(13)
> +#define	GPMC_CLOCKACTIVATIONTIME_2	BIT(14)
> +#define	GPMC_WRITETYPE_SYNC		BIT(15)
> +#define	GPMC_WRITEMULTIPLE		BIT(16)
> +#define	GPMC_READTYPE_SYNC		BIT(17)
> +#define	GPMC_READMULTIPLE		BIT(18)
> +#define	GPMC_WRAPBURST			BIT(19)
> +#define	GPMC_WAITPIN_0			BIT(20)
> +#define	GPMC_WAITPIN_1			BIT(21)
> +#define	GPMC_WAITPIN_2			BIT(22)
> +#define	GPMC_WAITPIN_3			BIT(23)
> +#define	GPMC_WAITPIN_MASK		(GPMC_WAITPIN_0 | GPMC_WAITPIN_1 | \
> +					GPMC_WAITPIN_2 | GPMC_WAITPIN_3)
> +#define	GPMC_WAITPIN_ACTIVE_LOW		BIT(24)
> +#define	GPMC_WAITPIN_ACTIVE_HIGH	BIT(25)
> +#define	GPMC_WAITPIN_POLARITY_MASK	(GPMC_WAITPIN_ACTIVE_LOW | \
> +					GPMC_WAITPIN_ACTIVE_HIGH)
> +#define	GPMC_WAITPIN_MONITOR_TIME_1	BIT(26)
> +#define	GPMC_WAITPIN_MONITOR_TIME_2	BIT(27)
> +#define	GPMC_CLOCKACTIVATION_TIME_1	BIT(28)
> +#define	GPMC_CLOCKACTIVATION_TIME_2	BIT(29)
> +#define	GPMC_READTYPE_ASYNC		BIT(30)
> +#define	GPMC_WRITETYPE_ASYNC		BIT(31)
> +

Please keep in mind that eventually this has to all be moved into device
tree and so at that point these configuration flags will have to be
re-worked. However, just a FYI.

Cheers
Jon

Hi Jon,

On Tue, May 01, 2012 at 23:23:00, Hunter, Jon wrote:
> Hi Afzal,
> 
> Looks good! Some minor comments ...

Thanks

> > +#define	GPMC_WAITPIN_IDX0			0x0
> > +#define	GPMC_WAITPIN_IDX1			0x1
> > +#define	GPMC_WAITPIN_IDX2			0x2
> > +#define	GPMC_WAITPIN_IDX3			0x3
> > +#define	GPMC_NR_WAITPIN				0x4
> 
> How about an enum here? Also OMAP4 only have 3 wait pins and so the
> number is device dependent.

Ok

> 
> >  #define GPMC_MEM_START		0x00000000
> >  #define GPMC_MEM_END		0x3FFFFFFF
> >  #define BOOT_ROM_SPACE		0x100000	/* 1MB */
> > @@ -64,6 +106,55 @@
> >  #define ENABLE_PREFETCH		(0x1 << 7)
> >  #define DMA_MPU_MODE		2
> >  
> > +#define	DRIVER_NAME	"omap-gpmc"
> > +
> > +#define	GPMC_NR_IRQ		2
> 
> Why has this been changed to 2? It was 6 in the previous version. As we
> discussed before the number of IRQs should be detected based upon GPMC
> IP version.

As mentioned in the cover letter,

"Additional features that currently boards in mainline does not make
use of like, waitpin interrupt handling, changes to leverage revision
6 IP differences has not been incorporated."

Priority in this series is to convert into a driver, get all boards working
on mainline. Once all boards are working with gpmc driver, these features
which are not required for currently supported boards can be added later.

> > +struct gpmc_device {
> > +	char			*name;
> > +	int			id;
> > +	void			*pdata;
> > +	unsigned		pdata_size;
> > +	struct resource		*per_res;
> > +	unsigned		per_res_cnt;
> > +	struct resource		*gpmc_res;
> > +	unsigned		gpmc_res_cnt;
> > +	bool			have_waitpin;
> > +	unsigned		waitpin;
> > +	int			waitpin_polarity;
> 
> I think that it make more sense to have the wait-pin information part of
> the gpmc_cs_data structure for the following reasons ...

Waitpin information is indeed a part of cs as far as boards are concerned,
it is only that it gets propogated to device

> 
> 1. If a device can use multiple CS, then it could use multiple wait signals.
> 2. Eventually, all board information needs to move to device tree. By
> having it in the pdata it will be easier to migrate to device tree.
> 
> It may be nice to have "have_waitpin" be an integer too, that indicates
> if wait is used for both read and write or just one or the other.
> 
> Also, any reason why waitpin_polarity is an int? I see you define LOW as
> -1, but I not sure why LOW cannot be 0 as 0 is programmed into the
> register for an active low wait signal.

Only intention is not to alter default waitpin polarity value, i.e. if
any board does make use of it w/o knowledge of Kernel, I don't want to
break it, there may be an argument saying that the board code is buggy,
while if some board does so, it is, but don't want to break working one.

Here unless user explicitly set the flag, it does nothing on polarity

> > +	if (idx != ~0x0) {
> > +		if (gd->have_waitpin) {
> > +			if (gd->waitpin != idx ||
> > +					gd->waitpin_polarity != polarity) {
> > +				dev_err(gpmc->dev, "error: conflict: waitpin %u with polarity %d on device %s.%d\n",
> > +					gd->waitpin, gd->waitpin_polarity,
> > +					gd->name, gd->id);
> > +				return -EBUSY;
> > +			}
> > +		} else {
> > +			gd->have_waitpin = true;
> > +			gd->waitpin = idx;
> > +			gd->waitpin_polarity = polarity;
> > +		}
> 
> I am not sure about the above code. What happens if a device has
> multiple CS signals and uses multiple wait signals?
> 
> I am also not sure why gpmc_setup_cs_waitpin and gpmc_setup_waitpin
> cannot be combined. I think that it would be a fair assumption to make
> that anyone using the waitpin does so inconjunction with the CS (I think
> that they would have too).
> 
> However, if there is a reason for splitting it up this way please
> explain why.

Initially it was done per CS, the problem happened while trying to
convert tusb6010. It had multiple CS, but using same waitpin. Problem
with incorporating this onto CS is we have to sacrifice on wait pin
conflict prevention capability. The code now handles case of wait pin
conflict per device, the code can be extended to have multiple
wait pin per device also. But I prefer to defer it to later, not as
part of this series, as this feature what you asking for is not required
for any of the existing boards. I can add it in this series if there is
a strong opinion in the community for the same in this series.

Policy that is being followed here is first sit, then straighten legs, ;)

> > +static int gpmc_setup_cs_nonres(struct gpmc *gpmc,
> > +			struct gpmc_device *gd, struct gpmc_cs_data *cs)
> 
> The name of this function is not 100% clear to me. What do you mean by
> "nonres"?

It means anything other than resources like memory & irq, happened for
want of better name, if you have one, name it, will put it.

> > +int gpmc_cs_reconfigure(char *name, int id, struct gpmc_cs_data *cs)
> 
> What scenario is this function used in? May be worth adding a comment
> about the function.

Ok, it was required for OneNAND, as it needs to reconfigure

> > +	for (i = 0, n = gdp->num_cs, cs = gdp->cs_data;
> > +				i < gdp->num_cs; i++, cs++)
> > +		n += hweight32(cs->irq_config);
> 
> As you know I am not a fan of these overloaded for-loops as I find them
> hard to read ;-)
> 
> Why not ...
> 
> n = gdp->num_cs;
> cs = gdp->cs_data;


Well, you also know that I am big fan of it; difference of opinion,
my preference is to keep loop control statements together with "for",
unless there is a cry from community that it should not be this way.
I believe that it is not that much unreadable.

> > +	for (i = 0, gdq = gp->device_pdata, gd = gpmc->device;
> > +			(i < gp->num_device) && (*gdq); i++, gdq++) {
> 
> You have num_devices now and so do you really need the "&& (*gdq)"?
> Seems redundant.

num_device is something that I never wanted to add, was happy with,
NULL entry termination, only because of your repeated comments,
I added this. And this additional check now prevents NULL dereference.

> > +	gpmc_setup_writeprotect(gpmc);
> 
> Write protect is a pin and there is only one. Like the waitpins and CS
> signals this needs to be associated with a device. It would make sense
> that this is associated with the cs data.

As far as platform are concerned, it is associated with cs, it is only
that while configuring CS, it is propagated such that it is done once.


> > +#define	GPMC_READTYPE_ASYNC		BIT(30)
> > +#define	GPMC_WRITETYPE_ASYNC		BIT(31)
> > +
> 
> Please keep in mind that eventually this has to all be moved into device
> tree and so at that point these configuration flags will have to be
> re-worked. However, just a FYI.

On device tree matters, my mind is next to blank, thanks for mentioning it.

Regards
Afzal

Hi Afzal,

On 05/03/2012 03:23 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Tue, May 01, 2012 at 23:23:00, Hunter, Jon wrote:
>> Hi Afzal,
>>
>> Looks good! Some minor comments ...
> 
> Thanks
> 
>>> +#define	GPMC_WAITPIN_IDX0			0x0
>>> +#define	GPMC_WAITPIN_IDX1			0x1
>>> +#define	GPMC_WAITPIN_IDX2			0x2
>>> +#define	GPMC_WAITPIN_IDX3			0x3
>>> +#define	GPMC_NR_WAITPIN				0x4
>>
>> How about an enum here? Also OMAP4 only have 3 wait pins and so the
>> number is device dependent.
> 
> Ok
> 
>>
>>>  #define GPMC_MEM_START		0x00000000
>>>  #define GPMC_MEM_END		0x3FFFFFFF
>>>  #define BOOT_ROM_SPACE		0x100000	/* 1MB */
>>> @@ -64,6 +106,55 @@
>>>  #define ENABLE_PREFETCH		(0x1 << 7)
>>>  #define DMA_MPU_MODE		2
>>>  
>>> +#define	DRIVER_NAME	"omap-gpmc"
>>> +
>>> +#define	GPMC_NR_IRQ		2
>>
>> Why has this been changed to 2? It was 6 in the previous version. As we
>> discussed before the number of IRQs should be detected based upon GPMC
>> IP version.
> 
> As mentioned in the cover letter,
> 
> "Additional features that currently boards in mainline does not make
> use of like, waitpin interrupt handling, changes to leverage revision
> 6 IP differences has not been incorporated."
> 
> Priority in this series is to convert into a driver, get all boards working
> on mainline. Once all boards are working with gpmc driver, these features
> which are not required for currently supported boards can be added later.

Yes, but I meant why 2 and not say 5? Anyway, I think that this should
be marked with a comment like a TODO so it is clear that this needs to
be re-visited.

>>> +struct gpmc_device {
>>> +	char			*name;
>>> +	int			id;
>>> +	void			*pdata;
>>> +	unsigned		pdata_size;
>>> +	struct resource		*per_res;
>>> +	unsigned		per_res_cnt;
>>> +	struct resource		*gpmc_res;
>>> +	unsigned		gpmc_res_cnt;
>>> +	bool			have_waitpin;
>>> +	unsigned		waitpin;
>>> +	int			waitpin_polarity;
>>
>> I think that it make more sense to have the wait-pin information part of
>> the gpmc_cs_data structure for the following reasons ...
> 
> Waitpin information is indeed a part of cs as far as boards are concerned,
> it is only that it gets propogated to device

Why does the device's driver care? From my point of view, the wait-pin
is just part of the interface setup/configuration. The external device
may require this and assumes that this has been setup along with the CS
and interface timing, but the device's driver should not care. Remember
this is just a ready signal used to stall the access. Once configured,
software should be unaware of it.

>> 1. If a device can use multiple CS, then it could use multiple wait signals.
>> 2. Eventually, all board information needs to move to device tree. By
>> having it in the pdata it will be easier to migrate to device tree.
>>
>> It may be nice to have "have_waitpin" be an integer too, that indicates
>> if wait is used for both read and write or just one or the other.
>>
>> Also, any reason why waitpin_polarity is an int? I see you define LOW as
>> -1, but I not sure why LOW cannot be 0 as 0 is programmed into the
>> register for an active low wait signal.
> 
> Only intention is not to alter default waitpin polarity value, i.e. if
> any board does make use of it w/o knowledge of Kernel, I don't want to
> break it, there may be an argument saying that the board code is buggy,
> while if some board does so, it is, but don't want to break working one.
> 
> Here unless user explicitly set the flag, it does nothing on polarity

Ok. Do such scenario's exist today? Please note that board code will be
removed in the future and device-tree will replace. So if there are no
cases today, I would not be concerned. Unless this could be something
that has already been configured by the bootloader. However, in that
case would be even call this function?

>>> +	if (idx != ~0x0) {
>>> +		if (gd->have_waitpin) {
>>> +			if (gd->waitpin != idx ||
>>> +					gd->waitpin_polarity != polarity) {
>>> +				dev_err(gpmc->dev, "error: conflict: waitpin %u with polarity %d on device %s.%d\n",
>>> +					gd->waitpin, gd->waitpin_polarity,
>>> +					gd->name, gd->id);
>>> +				return -EBUSY;
>>> +			}
>>> +		} else {
>>> +			gd->have_waitpin = true;
>>> +			gd->waitpin = idx;
>>> +			gd->waitpin_polarity = polarity;
>>> +		}
>>
>> I am not sure about the above code. What happens if a device has
>> multiple CS signals and uses multiple wait signals?
>>
>> I am also not sure why gpmc_setup_cs_waitpin and gpmc_setup_waitpin
>> cannot be combined. I think that it would be a fair assumption to make
>> that anyone using the waitpin does so inconjunction with the CS (I think
>> that they would have too).
>>
>> However, if there is a reason for splitting it up this way please
>> explain why.
> 
> Initially it was done per CS, the problem happened while trying to
> convert tusb6010. It had multiple CS, but using same waitpin. Problem
> with incorporating this onto CS is we have to sacrifice on wait pin
> conflict prevention capability. The code now handles case of wait pin
> conflict per device, the code can be extended to have multiple
> wait pin per device also. But I prefer to defer it to later, not as
> part of this series, as this feature what you asking for is not required
> for any of the existing boards. I can add it in this series if there is
> a strong opinion in the community for the same in this series.

Ok, yes this does make sense. I wonder if we can clean up the all the
nested if-statements in gpmc_setup_cs_waitpin(). Seems there could be
some redundancy.

> Policy that is being followed here is first sit, then straighten legs, ;)

Ha! Crawl before walking ;-)

>>> +static int gpmc_setup_cs_nonres(struct gpmc *gpmc,
>>> +			struct gpmc_device *gd, struct gpmc_cs_data *cs)
>>
>> The name of this function is not 100% clear to me. What do you mean by
>> "nonres"?
> 
> It means anything other than resources like memory & irq, happened for
> want of better name, if you have one, name it, will put it.

Not really, but may be worth adding a comment.

>>> +int gpmc_cs_reconfigure(char *name, int id, struct gpmc_cs_data *cs)
>>
>> What scenario is this function used in? May be worth adding a comment
>> about the function.
> 
> Ok, it was required for OneNAND, as it needs to reconfigure

Ok, but why is that? Unless this is something generic about one-nand I
don't comprehend. I have a high-level understanding of one-nand, but I
am not familiar with the specifics that require it to be reconfigured.

>>> +	for (i = 0, n = gdp->num_cs, cs = gdp->cs_data;
>>> +				i < gdp->num_cs; i++, cs++)
>>> +		n += hweight32(cs->irq_config);
>>
>> As you know I am not a fan of these overloaded for-loops as I find them
>> hard to read ;-)
>>
>> Why not ...
>>
>> n = gdp->num_cs;
>> cs = gdp->cs_data;
> 
> 
> Well, you also know that I am big fan of it; difference of opinion,
> my preference is to keep loop control statements together with "for",
> unless there is a cry from community that it should not be this way.
> I believe that it is not that much unreadable.

Ok.

>>> +	for (i = 0, gdq = gp->device_pdata, gd = gpmc->device;
>>> +			(i < gp->num_device) && (*gdq); i++, gdq++) {
>>
>> You have num_devices now and so do you really need the "&& (*gdq)"?
>> Seems redundant.
> 
> num_device is something that I never wanted to add, was happy with,
> NULL entry termination, only because of your repeated comments,
> I added this. And this additional check now prevents NULL dereference.

Ok, but I would go with one or the other. Make it NULL terminated but
maybe add a comment?

>>> +	gpmc_setup_writeprotect(gpmc);
>>
>> Write protect is a pin and there is only one. Like the waitpins and CS
>> signals this needs to be associated with a device. It would make sense
>> that this is associated with the cs data.
> 
> As far as platform are concerned, it is associated with cs, it is only
> that while configuring CS, it is propagated such that it is done once.

Hmmm ... but here it looks like if write-protect is used you are going
to turn it on. A feature like this seems that it does need to be handled
by the device's driver. Enabling and disabling write-protect are
functions that I would expect are exported to the device's driver and
not directly enabled in the gpmc driver during probe. However, maybe is
could be valid for the write protect to be enabled by default which
could make sense. However, I don't see anywhere else in the driver to
control it.

Shouldn't we warn on multiple devices trying to use write-protect when
parsing their configuration?

>>> +#define	GPMC_READTYPE_ASYNC		BIT(30)
>>> +#define	GPMC_WRITETYPE_ASYNC		BIT(31)
>>> +
>>
>> Please keep in mind that eventually this has to all be moved into device
>> tree and so at that point these configuration flags will have to be
>> re-worked. However, just a FYI.
> 
> On device tree matters, my mind is next to blank, thanks for mentioning it.

No problem. For now lets not worry too much.

Cheers
Jon

Hi Afzal,

On 05/01/2012 07:19 AM, Afzal Mohammed wrote:

[...]

> +static int gpmc_setup_cs_waitpin(struct gpmc *gpmc, struct gpmc_device *gd,
> +						unsigned cs, unsigned conf)
> +{
> +	u32 l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> +	unsigned idx = ~0x0;
> +	int polarity = 0;
>  
> -	l = gpmc_read_reg(GPMC_REVISION);
> -	printk(KERN_INFO "GPMC revision %d.%d\n", (l >> 4) & 0x0f, l & 0x0f);
> -	/* Set smart idle mode and automatic L3 clock gating */
> -	l = gpmc_read_reg(GPMC_SYSCONFIG);
> -	l &= 0x03 << 3;
> -	l |= (0x02 << 3) | (1 << 0);
> -	gpmc_write_reg(GPMC_SYSCONFIG, l);
> -	gpmc_mem_init();
> +	switch (conf & GPMC_WAITPIN_MASK) {
> +	case GPMC_WAITPIN_0:
> +		idx =  GPMC_WAITPIN_IDX0;
> +		break;
> +	case GPMC_WAITPIN_1:
> +		idx =  GPMC_WAITPIN_IDX1;
> +		break;
> +	case GPMC_WAITPIN_2:
> +		idx =  GPMC_WAITPIN_IDX2;
> +		break;
> +	case GPMC_WAITPIN_3:
> +		idx =  GPMC_WAITPIN_IDX3;
> +		break;
> +	/* no waitpin */
> +	case 0:
> +		break;
> +	default:
> +		dev_err(gpmc->dev, "multiple waitpins selected on CS:%u\n", cs);
> +		return -EINVAL;
> +		break;
> +	}

Why not combined case 0 and default? Both are invalid configurations so
just report invalid selection.

>  
> -	/* initalize the irq_chained */
> -	irq = OMAP_GPMC_IRQ_BASE;
> -	for (cs = 0; cs < GPMC_CS_NUM; cs++) {
> -		irq_set_chip_and_handler(irq, &dummy_irq_chip,
> -						handle_simple_irq);
> -		set_irq_flags(irq, IRQF_VALID);
> -		irq++;
> +	switch (conf & GPMC_WAITPIN_POLARITY_MASK) {
> +	case GPMC_WAITPIN_ACTIVE_LOW:
> +		polarity = LOW;
> +		break;
> +	case GPMC_WAITPIN_ACTIVE_HIGH:
> +		polarity = HIGH;
> +		break;
> +	/* no waitpin */
> +	case 0:
> +		break;
> +	default:
> +		dev_err(gpmc->dev, "waitpin polarity set to low & high\n");
> +		return -EINVAL;
> +		break;
>  	}

Again, combine case 0 and default as these are invalid.

>  
> -	ret = request_irq(gpmc_irq, gpmc_handle_irq, IRQF_SHARED, "gpmc", NULL);
> -	if (ret)
> -		pr_err("gpmc: irq-%d could not claim: err %d\n",
> -						gpmc_irq, ret);
> -	return ret;
> +	if (idx != ~0x0) {

If you combine the above cases, then you can drop the idx test here.

> +		if (gd->have_waitpin) {
> +			if (gd->waitpin != idx ||
> +					gd->waitpin_polarity != polarity) {
> +				dev_err(gpmc->dev, "error: conflict: waitpin %u with polarity %d on device %s.%d\n",
> +					gd->waitpin, gd->waitpin_polarity,
> +					gd->name, gd->id);
> +				return -EBUSY;
> +			}
> +		} else {

Don't need the else as you are going to return in the above.

> +			gd->have_waitpin = true;
> +			gd->waitpin = idx;
> +			gd->waitpin_polarity = polarity;
> +		}
> +
> +		l &= ~GPMC_CONFIG1_WAIT_PIN_SEL_MASK;
> +		l |= GPMC_CONFIG1_WAIT_PIN_SEL(idx);
> +		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
> +	} else if (polarity) {
> +		dev_err(gpmc->dev, "error: waitpin polarity specified with out wait pin number on device %s.%d\n",
> +							gd->name, gd->id);
> +		return -EINVAL;

Drop this else-if. The above switch statements will report the bad
configuration. This seems a bit redundant.

Cheers
Jon

Hi Jon,

On Fri, May 04, 2012 at 21:50:16, Hunter, Jon wrote:
> > As mentioned in the cover letter,
> > 
> > "Additional features that currently boards in mainline does not make
> > use of like, waitpin interrupt handling, changes to leverage revision
> > 6 IP differences has not been incorporated."
> > 
> > Priority in this series is to convert into a driver, get all boards working
> > on mainline. Once all boards are working with gpmc driver, these features
> > which are not required for currently supported boards can be added later.
> 
> Yes, but I meant why 2 and not say 5? Anyway, I think that this should
> be marked with a comment like a TODO so it is clear that this needs to
> be re-visited.

Ok, it will be marked with TODO

> >> I think that it make more sense to have the wait-pin information part of
> >> the gpmc_cs_data structure for the following reasons ...
> > 
> > Waitpin information is indeed a part of cs as far as boards are concerned,
> > it is only that it gets propogated to device
> 
> Why does the device's driver care? From my point of view, the wait-pin
> is just part of the interface setup/configuration. The external device
> may require this and assumes that this has been setup along with the CS
> and interface timing, but the device's driver should not care. Remember
> this is just a ready signal used to stall the access. Once configured,
> software should be unaware of it.

By device, it is referred to gpmc device which only gpmc driver is aware,
the peripheral device's driver is not at all aware.

> >> Also, any reason why waitpin_polarity is an int? I see you define LOW as
> >> -1, but I not sure why LOW cannot be 0 as 0 is programmed into the
> >> register for an active low wait signal.
> > 
> > Only intention is not to alter default waitpin polarity value, i.e. if
> > any board does make use of it w/o knowledge of Kernel, I don't want to
> > break it, there may be an argument saying that the board code is buggy,
> > while if some board does so, it is, but don't want to break working one.
> > 
> > Here unless user explicitly set the flag, it does nothing on polarity
> 
> Ok. Do such scenario's exist today? Please note that board code will be
> removed in the future and device-tree will replace. So if there are no
> cases today, I would not be concerned. Unless this could be something
> that has already been configured by the bootloader. However, in that
> case would be even call this function?

Let me check this, here only intent was to play safe, as I have
only two boards to test.

> >>> +int gpmc_cs_reconfigure(char *name, int id, struct gpmc_cs_data *cs)
> >>
> >> What scenario is this function used in? May be worth adding a comment
> >> about the function.
> > 
> > Ok, it was required for OneNAND, as it needs to reconfigure
> 
> Ok, but why is that? Unless this is something generic about one-nand I
> don't comprehend. I have a high-level understanding of one-nand, but I
> am not familiar with the specifics that require it to be reconfigured.

Not too familiar with OneNAND, from the code it has been understood
that to set into sync mode, first it may have to set to async mode, read
frequency from OneNAND, then setup sync mode for that frequency.


> >>> +	gpmc_setup_writeprotect(gpmc);
> >>
> >> Write protect is a pin and there is only one. Like the waitpins and CS
> >> signals this needs to be associated with a device. It would make sense
> >> that this is associated with the cs data.
> > 
> > As far as platform are concerned, it is associated with cs, it is only
> > that while configuring CS, it is propagated such that it is done once.
> 
> Hmmm ... but here it looks like if write-protect is used you are going
> to turn it on. A feature like this seems that it does need to be handled
> by the device's driver. Enabling and disabling write-protect are
> functions that I would expect are exported to the device's driver and
> not directly enabled in the gpmc driver during probe. However, maybe is
> could be valid for the write protect to be enabled by default which
> could make sense. However, I don't see anywhere else in the driver to
> control it.
> 
> Shouldn't we warn on multiple devices trying to use write-protect when
> parsing their configuration?

Even if a single device sets write protect, kernel will log it.

Regards
Afzal

Hi Jon,

On Fri, May 04, 2012 at 21:57:10, Hunter, Jon wrote:
> > -	gpmc_write_reg(GPMC_SYSCONFIG, l);
> > -	gpmc_mem_init();
> > +	switch (conf & GPMC_WAITPIN_MASK) {
> > +	case GPMC_WAITPIN_0:
> > +		idx =  GPMC_WAITPIN_IDX0;
> > +		break;
> > +	case GPMC_WAITPIN_1:
> > +		idx =  GPMC_WAITPIN_IDX1;
> > +		break;
> > +	case GPMC_WAITPIN_2:
> > +		idx =  GPMC_WAITPIN_IDX2;
> > +		break;
> > +	case GPMC_WAITPIN_3:
> > +		idx =  GPMC_WAITPIN_IDX3;
> > +		break;
> > +	/* no waitpin */
> > +	case 0:
> > +		break;
> > +	default:
> > +		dev_err(gpmc->dev, "multiple waitpins selected on CS:%u\n", cs);
> > +		return -EINVAL;
> > +		break;
> > +	}
> 
> Why not combined case 0 and default? Both are invalid configurations so
> just report invalid selection.

Case 0 is not invalid, a case where waitpin is not used, default refers
to when a user selects multiple waitpins wrongly.

> 
> >  
> > -	/* initalize the irq_chained */
> > -	irq = OMAP_GPMC_IRQ_BASE;
> > -	for (cs = 0; cs < GPMC_CS_NUM; cs++) {
> > -		irq_set_chip_and_handler(irq, &dummy_irq_chip,
> > -						handle_simple_irq);
> > -		set_irq_flags(irq, IRQF_VALID);
> > -		irq++;
> > +	switch (conf & GPMC_WAITPIN_POLARITY_MASK) {
> > +	case GPMC_WAITPIN_ACTIVE_LOW:
> > +		polarity = LOW;
> > +		break;
> > +	case GPMC_WAITPIN_ACTIVE_HIGH:
> > +		polarity = HIGH;
> > +		break;
> > +	/* no waitpin */
> > +	case 0:
> > +		break;
> > +	default:
> > +		dev_err(gpmc->dev, "waitpin polarity set to low & high\n");
> > +		return -EINVAL;
> > +		break;
> >  	}
> 
> Again, combine case 0 and default as these are invalid.

Similar to above

> 
> > +		if (gd->have_waitpin) {
> > +			if (gd->waitpin != idx ||
> > +					gd->waitpin_polarity != polarity) {
> > +				dev_err(gpmc->dev, "error: conflict: waitpin %u with polarity %d on device %s.%d\n",
> > +					gd->waitpin, gd->waitpin_polarity,
> > +					gd->name, gd->id);
> > +				return -EBUSY;
> > +			}
> > +		} else {
> 
> Don't need the else as you are going to return in the above.

Not always, only in case of error

> 
> > +			gd->have_waitpin = true;
> > +			gd->waitpin = idx;
> > +			gd->waitpin_polarity = polarity;
> > +		}
> > +
> > +		l &= ~GPMC_CONFIG1_WAIT_PIN_SEL_MASK;
> > +		l |= GPMC_CONFIG1_WAIT_PIN_SEL(idx);
> > +		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
> > +	} else if (polarity) {
> > +		dev_err(gpmc->dev, "error: waitpin polarity specified with out wait pin number on device %s.%d\n",
> > +							gd->name, gd->id);
> > +		return -EINVAL;
> 
> Drop this else-if. The above switch statements will report the bad
> configuration. This seems a bit redundant.

This is required as switch statements will not report error if polarity
is specified, w/o waitpin to be used.

Regards
Afzal

Hi Afzal,

On 05/07/2012 06:01 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Fri, May 04, 2012 at 21:57:10, Hunter, Jon wrote:
>>> -	gpmc_write_reg(GPMC_SYSCONFIG, l);
>>> -	gpmc_mem_init();
>>> +	switch (conf & GPMC_WAITPIN_MASK) {
>>> +	case GPMC_WAITPIN_0:
>>> +		idx =  GPMC_WAITPIN_IDX0;
>>> +		break;
>>> +	case GPMC_WAITPIN_1:
>>> +		idx =  GPMC_WAITPIN_IDX1;
>>> +		break;
>>> +	case GPMC_WAITPIN_2:
>>> +		idx =  GPMC_WAITPIN_IDX2;
>>> +		break;
>>> +	case GPMC_WAITPIN_3:
>>> +		idx =  GPMC_WAITPIN_IDX3;
>>> +		break;
>>> +	/* no waitpin */
>>> +	case 0:
>>> +		break;
>>> +	default:
>>> +		dev_err(gpmc->dev, "multiple waitpins selected on CS:%u\n", cs);
>>> +		return -EINVAL;
>>> +		break;
>>> +	}
>>
>> Why not combined case 0 and default? Both are invalid configurations so
>> just report invalid selection.
> 
> Case 0 is not invalid, a case where waitpin is not used, default refers
> to when a user selects multiple waitpins wrongly.

Ok. Then for case 0, just return here. If the polarity is set, you could
print an error here.

>>
>>>  
>>> -	/* initalize the irq_chained */
>>> -	irq = OMAP_GPMC_IRQ_BASE;
>>> -	for (cs = 0; cs < GPMC_CS_NUM; cs++) {
>>> -		irq_set_chip_and_handler(irq, &dummy_irq_chip,
>>> -						handle_simple_irq);
>>> -		set_irq_flags(irq, IRQF_VALID);
>>> -		irq++;
>>> +	switch (conf & GPMC_WAITPIN_POLARITY_MASK) {
>>> +	case GPMC_WAITPIN_ACTIVE_LOW:
>>> +		polarity = LOW;
>>> +		break;
>>> +	case GPMC_WAITPIN_ACTIVE_HIGH:
>>> +		polarity = HIGH;
>>> +		break;
>>> +	/* no waitpin */
>>> +	case 0:
>>> +		break;
>>> +	default:
>>> +		dev_err(gpmc->dev, "waitpin polarity set to low & high\n");
>>> +		return -EINVAL;
>>> +		break;
>>>  	}
>>
>> Again, combine case 0 and default as these are invalid.
> 
> Similar to above

If you return above, then case 0 is not needed.

>>
>>> +		if (gd->have_waitpin) {
>>> +			if (gd->waitpin != idx ||
>>> +					gd->waitpin_polarity != polarity) {
>>> +				dev_err(gpmc->dev, "error: conflict: waitpin %u with polarity %d on device %s.%d\n",
>>> +					gd->waitpin, gd->waitpin_polarity,
>>> +					gd->name, gd->id);
>>> +				return -EBUSY;
>>> +			}
>>> +		} else {
>>
>> Don't need the else as you are going to return in the above.
> 
> Not always, only in case of error

Ok, but seems that it can be simplified a little.

What happens if a device uses more than one wait-pin? In other words a
device with two chip-selects that uses two wait-pins?

>>
>>> +			gd->have_waitpin = true;
>>> +			gd->waitpin = idx;
>>> +			gd->waitpin_polarity = polarity;
>>> +		}
>>> +
>>> +		l &= ~GPMC_CONFIG1_WAIT_PIN_SEL_MASK;
>>> +		l |= GPMC_CONFIG1_WAIT_PIN_SEL(idx);
>>> +		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
>>> +	} else if (polarity) {
>>> +		dev_err(gpmc->dev, "error: waitpin polarity specified with out wait pin number on device %s.%d\n",
>>> +							gd->name, gd->id);
>>> +		return -EINVAL;
>>
>> Drop this else-if. The above switch statements will report the bad
>> configuration. This seems a bit redundant.
> 
> This is required as switch statements will not report error if polarity
> is specified, w/o waitpin to be used.

Ok, may be you can print that above when you detect that there are no
wait-pins selected.

Jon

Hi Afzal,

On 05/07/2012 05:57 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Fri, May 04, 2012 at 21:50:16, Hunter, Jon wrote:
>>> As mentioned in the cover letter,
>>>
>>> "Additional features that currently boards in mainline does not make
>>> use of like, waitpin interrupt handling, changes to leverage revision
>>> 6 IP differences has not been incorporated."
>>>
>>> Priority in this series is to convert into a driver, get all boards working
>>> on mainline. Once all boards are working with gpmc driver, these features
>>> which are not required for currently supported boards can be added later.
>>
>> Yes, but I meant why 2 and not say 5? Anyway, I think that this should
>> be marked with a comment like a TODO so it is clear that this needs to
>> be re-visited.
> 
> Ok, it will be marked with TODO
> 
>>>> I think that it make more sense to have the wait-pin information part of
>>>> the gpmc_cs_data structure for the following reasons ...
>>>
>>> Waitpin information is indeed a part of cs as far as boards are concerned,
>>> it is only that it gets propogated to device
>>
>> Why does the device's driver care? From my point of view, the wait-pin
>> is just part of the interface setup/configuration. The external device
>> may require this and assumes that this has been setup along with the CS
>> and interface timing, but the device's driver should not care. Remember
>> this is just a ready signal used to stall the access. Once configured,
>> software should be unaware of it.
> 
> By device, it is referred to gpmc device which only gpmc driver is aware,
> the peripheral device's driver is not at all aware.
> 
>>>> Also, any reason why waitpin_polarity is an int? I see you define LOW as
>>>> -1, but I not sure why LOW cannot be 0 as 0 is programmed into the
>>>> register for an active low wait signal.
>>>
>>> Only intention is not to alter default waitpin polarity value, i.e. if
>>> any board does make use of it w/o knowledge of Kernel, I don't want to
>>> break it, there may be an argument saying that the board code is buggy,
>>> while if some board does so, it is, but don't want to break working one.
>>>
>>> Here unless user explicitly set the flag, it does nothing on polarity
>>
>> Ok. Do such scenario's exist today? Please note that board code will be
>> removed in the future and device-tree will replace. So if there are no
>> cases today, I would not be concerned. Unless this could be something
>> that has already been configured by the bootloader. However, in that
>> case would be even call this function?
> 
> Let me check this, here only intent was to play safe, as I have
> only two boards to test.
> 
>>>>> +int gpmc_cs_reconfigure(char *name, int id, struct gpmc_cs_data *cs)
>>>>
>>>> What scenario is this function used in? May be worth adding a comment
>>>> about the function.
>>>
>>> Ok, it was required for OneNAND, as it needs to reconfigure
>>
>> Ok, but why is that? Unless this is something generic about one-nand I
>> don't comprehend. I have a high-level understanding of one-nand, but I
>> am not familiar with the specifics that require it to be reconfigured.
> 
> Not too familiar with OneNAND, from the code it has been understood
> that to set into sync mode, first it may have to set to async mode, read
> frequency from OneNAND, then setup sync mode for that frequency.
> 
> 
>>>>> +	gpmc_setup_writeprotect(gpmc);
>>>>
>>>> Write protect is a pin and there is only one. Like the waitpins and CS
>>>> signals this needs to be associated with a device. It would make sense
>>>> that this is associated with the cs data.
>>>
>>> As far as platform are concerned, it is associated with cs, it is only
>>> that while configuring CS, it is propagated such that it is done once.
>>
>> Hmmm ... but here it looks like if write-protect is used you are going
>> to turn it on. A feature like this seems that it does need to be handled
>> by the device's driver. Enabling and disabling write-protect are
>> functions that I would expect are exported to the device's driver and
>> not directly enabled in the gpmc driver during probe. However, maybe is
>> could be valid for the write protect to be enabled by default which
>> could make sense. However, I don't see anywhere else in the driver to
>> control it.
>>
>> Shouldn't we warn on multiple devices trying to use write-protect when
>> parsing their configuration?
> 
> Even if a single device sets write protect, kernel will log it.

That does not seem sufficient. It should be flagged as an error.

Write protect is a resource like the CS and waitpins and so I would have
thought that it should be reserved in the same way.

Jon

Hi Jon,

On Mon, May 07, 2012 at 21:32:58, Hunter, Jon wrote:

> >>> +	/* no waitpin */
> >>> +	case 0:
> >>> +		break;
> >>> +	default:
> >>> +		dev_err(gpmc->dev, "multiple waitpins selected on CS:%u\n", cs);
> >>> +		return -EINVAL;
> >>> +		break;
> >>> +	}
> >>
> >> Why not combined case 0 and default? Both are invalid configurations so
> >> just report invalid selection.
> > 
> > Case 0 is not invalid, a case where waitpin is not used, default refers
> > to when a user selects multiple waitpins wrongly.
> 
> Ok. Then for case 0, just return here. If the polarity is set, you could
> print an error here.

Different ways of doing things, this looks cleaner to me as already it is
checked, and time of execution in both cases would not differ much.

> >>> +		if (gd->have_waitpin) {
> >>> +			if (gd->waitpin != idx ||
> >>> +					gd->waitpin_polarity != polarity) {
> >>> +				dev_err(gpmc->dev, "error: conflict: waitpin %u with polarity %d on device %s.%d\n",
> >>> +					gd->waitpin, gd->waitpin_polarity,
> >>> +					gd->name, gd->id);
> >>> +				return -EBUSY;
> >>> +			}
> >>> +		} else {
> >>
> >> Don't need the else as you are going to return in the above.
> > 
> > Not always, only in case of error
> 
> Ok, but seems that it can be simplified a little.
> 
> What happens if a device uses more than one wait-pin? In other words a
> device with two chip-selects that uses two wait-pins?

Please re-read http://www.mail-archive.com/linux-omap@vger.kernel.org/msg67702.html
and your reply

Regards
Afzal

Hi Jon,

On Mon, May 07, 2012 at 21:53:34, Hunter, Jon wrote:

> >>>> Write protect is a pin and there is only one. Like the waitpins and CS
> >>>> signals this needs to be associated with a device. It would make sense
> >>>> that this is associated with the cs data.
> >>>
> >>> As far as platform are concerned, it is associated with cs, it is only
> >>> that while configuring CS, it is propagated such that it is done once.
> >>
> >> Hmmm ... but here it looks like if write-protect is used you are going
> >> to turn it on. A feature like this seems that it does need to be handled
> >> by the device's driver. Enabling and disabling write-protect are
> >> functions that I would expect are exported to the device's driver and
> >> not directly enabled in the gpmc driver during probe. However, maybe is
> >> could be valid for the write protect to be enabled by default which
> >> could make sense. However, I don't see anywhere else in the driver to
> >> control it.
> >>
> >> Shouldn't we warn on multiple devices trying to use write-protect when
> >> parsing their configuration?
> > 
> > Even if a single device sets write protect, kernel will log it.
> 
> That does not seem sufficient. It should be flagged as an error.
> 
> Write protect is a resource like the CS and waitpins and so I would have
> thought that it should be reserved in the same way.

Isn't it valid for multiple devices to make use of write protect pin ?

Regards
Afzal

Hi  Afzal,

On 05/08/2012 01:18 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Mon, May 07, 2012 at 21:32:58, Hunter, Jon wrote:
> 
>>>>> +	/* no waitpin */
>>>>> +	case 0:
>>>>> +		break;
>>>>> +	default:
>>>>> +		dev_err(gpmc->dev, "multiple waitpins selected on CS:%u\n", cs);
>>>>> +		return -EINVAL;
>>>>> +		break;
>>>>> +	}
>>>>
>>>> Why not combined case 0 and default? Both are invalid configurations so
>>>> just report invalid selection.
>>>
>>> Case 0 is not invalid, a case where waitpin is not used, default refers
>>> to when a user selects multiple waitpins wrongly.
>>
>> Ok. Then for case 0, just return here. If the polarity is set, you could
>> print an error here.
> 
> Different ways of doing things, this looks cleaner to me as already it is
> checked, and time of execution in both cases would not differ much.

Sure. However, I think that this could be simplified so that it is
easier to read. At a minimum you may wish to add some comments to
explain the purposes of the multi-level if-statements as it is not
intuitive for the reader.

>>>>> +		if (gd->have_waitpin) {
>>>>> +			if (gd->waitpin != idx ||
>>>>> +					gd->waitpin_polarity != polarity) {
>>>>> +				dev_err(gpmc->dev, "error: conflict: waitpin %u with polarity %d on device %s.%d\n",
>>>>> +					gd->waitpin, gd->waitpin_polarity,
>>>>> +					gd->name, gd->id);
>>>>> +				return -EBUSY;
>>>>> +			}
>>>>> +		} else {
>>>>
>>>> Don't need the else as you are going to return in the above.
>>>
>>> Not always, only in case of error
>>
>> Ok, but seems that it can be simplified a little.
>>
>> What happens if a device uses more than one wait-pin? In other words a
>> device with two chip-selects that uses two wait-pins?
> 
> Please re-read http://www.mail-archive.com/linux-omap@vger.kernel.org/msg67702.html
> and your reply

Ok thats fine. Sorry for my repeated questions, but I think that this
just highlights that this code is not clear in it purpose. So again may
be some comments would make this all clearer.

Jon

Hi Jon,

On Tue, May 08, 2012 at 20:38:24, Hunter, Jon wrote:

> > Different ways of doing things, this looks cleaner to me as already it is
> > checked, and time of execution in both cases would not differ much.
> 
> Sure. However, I think that this could be simplified so that it is
> easier to read. At a minimum you may wish to add some comments to
> explain the purposes of the multi-level if-statements as it is not
> intuitive for the reader.

Ok

> >> What happens if a device uses more than one wait-pin? In other words a
> >> device with two chip-selects that uses two wait-pins?
> > 
> > Please re-read http://www.mail-archive.com/linux-omap@vger.kernel.org/msg67702.html
> > and your reply
> 
> Ok thats fine. Sorry for my repeated questions, but I think that this
> just highlights that this code is not clear in it purpose. So again may
> be some comments would make this all clearer.

Ok

Regards
Afzal