mbox series

[v1,0/2] Add LT9611UXC DSI to HDMI bridge support

Message ID 20200828120431.1636402-1-dmitry.baryshkov@linaro.org
Headers show
Series Add LT9611UXC DSI to HDMI bridge support | expand

Message

Dmitry Baryshkov Aug. 28, 2020, 12:04 p.m. UTC 
Hi,

This series adds support for Lontium LT9611UXC bridge chip which takes
MIPI DSI as input and provides HDMI signal as output.

The chip can be found in Qualcomm RB5 platform [1], [2].

[1] https://www.qualcomm.com/products/qualcomm-robotics-rb5-platform
[2] https://www.thundercomm.com/app_en/product/1590131656070623

Comments

Vinod Koul Aug. 28, 2020, 2 p.m. UTC | #1 
On 28-08-20, 15:04, Dmitry Baryshkov wrote:
> Lontium LT9611UXC is a DSI to HDMI bridge which supports 2 DSI ports
> and I2S port as input and one HDMI port as output. The LT9611UXC chip is
> handled by a separate driver, but the bindings used are fully compatible
> with the LT9611 chip, so let's reuse the lt9611.yaml schema.

Acked-By: Vinod Koul <vkoul@kernel.org>
Laurent Pinchart Aug. 28, 2020, 2:33 p.m. UTC | #2 
On Fri, Aug 28, 2020 at 07:48:48PM +0530, Vinod Koul wrote:
> On 28-08-20, 15:04, Dmitry Baryshkov wrote:

> 

> > +#define EDID_BLOCK_SIZE	128

> > +#define EDID_NUM_BLOCKS 2

> 

> tab or space either one, not both ;)

> 

> > +static struct mipi_dsi_device *lt9611uxc_attach_dsi(struct lt9611uxc *lt9611uxc,

> > +						 struct device_node *dsi_node)

> 

> Please align this with open parenthesis of preceding line (checkpatch

> with --strict option will check this)

> 

> > +static int lt9611uxc_bridge_attach(struct drm_bridge *bridge,

> > +				enum drm_bridge_attach_flags flags)

