| Message ID | 20161020034344.14154-2-wens@csie.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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
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) { >
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); > +} > + -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 --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) {
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(+)