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

[Ajay kumar]

Sean,


On Tue, May 6, 2014 at 9:24 PM, Sean Paul <seanpaul at chromium.org> wrote:
> On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs at 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 at 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
>>