[PATCH v1 0/2] Revert chrontel-ch7033 byteswap order series

[PATCH v1 0/2] Revert chrontel-ch7033 byteswap order series

[Robert Foss]

After applying the "chrontel-ch7033: Add byteswap order option" series,
Laurent reported an issues with the approach. Since no fix has been submitted
for the issues outlined in time for the next kernel release, I'd like to
revert this series for now.

Just to be clear I would very much like to see a v3 of this[1] series, where the
issues outlined have been fixed.

[1] https://lore.kernel.org/all/20220902153906.31000-1-macroalpha82@gmail.com/

Robert Foss (2):
  Revert "dt-bindings: Add byteswap order to chrontel ch7033"
  Revert "drm/bridge: ti-sn65dsi86: Implement bridge connector
    operations for DP"

 .../display/bridge/chrontel,ch7033.yaml       | 13 ---------
 drivers/gpu/drm/bridge/ti-sn65dsi86.c         | 28 -------------------
 2 files changed, 41 deletions(-)

-- 
2.34.1

[PATCH v1 1/2] Revert "dt-bindings: Add byteswap order to chrontel ch7033"

[Robert Foss]

As reported by Laurent in response to this commit[1], this functionality should
not be implemented using the devicetree, because of this let's revert this series
for now.

This reverts commit a4be71430c76eca43679e8485085c230afa84460.

[1] https://lore.kernel.org/all/20220902153906.31000-2-macroalpha82@gmail.com/

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
 .../bindings/display/bridge/chrontel,ch7033.yaml    | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml b/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml
index 984b90893583..bb6289c7d375 100644
--- a/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml
@@ -14,19 +14,6 @@ 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.
-
   reg:
     maxItems: 1
     description: I2C address of the device
-- 
2.34.1

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

[Rob Herring]

On Mon, Sep 12, 2022 at 01:38:57PM +0200, Robert Foss wrote:
> As reported by Laurent in response to this commit[1], this functionality should
> not be implemented using the devicetree, because of this let's revert this series
> for now.
> 
> This reverts commit a4be71430c76eca43679e8485085c230afa84460.
> 
> [1] https://lore.kernel.org/all/20220902153906.31000-2-macroalpha82@gmail.com/
> 
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>  .../bindings/display/bridge/chrontel,ch7033.yaml    | 13 -------------
>  1 file changed, 13 deletions(-)

In the future, can you wait for a DT maintainer review.

For the revert:

Acked-by: Rob Herring <robh@kernel.org>

Rob

[PATCH v1 2/2] Revert "drm/bridge: ti-sn65dsi86: Implement bridge connector operations for DP"

[Robert Foss]

As reported by Laurent in response to this commit[1], this functionality should
not be implemented using the devicetree, because of this let's revert this series
for now.

This reverts commit c312b0df3b13e4c533743bb2c37fd1bc237368e5.

[1] https://lore.kernel.org/all/20220902153906.31000-2-macroalpha82@gmail.com/

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 28 ---------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 3c3561942eb6..6e053e2af229 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -29,7 +29,6 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_bridge_connector.h>
-#include <drm/drm_edid.h>
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_of.h>
 #include <drm/drm_panel.h>
@@ -69,7 +68,6 @@
 #define  BPP_18_RGB				BIT(0)
 #define SN_HPD_DISABLE_REG			0x5C
 #define  HPD_DISABLE				BIT(0)
-#define  HPD_DEBOUNCED_STATE			BIT(4)
 #define SN_GPIO_IO_REG				0x5E
 #define  SN_GPIO_INPUT_SHIFT			4
 #define  SN_GPIO_OUTPUT_SHIFT			0
@@ -1160,33 +1158,10 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
 	pm_runtime_put_sync(pdata->dev);
 }
 
-static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
-{
-	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
-	int val = 0;
-
-	pm_runtime_get_sync(pdata->dev);
-	regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
-	pm_runtime_put_autosuspend(pdata->dev);
-
-	return val & HPD_DEBOUNCED_STATE ? connector_status_connected
-					 : connector_status_disconnected;
-}
-
-static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
-					  struct drm_connector *connector)
-{
-	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
-
-	return drm_get_edid(connector, &pdata->aux.ddc);
-}
-
 static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.attach = ti_sn_bridge_attach,
 	.detach = ti_sn_bridge_detach,
 	.mode_valid = ti_sn_bridge_mode_valid,
-	.get_edid = ti_sn_bridge_get_edid,
-	.detect = ti_sn_bridge_detect,
 	.atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
 	.atomic_enable = ti_sn_bridge_atomic_enable,
 	.atomic_disable = ti_sn_bridge_atomic_disable,
@@ -1282,9 +1257,6 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 	pdata->bridge.type = pdata->next_bridge->type == DRM_MODE_CONNECTOR_DisplayPort
 			   ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
 
-	if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
-		pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
-
 	drm_bridge_add(&pdata->bridge);
 
 	ret = ti_sn_attach_host(pdata);
-- 
2.34.1

Re: [PATCH v1 2/2] Revert "drm/bridge: ti-sn65dsi86: Implement bridge connector operations for DP"

[Doug Anderson]

Robert,

On Mon, Sep 12, 2022 at 12:43 PM Robert Foss <robert.foss@linaro.org> wrote:
>
> As reported by Laurent in response to this commit[1], this functionality should
> not be implemented using the devicetree, because of this let's revert this series
> for now.
>
> This reverts commit c312b0df3b13e4c533743bb2c37fd1bc237368e5.
>
> [1] https://lore.kernel.org/all/20220902153906.31000-2-macroalpha82@gmail.com/
>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 28 ---------------------------
>  1 file changed, 28 deletions(-)

