Re: [PATCH V2 2/2] mcx: support for HTKW mcx board

[Ilya Yanok <yanok@emcraft.com>]

Hi Igor,

On 15.12.2011 14:40, Igor Grinberg wrote:
>> +	r = gpio_request_array(mcx_dss_gpios, ARRAY_SIZE(mcx_dss_gpios));
>> +	if (r) {
>> +		pr_err("failed to get DSS control GPIOs\n");
>> +		return;
>> +	}
>> +
>> +	omap_mux_init_gpio(LCD_BKLIGHT_EN, OMAP_PIN_OUTPUT);
>> +	omap_mux_init_gpio(LCD_LVL_SFHT_BUF_ENn, OMAP_PIN_OUTPUT);
>> +	omap_mux_init_gpio(LCD_PWR_ENn, OMAP_PIN_OUTPUT);
>> +	omap_mux_init_gpio(HDMI_TRCVR_PDn, OMAP_PIN_OUTPUT);
> 
> Shouldn't you mux the pins, before you access the GPIO
> (e.g. before the gpio_request_array()).
> Are there any safety problems?

No, there are no problems. I will move mux settings above
gpio_request_array() call.

>> +static struct omap2_hsmmc_info mmc[] = {
>> +	{
>> +		.mmc		= 1,
>> +		.caps		= MMC_CAP_4_BIT_DATA,
>> +		.gpio_cd        = SD_CARD_CD,
>> +		.gpio_wp        = SD_CARD_WP,
>> +		.ocr_mask	= MMC_VDD_32_33 | MMC_VDD_33_34 |
>> +					MMC_VDD_165_195,
> 
> The ocr_mask will be overridden, by the following patch:
> -----------------
> commit e89715a7e48d505f42813a4e3ee0f0efb49832ba
> Author: Abhilash K V <abhilash.kv@ti.com>
> Date:   Fri Dec 9 12:27:36 2011 -0800
> 
>     ARM: OMAP: hsmmc: Support for AM3517 MMC1 voltages
> --------------
> 
> in Tony's hsmmc branch.
> 
> IMO it should be fixed, by adding a check if the ocr_mask is
> already set...
> I can't send a patch for this right now...

Well, I think I should just drop the .ocr_mask field then. Everything
works fine for me with the above mentioned patch.

>> +#ifdef CONFIG_OMAP_MUX
>> +static struct omap_board_mux board_mux[] __initdata = {
>> +	OMAP3_MUX(CHASSIS_DMAREQ3, OMAP_MUX_MODE0 | OMAP_PIN_INPUT_PULLDOWN),
>> +	OMAP3_MUX(UART1_TX, OMAP_MUX_MODE0 | OMAP_PIN_OUTPUT |
>> +			OMAP_PULL_ENA | OMAP_PULL_UP),
>> +	OMAP3_MUX(UART1_RX, OMAP_MUX_MODE0 | OMAP_PIN_INPUT),
>> +	OMAP3_MUX(UART1_RTS, OMAP_MUX_MODE0 | OMAP_PIN_INPUT),
>> +	OMAP3_MUX(UART1_CTS, OMAP_MUX_MODE0 | OMAP_PIN_OUTPUT |
>> +			OMAP_PULL_ENA | OMAP_PULL_UP),
> 
> Hmm... pullup for output? Is this needed for kind of safety?

This is a mistake indeed. Will remove the pullups.

>> +static void __init mcx_init(void)
>> +{
>> +	int err;
>> +
>> +	omap3_mux_init(board_mux, OMAP_PACKAGE_CBB);
>> +	mcx_i2c_init();
>> +	platform_add_devices(mcx_devices, ARRAY_SIZE(mcx_devices));
>> +	omap_serial_init();
> 
> Shouldn't this one be before the mcx_i2c_init() call?

Well, I think I've taken this order from some other board init... I
think the idea was to bring up regulator chip earlier. But I can move
serial up with no problem.

>> +	mcx_display_init();
>> +
>> +	/* Configure EHCI ports */
>> +	err = gpio_request_one(USB_HOST_PWR_EN, GPIOF_OUT_INIT_HIGH,
>> +			"USB_HOST_PWR_EN");
>> +	if (err)
>> +		pr_warn("Failed to request USB host power enable GPIO\n");
> 
> empty line here will improve the readability.

Ok.

Regards, Ilya.