Re: [PATCH 4/5] drm/bridge: ti-tfp410: Add support for the powerdown GPIO

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jyri,

On Tuesday, 11 December 2018 15:01:52 EET Jyri Sarha wrote:
> On 06/12/2018 22:26, Laurent Pinchart wrote:
> > The TFP410 has a powerdown pin that can be connected to a GPIO to
> > control power saving. The DT bindings define a corresponding property,
> > but the driver doesn't implement support for it. Fix that.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> 
> I find the naming of the the gpio bit backwards, but I guess it si fine
> since it is also the name of the pin.

I would have gone for "enable-gpios", but given that we have DTs in the wild 
using the "powerdown-gpios" property (with the omapdrm-specific TFP410 driver 
which uses the same compatible string as the drm_bridge-based driver) I think 
we need to stick to it. As you mentioned this is the pin's name, so it's not 
too big of a deal.

I'll now wait for your review of patch 5/5 before merging these changes :-) 
The plan is to get them in drm-misc for v4.22, please let me know if that 
causes any problem for you. Also, please feel free to review 1/5 and 2/5 if 
you have time.

> Reviewed-by: Jyri Sarha <jsarha@ti.com>
> 
> >  drivers/gpu/drm/bridge/ti-tfp410.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
> > b/drivers/gpu/drm/bridge/ti-tfp410.c index e4280f5af9f5..d25d23cfe3f5
> > 100644
> > --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> > +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> > @@ -32,6 +32,7 @@ struct tfp410 {
> >  	struct i2c_adapter	*ddc;
> >  	struct gpio_desc	*hpd;
> >  	struct delayed_work	hpd_work;
> > +	struct gpio_desc	*powerdown;
> >  	struct device *dev;
> >  };
> > 
> > @@ -139,8 +140,24 @@ static int tfp410_attach(struct drm_bridge *bridge)
> >  	return 0;
> >  }
> > 
> > +static void tfp410_enable(struct drm_bridge *bridge)
> > +{
> > +	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
> > +
> > +	gpiod_set_value_cansleep(dvi->powerdown, 0);
> > +}
> > +
> > +static void tfp410_disable(struct drm_bridge *bridge)
> > +{
> > +	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
> > +
> > +	gpiod_set_value_cansleep(dvi->powerdown, 1);
> > +}
> > +
> >  static const struct drm_bridge_funcs tfp410_bridge_funcs = {
> >  	.attach		= tfp410_attach,
> > +	.enable		= tfp410_enable,
> > +	.disable	= tfp410_disable,
> >  };
> >  
> >  static void tfp410_hpd_work_func(struct work_struct *work)
> > @@ -229,6 +246,13 @@ static int tfp410_init(struct device *dev)
> >  	if (ret)
> >  		goto fail;
> > 
> > +	dvi->powerdown = devm_gpiod_get_optional(dev, "powerdown",
> > +						 GPIOD_OUT_HIGH);
> > +	if (IS_ERR(dvi->powerdown)) {
> > +		dev_err(dev, "failed to parse powerdown gpio\n");
> > +		return PTR_ERR(dvi->powerdown);
> > +	}
> > +
> >  	if (dvi->hpd) {
> >  		INIT_DELAYED_WORK(&dvi->hpd_work, tfp410_hpd_work_func);

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel