Re: [PATCH v3 0/7] drm/i2c: adv7511: ADV7533 support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Archit,

On Monday 18 Apr 2016 15:18:30 Archit Taneja wrote:
> On 04/17/2016 05:01 PM, Xinliang Liu wrote:
> > On 9 March 2016 at 18:57, Archit Taneja <architt@codeaurora.org> wrote:
> >> ADV7533 is a DSI to HDMI encoder chip. It's like ADV7511, but with an
> >> additional DSI RX block that takes in DSI video mode output.
> >> 
> >> Trying to get this driver merged has had some challenges:
> >> 
> >> - ADV7533 has an I2C control bus, but acts as a DSI peripheral too.
> >>   After discussions, it was concluded that we'd want to provide an
> >>   API to create MIPI DSI devices, rather than expose two different
> >>   interfaces on DT. The first version [1] tried the former approach
> >>   the second version [2] showed how the driver would look like if
> >>   exposed 2 DT nodes. This lateset patchset relies on the MIPI DSI
> >>   device creation API provided by [3], this has been accepted and
> >>   should be merged for 4.6.
> >> 
> >> - The driver was designed as an I2C slave encoder. When ADV7533
> >>   patches were posted [1], it was modelled as a bridge, but ADV7511
> >>   and others were still left as I2C slave encoders. This wasn't
> >>   accepted. After discussions, it was decided that ADV7511 too would
> >>   be converted into a bridge driver, and all the users of ADV7511
> >>   should assume it is a bridge. This bridge conversion was done in
> >>   [4]. There is still some debate over whether the bridge driver be
> >>   involved in the connector creation, or the KMS driver that has
> >>   the whole view of the display pipeline. This discussion shouldn't
> >>   affect this patch set, though.
> > 
> > I also agree with Laurent Pinchart that bridge driver shoudn't get
> > involved in in the connector creation.
> > It is better for KMS driver that has the whole view of the display
> > pipeline.
>
> Yes, that's the eventual plan. We were thinking of creating an
> additional drm bridge api (drm_bridge_create_connector) that
> helps the KMS driver create a connector for the bridge.
> 
> Since there are  certain connector related properties that the KMS
> driver may not be aware of, we were thinking of creating another
> drm_bridge op which fills up connector properties. Some properties
> (like whether we want POLLED HPD or not) are more platform specific,
> and would be parsed via the connector's DT node (that is, if DT is
> supported on that platform)
> 
> For now, this series creates the connector in the bridge's
> attach op, but relies on the KMS driver to call
> drm_connector_register. The other stuff I mentioned above can come
> later.

Do you think you'd have time to lead that effort ?

> > In this case, I mean creating bridge driver to support ADV7533. I
> > think this is something look like panel driver. Maybe we need to add
> > some callback to the bridge to avoid creating connector in bridge
> > driver.
> > We can refer to panel entity to see what callbacks to be added. One
> > callback I can see is the get_modes callback might be need.
> > 
> >> This patch set enables ADV7533 support with the above two issues
> >> now resolved. It also incorporates ADV7533 specific features and fixes
> >> that we've discovered since the first version of this patch was posted.
> >> 
> >> Tested on ADV7533 chips on DB410c. It should work on the Hikey board too.
> > 
> > I have tested the this tracking branch :
> > https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/shortlog/
> > refs/heads/tracking-qcomlt-adv7511 it works for HiKey.
> > But it seems this patch set is a little different from the above
> > branch. I think I need to tested this patch set.
> 
> Yes, it would be great if you could test the posted patch set and
> provided a Tested-by. The patches "drm/i2c: adv7511: Init regulators"
> and above need to still be applied from this branch. I'll post these
> out in another patch set later.
> 
> >> I'd appreaciate if someone could test it on a ADV7511 platform since I
> >> don't have one.
> >> 
> >> [4]
> >> https://lists.freedesktop.org/archives/dri-devel/2016-January/098287.html
> >> [3]
> >> https://lkml.org/lkml/2016/2/12/67
> >> [2]
> >> https://lists.freedesktop.org/archives/dri-devel/2015-September/089884.ht
> >> ml
> >> [1]:
> >> https://lists.freedesktop.org/archives/dri-devel/2015-July/087088.html
> >> 
> >> Archit Taneja (7):
> >>    drm/i2c: adv7511: Convert to drm_bridge
> >>    drm/i2c: adv7511: Fix mutex deadlock when interrupts are disabled
> >>    drm/i2c: adv7511: Initial support for ADV7533
> >>    drm/i2c: adv7511: Create a MIPI DSI device
> >>    drm/i2c: adv7511: Use internal timing generator
> >>    drm/i2c: adv7511: Change number of DSI lanes dynamically
> >>    dt-bindings: drm/bridge: Update bindings for ADV7533
> >>   
> >>   .../bindings/display/bridge/adi,adv7511.txt        |  25 +-
> >>   drivers/gpu/drm/i2c/adv7511.c                      | 539 ++++++++++----
> >>   2 files changed, 476 insertions(+), 88 deletions(-)

-- 
Regards,

Laurent Pinchart