* Sukumar Ghorai <s-ghorai@xxxxxx> [100604 10:34]:
> few functions added in gpmc module and to be used by other drivers like NAND.
> E.g.: - ioctl function
> - ecc functions
Uhh, let's leave out ioctl from the description.. Otherwise people
think you're adding new ioctls in this patch.
> @@ -46,8 +46,9 @@
> #define GPMC_ECC_CONFIG 0x1f4
> #define GPMC_ECC_CONTROL 0x1f8
> #define GPMC_ECC_SIZE_CONFIG 0x1fc
> +#define GPMC_ECC1_RESULT 0x200
>
> -#define GPMC_CS0 0x60
> +#define GPMC_CS0_BASE 0x60
> #define GPMC_CS_SIZE 0x30
>
> #define GPMC_MEM_START 0x00000000
Why changing GPMC_CS0 to GPMC_CS0_BASE? Should it rather be
GPMC_CS0_OFFSET?
> @@ -419,8 +437,100 @@ void gpmc_cs_free(int cs)
> EXPORT_SYMBOL(gpmc_cs_free);
>
> /**
> + * gpmc_hwcontrol - hardware specific access (read/ write) control
> + * @cs: chip select number
> + * @cmd: command type
> + * @write: 1 for write; 0 for read
> + * @wval: value to write
> + * @rval: read pointer
> + */
> +int gpmc_hwcontrol(int cs, int cmd, int write, int wval, int *rval)
> +{
> + u32 regval = 0;
> +
> + if (!write && !rval)
> + return -EINVAL;
You pass int write, then return immediately if it's not set?
> + switch (cmd) {
> + case GPMC_STATUS_BUFFER:
> + regval = gpmc_read_reg(GPMC_STATUS);
> + /* 1 : buffer is available to write */
> + *rval = regval & GPMC_STATUS_BUFF_EMPTY;
> + break;
> +
> + case GPMC_GET_SET_IRQ_STATUS:
> + if (write)
> + gpmc_write_reg(GPMC_IRQSTATUS, wval);
> + else
> + *rval = gpmc_read_reg(GPMC_IRQSTATUS);
> + break;
> +
> + case GPMC_PREFETCH_FIFO_CNT:
> + regval = gpmc_read_reg(GPMC_PREFETCH_STATUS);
> + *rval = GPMC_PREFETCH_STATUS_FIFO_CNT(regval);
> + break;
> +
> + case GPMC_PREFETCH_COUNT:
> + regval = gpmc_read_reg(GPMC_PREFETCH_STATUS);
> + *rval = GPMC_PREFETCH_STATUS_COUNT(regval);
> + break;
> +
> + case GPMC_CONFIG_WP:
> + regval = gpmc_read_reg(GPMC_CONFIG);
> + if (wval)
> + regval &= ~GPMC_CONFIG_WRITEPROTECT; /* WP is ON */
> + else
> + regval |= GPMC_CONFIG_WRITEPROTECT; /* WP is OFF */
> + gpmc_write_reg(GPMC_CONFIG, regval);
> + break;
> +
> + case GPMC_CONFIG_RDY_BSY:
> + regval = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> + regval |= WR_RD_PIN_MONITORING;
> + gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
> + break;
> +
> + case GPMC_CONFIG_DEV_SIZE:
> + regval = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> + regval |= GPMC_CONFIG1_DEVICESIZE(wval);
> + gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
> + break;
> +
> + case GPMC_CONFIG_DEV_TYPE:
> + regval = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> + regval |= GPMC_CONFIG1_DEVICETYPE(wval);
> + if (wval == GPMC_DEVICETYPE_NOR)
> + regval |= GPMC_CONFIG1_MUXADDDATA;
> + gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
> + break;
> +
> + case GPMC_NAND_COMMAND:
> + gpmc_cs_write_byte(cs, GPMC_CS_NAND_COMMAND, wval);
> + break;
> +
> + case GPMC_NAND_ADDRESS:
> + gpmc_cs_write_byte(cs, GPMC_CS_NAND_ADDRESS, wval);
> + break;
> +
> + case GPMC_NAND_DATA:
> + if (write)
> + gpmc_cs_write_byte(cs, GPMC_CS_NAND_DATA, wval);
> + else
> + *rval = gpmc_cs_read_byte(cs, GPMC_CS_NAND_DATA);
> + break;
> +
> + default:
> + printk(KERN_ERR "gpmc_hwcontrol: Not supported\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(gpmc_hwcontrol);
You should just replace this function with simple functions like we already
have in gpmc.c rather than trying to pack everything into one function.
Just add various gpmc_xxx_get/set functions rather than pass int *rval.
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
