diff mbox

[v2,1/8] drm/bridge: rgb-to-vga: Support an enable GPIO

Message ID 20161020034344.14154-2-wens@csie.org (mailing list archive)
State New, archived
Headers show

Commit Message

Chen-Yu Tsai Oct. 20, 2016, 3:43 a.m. UTC
Some rgb-to-vga bridges have an enable GPIO, either directly tied to
an enable pin on the bridge IC, or indirectly controlling a power
switch.

Add support for it.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 .../bindings/display/bridge/dumb-vga-dac.txt       |  2 ++
 drivers/gpu/drm/bridge/dumb-vga-dac.c              | 28 ++++++++++++++++++++++
 2 files changed, 30 insertions(+)

Comments

Maxime Ripard Oct. 20, 2016, 1 p.m. UTC | #1
On Thu, Oct 20, 2016 at 11:43:37AM +0800, Chen-Yu Tsai wrote:
> Some rgb-to-vga bridges have an enable GPIO, either directly tied to
> an enable pin on the bridge IC, or indirectly controlling a power
> switch.
> 
> Add support for it.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
Maxime
Archit Taneja Oct. 25, 2016, 8:09 a.m. UTC | #2
Hi,

On 10/20/2016 09:13 AM, Chen-Yu Tsai wrote:
> Some rgb-to-vga bridges have an enable GPIO, either directly tied to
> an enable pin on the bridge IC, or indirectly controlling a power
> switch.
>
> Add support for it.

Does the bridge on your platform have an active/passive DAC, or is it a
smarter encoder chip that is capable of doing more? If so, it might be
good to have a separate DT compatible string to it, like what's done
in the patch titled:

drm: bridge: vga-dac: Add adi,adv7123 compatible string

so that we can switch to a different driver later if needed.

Thanks,
Archit

