| Message ID | 1399319548-16567-4-git-send-email-ajaykumar.rs@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 >
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 >>
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 --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);
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(-)