Re: [PATCH 1/2] drm: bridge: dw-hdmi: Pass dw_hdmi pointer to .mode_valid() operation

[Neil Armstrong <narmstrong@baylibre.com>]

On 14/05/2020 17:28, Laurent Pinchart wrote:
> Hi Neil,
> 
> On Thu, May 14, 2020 at 11:15:52AM +0200, Neil Armstrong wrote:
>> On 14/05/2020 03:17, Laurent Pinchart wrote:
>>> Platform glue drivers for dw_hdmi may need to access device-specific
>>> data from their .mode_valid() implementation. They currently have no
>>> clean way to do so, and one driver hacks around it by accessing the
>>> dev_private data of the drm_device retrieved from the connector.
>>>
>>> Pass the dw_hdmi pointer to .mode_valid() in order give context
>>> information to drivers, and add a dw_hdmi_device() to retrieve the
>>> struct device related to the context.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c   | 13 ++++++++++++-
>>>  drivers/gpu/drm/imx/dw_hdmi-imx.c           |  4 ++--
>>>  drivers/gpu/drm/meson/meson_dw_hdmi.c       |  3 ++-
>>>  drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c      |  2 +-
>>>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  3 ++-
>>>  drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c       |  6 ++++--
>>>  drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h       |  3 ++-
>>>  include/drm/bridge/dw_hdmi.h                |  4 +++-
>>>  8 files changed, 28 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> index 30681398cfb0..97c7a9a4983c 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -2778,7 +2778,8 @@ dw_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
>>>  		return MODE_BAD;
>>>  
>>>  	if (hdmi->plat_data->mode_valid)
>>> -		mode_status = hdmi->plat_data->mode_valid(connector, mode);
>>> +		mode_status = hdmi->plat_data->mode_valid(hdmi, connector,
>>> +							  mode);
>>
>> Can't it pass `struct dw_hdmi *hdmi, void *data` like the phy_ops ?
> 
> We could, if we had a void *data pointer :-) The PHY ops have a void
> *phy_data in dw_hdmi_plat_data, but for .mode_valid() (and
> .configure_phy()) we don't have a data field. Would you add one to
> dw_hdmi_plat_data ? I wonder which of them should be passed to
> .configure_phy() in that case, as it's a PHY-related function, but not
> applicable to vendor PHYs. dw_hdmi_plat_data is quite messy :-S
> 
> I wonder if we could merge all private data. Or do you think the PHY ops
> and the other ops would be handled by different pieces of code that
> would each require their own private data ?


No reason to be separated, I think I did a separation to prepare a switch
to extract PHY handling out of dw-hdmi, but I'm not sure it's possible/viable
for the currently handled PHYs.

Neil