> > +{

> > +	struct lt9611uxc *lt9611uxc = bridge_to_lt9611uxc(bridge);

> > +	int ret;

> > +

> > +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {

> > +		dev_err(lt9611uxc->dev, "Fix bridge driver to make connector optional!");

> 

> Can we support both modes as I have done in lt9611, that way once the

> conversion is done we can drop the init part and support conversion.


I was going to mention that :-) New drivers should support the
DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.

> I have patch for msm driver to set DRM_BRIDGE_ATTACH_NO_CONNECTOR, you

> can use that to test

> 

> > +static int lt9611uxc_hdmi_hw_params(struct device *dev, void *data,

> > +				 struct hdmi_codec_daifmt *fmt,

> > +				 struct hdmi_codec_params *hparms)

> > +{

> > +	/*

> > +	 * LT9611UXC will automatically detect rate and sample size, so no need

> > +	 * to setup anything here.

> > +	 */

> > +	return 0;

> > +}

> 

> Do we need dummy function?


-- 
Regards,

Laurent Pinchart
Dmitry Baryshkov Aug. 28, 2020, 3:01 p.m. UTC | #3 
On 28/08/2020 17:18, Vinod Koul wrote:
> On 28-08-20, 15:04, Dmitry Baryshkov wrote:

>> +static int lt9611uxc_bridge_attach(struct drm_bridge *bridge,

>> +				enum drm_bridge_attach_flags flags)

>> +{

>> +	struct lt9611uxc *lt9611uxc = bridge_to_lt9611uxc(bridge);

>> +	int ret;

>> +

>> +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {

>> +		dev_err(lt9611uxc->dev, "Fix bridge driver to make connector optional!");

> 

> Can we support both modes as I have done in lt9611, that way once the

> conversion is done we can drop the init part and support conversion.

> 

> I have patch for msm driver to set DRM_BRIDGE_ATTACH_NO_CONNECTOR, you

> can use that to test


Probably the message text is misleading. The driver as is does not work 
w/o DRM_BRIDGE_ATTACH_NO_CONNECTOR. Do you plan to push that patch into 
upstream tree?

>> +static int lt9611uxc_hdmi_hw_params(struct device *dev, void *data,

>> +				 struct hdmi_codec_daifmt *fmt,

>> +				 struct hdmi_codec_params *hparms)

>> +{

>> +	/*

>> +	 * LT9611UXC will automatically detect rate and sample size, so no need

>> +	 * to setup anything here.

>> +	 */

>> +	return 0;

>> +}

> 

> Do we need dummy function?


Yes, this callback is mandatory (and audio_shutdown).


-- 
With best wishes
Dmitry
Laurent Pinchart Aug. 28, 2020, 3:04 p.m. UTC | #4 
On Fri, Aug 28, 2020 at 05:33:00PM +0300, Laurent Pinchart wrote:
> On Fri, Aug 28, 2020 at 07:48:48PM +0530, Vinod Koul wrote:

> > On 28-08-20, 15:04, Dmitry Baryshkov wrote:

> > 

> > > +#define EDID_BLOCK_SIZE	128

> > > +#define EDID_NUM_BLOCKS 2

> > 

> > tab or space either one, not both ;)

> > 

> > > +static struct mipi_dsi_device *lt9611uxc_attach_dsi(struct lt9611uxc *lt9611uxc,

> > > +						 struct device_node *dsi_node)

> > 

> > Please align this with open parenthesis of preceding line (checkpatch

> > with --strict option will check this)

> > 

> > > +static int lt9611uxc_bridge_attach(struct drm_bridge *bridge,

> > > +				enum drm_bridge_attach_flags flags)

> > > +{

> > > +	struct lt9611uxc *lt9611uxc = bridge_to_lt9611uxc(bridge);

> > > +	int ret;

> > > +

> > > +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {

> > > +		dev_err(lt9611uxc->dev, "Fix bridge driver to make connector optional!");

> > 

> > Can we support both modes as I have done in lt9611, that way once the

> > conversion is done we can drop the init part and support conversion.

> 

> I was going to mention that :-) New drivers should support the

> DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.


Please ignore this comment, I just realized that the driver supports
DRM_BRIDGE_ATTACH_NO_CONNECTOR, it's the !DRM_BRIDGE_ATTACH_NO_CONNECTOR
case that is not supported, and that's totally fine.

> > I have patch for msm driver to set DRM_BRIDGE_ATTACH_NO_CONNECTOR, you

> > can use that to test

> > 

> > > +static int lt9611uxc_hdmi_hw_params(struct device *dev, void *data,

> > > +				 struct hdmi_codec_daifmt *fmt,

> > > +				 struct hdmi_codec_params *hparms)

> > > +{

> > > +	/*

> > > +	 * LT9611UXC will automatically detect rate and sample size, so no need

> > > +	 * to setup anything here.

> > > +	 */

> > > +	return 0;

> > > +}

> > 

> > Do we need dummy function?


-- 
Regards,

Laurent Pinchart
Vinod Koul Aug. 28, 2020, 3:06 p.m. UTC | #5 
On 28-08-20, 18:01, Dmitry Baryshkov wrote:
> On 28/08/2020 17:18, Vinod Koul wrote:

> > On 28-08-20, 15:04, Dmitry Baryshkov wrote:

> > > +static int lt9611uxc_bridge_attach(struct drm_bridge *bridge,

> > > +				enum drm_bridge_attach_flags flags)

> > > +{

> > > +	struct lt9611uxc *lt9611uxc = bridge_to_lt9611uxc(bridge);

> > > +	int ret;

> > > +

> > > +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {

> > > +		dev_err(lt9611uxc->dev, "Fix bridge driver to make connector optional!");

> > 

> > Can we support both modes as I have done in lt9611, that way once the

> > conversion is done we can drop the init part and support conversion.

> > 

> > I have patch for msm driver to set DRM_BRIDGE_ATTACH_NO_CONNECTOR, you

> > can use that to test

> 

> Probably the message text is misleading. The driver as is does not work w/o

> DRM_BRIDGE_ATTACH_NO_CONNECTOR. Do you plan to push that patch into upstream

> tree?


It causes regression in laptop so have removed it ;( I need to fix that
first
The patch is here though and works on rb3 and db410c.
git.linaro.org/people/vinod.koul/kernel.git drm/no_connector

-- 
~Vinod