Message ID | 20200708103559.132300-1-vkoul@kernel.org |
---|---|
Headers | show |
Series | Add LT9611 DSI to HDMI bridge | expand |
Hello, On 08-07-20, 16:05, Vinod Koul wrote: > Hi, > > This series adds driver and bindings for Lontium LT9611 bridge chip which > takes MIPI DSI as input and HDMI as output. > > This chip can be found in 96boards RB3 platform [1] commonly called DB845c. Any feedback on this series? > > [1]: https://www.96boards.org/product/rb3-platform/ > > Changes in v5: > - make symbol static, reported by kbuild-bot > > Changes in v4: > - Add msm/dsi patch to create connector and support DRM_BRIDGE_ATTACH_NO_CONNECTOR > - Fix comments provided by Sam > > Changes in v3: > - fix kbuild reported error > - rebase on v5.8-rc1 > > Changes in v2: > - Add acks by Rob > - Fix comments reported by Emil and rename the file to lontium-lt9611.c > - Fix comments reported by Laurent on binding and driver > - Add HDMI audio support > > Vinod Koul (4): > dt-bindings: vendor-prefixes: Add Lontium vendor prefix > dt-bindings: display: bridge: Add documentation for LT9611 > drm/bridge: Introduce LT9611 DSI to HDMI bridge > drm/msm/dsi: attach external bridge with > DRM_BRIDGE_ATTACH_NO_CONNECTOR > > .../display/bridge/lontium,lt9611.yaml | 176 +++ > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > drivers/gpu/drm/bridge/Kconfig | 13 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/lontium-lt9611.c | 1142 +++++++++++++++++ > drivers/gpu/drm/msm/dsi/dsi.c | 7 +- > drivers/gpu/drm/msm/dsi/dsi_manager.c | 27 +- > 7 files changed, 1348 insertions(+), 20 deletions(-) > create mode 100644 Documentation/devicetree/bindings/display/bridge/lontium,lt9611.yaml > create mode 100644 drivers/gpu/drm/bridge/lontium-lt9611.c > > -- > 2.26.2
Hi Vinod. Three trivial points below. The rest looks good. With these fixed you can add: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Sam On Wed, Jul 08, 2020 at 04:05:58PM +0530, Vinod Koul wrote: > Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and > I2S port as an input and HDMI port as output > > Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Co-developed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Tested-by: John Stultz <john.stultz@linaro.org> > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/gpu/drm/bridge/Kconfig | 13 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/lontium-lt9611.c | 1142 +++++++++++++++++++++++ > 3 files changed, 1156 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/lontium-lt9611.c > > + > +#include <drm/drm_probe_helper.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_mipi_dsi.h> > +#include <drm/drm_print.h> In alphabetical order. drm_probe_helper needs to be moved. > + > +#define EDID_SEG_SIZE 256 > +#define EDID_LEN 32 > +#define EDID_LOOP 8 > +#define KEY_DDC_ACCS_DONE 0x02 > +#define DDC_NO_ACK 0x50 > + > +static void lt9611_pcr_setup(struct lt9611 *lt9611, const struct drm_display_mode *mode) > +{ > + const struct reg_sequence reg_cfg[] = { > + { 0x830b, 0x01 }, > + { 0x830c, 0x10 }, > + { 0x8348, 0x00 }, > + { 0x8349, 0x81 }, > + > + /* stage 1 */ > + { 0x8321, 0x4a }, > + { 0x8324, 0x71 }, > + { 0x8325, 0x30 }, > + { 0x832a, 0x01 }, > + > + /* stage 2 */ > + { 0x834a, 0x40 }, > + { 0x831d, 0x10 }, > + > + /* MK limit */ > + { 0x832d, 0x38 }, > + { 0x8331, 0x08 }, > + }; > + const struct reg_sequence reg_cfg2[] = { > + { 0x830b, 0x03 }, > + { 0x830c, 0xd0 }, > + { 0x8348, 0x03 }, > + { 0x8349, 0xe0 }, > + { 0x8324, 0x72 }, > + { 0x8325, 0x00 }, > + { 0x832a, 0x01 }, > + { 0x834a, 0x10 }, > + { 0x831d, 0x10 }, > + { 0x8326, 0x37 }, Block above is indented one tab too much. > +static int lt9611_bridge_attach(struct drm_bridge *bridge, > + enum drm_bridge_attach_flags flags) > +{ > + struct lt9611 *lt9611 = bridge_to_lt9611(bridge); > + int ret; > + > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > + dev_err(lt9611->dev, "Fix bridge driver to make connector optional!"); > + return -EINVAL; > + } This should say that the display driver should be fixed. If a display driver expects this bridge to create the connector it would not work.
Hi Sam, On 19-07-20, 19:18, Sam Ravnborg wrote: > Hi Vinod. > > Three trivial points below. > The rest looks good. > > With these fixed you can add: > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Thanks, I will send an update with nits fixed
Hello, On Sun, Jul 19, 2020 at 07:18:06PM +0200, Sam Ravnborg wrote: > Hi Vinod. > > Three trivial points below. > The rest looks good. > > With these fixed you can add: > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > Sam > > On Wed, Jul 08, 2020 at 04:05:58PM +0530, Vinod Koul wrote: > > Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and > > I2S port as an input and HDMI port as output > > > > Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > Co-developed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > Tested-by: John Stultz <john.stultz@linaro.org> > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > > --- > > drivers/gpu/drm/bridge/Kconfig | 13 + > > drivers/gpu/drm/bridge/Makefile | 1 + > > drivers/gpu/drm/bridge/lontium-lt9611.c | 1142 +++++++++++++++++++++++ > > 3 files changed, 1156 insertions(+) > > create mode 100644 drivers/gpu/drm/bridge/lontium-lt9611.c > > > > + > > +#include <drm/drm_probe_helper.h> > > +#include <drm/drm_atomic_helper.h> > > +#include <drm/drm_bridge.h> > > +#include <drm/drm_mipi_dsi.h> > > +#include <drm/drm_print.h> > > In alphabetical order. drm_probe_helper needs to be moved. > > > + > > +#define EDID_SEG_SIZE 256 > > +#define EDID_LEN 32 > > +#define EDID_LOOP 8 > > +#define KEY_DDC_ACCS_DONE 0x02 > > +#define DDC_NO_ACK 0x50 > > + > > > +static void lt9611_pcr_setup(struct lt9611 *lt9611, const struct drm_display_mode *mode) > > +{ > > + const struct reg_sequence reg_cfg[] = { > > + { 0x830b, 0x01 }, > > + { 0x830c, 0x10 }, > > + { 0x8348, 0x00 }, > > + { 0x8349, 0x81 }, > > + > > + /* stage 1 */ > > + { 0x8321, 0x4a }, > > + { 0x8324, 0x71 }, > > + { 0x8325, 0x30 }, > > + { 0x832a, 0x01 }, > > + > > + /* stage 2 */ > > + { 0x834a, 0x40 }, > > + { 0x831d, 0x10 }, > > + > > + /* MK limit */ > > + { 0x832d, 0x38 }, > > + { 0x8331, 0x08 }, > > + }; > > + const struct reg_sequence reg_cfg2[] = { > > + { 0x830b, 0x03 }, > > + { 0x830c, 0xd0 }, > > + { 0x8348, 0x03 }, > > + { 0x8349, 0xe0 }, > > + { 0x8324, 0x72 }, > > + { 0x8325, 0x00 }, > > + { 0x832a, 0x01 }, > > + { 0x834a, 0x10 }, > > + { 0x831d, 0x10 }, > > + { 0x8326, 0x37 }, > > Block above is indented one tab too much. > > > +static int lt9611_bridge_attach(struct drm_bridge *bridge, > > + enum drm_bridge_attach_flags flags) > > +{ > > + struct lt9611 *lt9611 = bridge_to_lt9611(bridge); > > + int ret; > > + > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > > + dev_err(lt9611->dev, "Fix bridge driver to make connector optional!"); > > + return -EINVAL; > > + } > > This should say that the display driver should be fixed. > If a display driver expects this bridge to create the connector > it would not work. Actually, for new bridge drivers, connector creation should be optional from the start. We don't want a failure in that case, the feature should be implemented.
Hi Sam, Laurent, On 22-07-20, 16:14, Laurent Pinchart wrote: > > > +static int lt9611_bridge_attach(struct drm_bridge *bridge, > > > + enum drm_bridge_attach_flags flags) > > > +{ > > > + struct lt9611 *lt9611 = bridge_to_lt9611(bridge); > > > + int ret; > > > + > > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > > > + dev_err(lt9611->dev, "Fix bridge driver to make connector optional!"); > > > + return -EINVAL; > > > + } > > > > This should say that the display driver should be fixed. > > If a display driver expects this bridge to create the connector > > it would not work. > > Actually, for new bridge drivers, connector creation should be optional > from the start. We don't want a failure in that case, the feature should > be implemented. Yes this is causing issues for me now !. The patch 4/4 adds support in msm/dsi but causes regression on qualcomm laptops with ti-sn65dsi86 eDP bridge. I tried to fix that up with changes like Laurent has done for adv7511, but it hasnt worked yet for me (remote debug of this is bit painful) So I am going to drop patch 4 from this series and add support for both DRM_BRIDGE_ATTACH_NO_CONNECTOR set and cleared (like we have in adv7511) so that it can work in both cases, while I fix all bridge uses of msm/dsi and then we can drop these. Does that sound okay to you folks?
On Thu, Jul 23, 2020 at 04:11:51PM +0530, Vinod Koul wrote: > Hi Sam, Laurent, > > On 22-07-20, 16:14, Laurent Pinchart wrote: > > > > +static int lt9611_bridge_attach(struct drm_bridge *bridge, > > > > + enum drm_bridge_attach_flags flags) > > > > +{ > > > > + struct lt9611 *lt9611 = bridge_to_lt9611(bridge); > > > > + int ret; > > > > + > > > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > > > > + dev_err(lt9611->dev, "Fix bridge driver to make connector optional!"); > > > > + return -EINVAL; > > > > + } > > > > > > This should say that the display driver should be fixed. > > > If a display driver expects this bridge to create the connector > > > it would not work. > > > > Actually, for new bridge drivers, connector creation should be optional > > from the start. We don't want a failure in that case, the feature should > > be implemented. > > Yes this is causing issues for me now !. The patch 4/4 adds support in > msm/dsi but causes regression on qualcomm laptops with ti-sn65dsi86 eDP > bridge. I tried to fix that up with changes like Laurent has done for > adv7511, but it hasnt worked yet for me (remote debug of this is bit > painful) > > So I am going to drop patch 4 from this series and add support for both > DRM_BRIDGE_ATTACH_NO_CONNECTOR set and cleared (like we have in adv7511) > so that it can work in both cases, while I fix all bridge uses of > msm/dsi and then we can drop these. Does that sound okay to you folks? Yes, sounds like a good plan. Only when all display drivers are migrated over can we drop all the workarounds in the bridge drivers. I had hoped all users of this bridge was converted - alas that was not the case. Sam
On 23-07-20, 13:39, Sam Ravnborg wrote: > On Thu, Jul 23, 2020 at 04:11:51PM +0530, Vinod Koul wrote: > > Hi Sam, Laurent, > > > > On 22-07-20, 16:14, Laurent Pinchart wrote: > > > > > +static int lt9611_bridge_attach(struct drm_bridge *bridge, > > > > > + enum drm_bridge_attach_flags flags) > > > > > +{ > > > > > + struct lt9611 *lt9611 = bridge_to_lt9611(bridge); > > > > > + int ret; > > > > > + > > > > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > > > > > + dev_err(lt9611->dev, "Fix bridge driver to make connector optional!"); > > > > > + return -EINVAL; > > > > > + } > > > > > > > > This should say that the display driver should be fixed. > > > > If a display driver expects this bridge to create the connector > > > > it would not work. > > > > > > Actually, for new bridge drivers, connector creation should be optional > > > from the start. We don't want a failure in that case, the feature should > > > be implemented. > > > > Yes this is causing issues for me now !. The patch 4/4 adds support in > > msm/dsi but causes regression on qualcomm laptops with ti-sn65dsi86 eDP > > bridge. I tried to fix that up with changes like Laurent has done for > > adv7511, but it hasnt worked yet for me (remote debug of this is bit > > painful) > > > > So I am going to drop patch 4 from this series and add support for both > > DRM_BRIDGE_ATTACH_NO_CONNECTOR set and cleared (like we have in adv7511) > > so that it can work in both cases, while I fix all bridge uses of > > msm/dsi and then we can drop these. Does that sound okay to you folks? > Yes, sounds like a good plan. > Only when all display drivers are migrated over can we drop all the > workarounds in the bridge drivers. > I had hoped all users of this bridge was converted - alas that was not > the case. Thanks, I will send updated patchset fixing the nits and supporting both the cases and will drop msm/dsi patch for now