Re: [PATCH v2 1/3] OMAPDSS: HDMI: HPD support in boardfile

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3389 bytes --]

On Fri, 2012-01-06 at 18:14 +0530, mythripk@ti.com wrote:
> From: Mythri P K <mythripk@ti.com>
> 
> Add support for HPD GPIO configuration in board file.
> Also remove the enabling  of GPIO's required for HDMI from
> hdmi driver file to display.c based on the GPIO #'s sent from
> board file.
> 
> Signed-off-by: Mythri P K <mythripk@ti.com>
> Acked-by: Tony Lindgren <tony@atomide.com>
> ---

You are doing too much in this patch. This changes the HPD gpio to
CT_CP_HPD, changes how gpios are allocated, moves gpio allocation into
another file, changes the pulls of the gpios, introduces a new field
into dss device...

You should do just one logical thing in a patch.

> -static void omap4_hdmi_mux_pads(enum omap_hdmi_flags flags)
> +static void omap4_hdmi_mux_pads(enum omap_hdmi_flags flags, int hpd_gpio)
>  {
>  	u32 reg;
>  	u16 control_i2c_1;
>  
>  	/* PAD0_HDMI_HPD_PAD1_HDMI_CEC */
>  	omap_mux_init_signal("hdmi_hpd",
> -			OMAP_PIN_INPUT_PULLUP);
> +			OMAP_PIN_INPUT_PULLDOWN);

Is the "hdmi_hpd" always the same pin? If so, it can be handled in
display.c, no need to handle it in the board files.

>  	omap_mux_init_signal("hdmi_cec",
>  			OMAP_PIN_INPUT_PULLUP);
>  	/* PAD0_HDMI_DDC_SCL_PAD1_HDMI_DDC_SDA */
> @@ -112,6 +113,10 @@ static void omap4_hdmi_mux_pads(enum omap_hdmi_flags flags)
>  			OMAP_PIN_INPUT_PULLUP);
>  	omap_mux_init_signal("hdmi_ddc_sda",
>  			OMAP_PIN_INPUT_PULLUP);
> +	omap_mux_init_signal("gpmc_wait2.gpio_100",
> +			OMAP_PIN_INPUT_PULLDOWN);

Hmm, what is "gpmc_wait2.gpio_100"?

> +	/* HPD GPIO needs to be muxed*/
> +	omap_mux_init_gpio(hpd_gpio, OMAP_PIN_INPUT | OMAP_PULL_ENA);

Isn't this the same as the "hdmi_hpd" case above? Why are both needed?

>  	/*
>  	 * CONTROL_I2C_1: HDMI_DDC_SDA_PULLUPRESX (bit 28) and
> @@ -160,10 +165,18 @@ static int omap4_dsi_mux_pads(int dsi_id, unsigned lanes)
>  	return 0;
>  }
>  
> -int omap_hdmi_init(enum omap_hdmi_flags flags)
> +int omap_hdmi_init(enum omap_hdmi_flags flags, int hpd_gpio,
> +		struct gpio hdmi_gpios[], int size)
>  {
> +	int status;
> +
> +	status = gpio_request_array(hdmi_gpios, size);
> +	if (status) {
> +		pr_err("%s: Cannot request HDMI GPIOs\n", __func__);
> +		return status;
> +	}

I don't see much point in this. The GPIOs are still declared in the
board files, and this function doesn't use the GPIOs for anything. It's
better to request them in the board file.

> @@ -319,7 +320,8 @@ struct omap_dss_board_info {
>  /* Init with the board info */
>  extern int omap_display_init(struct omap_dss_board_info *board_data);
>  /* HDMI mux init*/
> -extern int omap_hdmi_init(enum omap_hdmi_flags flags);
> +extern int omap_hdmi_init(enum omap_hdmi_flags flags, int hpd_gpio,
> +			struct gpio hdmi_gpios[], int size);
>  
>  struct omap_display_platform_data {
>  	struct omap_dss_board_info *board_data;
> @@ -568,6 +570,8 @@ struct omap_dss_device {
>  
>  	int reset_gpio;
>  
> +	int hpd_gpio;

The hpd_gpio is for the hdmi device, not for all dss devices. So it
shouldn't be in the omap_dss_device. Did you check the example patch I
made to fix the phy issue?

I still think we should try to fix the bug at hand, not try to implement
proper HPD support. Was there something wrong with the example patch I
gave? (other than it needs some splitting up).

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]