Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH RFC v2 5/8] drm/bridge: dw-hdmi: support dynamically get
 input/out color info
To:     Neil Armstrong <narmstrong@baylibre.com>, architt@codeaurora.org,
        Laurent.pinchart@ideasonboard.com,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Sandy Huang <hjc@rock-chips.com>,
        =?UTF-8?Q?Heiko_St=c3=bcbner?= <heiko@sntech.de>,
        maxime.ripard@bootlin.com
Cc:     Zheng Yang <zhengyang@rock-chips.com>,
        dri-devel@lists.freedesktop.org, linux-amlogic@lists.infradead.org,
        linux-kernel@vger.kernel.org
From:   Andrzej Hajda <a.hajda@samsung.com>
Message-ID: <3cd791c0-8a7b-a3d1-09fa-5ddf0ea9c380@samsung.com>
Date:   Wed, 19 Dec 2018 08:26:08 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
        Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <20181130134301.17963-6-narmstrong@baylibre.com>
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20181130134301.17963-1-narmstrong@baylibre.com>
        <CGME20181130134411epcas3p2dc841f297a16ae25f17323c766a386d6@epcas3p2.samsung.com>
        <20181130134301.17963-6-narmstrong@baylibre.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 30.11.2018 14:42, Neil Armstrong wrote:
> From: Zheng Yang <zhengyang@rock-chips.com>
>
> To get input/output bus_format/enc_format dynamically, this patch
> introduce following funstion in plat_data:
> 	- get_input_bus_format
> 	- get_output_bus_format
> 	- get_enc_in_encoding
> 	- get_enc_out_encoding


It seems fishy. On one side description says about dynamic resolution of
formats and encodings.

On the other side these functions as only argument takes platform_data
which should be rather static.

Where is this "dynamic" thing? The only usage of these callbacks I have
found in next patches is also not dynamic, the functions just return
some static value.

Moreover function takes void* argument, which is again something
suspicious, why cannot you pass know structure?

And finally encoding usually should depend on display mode, it should
not depend only static data.


What kind of problems do you want to solve here?


Regards

Andrzej


>
> Signed-off-by: Zheng Yang <zhengyang@rock-chips.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 28 +++++++++++++++++------
>  include/drm/bridge/dw_hdmi.h              |  5 ++++
>  2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 4a9a24e854db..bd564ffdf18b 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1810,6 +1810,7 @@ static void hdmi_disable_overflow_interrupts(struct dw_hdmi *hdmi)
>  static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  {
>  	int ret;
> +	void *data = hdmi->plat_data->phy_data;
>  
>  	hdmi_disable_overflow_interrupts(hdmi);
>  
> @@ -1821,10 +1822,13 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  		dev_dbg(hdmi->dev, "CEA mode used vic=%d\n", hdmi->vic);
>  	}
>  
> -	if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
> -	    (hdmi->vic == 21) || (hdmi->vic == 22) ||
> -	    (hdmi->vic == 2) || (hdmi->vic == 3) ||
> -	    (hdmi->vic == 17) || (hdmi->vic == 18))
> +	if (hdmi->plat_data->get_enc_out_encoding)
> +		hdmi->hdmi_data.enc_out_encoding =
> +			hdmi->plat_data->get_enc_out_encoding(data);
> +	else if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
> +		 (hdmi->vic == 21) || (hdmi->vic == 22) ||
> +		 (hdmi->vic == 2) || (hdmi->vic == 3) ||
> +		 (hdmi->vic == 17) || (hdmi->vic == 18))
>  		hdmi->hdmi_data.enc_out_encoding = V4L2_YCBCR_ENC_601;
>  	else
>  		hdmi->hdmi_data.enc_out_encoding = V4L2_YCBCR_ENC_709;
> @@ -1833,21 +1837,31 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  	hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;
>  
>  	/* TOFIX: Get input format from plat data or fallback to RGB888 */
> -	if (hdmi->plat_data->input_bus_format)
> +	if (hdmi->plat_data->get_input_bus_format)
> +		hdmi->hdmi_data.enc_in_bus_format =
> +			hdmi->plat_data->get_input_bus_format(data);
> +	else if (hdmi->plat_data->input_bus_format)
>  		hdmi->hdmi_data.enc_in_bus_format =
>  			hdmi->plat_data->input_bus_format;
>  	else
>  		hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>  
>  	/* TOFIX: Get input encoding from plat data or fallback to none */
> -	if (hdmi->plat_data->input_bus_encoding)
> +	if (hdmi->plat_data->get_enc_in_encoding)
> +		hdmi->hdmi_data.enc_in_encoding =
> +			hdmi->plat_data->get_enc_in_encoding(data);
> +	else if (hdmi->plat_data->input_bus_encoding)
>  		hdmi->hdmi_data.enc_in_encoding =
>  			hdmi->plat_data->input_bus_encoding;
>  	else
>  		hdmi->hdmi_data.enc_in_encoding = V4L2_YCBCR_ENC_DEFAULT;
>  
>  	/* TOFIX: Default to RGB888 output format */
> -	hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +	if (hdmi->plat_data->get_output_bus_format)
> +		hdmi->hdmi_data.enc_out_bus_format =
> +			hdmi->plat_data->get_output_bus_format(data);
> +	else
> +		hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>  
>  	hdmi->hdmi_data.pix_repet_factor = 0;
>  	hdmi->hdmi_data.hdcp_enable = 0;
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 7a02744ce0bc..2e797f782c51 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -142,6 +142,11 @@ struct dw_hdmi_plat_data {
>  	int (*configure_phy)(struct dw_hdmi *hdmi,
>  			     const struct dw_hdmi_plat_data *pdata,
>  			     unsigned long mpixelclock);
> +
> +	unsigned long (*get_input_bus_format)(void *data);
> +	unsigned long (*get_output_bus_format)(void *data);
> +	unsigned long (*get_enc_in_encoding)(void *data);
> +	unsigned long (*get_enc_out_encoding)(void *data);
>  };
>  
>  struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,