> 
>>>  
>>>  	return mode_status;
>>>  }
>>> @@ -3395,6 +3396,16 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
>>>  		i2c_put_adapter(hdmi->ddc);
>>>  }
>>>  
>>> +/*
>>> + * Retrieve the device passed to the dw_hdmi_probe() or dw_hdmi_bind()
>>> + * functions.
>>> + */
>>> +struct device *dw_hdmi_device(struct dw_hdmi *hdmi)
>>> +{
>>> +	return hdmi->dev;
>>> +}
>>> +EXPORT_SYMBOL_GPL(dw_hdmi_device);
>>
>> This looks really hackish, passing data like the phy_ops looks cleaner.
>>
>>> +
>>>  /* -----------------------------------------------------------------------------
>>>   * Probe/remove API, used from platforms based on the DRM bridge API.
>>>   */
>>> diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
>>> index ba4ca17fd4d8..ff5b03a4a86a 100644
>>> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
>>> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
>>> @@ -145,7 +145,7 @@ static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs =
>>>  };
>>>  
>>>  static enum drm_mode_status
>>> -imx6q_hdmi_mode_valid(struct drm_connector *con,
>>> +imx6q_hdmi_mode_valid(struct dw_hdmi *hdmi, struct drm_connector *con,
>>>  		      const struct drm_display_mode *mode)
>>>  {
>>>  	if (mode->clock < 13500)
>>> @@ -158,7 +158,7 @@ imx6q_hdmi_mode_valid(struct drm_connector *con,
>>>  }
>>>  
>>>  static enum drm_mode_status
>>> -imx6dl_hdmi_mode_valid(struct drm_connector *con,
>>> +imx6dl_hdmi_mode_valid(struct dw_hdmi *hdmi, struct drm_connector *con,
>>>  		       const struct drm_display_mode *mode)
>>>  {
>>>  	if (mode->clock < 13500)
>>> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>>> index 5be963e9db05..174d45ecdeda 100644
>>> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
>>> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>>> @@ -630,7 +630,8 @@ static irqreturn_t dw_hdmi_top_thread_irq(int irq, void *dev_id)
>>>  }
>>>  
>>>  static enum drm_mode_status
>>> -dw_hdmi_mode_valid(struct drm_connector *connector,
>>> +dw_hdmi_mode_valid(struct dw_hdmi *hdmi,
>>> +		   struct drm_connector *connector,
>>>  		   const struct drm_display_mode *mode)
>>>  {
>>>  	struct meson_drm *priv = connector->dev->dev_private;
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
>>> index 452461dc96f2..3d2fdbeeb82d 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
>>> @@ -38,7 +38,7 @@ static const struct rcar_hdmi_phy_params rcar_hdmi_phy_params[] = {
>>>  };
>>>  
>>>  static enum drm_mode_status
>>> -rcar_hdmi_mode_valid(struct drm_connector *connector,
>>> +rcar_hdmi_mode_valid(struct dw_hdmi *hdmi, struct drm_connector *connector,
>>>  		     const struct drm_display_mode *mode)
>>>  {
>>>  	/*
>>> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>> index 121aa8a63a76..32acfe2c3f58 100644
>>> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>>> @@ -220,7 +220,8 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)
>>>  }
>>>  
>>>  static enum drm_mode_status
>>> -dw_hdmi_rockchip_mode_valid(struct drm_connector *connector,
>>> +dw_hdmi_rockchip_mode_valid(struct dw_hdmi *hdmi,
>>> +			    struct drm_connector *connector,
>>>  			    const struct drm_display_mode *mode)
>>>  {
>>>  	const struct dw_hdmi_mpll_config *mpll_cfg = rockchip_mpll_cfg;
>>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
>>> index 972682bb8000..055ffefd1b60 100644
>>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
>>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
>>> @@ -31,7 +31,8 @@ sun8i_dw_hdmi_encoder_helper_funcs = {
>>>  };
>>>  
>>>  static enum drm_mode_status
>>> -sun8i_dw_hdmi_mode_valid_a83t(struct drm_connector *connector,
>>> +sun8i_dw_hdmi_mode_valid_a83t(struct dw_hdmi *hdmi,
>>> +			      struct drm_connector *connector,
>>>  			      const struct drm_display_mode *mode)
>>>  {
>>>  	if (mode->clock > 297000)
>>> @@ -41,7 +42,8 @@ sun8i_dw_hdmi_mode_valid_a83t(struct drm_connector *connector,
>>>  }
>>>  
>>>  static enum drm_mode_status
>>> -sun8i_dw_hdmi_mode_valid_h6(struct drm_connector *connector,
>>> +sun8i_dw_hdmi_mode_valid_h6(struct dw_hdmi *hdmi,
>>> +			    struct drm_connector *connector,
>>>  			    const struct drm_display_mode *mode)
>>>  {
>>>  	/*
>>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
>>> index 8e64945167e9..f831cb351d72 100644
>>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
>>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
>>> @@ -176,7 +176,8 @@ struct sun8i_hdmi_phy {
>>>  };
>>>  
>>>  struct sun8i_dw_hdmi_quirks {
>>> -	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
>>> +	enum drm_mode_status (*mode_valid)(struct dw_hdmi *hdmi,
>>> +					   struct drm_connector *connector,
>>>  					   const struct drm_display_mode *mode);
>>>  	unsigned int set_rate : 1;
>>>  	unsigned int use_drm_infoframe : 1;
>>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>>> index 0b34a12c4a1c..c98010a53683 100644
>>> --- a/include/drm/bridge/dw_hdmi.h
>>> +++ b/include/drm/bridge/dw_hdmi.h
>>> @@ -124,7 +124,8 @@ struct dw_hdmi_phy_ops {
>>>  
>>>  struct dw_hdmi_plat_data {
>>>  	struct regmap *regm;
>>> -	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
>>> +	enum drm_mode_status (*mode_valid)(struct dw_hdmi *hdmi,
>>> +					   struct drm_connector *connector,
>>>  					   const struct drm_display_mode *mode);
>>>  	unsigned long input_bus_format;
>>>  	unsigned long input_bus_encoding;
>>> @@ -153,6 +154,7 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi);
>>>  struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev,
>>>  			     struct drm_encoder *encoder,
>>>  			     const struct dw_hdmi_plat_data *plat_data);
>>> +struct device *dw_hdmi_device(struct dw_hdmi *hdmi);
>>>  
>>>  void dw_hdmi_resume(struct dw_hdmi *hdmi);
>>>  
>