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

[Jon Hunter]

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