>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  .../bindings/display/bridge/dumb-vga-dac.txt       |  2 ++
>  drivers/gpu/drm/bridge/dumb-vga-dac.c              | 28 ++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> index 003bc246a270..d3484822bf77 100644
> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> @@ -16,6 +16,8 @@ graph bindings specified in Documentation/devicetree/bindings/graph.txt.
>  - Video port 0 for RGB input
>  - Video port 1 for VGA output
>
> +Optional properties:
> +- enable-gpios: GPIO pin to enable or disable the bridge
>
>  Example
>  -------
> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> index afec232185a7..b487e5e9b56d 100644
> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
> @@ -10,6 +10,7 @@
>   * the License, or (at your option) any later version.
>   */
>
> +#include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/of_graph.h>
>
> @@ -23,6 +24,7 @@ struct dumb_vga {
>  	struct drm_connector	connector;
>
>  	struct i2c_adapter	*ddc;
> +	struct gpio_desc	*enable_gpio;
>  };
>
>  static inline struct dumb_vga *
> @@ -124,8 +126,26 @@ static int dumb_vga_attach(struct drm_bridge *bridge)
>  	return 0;
>  }
>
> +static void dumb_vga_enable(struct drm_bridge *bridge)
> +{
> +	struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
> +
> +	if (vga->enable_gpio)
> +		gpiod_set_value_cansleep(vga->enable_gpio, 1);
> +}
> +
> +static void dumb_vga_disable(struct drm_bridge *bridge)
> +{
> +	struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
> +
> +	if (vga->enable_gpio)
> +		gpiod_set_value_cansleep(vga->enable_gpio, 0);
> +}
> +
>  static const struct drm_bridge_funcs dumb_vga_bridge_funcs = {
>  	.attach		= dumb_vga_attach,
> +	.enable		= dumb_vga_enable,
> +	.disable	= dumb_vga_disable,
>  };
>
>  static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev)
> @@ -169,6 +189,14 @@ static int dumb_vga_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, vga);
>
> +	vga->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> +						   GPIOD_OUT_LOW);
> +	if (IS_ERR(vga->enable_gpio)) {
> +		ret = PTR_ERR(vga->enable_gpio);
> +		dev_err(&pdev->dev, "failed to request GPIO: %d\n", ret);
> +		return ret;
> +	}
> +
>  	vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev);
>  	if (IS_ERR(vga->ddc)) {
>  		if (PTR_ERR(vga->ddc) == -ENODEV) {
>
Chen-Yu Tsai Oct. 25, 2016, 8:59 a.m. UTC | #3
On Tue, Oct 25, 2016 at 4:09 PM, Archit Taneja <architt@codeaurora.org> wrote:
> Hi,
>
> On 10/20/2016 09:13 AM, Chen-Yu Tsai wrote:
>>
>> Some rgb-to-vga bridges have an enable GPIO, either directly tied to
>> an enable pin on the bridge IC, or indirectly controlling a power
>> switch.
>>
>> Add support for it.
>
>
> Does the bridge on your platform have an active/passive DAC, or is it a
> smarter encoder chip that is capable of doing more? If so, it might be
> good to have a separate DT compatible string to it, like what's done
> in the patch titled:
>
> drm: bridge: vga-dac: Add adi,adv7123 compatible string
>
> so that we can switch to a different driver later if needed.

The chip is GM7123. It is not configurable. It just takes the LCD RGB/SYNC
signals and converts them to analog. The only things you can change are
putting it into sleep mode and tweaking the output drive strength by
changing the external reference resistor. The latter would be a hardware
design decision. I would say this qualifies as "dumb".

I revisited the board schematics, and the enable GPIO actually toggles
an external LDO regulator. So this might be better modeled as a regulator
supply?


Thanks
ChenYu

>
> Thanks,
> Archit
>
>
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>  .../bindings/display/bridge/dumb-vga-dac.txt       |  2 ++
>>  drivers/gpu/drm/bridge/dumb-vga-dac.c              | 28
>> ++++++++++++++++++++++
>>  2 files changed, 30 insertions(+)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> index 003bc246a270..d3484822bf77 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>> @@ -16,6 +16,8 @@ graph bindings specified in
>> Documentation/devicetree/bindings/graph.txt.
>>  - Video port 0 for RGB input
>>  - Video port 1 for VGA output
>>
>> +Optional properties:
>> +- enable-gpios: GPIO pin to enable or disable the bridge
>>
>>  Example
>>  -------
>> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> b/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> index afec232185a7..b487e5e9b56d 100644
>> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
>> @@ -10,6 +10,7 @@
>>   * the License, or (at your option) any later version.
>>   */
>>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/module.h>
>>  #include <linux/of_graph.h>
>>
>> @@ -23,6 +24,7 @@ struct dumb_vga {
>>         struct drm_connector    connector;
>>
>>         struct i2c_adapter      *ddc;
>> +       struct gpio_desc        *enable_gpio;
>>  };
>>
>>  static inline struct dumb_vga *
>> @@ -124,8 +126,26 @@ static int dumb_vga_attach(struct drm_bridge *bridge)
>>         return 0;
>>  }
>>
>> +static void dumb_vga_enable(struct drm_bridge *bridge)
>> +{
>> +       struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
>> +
>> +       if (vga->enable_gpio)
>> +               gpiod_set_value_cansleep(vga->enable_gpio, 1);
>> +}
>> +
>> +static void dumb_vga_disable(struct drm_bridge *bridge)
>> +{
>> +       struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
>> +
>> +       if (vga->enable_gpio)
>> +               gpiod_set_value_cansleep(vga->enable_gpio, 0);
>> +}
>> +
>>  static const struct drm_bridge_funcs dumb_vga_bridge_funcs = {
>>         .attach         = dumb_vga_attach,
>> +       .enable         = dumb_vga_enable,
>> +       .disable        = dumb_vga_disable,
>>  };
>>
>>  static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev)
>> @@ -169,6 +189,14 @@ static int dumb_vga_probe(struct platform_device
>> *pdev)
>>                 return -ENOMEM;
>>         platform_set_drvdata(pdev, vga);
>>
>> +       vga->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
>> +                                                  GPIOD_OUT_LOW);
>> +       if (IS_ERR(vga->enable_gpio)) {
>> +               ret = PTR_ERR(vga->enable_gpio);
>> +               dev_err(&pdev->dev, "failed to request GPIO: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>>         vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev);
>>         if (IS_ERR(vga->ddc)) {
>>                 if (PTR_ERR(vga->ddc) == -ENODEV) {
>>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
Rob Herring Oct. 26, 2016, 10:13 p.m. UTC | #4
On Thu, Oct 20, 2016 at 11:43:37AM +0800, Chen-Yu Tsai wrote:
> Some rgb-to-vga bridges have an enable GPIO, either directly tied to
> an enable pin on the bridge IC, or indirectly controlling a power
> switch.
> 
> Add support for it.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  .../bindings/display/bridge/dumb-vga-dac.txt       |  2 ++
>  drivers/gpu/drm/bridge/dumb-vga-dac.c              | 28 ++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> index 003bc246a270..d3484822bf77 100644
> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> @@ -16,6 +16,8 @@ graph bindings specified in Documentation/devicetree/bindings/graph.txt.
>  - Video port 0 for RGB input
>  - Video port 1 for VGA output
>  
> +Optional properties:
> +- enable-gpios: GPIO pin to enable or disable the bridge

This should also define the active state.

> +static void dumb_vga_enable(struct drm_bridge *bridge)
> +{
> +	struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
> +
> +	if (vga->enable_gpio)
> +		gpiod_set_value_cansleep(vga->enable_gpio, 1);

So the driver should allow either active high or low.

> +}
> +
> +static void dumb_vga_disable(struct drm_bridge *bridge)
> +{
> +	struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
> +
> +	if (vga->enable_gpio)
> +		gpiod_set_value_cansleep(vga->enable_gpio, 0);
> +}
> +
Maxime Ripard Oct. 26, 2016, 10:17 p.m. UTC | #5
Hi Rob,

On Wed, Oct 26, 2016 at 05:13:46PM -0500, Rob Herring wrote:
> On Thu, Oct 20, 2016 at 11:43:37AM +0800, Chen-Yu Tsai wrote:
> > Some rgb-to-vga bridges have an enable GPIO, either directly tied to
> > an enable pin on the bridge IC, or indirectly controlling a power
> > switch.
> > 
> > Add support for it.
> > 
> > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > ---
> >  .../bindings/display/bridge/dumb-vga-dac.txt       |  2 ++
> >  drivers/gpu/drm/bridge/dumb-vga-dac.c              | 28 ++++++++++++++++++++++
> >  2 files changed, 30 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> > index 003bc246a270..d3484822bf77 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> > +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> > @@ -16,6 +16,8 @@ graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> >  - Video port 0 for RGB input
> >  - Video port 1 for VGA output
> >  
> > +Optional properties:
> > +- enable-gpios: GPIO pin to enable or disable the bridge
> 
> This should also define the active state.
> 
> > +static void dumb_vga_enable(struct drm_bridge *bridge)
> > +{
> > +	struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
> > +
> > +	if (vga->enable_gpio)
> > +		gpiod_set_value_cansleep(vga->enable_gpio, 1);
> 
> So the driver should allow either active high or low.

You mean like having a enable-active-high property? Isn't that
redundant with the GPIO flags?

Maxime
Archit Taneja Oct. 27, 2016, 6:40 a.m. UTC | #6
On 10/25/2016 02:29 PM, Chen-Yu Tsai wrote:
> On Tue, Oct 25, 2016 at 4:09 PM, Archit Taneja <architt@codeaurora.org> wrote:
>> Hi,
>>
>> On 10/20/2016 09:13 AM, Chen-Yu Tsai wrote:
>>>
>>> Some rgb-to-vga bridges have an enable GPIO, either directly tied to
>>> an enable pin on the bridge IC, or indirectly controlling a power
>>> switch.
>>>
>>> Add support for it.
>>
>>
>> Does the bridge on your platform have an active/passive DAC, or is it a
>> smarter encoder chip that is capable of doing more? If so, it might be
>> good to have a separate DT compatible string to it, like what's done
>> in the patch titled:
>>
>> drm: bridge: vga-dac: Add adi,adv7123 compatible string
>>
>> so that we can switch to a different driver later if needed.
>
> The chip is GM7123. It is not configurable. It just takes the LCD RGB/SYNC
> signals and converts them to analog. The only things you can change are
> putting it into sleep mode and tweaking the output drive strength by

Is sleep mode the thing that's controlled by this gpio?


> changing the external reference resistor. The latter would be a hardware
> design decision. I would say this qualifies as "dumb".

Yeah, I agree. I'd want feedback from Laurent too, since he had comments
on the usage of the original dumb-vga-dac driver.

>
> I revisited the board schematics, and the enable GPIO actually toggles
> an external LDO regulator. So this might be better modeled as a regulator
> supply?

If you model it as a regulator, how would you toggle the GPIO on your
platform?

Looking at the chip pin out, there is a 3.3V VDD supply needed for the
chip, so it would be good to have an optional 'power' regulator supply
anyway.

Archit

>
>
> Thanks
> ChenYu
>
>>
>> Thanks,
>> Archit
>>
>>
>>>
>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>>> ---
>>>  .../bindings/display/bridge/dumb-vga-dac.txt       |  2 ++
>>>  drivers/gpu/drm/bridge/dumb-vga-dac.c              | 28
>>> ++++++++++++++++++++++
>>>  2 files changed, 30 insertions(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>>> b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>>> index 003bc246a270..d3484822bf77 100644
>>> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>>> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>>> @@ -16,6 +16,8 @@ graph bindings specified in
>>> Documentation/devicetree/bindings/graph.txt.
>>>  - Video port 0 for RGB input
>>>  - Video port 1 for VGA output
>>>
>>> +Optional properties:
>>> +- enable-gpios: GPIO pin to enable or disable the bridge
>>>
>>>  Example
>>>  -------
>>> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
>>> b/drivers/gpu/drm/bridge/dumb-vga-dac.c
>>> index afec232185a7..b487e5e9b56d 100644
>>> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
>>> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
>>> @@ -10,6 +10,7 @@
>>>   * the License, or (at your option) any later version.
>>>   */
>>>
>>> +#include <linux/gpio/consumer.h>
>>>  #include <linux/module.h>
>>>  #include <linux/of_graph.h>
>>>
>>> @@ -23,6 +24,7 @@ struct dumb_vga {
>>>         struct drm_connector    connector;
>>>
>>>         struct i2c_adapter      *ddc;
>>> +       struct gpio_desc        *enable_gpio;
>>>  };
>>>
>>>  static inline struct dumb_vga *
>>> @@ -124,8 +126,26 @@ static int dumb_vga_attach(struct drm_bridge *bridge)
>>>         return 0;
>>>  }
>>>
>>> +static void dumb_vga_enable(struct drm_bridge *bridge)
>>> +{
>>> +       struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
>>> +
>>> +       if (vga->enable_gpio)
>>> +               gpiod_set_value_cansleep(vga->enable_gpio, 1);
>>> +}
>>> +
>>> +static void dumb_vga_disable(struct drm_bridge *bridge)
>>> +{
>>> +       struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
>>> +
>>> +       if (vga->enable_gpio)
>>> +               gpiod_set_value_cansleep(vga->enable_gpio, 0);
>>> +}
>>> +
>>>  static const struct drm_bridge_funcs dumb_vga_bridge_funcs = {
>>>         .attach         = dumb_vga_attach,
>>> +       .enable         = dumb_vga_enable,
>>> +       .disable        = dumb_vga_disable,
>>>  };
>>>
>>>  static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev)
>>> @@ -169,6 +189,14 @@ static int dumb_vga_probe(struct platform_device
>>> *pdev)
>>>                 return -ENOMEM;
>>>         platform_set_drvdata(pdev, vga);
>>>
>>> +       vga->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
>>> +                                                  GPIOD_OUT_LOW);
>>> +       if (IS_ERR(vga->enable_gpio)) {
>>> +               ret = PTR_ERR(vga->enable_gpio);
>>> +               dev_err(&pdev->dev, "failed to request GPIO: %d\n", ret);
>>> +               return ret;
>>> +       }
>>> +
>>>         vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev);
>>>         if (IS_ERR(vga->ddc)) {
>>>                 if (PTR_ERR(vga->ddc) == -ENODEV) {
>>>
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
Chen-Yu Tsai Oct. 27, 2016, 6:52 a.m. UTC | #7
On Thu, Oct 27, 2016 at 2:40 PM, Archit Taneja <architt@codeaurora.org> wrote:
>
>
> On 10/25/2016 02:29 PM, Chen-Yu Tsai wrote:
>>
>> On Tue, Oct 25, 2016 at 4:09 PM, Archit Taneja <architt@codeaurora.org>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 10/20/2016 09:13 AM, Chen-Yu Tsai wrote:
>>>>
>>>>
>>>> Some rgb-to-vga bridges have an enable GPIO, either directly tied to
>>>> an enable pin on the bridge IC, or indirectly controlling a power
>>>> switch.
>>>>
>>>> Add support for it.
>>>
>>>
>>>
>>> Does the bridge on your platform have an active/passive DAC, or is it a
>>> smarter encoder chip that is capable of doing more? If so, it might be
>>> good to have a separate DT compatible string to it, like what's done
>>> in the patch titled:
>>>
>>> drm: bridge: vga-dac: Add adi,adv7123 compatible string
>>>
>>> so that we can switch to a different driver later if needed.
>>
>>
>> The chip is GM7123. It is not configurable. It just takes the LCD RGB/SYNC
>> signals and converts them to analog. The only things you can change are
>> putting it into sleep mode and tweaking the output drive strength by
>
>
> Is sleep mode the thing that's controlled by this gpio?

Not on this particular board. The gpio controls the external LDO that
supplies 3.3V to VDD.

>
>> changing the external reference resistor. The latter would be a hardware
>> design decision. I would say this qualifies as "dumb".
>
>
> Yeah, I agree. I'd want feedback from Laurent too, since he had comments
> on the usage of the original dumb-vga-dac driver.
>
>>
>> I revisited the board schematics, and the enable GPIO actually toggles
>> an external LDO regulator. So this might be better modeled as a regulator
>> supply?
>
>
> If you model it as a regulator, how would you toggle the GPIO on your
> platform?
>
> Looking at the chip pin out, there is a 3.3V VDD supply needed for the
> chip, so it would be good to have an optional 'power' regulator supply
> anyway.

Yes, that it what I plan to do. I'll drop the enable-gpios property,
and add a power-supply property for the regulator.

Regards
ChenYu

>
> Archit
>
>
>>
>>
>> Thanks
>> ChenYu
>>
>>>
>>> Thanks,
>>> Archit
>>>
>>>
>>>>
>>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>>>> ---
>>>>  .../bindings/display/bridge/dumb-vga-dac.txt       |  2 ++
>>>>  drivers/gpu/drm/bridge/dumb-vga-dac.c              | 28
>>>> ++++++++++++++++++++++
>>>>  2 files changed, 30 insertions(+)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>>>> b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>>>> index 003bc246a270..d3484822bf77 100644
>>>> --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>>>> +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
>>>> @@ -16,6 +16,8 @@ graph bindings specified in
>>>> Documentation/devicetree/bindings/graph.txt.
>>>>  - Video port 0 for RGB input
>>>>  - Video port 1 for VGA output
>>>>
>>>> +Optional properties:
>>>> +- enable-gpios: GPIO pin to enable or disable the bridge
>>>>
>>>>  Example
>>>>  -------
>>>> diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c
>>>> b/drivers/gpu/drm/bridge/dumb-vga-dac.c
>>>> index afec232185a7..b487e5e9b56d 100644
>>>> --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
>>>> +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
>>>> @@ -10,6 +10,7 @@
>>>>   * the License, or (at your option) any later version.
>>>>   */
>>>>
>>>> +#include <linux/gpio/consumer.h>
>>>>  #include <linux/module.h>
>>>>  #include <linux/of_graph.h>
>>>>
>>>> @@ -23,6 +24,7 @@ struct dumb_vga {
>>>>         struct drm_connector    connector;
>>>>
>>>>         struct i2c_adapter      *ddc;
>>>> +       struct gpio_desc        *enable_gpio;
>>>>  };
>>>>
>>>>  static inline struct dumb_vga *
>>>> @@ -124,8 +126,26 @@ static int dumb_vga_attach(struct drm_bridge
>>>> *bridge)
>>>>         return 0;
>>>>  }
>>>>
>>>> +static void dumb_vga_enable(struct drm_bridge *bridge)
>>>> +{
>>>> +       struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
>>>> +
>>>> +       if (vga->enable_gpio)
>>>> +               gpiod_set_value_cansleep(vga->enable_gpio, 1);
>>>> +}
>>>> +
>>>> +static void dumb_vga_disable(struct drm_bridge *bridge)
>>>> +{
>>>> +       struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
>>>> +
>>>> +       if (vga->enable_gpio)
>>>> +               gpiod_set_value_cansleep(vga->enable_gpio, 0);
>>>> +}
>>>> +
>>>>  static const struct drm_bridge_funcs dumb_vga_bridge_funcs = {
>>>>         .attach         = dumb_vga_attach,
>>>> +       .enable         = dumb_vga_enable,
>>>> +       .disable        = dumb_vga_disable,
>>>>  };
>>>>
>>>>  static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev)
>>>> @@ -169,6 +189,14 @@ static int dumb_vga_probe(struct platform_device
>>>> *pdev)
>>>>                 return -ENOMEM;
>>>>         platform_set_drvdata(pdev, vga);
>>>>
>>>> +       vga->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
>>>> +                                                  GPIOD_OUT_LOW);
>>>> +       if (IS_ERR(vga->enable_gpio)) {
>>>> +               ret = PTR_ERR(vga->enable_gpio);
>>>> +               dev_err(&pdev->dev, "failed to request GPIO: %d\n",
>>>> ret);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>>         vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev);
>>>>         if (IS_ERR(vga->ddc)) {
>>>>                 if (PTR_ERR(vga->ddc) == -ENODEV) {
>>>>
>>>
>>> --
>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>>> a Linux Foundation Collaborative Project
>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
Russell King (Oracle) Oct. 31, 2016, 3:56 p.m. UTC | #8
On Thu, Oct 27, 2016 at 12:17:24AM +0200, Maxime Ripard wrote:
> Hi Rob,
> 
> On Wed, Oct 26, 2016 at 05:13:46PM -0500, Rob Herring wrote:
> > On Thu, Oct 20, 2016 at 11:43:37AM +0800, Chen-Yu Tsai wrote:
> > > Some rgb-to-vga bridges have an enable GPIO, either directly tied to
> > > an enable pin on the bridge IC, or indirectly controlling a power
> > > switch.
> > > 
> > > Add support for it.
> > > 
> > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > > ---
> > >  .../bindings/display/bridge/dumb-vga-dac.txt       |  2 ++
> > >  drivers/gpu/drm/bridge/dumb-vga-dac.c              | 28 ++++++++++++++++++++++
> > >  2 files changed, 30 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> > > index 003bc246a270..d3484822bf77 100644
> > > --- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> > > +++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
> > > @@ -16,6 +16,8 @@ graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> > >  - Video port 0 for RGB input
> > >  - Video port 1 for VGA output
> > >  
> > > +Optional properties:
> > > +- enable-gpios: GPIO pin to enable or disable the bridge
> > 
> > This should also define the active state.
> > 
> > > +static void dumb_vga_enable(struct drm_bridge *bridge)
> > > +{
> > > +	struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
> > > +
> > > +	if (vga->enable_gpio)
> > > +		gpiod_set_value_cansleep(vga->enable_gpio, 1);
> > 
> > So the driver should allow either active high or low.
> 
> You mean like having a enable-active-high property? Isn't that
> redundant with the GPIO flags?

Correct - the gpiod APIs remove the need for drivers to know the
polarity of the signal, handling it inside the GPIO subsystem
instead, controlled either from the gpiod lookup tables in legacy
board files, or the DT specification for the GPIO.

So, in drivers, gpiod_set_value*(, 1) means "set gpio to active
level" and gpiod_set_value*(, 0) means "set gpio to inactive level".

Far nicer than all the bugs we've had with the legacy GPIO interfaces
with random different drivers implementing random different ways to
invert the signal, with all the pain that brings with it when a
platform comes along with a different inversion state.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
index 003bc246a270..d3484822bf77 100644
--- a/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
+++ b/Documentation/devicetree/bindings/display/bridge/dumb-vga-dac.txt
@@ -16,6 +16,8 @@  graph bindings specified in Documentation/devicetree/bindings/graph.txt.
 - Video port 0 for RGB input
 - Video port 1 for VGA output
 
+Optional properties:
+- enable-gpios: GPIO pin to enable or disable the bridge
 
 Example
 -------
diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index afec232185a7..b487e5e9b56d 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -10,6 +10,7 @@ 
  * the License, or (at your option) any later version.
  */
 
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of_graph.h>
 
@@ -23,6 +24,7 @@  struct dumb_vga {
 	struct drm_connector	connector;
 
 	struct i2c_adapter	*ddc;
+	struct gpio_desc	*enable_gpio;
 };
 
 static inline struct dumb_vga *
@@ -124,8 +126,26 @@  static int dumb_vga_attach(struct drm_bridge *bridge)
 	return 0;
 }
 
+static void dumb_vga_enable(struct drm_bridge *bridge)
+{
+	struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
+
+	if (vga->enable_gpio)
+		gpiod_set_value_cansleep(vga->enable_gpio, 1);
+}
+
+static void dumb_vga_disable(struct drm_bridge *bridge)
+{
+	struct dumb_vga *vga = drm_bridge_to_dumb_vga(bridge);
+
+	if (vga->enable_gpio)
+		gpiod_set_value_cansleep(vga->enable_gpio, 0);
+}
+
 static const struct drm_bridge_funcs dumb_vga_bridge_funcs = {
 	.attach		= dumb_vga_attach,
+	.enable		= dumb_vga_enable,
+	.disable	= dumb_vga_disable,
 };
 
 static struct i2c_adapter *dumb_vga_retrieve_ddc(struct device *dev)
@@ -169,6 +189,14 @@  static int dumb_vga_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, vga);
 
+	vga->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
+						   GPIOD_OUT_LOW);
+	if (IS_ERR(vga->enable_gpio)) {
+		ret = PTR_ERR(vga->enable_gpio);
+		dev_err(&pdev->dev, "failed to request GPIO: %d\n", ret);
+		return ret;
+	}
+
 	vga->ddc = dumb_vga_retrieve_ddc(&pdev->dev);
 	if (IS_ERR(vga->ddc)) {
 		if (PTR_ERR(vga->ddc) == -ENODEV) {