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 ?
> >
> > 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);
> >
--
Regards,
Laurent Pinchart
