lkml.org 
[lkml]   [2018]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc
    Hi Laurent,

    On Fri, Apr 20, 2018 at 01:18:13PM +0300, Laurent Pinchart wrote:
    > Hello,
    >
    > On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote:
    > > Hi Peter,
    > > I've been a bit a pain in the arse for you recently, but please
    > > bear with me a bit more, and sorry for jumping late on the band wagon.
    > >
    > > On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote:
    > > > Hi!
    > > >
    > > > I naively thought that since there was support for both nxp,tda19988 (in
    > > > the tda998x driver) and the atmel-hlcdc, things would be a smooth ride.
    > > > But it wasn't, so I started looking around and realized I had to fix
    > > > things.
    > > >
    > > > In v1 and v2 I fixed things by making the atmel-hlcdc driver a master
    > > > component, but now in v3 I fix things by making the tda998x driver
    > > > a bridge instead. This was after a suggestion from Boris Brezillion
    > > > (the atmel-hlcdc maintainer), so there was some risk of bias ... but
    > > > after comparing what was needed, I too find the bridge approach better.
    > > >
    > > > In addition to the above, our PCB interface between the SAMA5D3 and the
    > > > HDMI encoder is only using 16 bits, and this has to be described
    > > > somewhere, or the atmel-hlcdc driver have no chance of selecting the
    > > > correct output mode. Since I have similar problems with a ds90c185 lvds
    > > > encoder I added patches to override the atmel-hlcdc output format via
    > > > DT properties compatible with the media video-interface binding and
    > > > things start to play together.
    > > >
    > > > Since this series superseeds the bridge series [1], I have included the
    > > > leftover bindings patch for the ti,ds90c185 here.
    > >
    > > I feel like this series would look better if it would make use of the
    > > proposed bridge media bus format support I have recently sent out [1]
    > > (and which was not there when you first sent v1).
    > >
    > > I understand your fundamental problem here is that the bus format
    > > that should be reported by your bridge is different from the ones
    > > actually supported by the TDA19988 chip, as the wirings ground some
    > > of the input pins.
    > >
    > > Although this is defintely something that could be described in the
    > > bridge's own OF node with the 'bus_width' property, and what I would do,
    > > now that you have made a bridge out from the tda19988 driver, is:
    > >
    > > 1) Set the bridge accepted input bus_format parsing its pin
    > > configuration, or default it if that's not implemented yet.
    > > This will likely be rgb888. You can do that using the trivial
    > > support for bridge input image formats implemented by my series.
    > > 2) Specify in the bridge endpoint a 'bus_width' of <16>
    > > 3) In your atmel-hlcd get both the image format of the bridge (rgb888)
    > > and parse the remote endpoint property 'bus_width' and get the <16>
    > > value back.
    >
    > Parsing properties of remote nodes should be avoided as much as possible, as
    > they need to be interpreted in the context of the DT bindings related to the
    > compatible string applicable to that node. I'd rather have the bus_width
    > property in the local endpoint node.

    Right, I see...
    Well, in Peter's case, the bus width, being a consequence of
    wirings, can be described safely in both endpoints, right?

    >
    > > 4) Set the correct format in the atmel hlcd driver to accommodate a
    > > bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565,
    > > or are there other possible combinations I am missing?)
    > >
    > > I would consider this better mostly because in this series you are
    > > creating a semantic for the whole DRM subsystem on the 'bus_width'
    > > property, where numerical values as '12', '16' etc are arbitrary tied
    > > to the selection of a media bus format. At least you should use a
    > > common set of defines [1] between the device tree and the driver,
    > > but this looks anyway fragile imho.
    >
    > This I agree with though. Combining the remote bus format with the local bus
    > width should fix the problem without having to parse remote properties.
    >
    > > Have I maybe missed some parts of the problem you are trying to solve
    > > here?
    > >
    > > Thank you
    > > j
    > >
    > > [1] drm: bridge: Add support for static image formats
    > > https://lwn.net/Articles/752296/
    > > [2] include/uapi/linux/media-bus-format.h
    > >
    > > > Anyway, this series solves some real issues for my HW.
    > > >
    > > > Cheers,
    > > > Peter
    > > >
    > > > Changes since v2 https://lkml.org/lkml/2018/4/17/385
    > > > - patch 2/7 fixed spelling and added an example
    > > > - patch 4/7 parse the DT up front and store the result indexed by encoder
    > > > - old patch 5/6 and 6/6 dropped
    > > > - patch 5-7/7 are new and makes the tda998x driver a drm_bridge
    > > >
    > > > Changes since v1 https://lkml.org/lkml/2018/4/9/294
    > > > - added reviewed-by from Rob to patch 1/6
    > > > - patch 2/6 changed so that the bus format override is in the endpoint
    > > > DT node, and follows the binding of media video-interfaces.
    > > > - patch 3/6 is new, it adds drm_of_media_bus_fmt which parses above
    > > > media video-interface binding (partially).
    > > > - patch 4/6 now makes use of the above helper (and also fixes problems
    > > > with the 3/5 patch from v1 when no override was specified).
    > > > - do not mention unrelated connector display_info details in the cover
    > > > letter and commit messages.
    > > >
    > > > [1]
    > > > "Bridge" series v2 https://lkml.org/lkml/2018/3/26/610
    > > > "Bridge" series v1 https://lkml.org/lkml/2018/3/17/221
    > > >
    > > > Peter Rosin (7):
    > > > dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
    > > > dt-bindings: display: atmel: optional video-interface of endpoints
    > > > drm: of: introduce drm_of_media_bus_fmt
    > > > drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes
    > > > drm/i2c: tda998x: find the drm_device via the drm_connector
    > > > drm/i2c: tda998x: split encoder and component functions from the work
    > > > drm/i2c: tda998x: register as a drm bridge
    > > >
    > > > .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 +++
    > > > .../bindings/display/bridge/lvds-transmitter.txt | 8 +-
    > > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 71 ++++++--
    > > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 2 +
    > > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 40 +++-
    > > > drivers/gpu/drm/drm_of.c | 38 ++++
    > > > drivers/gpu/drm/i2c/tda998x_drv.c | 201 ++++++++++++++--
    > > > include/drm/drm_of.h | 7 +
    > > > 8 files changed, 342 insertions(+), 51 deletions(-)
    >
    > --
    > Regards,
    >
    > Laurent Pinchart
    >
    >
    >
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2018-04-20 13:22    [W:3.984 / U:0.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site