[PATCH v2 15/50] drm/bridge: tfp410: Replace manual connector handling with bridge

[Boris Brezillon]

On Thu, 22 Aug 2019 19:54:56 +0300
Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:

> Hi Boris,
> 
> On Thu, Aug 22, 2019 at 06:36:45PM +0200, Boris Brezillon wrote:
> > On Tue, 20 Aug 2019 04:16:46 +0300 Laurent Pinchart wrote:
> >   
> > > Now that a driver is available for display connectors, replace the
> > > manual connector handling code with usage of the DRM bridge API. The
> > > tfp410 driver doesn't deal with the display connector directly anymore,
> > > but still delegates drm_connector operations to the next bridge. This
> > > brings us one step closer to having the tfp410 driver handling the
> > > TFP410 only.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  drivers/gpu/drm/bridge/ti-tfp410.c | 195 ++++++++++-------------------
> > >  1 file changed, 68 insertions(+), 127 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> > > index 4a468f44ef69..65651ae6c553 100644
> > > --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> > > +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> > > @@ -4,14 +4,12 @@
> > >   * Author: Jyri Sarha <jsarha at ti.com>
> > >   */
> > >  
> > > -#include <linux/delay.h>
> > > -#include <linux/fwnode.h>
> > >  #include <linux/gpio/consumer.h>
> > >  #include <linux/i2c.h>
> > > -#include <linux/irq.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of_graph.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/workqueue.h>
> > >  
> > >  #include <drm/drm_atomic_helper.h>
> > >  #include <drm/drm_bridge.h>
> > > @@ -24,16 +22,13 @@
> > >  struct tfp410 {
> > >  	struct drm_bridge	bridge;
> > >  	struct drm_connector	connector;
> > > -	unsigned int		connector_type;
> > >  
> > >  	u32			bus_format;
> > > -	struct i2c_adapter	*ddc;
> > > -	struct gpio_desc	*hpd;
> > > -	int			hpd_irq;
> > >  	struct delayed_work	hpd_work;
> > >  	struct gpio_desc	*powerdown;
> > >  
> > >  	struct drm_bridge_timings timings;
> > > +	struct drm_bridge	*next_bridge;
> > >  
> > >  	struct device *dev;
> > >  };
> > > @@ -56,10 +51,10 @@ static int tfp410_get_modes(struct drm_connector *connector)
> > >  	struct edid *edid;
> > >  	int ret;
> > >  
> > > -	if (!dvi->ddc)
> > > +	if (!(dvi->next_bridge->ops & DRM_BRIDGE_OP_EDID))
> > >  		goto fallback;
> > >  
> > > -	edid = drm_get_edid(connector, dvi->ddc);
> > > +	edid = dvi->next_bridge->funcs->get_edid(dvi->next_bridge, connector);  
> > 
> > Can we create a drm_bridge_get_edid() wrapper for that?
> > Something like:
> > 
> > int drm_bridge_get_edid(struct drm_bridge *bridge,
> > 			struct drm_connector *conn)
> > {
> > 	if (!(dvi->next_bridge->ops & DRM_BRIDGE_OP_EDID))
> > 		return -ENOTSUPP;  
> 
> I assume you mean ERR_PTR(-ENOTSUPP) with the return type changed to
> struct drm_edid *.
> 
> > 
> > 	return bridge->funcs->get_edid(bridge, connector);
> > }  
> 
> I've thought about it, but I'm not sure it's worth it. These operations
> are not meant to be called manually by bridges like in here. This is an
> interim solution until support for drm_connector can be dropped from the
> tfp410 driver, once its users will be converted. Do you think I should
> still create a wrapper (which I would make static inline then) ?

Well, this conversion is likely to take time, not to mention that other
drivers will go through the same process. And every time a bridge
driver is converted, it requires patching all display controller drivers
that are known to be connected to it before you can get rid of this
temporary solution. So yes, I still think it's worth adding those
helpers, maybe with a comment explaining that they should only be used
during the conversion phase (IOW, until the driver starts rejecting
the !DRM_BRIDGE_ATTACH_NO_CONNECTOR case).