diff mbox

[RFC,V2,3/3] drm/bridge: ptn3460: support bridge chaining

Message ID 1399319548-16567-4-git-send-email-ajaykumar.rs@samsung.com
State Superseded
Headers show

Commit Message

Ajay Kumar May 5, 2014, 7:52 p.m. UTC
modify the driver to call the bridge->funcs of the next bridge
in the chain.
We assume that the bridge sitting next to ptn3460 is a bridge-panel.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/gpu/drm/bridge/ptn3460.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Comments

Sean Paul May 6, 2014, 3:54 p.m. UTC | #1
On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
> modify the driver to call the bridge->funcs of the next bridge
> in the chain.
> We assume that the bridge sitting next to ptn3460 is a bridge-panel.
>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  drivers/gpu/drm/bridge/ptn3460.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
> index b171901..969869a 100644
> --- a/drivers/gpu/drm/bridge/ptn3460.c
> +++ b/drivers/gpu/drm/bridge/ptn3460.c
> @@ -126,6 +126,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>                 gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>         }
>
> +       if (bridge->next_bridge)
> +               bridge->next_bridge->funcs->pre_enable(bridge->next_bridge);
>         /*
>          * There's a bug in the PTN chip where it falsely asserts hotplug before
>          * it is fully functional. We're forced to wait for the maximum start up
> @@ -142,6 +144,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>
>  static void ptn3460_enable(struct drm_bridge *bridge)
>  {
> +       if (bridge->next_bridge)
> +               bridge->next_bridge->funcs->enable(bridge->next_bridge);
>  }
>
>  static void ptn3460_disable(struct drm_bridge *bridge)
> @@ -153,6 +157,11 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>
>         ptn_bridge->enabled = false;
>
> +       if (bridge->next_bridge) {
> +               bridge->next_bridge->funcs->disable(bridge->next_bridge);
> +               bridge->next_bridge->funcs->post_disable(bridge->next_bridge);

Shouldn't post_disable be called from ptn_3460_post_disable, instead of here?

> +       }
> +
>         if (gpio_is_valid(ptn_bridge->gpio_rst_n))
>                 gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>
> @@ -197,7 +206,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
>                 return drm_add_edid_modes(connector, ptn_bridge->edid);
>
>         power_off = !ptn_bridge->enabled;
> -       ptn3460_pre_enable(ptn_bridge->bridge);
> +       if (power_off) {
> +               ptn3460_pre_enable(ptn_bridge->bridge);
> +               ptn3460_enable(ptn_bridge->bridge);

In this case, I don't think we need to power on the entire bridge
chain since we're just reading the edid from the bridge chip itself.
So I think I'd prefer to break out the power_on/power_off code from
the bridge chain such that we can just power up the bridge chip, check
the edid and then turn it off.

In other bridges, where we're actually reading the edid from a
downstream source, this decision might be different.



> +       }
>
>         edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
>         if (!edid) {
> @@ -219,8 +231,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
>         num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
>
>  out:
> -       if (power_off)
> +       if (power_off) {
>                 ptn3460_disable(ptn_bridge->bridge);
> +               ptn3460_post_disable(ptn_bridge->bridge);
> +       }
>
>         return num_modes;
>  }
> @@ -318,14 +332,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
>                 goto err;
>         }
>
> -       ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs);
> +       ret = drm_bridge_init(dev, bridge, encoder, &ptn3460_bridge_funcs);
>         if (ret) {
>                 DRM_ERROR("Failed to initialize bridge with drm\n");
>                 goto err;
>         }
>
>         bridge->driver_private = ptn_bridge;
> -       encoder->bridge = bridge;
>
>         ret = drm_connector_init(dev, &ptn_bridge->connector,
>                         &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
> --
> 1.8.1.2
>
Ajay kumar May 6, 2014, 8:03 p.m. UTC | #2
Sean,


On Tue, May 6, 2014 at 9:24 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>> modify the driver to call the bridge->funcs of the next bridge
>> in the chain.
>> We assume that the bridge sitting next to ptn3460 is a bridge-panel.
>>
>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> ---
>>  drivers/gpu/drm/bridge/ptn3460.c | 21 +++++++++++++++++----
>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
>> index b171901..969869a 100644
>> --- a/drivers/gpu/drm/bridge/ptn3460.c
>> +++ b/drivers/gpu/drm/bridge/ptn3460.c
>> @@ -126,6 +126,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>>                 gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>>         }
>>
>> +       if (bridge->next_bridge)
>> +               bridge->next_bridge->funcs->pre_enable(bridge->next_bridge);
>>         /*
>>          * There's a bug in the PTN chip where it falsely asserts hotplug before
>>          * it is fully functional. We're forced to wait for the maximum start up
>> @@ -142,6 +144,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>>
>>  static void ptn3460_enable(struct drm_bridge *bridge)
>>  {
>> +       if (bridge->next_bridge)
>> +               bridge->next_bridge->funcs->enable(bridge->next_bridge);
>>  }
>>
>>  static void ptn3460_disable(struct drm_bridge *bridge)
>> @@ -153,6 +157,11 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>>
>>         ptn_bridge->enabled = false;
>>
>> +       if (bridge->next_bridge) {
>> +               bridge->next_bridge->funcs->disable(bridge->next_bridge);
>> +               bridge->next_bridge->funcs->post_disable(bridge->next_bridge);
>
> Shouldn't post_disable be called from ptn_3460_post_disable, instead of here?
Umm, right. But no point in delaying ->post_disable of the panel(which
switches off power to LCD) since
backlight would be already down in ->disable call itself. So, I
thought of calling post_disable here itself.

>> +       }
>> +
>>         if (gpio_is_valid(ptn_bridge->gpio_rst_n))
>>                 gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>>
>> @@ -197,7 +206,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
>>                 return drm_add_edid_modes(connector, ptn_bridge->edid);
>>
>>         power_off = !ptn_bridge->enabled;
>> -       ptn3460_pre_enable(ptn_bridge->bridge);
>> +       if (power_off) {
>> +               ptn3460_pre_enable(ptn_bridge->bridge);
>> +               ptn3460_enable(ptn_bridge->bridge);
>
> In this case, I don't think we need to power on the entire bridge
> chain since we're just reading the edid from the bridge chip itself.
> So I think I'd prefer to break out the power_on/power_off code from
> the bridge chain such that we can just power up the bridge chip, check
> the edid and then turn it off.
Those extra calls were added to make sure that regulator_enable/disable would
be in sync for the panel. Check the other patch [2/3].
Another way of doing this is to just check if the bridge is off  and
switch it on by
explicitly setting up the gpios here, instead of just calling
ptn3460_pre_enable.

Ajay
> In other bridges, where we're actually reading the edid from a
> downstream source, this decision might be different.
>
>
>
>> +       }
>>
>>         edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
>>         if (!edid) {
>> @@ -219,8 +231,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
>>         num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
>>
>>  out:
>> -       if (power_off)
>> +       if (power_off) {
>>                 ptn3460_disable(ptn_bridge->bridge);
>> +               ptn3460_post_disable(ptn_bridge->bridge);
>> +       }
>>
>>         return num_modes;
>>  }
>> @@ -318,14 +332,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
>>                 goto err;
>>         }
>>
>> -       ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs);
>> +       ret = drm_bridge_init(dev, bridge, encoder, &ptn3460_bridge_funcs);
>>         if (ret) {
>>                 DRM_ERROR("Failed to initialize bridge with drm\n");
>>                 goto err;
>>         }
>>
>>         bridge->driver_private = ptn_bridge;
>> -       encoder->bridge = bridge;
>>
>>         ret = drm_connector_init(dev, &ptn_bridge->connector,
>>                         &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
>> --
>> 1.8.1.2
>>
Sean Paul May 6, 2014, 8:05 p.m. UTC | #3
On Tue, May 6, 2014 at 1:03 PM, Ajay kumar <ajaynumb@gmail.com> wrote:
> Sean,
>
>
> On Tue, May 6, 2014 at 9:24 PM, Sean Paul <seanpaul@chromium.org> wrote:
>> On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>>> modify the driver to call the bridge->funcs of the next bridge
>>> in the chain.
>>> We assume that the bridge sitting next to ptn3460 is a bridge-panel.
>>>
>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>>> ---
>>>  drivers/gpu/drm/bridge/ptn3460.c | 21 +++++++++++++++++----
>>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
>>> index b171901..969869a 100644
>>> --- a/drivers/gpu/drm/bridge/ptn3460.c
>>> +++ b/drivers/gpu/drm/bridge/ptn3460.c
>>> @@ -126,6 +126,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>>>                 gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>>>         }
>>>
>>> +       if (bridge->next_bridge)
>>> +               bridge->next_bridge->funcs->pre_enable(bridge->next_bridge);
>>>         /*
>>>          * There's a bug in the PTN chip where it falsely asserts hotplug before
>>>          * it is fully functional. We're forced to wait for the maximum start up
>>> @@ -142,6 +144,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>>>
>>>  static void ptn3460_enable(struct drm_bridge *bridge)
>>>  {
>>> +       if (bridge->next_bridge)
>>> +               bridge->next_bridge->funcs->enable(bridge->next_bridge);
>>>  }
>>>
>>>  static void ptn3460_disable(struct drm_bridge *bridge)
>>> @@ -153,6 +157,11 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>>>
>>>         ptn_bridge->enabled = false;
>>>
>>> +       if (bridge->next_bridge) {
>>> +               bridge->next_bridge->funcs->disable(bridge->next_bridge);
>>> +               bridge->next_bridge->funcs->post_disable(bridge->next_bridge);
>>
>> Shouldn't post_disable be called from ptn_3460_post_disable, instead of here?
> Umm, right. But no point in delaying ->post_disable of the panel(which
> switches off power to LCD) since
> backlight would be already down in ->disable call itself. So, I
> thought of calling post_disable here itself.
>
>>> +       }
>>> +
>>>         if (gpio_is_valid(ptn_bridge->gpio_rst_n))
>>>                 gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>>>
>>> @@ -197,7 +206,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
>>>                 return drm_add_edid_modes(connector, ptn_bridge->edid);
>>>
>>>         power_off = !ptn_bridge->enabled;
>>> -       ptn3460_pre_enable(ptn_bridge->bridge);
>>> +       if (power_off) {
>>> +               ptn3460_pre_enable(ptn_bridge->bridge);
>>> +               ptn3460_enable(ptn_bridge->bridge);
>>
>> In this case, I don't think we need to power on the entire bridge
>> chain since we're just reading the edid from the bridge chip itself.
>> So I think I'd prefer to break out the power_on/power_off code from
>> the bridge chain such that we can just power up the bridge chip, check
>> the edid and then turn it off.
> Those extra calls were added to make sure that regulator_enable/disable would
> be in sync for the panel. Check the other patch [2/3].

Sure, but we don't need the panel to read the edid. This bridge in
particular provides the edid itself, so in this case, we should leave
the panel off.

Sean


> Another way of doing this is to just check if the bridge is off  and
> switch it on by
> explicitly setting up the gpios here, instead of just calling
> ptn3460_pre_enable.
>
> Ajay
>> In other bridges, where we're actually reading the edid from a
>> downstream source, this decision might be different.
>>
>>
>>
>>> +       }
>>>
>>>         edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
>>>         if (!edid) {
>>> @@ -219,8 +231,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
>>>         num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
>>>
>>>  out:
>>> -       if (power_off)
>>> +       if (power_off) {
>>>                 ptn3460_disable(ptn_bridge->bridge);
>>> +               ptn3460_post_disable(ptn_bridge->bridge);
>>> +       }
>>>
>>>         return num_modes;
>>>  }
>>> @@ -318,14 +332,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
>>>                 goto err;
>>>         }
>>>
>>> -       ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs);
>>> +       ret = drm_bridge_init(dev, bridge, encoder, &ptn3460_bridge_funcs);
>>>         if (ret) {
>>>                 DRM_ERROR("Failed to initialize bridge with drm\n");
>>>                 goto err;
>>>         }
>>>
>>>         bridge->driver_private = ptn_bridge;
>>> -       encoder->bridge = bridge;
>>>
>>>         ret = drm_connector_init(dev, &ptn_bridge->connector,
>>>                         &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
>>> --
>>> 1.8.1.2
>>>
diff mbox

Patch

diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
index b171901..969869a 100644
--- a/drivers/gpu/drm/bridge/ptn3460.c
+++ b/drivers/gpu/drm/bridge/ptn3460.c
@@ -126,6 +126,8 @@  static void ptn3460_pre_enable(struct drm_bridge *bridge)
 		gpio_set_value(ptn_bridge->gpio_rst_n, 1);
 	}
 
+	if (bridge->next_bridge)
+		bridge->next_bridge->funcs->pre_enable(bridge->next_bridge);
 	/*
 	 * There's a bug in the PTN chip where it falsely asserts hotplug before
 	 * it is fully functional. We're forced to wait for the maximum start up
@@ -142,6 +144,8 @@  static void ptn3460_pre_enable(struct drm_bridge *bridge)
 
 static void ptn3460_enable(struct drm_bridge *bridge)
 {
+	if (bridge->next_bridge)
+		bridge->next_bridge->funcs->enable(bridge->next_bridge);
 }
 
 static void ptn3460_disable(struct drm_bridge *bridge)
@@ -153,6 +157,11 @@  static void ptn3460_disable(struct drm_bridge *bridge)
 
 	ptn_bridge->enabled = false;
 
+	if (bridge->next_bridge) {
+		bridge->next_bridge->funcs->disable(bridge->next_bridge);
+		bridge->next_bridge->funcs->post_disable(bridge->next_bridge);
+	}
+
 	if (gpio_is_valid(ptn_bridge->gpio_rst_n))
 		gpio_set_value(ptn_bridge->gpio_rst_n, 1);
 
@@ -197,7 +206,10 @@  int ptn3460_get_modes(struct drm_connector *connector)
 		return drm_add_edid_modes(connector, ptn_bridge->edid);
 
 	power_off = !ptn_bridge->enabled;
-	ptn3460_pre_enable(ptn_bridge->bridge);
+	if (power_off) {
+		ptn3460_pre_enable(ptn_bridge->bridge);
+		ptn3460_enable(ptn_bridge->bridge);
+	}
 
 	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
 	if (!edid) {
@@ -219,8 +231,10 @@  int ptn3460_get_modes(struct drm_connector *connector)
 	num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
 
 out:
-	if (power_off)
+	if (power_off) {
 		ptn3460_disable(ptn_bridge->bridge);
+		ptn3460_post_disable(ptn_bridge->bridge);
+	}
 
 	return num_modes;
 }
@@ -318,14 +332,13 @@  int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
 		goto err;
 	}
 
-	ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs);
+	ret = drm_bridge_init(dev, bridge, encoder, &ptn3460_bridge_funcs);
 	if (ret) {
 		DRM_ERROR("Failed to initialize bridge with drm\n");
 		goto err;
 	}
 
 	bridge->driver_private = ptn_bridge;
-	encoder->bridge = bridge;
 
 	ret = drm_connector_init(dev, &ptn_bridge->connector,
 			&ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);