Re: [PATCH V2 1/2] dt-bindings: Add byteswap order to chrontel ch7033

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

Hi Chris,

On Wed, Sep 07, 2022 at 08:29:06AM -0500, Chris Morgan wrote:
> On Mon, Sep 05, 2022 at 05:20:57PM +0200, Robert Foss wrote:
> > On Sat, 3 Sept 2022 at 02:17, Laurent Pinchart wrote:
> > > On Fri, Sep 02, 2022 at 10:39:05AM -0500, Chris Morgan wrote:
> > > > From: Chris Morgan <macromorgan@hotmail.com>
> > > >
> > > > Update dt-binding documentation to add support for setting byteswap of
> > > > chrontel ch7033.
> > > >
> > > > New property name of chrontel,byteswap added to set the byteswap order.
> > > > This property is optional.
> > > >
> > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > > > Reviewed-by: Robert Foss <robert.foss@linaro.org>
> > > > ---
> > > >  .../bindings/display/bridge/chrontel,ch7033.yaml    | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml b/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml
> > > > index bb6289c7d375..984b90893583 100644
> > > > --- a/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml
> > > > +++ b/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml
> > > > @@ -14,6 +14,19 @@ properties:
> > > >    compatible:
> > > >      const: chrontel,ch7033
> > > >
> > > > +  chrontel,byteswap:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > > > +    enum:
> > > > +      - 0  # BYTE_SWAP_RGB
> > > > +      - 1  # BYTE_SWAP_RBG
> > > > +      - 2  # BYTE_SWAP_GRB
> > > > +      - 3  # BYTE_SWAP_GBR
> > > > +      - 4  # BYTE_SWAP_BRG
> > > > +      - 5  # BYTE_SWAP_BGR
> > > > +    description: |
> > > > +      Set the byteswap value of the bridge. This is optional and if not
> > > > +      set value of BYTE_SWAP_BGR is used.
> > >
> > > I don't think this belongs to the device tree. The source of data
> > > connected to the CH7033 input could use different formats. This
> > > shouldn't be hardcoded, but queried at runtime, using the input and
> > > output media bus formats infrastructure that the DRM bridge framework
> > > includes.
> > 
> > Chris, will you have a look at submitting a fix for this during the coming days?
> > 
> > If not, we can revert this series and apply a fixed version later.
> 
> I'm not sure I understand (or know) what needs to be fixed. Presumably
> using something like EDID we can predict what color format we need to
> use for the connection between the bridge and the HDMI device, but
> wouldn't the color format between the SoC and bridge need to be
> constant?

Not necessarily. Some display engines can output different formats. You
should be able to get the selected format at the bridge input from the
drm_bridge_state. Could you please give that a try ?

> If there's something I'm missing please let me know, I'm relatively
> unfamiliar with the display subsystems as a whole. I'll be happy
> to make the change once I'm clear what I need to change.
> 
> Thank you for your help.
> 
> > > > +
> > > >    reg:
> > > >      maxItems: 1
> > > >      description: I2C address of the device

-- 
Regards,

Laurent Pinchart