Re: [PATCH v5 06/14] ARM: OMAP2+: gpmc: CS configuration helper

[Jon Hunter <jon-hunter@ti.com>]

On 06/11/2012 09:26 AM, Afzal Mohammed wrote:
> Helper for configuring given CS based on flags.
> 
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> ---
>  arch/arm/mach-omap2/gpmc.c             |   33 ++++++++++++++++++++++++++++++++
>  arch/arm/plat-omap/include/plat/gpmc.h |    5 +++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 652b245..4e19010 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -927,6 +927,39 @@ static __devinit int gpmc_setup_cs_mem(struct gpmc_cs_data *cs,
>  	return 1;
>  }
>  
> +static void gpmc_setup_cs_config(unsigned cs, unsigned conf)
> +{
> +	u32 l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);

Why is it necessary to read the register first? I thought you wanted to
get away from relying on bootloader settings?

> +	l &= ~(GPMC_CONFIG1_MUXADDDATA |
> +		GPMC_CONFIG1_WRITETYPE_SYNC |
> +		GPMC_CONFIG1_WRITEMULTIPLE_SUPP |
> +		GPMC_CONFIG1_READTYPE_SYNC |
> +		GPMC_CONFIG1_READMULTIPLE_SUPP |
> +		GPMC_CONFIG1_WRAPBURST_SUPP |
> +		GPMC_CONFIG1_DEVICETYPE(~0) |
> +		GPMC_CONFIG1_DEVICESIZE(~0) |
> +		GPMC_CONFIG1_PAGE_LEN(~0));
> +
> +	l |= conf & GPMC_CONFIG1_DEVICETYPE_NAND;
> +	l |= conf & GPMC_CONFIG1_DEVICESIZE_16;

I can see that it works to use the above definitions as masks because of
the possible values that can be programmed into these fields. However,
from a read-ability standpoint this is unclear and requires people to
review the documentation to understand what you are doing here.
Furthermore, if any new device-types or sizes were added in the future
this could lead to bugs. Hence, it would be better to define a mask for
these fields.

Now you may say what happens if someone pass a bad device-type or
device-size that would equate to a reserved value being programmed. Well
if someone passes a bad value we don't know what they intended to
program and that raises the question should we be checking the conf
value of bad device types, size, and page lengths here?

Jon