Any chance you got confused and reverted the wrong patch? This
ti-sn65dsi86 patch doesn't seem relevant to the problems talked about
in the commit or the cover letter. Maybe I'm missing something?

-Doug

Re: [PATCH v1 2/2] Revert "drm/bridge: ti-sn65dsi86: Implement bridge connector operations for DP"

[Laurent Pinchart]

On Mon, Sep 12, 2022 at 03:29:52PM +0100, Doug Anderson wrote:
> On Mon, Sep 12, 2022 at 12:43 PM Robert Foss <robert.foss@linaro.org> wrote:
> >
> > As reported by Laurent in response to this commit[1], this functionality should
> > not be implemented using the devicetree, because of this let's revert this series
> > for now.
> >
> > This reverts commit c312b0df3b13e4c533743bb2c37fd1bc237368e5.
> >
> > [1] https://lore.kernel.org/all/20220902153906.31000-2-macroalpha82@gmail.com/
> >
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 28 ---------------------------
> >  1 file changed, 28 deletions(-)
> 
> Any chance you got confused and reverted the wrong patch? This
> ti-sn65dsi86 patch doesn't seem relevant to the problems talked about
> in the commit or the cover letter. Maybe I'm missing something?

Aarghhh I missed that when checking the cover letter :-( This indeed
seems wrong.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 2/2] Revert "drm/bridge: ti-sn65dsi86: Implement bridge connector operations for DP"

[Robert Foss]

On Mon, 12 Sept 2022 at 16:43, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, Sep 12, 2022 at 03:29:52PM +0100, Doug Anderson wrote:
> > On Mon, Sep 12, 2022 at 12:43 PM Robert Foss <robert.foss@linaro.org> wrote:
> > >
> > > As reported by Laurent in response to this commit[1], this functionality should
> > > not be implemented using the devicetree, because of this let's revert this series
> > > for now.
> > >
> > > This reverts commit c312b0df3b13e4c533743bb2c37fd1bc237368e5.
> > >
> > > [1] https://lore.kernel.org/all/20220902153906.31000-2-macroalpha82@gmail.com/
> > >
> > > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > > ---
> > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 28 ---------------------------
> > >  1 file changed, 28 deletions(-)
> >
> > Any chance you got confused and reverted the wrong patch? This
> > ti-sn65dsi86 patch doesn't seem relevant to the problems talked about
> > in the commit or the cover letter. Maybe I'm missing something?
>
> Aarghhh I missed that when checking the cover letter :-( This indeed
> seems wrong.

Yep. This is a mistake. I copy/pasted the wrong line and then assumed
that ti-sn65dsi86 & chrontel-ch7033 shared a driver. I'll look into my
workflows to try to prevent future mistakes of this nature.

A series fixing this has been posted.
https://lore.kernel.org/all/20220915102924.370090-1-robert.foss@linaro.org/

Thanks for catching this Doug!


Rob.

Re: [PATCH v1 0/2] Revert chrontel-ch7033 byteswap order series

[Laurent Pinchart]

Hi Rob,

On Mon, Sep 12, 2022 at 01:38:54PM +0200, Robert Foss wrote:
> After applying the "chrontel-ch7033: Add byteswap order option" series,
> Laurent reported an issues with the approach. Since no fix has been submitted
> for the issues outlined in time for the next kernel release, I'd like to
> revert this series for now.
> 
> Just to be clear I would very much like to see a v3 of this[1] series, where the
> issues outlined have been fixed.
> 
> [1] https://lore.kernel.org/all/20220902153906.31000-1-macroalpha82@gmail.com/
> 
> Robert Foss (2):
>   Revert "dt-bindings: Add byteswap order to chrontel ch7033"
>   Revert "drm/bridge: ti-sn65dsi86: Implement bridge connector
>     operations for DP"

For the series,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  .../display/bridge/chrontel,ch7033.yaml       | 13 ---------
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c         | 28 -------------------
>  2 files changed, 41 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 0/2] Revert chrontel-ch7033 byteswap order series

[Chris Morgan]

On Mon, Sep 12, 2022 at 01:38:54PM +0200, Robert Foss wrote:
> After applying the "chrontel-ch7033: Add byteswap order option" series,
> Laurent reported an issues with the approach. Since no fix has been submitted
> for the issues outlined in time for the next kernel release, I'd like to
> revert this series for now.
> 
> Just to be clear I would very much like to see a v3 of this[1] series, where the
> issues outlined have been fixed.

I will work on a v3 soon, I just have to finish a few other things first.

That said, I'm not very familiar with what we're trying to do in an automated
fashion. In my use case I have DPI output (from an Allwinner R8 to this bridge)
which then connects via HDMI. I'm aware that we should be able to get the color
space information from the HDMI connector, correct? Is it that information I
would then need to use to set the bridge colorspace, or is it the color info
from the DPI connector I'm using?

I'm still pretty new to DRM drivers so this is mostly new to me. Thank you.

> 
> [1] https://lore.kernel.org/all/20220902153906.31000-1-macroalpha82@gmail.com/
> 
> Robert Foss (2):
>   Revert "dt-bindings: Add byteswap order to chrontel ch7033"
>   Revert "drm/bridge: ti-sn65dsi86: Implement bridge connector
>     operations for DP"
> 
>  .../display/bridge/chrontel,ch7033.yaml       | 13 ---------
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c         | 28 -------------------
>  2 files changed, 41 deletions(-)
> 
> -- 
> 2.34.1
>