mbox series

[v5,0/4] Add LT9611 DSI to HDMI bridge

Message ID 20200708103559.132300-1-vkoul@kernel.org
Headers show
Series Add LT9611 DSI to HDMI bridge | expand

Message

Vinod Koul July 8, 2020, 10:35 a.m. UTC 
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.

[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

Comments

Sam Ravnborg July 19, 2020, 5:18 p.m. UTC | #1 
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.
Vinod Koul July 20, 2020, 4:03 a.m. UTC | #2 
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

-- 
~Vinod
Laurent Pinchart July 22, 2020, 1:14 p.m. UTC | #3 
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.

-- 
Regards,

Laurent Pinchart
Sam Ravnborg July 23, 2020, 11:39 a.m. UTC | #4 
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
Vinod Koul July 23, 2020, 12:10 p.m. UTC | #5 
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

-- 
~Vinod