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@xxxxxxxxxxxxxxxx>
>>> ---
>>> 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);
>>>
>
