[PATCH V2 Resend 12/12] ST SPEAr: Adding gpio pad multiplexing support

[Viresh KUMAR]

On 4/3/2010 10:44 PM, Shiraz HASHIM wrote:
>>  /* Add spear3xx family function declarations here */
>> +void __init clk_init(void);
>>  void __init spear3xx_map_io(void);
>>  void __init spear3xx_init_irq(void);
>>  void __init spear3xx_init(void);
>> -void __init spear300_init(void);
>> -void __init spear310_init(void);
>> -void __init spear320_init(void);
>> -void __init clk_init(void);
>> +void spear_pmx_init(struct pmx_driver *pmx_driver, uint base, uint size);
>>  
>> -/* Add spear300 machine device structure declarations here */
>> +/* pad mux declarations */
> These are spear300 soecific declarations, isn't it?

No. I have added few spear3xx specific declarations here and moved spear300 
declarations later in the list.

> 
>> +#define PMX_FIRDA_MASK		(1 << 14)
>> +#define PMX_I2C_MASK		(1 << 13)
>> +#define PMX_SSP_CS_MASK		(1 << 12)
>> +#define PMX_SSP_MASK		(1 << 11)
>> +#define PMX_MII_MASK		(1 << 10)
>> +#define PMX_GPIO_PIN0_MASK	(1 << 9)
>> +#define PMX_GPIO_PIN1_MASK	(1 << 8)
>> +#define PMX_GPIO_PIN2_MASK	(1 << 7)
>> +#define PMX_GPIO_PIN3_MASK	(1 << 6)
>> +#define PMX_GPIO_PIN4_MASK	(1 << 5)
>> +#define PMX_GPIO_PIN5_MASK	(1 << 4)
>> +#define PMX_UART0_MODEM_MASK	(1 << 3)
>> +#define PMX_UART0_MASK		(1 << 2)
>> +#define PMX_TIMER_3_4_MASK	(1 << 1)
>> +#define PMX_TIMER_1_2_MASK	(1 << 0)
>> +
>> +extern struct pmx_driver pmx_driver;
>> +
>> +/* spear300 declarations */
>>  #ifdef CONFIG_MACH_SPEAR300
> 
> see above comment 

explained above

> 
>> +/* Add spear300 machine device structure declarations here */
>>  extern struct amba_device gpio1_device;
>> +
>> +/* pad mux modes */
>> +extern struct pmx_mode nand_mode;
>> +extern struct pmx_mode nor_mode;
>> +extern struct pmx_mode photo_frame_mode;
>> +extern struct pmx_mode lend_ip_phone_mode;
>> +extern struct pmx_mode hend_ip_phone_mode;
>> +extern struct pmx_mode lend_wifi_phone_mode;
>> +extern struct pmx_mode hend_wifi_phone_mode;
>> +extern struct pmx_mode ata_pabx_wi2s_mode;
>> +extern struct pmx_mode ata_pabx_i2s_mode;
>> +extern struct pmx_mode caml_lcdw_mode;
>> +extern struct pmx_mode camu_lcd_mode;
>> +extern struct pmx_mode camu_wlcd_mode;
>> +extern struct pmx_mode caml_lcd_mode;
>> +
>> +
>> +struct pmx_dev pmx_gpio1 = {
>> +	.name = "arm gpio1",
>> +	.modes = pmx_gpio1_modes,
>> +	.mode_count = ARRAY_SIZE(pmx_gpio1_modes),
>> +	.enb_on_reset = 1,
>> +};
> 
> We have generalized pmx_dev in mode and mask attributes. Is it generic enough?
> What if we have several mux register in same mode?
> 

This layer is spear specific and as far as our socs are concerned it is okay.
In later socs if we have something which doesn't suit this framework then we
can do minor modifications here with little effort.

>> +
>> +/* pmx driver structure */
>> +struct pmx_driver pmx_driver = {
>> +	.mode_reg = {.offset = MODE_CONFIG_REG, .mask = 0x0000000f},
>> +	.mux_reg = {.offset = PAD_MUX_CONFIG_REG, .mask = 0x00007fff},
>> +};
> 
> We assume that all SoCs would either have a mode register or a mux register or
> both. Is this assumption generic enough. What if we have
>  - more than 1 mux register
>  - more than 1 mode register
>  - may be a submode register in some new silicon.

Same as above.

> 
>> +
>> +	/* padmux initialization */
>> +	pmx_driver.mode = &photo_frame_mode;
>> +	pmx_driver.devs = pmx_devs;
>> +	pmx_driver.devs_count = ARRAY_SIZE(pmx_devs);
>> +	spear300_pmx_init();
> 
> Intializing pmx_driver and not using it directly obstructs readability.
> Better to pass pmx_driver and other info in spear300_pmx_init itself.
> What is your opinion?
> 

Explained below.

>> +
>> +	/* padmux initialization */
>> +	pmx_driver.mode = NULL;
>> +	pmx_driver.devs = pmx_devs;
>> +	pmx_driver.devs_count = ARRAY_SIZE(pmx_devs);
>> +	spear310_pmx_init();
> 
> I see your point in not passing pmx_driver here. It is due to initialization of
> pmx_driver.mux fields. This field is specific to mach so may not be defined in
> board file. Any other idea to overcome this little readability issue?

There are few things divided between mach and board. Now this situation will 
always have some readability issue. I feel it is just fine.

> 
>> +	val = readl(pmx->base + pmx->mux_reg.offset);
>> +	for (i = 0; i < count; i++) {
>> +		u8 j = 0;
>> +
>> +		if (!devs[i]->name || !devs[i]->modes) {
>> +			printk(KERN_ERR "padmux: dev name or modes is null\n");
>> +			continue;
>> +		}
>> +		/* check if peripheral exists in active mode */
>> +		if (pmx->active_mode) {
>> +			bool found = false;
>> +			for (j = 0; j < devs[i]->mode_count; j++) {
>> +				if (devs[i]->modes[j].ids &
>> +						pmx->active_mode->id) {
>> +					found = true;
>> +					break;
>> +				}
>> +			}
>> +			if (found == false) {
>> +				printk(KERN_ERR "%s device not available in %s"\
>> +						"mode\n", devs[i]->name,
>> +						pmx->active_mode->name);
>> +				continue;
>> +			}
>> +		}
>> +
>> +		/* enable peripheral */
>> +		mask = devs[i]->modes[j].mask & pmx->mux_reg.mask;
> 
> Didn't understand this line.
> 

Here we are preparing mask to be written for enabling a peripheral in a specific
mode. devs[i]->modes[j].mask will give mask of dev[i] for mode[j], this must be
& with valid mask of mux register.

regards,
viresh kumar