mbox series

[v3,0/6] Add Synopsys DesignWare HDMI RX Controller

Message ID 20240327225057.672304-1-shreeya.patel@collabora.com (mailing list archive)
Headers
Series Add Synopsys DesignWare HDMI RX Controller |

Message

Shreeya Patel March 27, 2024, 10:50 p.m. UTC 
  This series implements support for the Synopsys DesignWare
HDMI RX Controller, being compliant with standard HDMI 1.4b
and HDMI 2.0.

Features that are currently supported by the HDMI RX driver
have been tested on rock5b board using a HDMI to micro-HDMI cable.
It is recommended to use a good quality cable as there were
multiple issues seen during testing the driver.

Please note the below information :-
* While testing the driver on rock5b we noticed that the binary BL31
from Rockchip contains some unknown code to get the HDMI-RX PHY
access working. With TF-A BL31, the HDMI-RX PHY doesn't work as
expected since there are no interrupts seen for rk_hdmirx-hdmi
leading to some failures in the driver [0].
* We have tested the working of OBS studio with HDMIRX driver and
there were no issues seen.
* We also tested and verified the support for interlaced video.

[0] https://gitlab.collabora.com/hardware-enablement/rockchip-3588/trusted-firmware-a/-/issues/1

To test the HDMI RX Controller driver, following example commands can be used :-

root@debian-rockchip-rock5b-rk3588:~# v4l2-ctl --verbose -d /dev/video0 \
--set-fmt-video=width=1920,height=1080,pixelformat='BGR3' --stream-mmap=4 \
--stream-skip=3 --stream-count=100 --stream-to=/home/hdmiin4k.raw --stream-poll

root@debian-rockchip-rock5b-rk3588:~# ffmpeg -f rawvideo -vcodec rawvideo \
-s 1920x1080 -r 60 -pix_fmt bgr24 -i /home/hdmiin4k.raw output.mkv


Following is the v4l2-compliance test result :-

root@debian-rockchip-rock5b-rk3588:~# v4l2-compliance -d /dev/video0
v4l2-compliance 1.27.0-5174, 64 bits, 64-bit time_t
v4l2-compliance SHA: d700deb14368 2024-01-18 12:19:05

Compliance test for snps_hdmirx device /dev/video0:

Driver Info:
        Driver name      : snps_hdmirx
        Card type        : snps_hdmirx
        Bus info         : platform: snps_hdmirx
        Driver version   : 6.8.0
        Capabilities     : 0x84201000
                Video Capture Multiplanar
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps      : 0x04201000
                Video Capture Multiplanar
                Streaming
                Extended Pix Format

Required ioctls:
        test VIDIOC_QUERYCAP: OK
        test invalid ioctls: OK

Allow for multiple opens:
        test second /dev/video0 open: OK
        test VIDIOC_QUERYCAP: OK
        test VIDIOC_G/S_PRIORITY: OK
        test for unlimited opens: OK

Debug ioctls:
        test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
        test VIDIOC_LOG_STATUS: OK

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK
        test VIDIOC_DV_TIMINGS_CAP: OK
        test VIDIOC_G/S_EDID: OK

Control ioctls (Input 0):
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
        test VIDIOC_G/S_CTRL: OK
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 2 Private Controls: 0

Format ioctls (Input 0):
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
        test VIDIOC_G/S_PARM: OK
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK
        test VIDIOC_TRY_FMT: OK
        test VIDIOC_S_FMT: OK
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
        test CREATE_BUFS maximum buffers: OK
        test VIDIOC_EXPBUF: OK
        test Requests: OK (Not Supported)

Total for snps_hdmirx device /dev/video0: 46, Succeeded: 46, Failed: 0, Warnings: 0

Changes in v3 :-
  - Use v4l2-common helpers in the HDMIRX driver
  - Rename cma node and phandle names
  - Elaborate the comment to explain 160MiB calculation
  - Move &hdmi_receiver_cma to the rock5b dts file
  - Add information about interlaced video testing in the
    cover-letter

Changes in v2 :-
  - Fix checkpatch --strict warnings
  - Move the dt-binding include file changes in a separate patch
  - Add a description for the hardware in the dt-bindings file
  - Rename resets, vo1 grf and HPD properties
  - Add a proper description for grf and vo1-grf phandles in the
    bindings
  - Rename the HDMI RX node name to hdmi-receiver
  - Include gpio header file in binding example to fix the
    dt_binding_check failure
  - Move hdmirx_cma node to the rk3588.dtsi file
  - Add an entry to MAINTAINERS file for the HDMIRX driver

Shreeya Patel (6):
  dt-bindings: reset: Define reset id used for HDMI Receiver
  clk: rockchip: rst-rk3588: Add reset line for HDMI Receiver
  dt-bindings: media: Document HDMI RX Controller
  arm64: dts: rockchip: Add device tree support for HDMI RX Controller
  media: platform: synopsys: Add support for hdmi input driver
  MAINTAINERS: Add entry for Synopsys DesignWare HDMI RX Driver

 .../bindings/media/snps,dw-hdmi-rx.yaml       |  132 +
 MAINTAINERS                                   |    8 +
 .../boot/dts/rockchip/rk3588-pinctrl.dtsi     |   41 +
 .../boot/dts/rockchip/rk3588-rock-5b.dts      |   19 +
 arch/arm64/boot/dts/rockchip/rk3588.dtsi      |   56 +
 drivers/clk/rockchip/rst-rk3588.c             |    1 +
 drivers/media/platform/Kconfig                |    1 +
 drivers/media/platform/Makefile               |    1 +
 drivers/media/platform/synopsys/Kconfig       |    3 +
 drivers/media/platform/synopsys/Makefile      |    2 +
 .../media/platform/synopsys/hdmirx/Kconfig    |   18 +
 .../media/platform/synopsys/hdmirx/Makefile   |    4 +
 .../platform/synopsys/hdmirx/snps_hdmirx.c    | 2726 +++++++++++++++++
 .../platform/synopsys/hdmirx/snps_hdmirx.h    |  394 +++
 .../synopsys/hdmirx/snps_hdmirx_cec.c         |  289 ++
 .../synopsys/hdmirx/snps_hdmirx_cec.h         |   46 +
 .../dt-bindings/reset/rockchip,rk3588-cru.h   |    2 +
 17 files changed, 3743 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
 create mode 100644 drivers/media/platform/synopsys/Kconfig
 create mode 100644 drivers/media/platform/synopsys/Makefile
 create mode 100644 drivers/media/platform/synopsys/hdmirx/Kconfig
 create mode 100644 drivers/media/platform/synopsys/hdmirx/Makefile
 create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
 create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
 create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx_cec.c
 create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx_cec.h

Comments

Shreeya Patel April 3, 2024, 9:24 a.m. UTC | #1 
On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:

> This series implements support for the Synopsys DesignWare
> HDMI RX Controller, being compliant with standard HDMI 1.4b
> and HDMI 2.0.
> 

Hi Mauro and Hans,

I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.


Thanks,
Shreeya Patel

> Features that are currently supported by the HDMI RX driver
> have been tested on rock5b board using a HDMI to micro-HDMI cable.
> It is recommended to use a good quality cable as there were
> multiple issues seen during testing the driver.
> 
> Please note the below information :-
> * While testing the driver on rock5b we noticed that the binary BL31
> from Rockchip contains some unknown code to get the HDMI-RX PHY
> access working. With TF-A BL31, the HDMI-RX PHY doesn't work as
> expected since there are no interrupts seen for rk_hdmirx-hdmi
> leading to some failures in the driver [0].
> * We have tested the working of OBS studio with HDMIRX driver and
> there were no issues seen.
> * We also tested and verified the support for interlaced video.
> 
> [0] https://gitlab.collabora.com/hardware-enablement/rockchip-3588/trusted-firmware-a/-/issues/1
> 
> To test the HDMI RX Controller driver, following example commands can be used :-
> 
> root@debian-rockchip-rock5b-rk3588:~# v4l2-ctl --verbose -d /dev/video0 \
> --set-fmt-video=width=1920,height=1080,pixelformat='BGR3' --stream-mmap=4 \
> --stream-skip=3 --stream-count=100 --stream-to=/home/hdmiin4k.raw --stream-poll
> 
> root@debian-rockchip-rock5b-rk3588:~# ffmpeg -f rawvideo -vcodec rawvideo \
> -s 1920x1080 -r 60 -pix_fmt bgr24 -i /home/hdmiin4k.raw output.mkv
> 
> 
> Following is the v4l2-compliance test result :-
> 
> root@debian-rockchip-rock5b-rk3588:~# v4l2-compliance -d /dev/video0
> v4l2-compliance 1.27.0-5174, 64 bits, 64-bit time_t
> v4l2-compliance SHA: d700deb14368 2024-01-18 12:19:05
> 
> Compliance test for snps_hdmirx device /dev/video0:
> 
> Driver Info:
>         Driver name      : snps_hdmirx
>         Card type        : snps_hdmirx
>         Bus info         : platform: snps_hdmirx
>         Driver version   : 6.8.0
>         Capabilities     : 0x84201000
>                 Video Capture Multiplanar
>                 Streaming
>                 Extended Pix Format
>                 Device Capabilities
>         Device Caps      : 0x04201000
>                 Video Capture Multiplanar
>                 Streaming
>                 Extended Pix Format
> 
> Required ioctls:
>         test VIDIOC_QUERYCAP: OK
>         test invalid ioctls: OK
> 
> Allow for multiple opens:
>         test second /dev/video0 open: OK
>         test VIDIOC_QUERYCAP: OK
>         test VIDIOC_G/S_PRIORITY: OK
>         test for unlimited opens: OK
> 
> Debug ioctls:
>         test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
>         test VIDIOC_LOG_STATUS: OK
> 
> Input ioctls:
>         test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
>         test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
>         test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
>         test VIDIOC_ENUMAUDIO: OK (Not Supported)
>         test VIDIOC_G/S/ENUMINPUT: OK
>         test VIDIOC_G/S_AUDIO: OK (Not Supported)
>         Inputs: 1 Audio Inputs: 0 Tuners: 0
> 
> Output ioctls:
>         test VIDIOC_G/S_MODULATOR: OK (Not Supported)
>         test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
>         test VIDIOC_ENUMAUDOUT: OK (Not Supported)
>         test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
>         test VIDIOC_G/S_AUDOUT: OK (Not Supported)
>         Outputs: 0 Audio Outputs: 0 Modulators: 0
> 
> Input/Output configuration ioctls:
>         test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
>         test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK
>         test VIDIOC_DV_TIMINGS_CAP: OK
>         test VIDIOC_G/S_EDID: OK
> 
> Control ioctls (Input 0):
>         test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
>         test VIDIOC_QUERYCTRL: OK
>         test VIDIOC_G/S_CTRL: OK
>         test VIDIOC_G/S/TRY_EXT_CTRLS: OK
>         test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
>         test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
>         Standard Controls: 2 Private Controls: 0
> 
> Format ioctls (Input 0):
>         test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
>         test VIDIOC_G/S_PARM: OK
>         test VIDIOC_G_FBUF: OK (Not Supported)
>         test VIDIOC_G_FMT: OK
>         test VIDIOC_TRY_FMT: OK
>         test VIDIOC_S_FMT: OK
>         test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
>         test Cropping: OK (Not Supported)
>         test Composing: OK (Not Supported)
>         test Scaling: OK (Not Supported)
> 
> Codec ioctls (Input 0):
>         test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
>         test VIDIOC_G_ENC_INDEX: OK (Not Supported)
>         test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
> 
> Buffer ioctls (Input 0):
>         test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>         test CREATE_BUFS maximum buffers: OK
>         test VIDIOC_EXPBUF: OK
>         test Requests: OK (Not Supported)
> 
> Total for snps_hdmirx device /dev/video0: 46, Succeeded: 46, Failed: 0, Warnings: 0
> 
> Changes in v3 :-
>   - Use v4l2-common helpers in the HDMIRX driver
>   - Rename cma node and phandle names
>   - Elaborate the comment to explain 160MiB calculation
>   - Move &hdmi_receiver_cma to the rock5b dts file
>   - Add information about interlaced video testing in the
>     cover-letter
> 
> Changes in v2 :-
>   - Fix checkpatch --strict warnings
>   - Move the dt-binding include file changes in a separate patch
>   - Add a description for the hardware in the dt-bindings file
>   - Rename resets, vo1 grf and HPD properties
>   - Add a proper description for grf and vo1-grf phandles in the
>     bindings
>   - Rename the HDMI RX node name to hdmi-receiver
>   - Include gpio header file in binding example to fix the
>     dt_binding_check failure
>   - Move hdmirx_cma node to the rk3588.dtsi file
>   - Add an entry to MAINTAINERS file for the HDMIRX driver
> 
> Shreeya Patel (6):
>   dt-bindings: reset: Define reset id used for HDMI Receiver
>   clk: rockchip: rst-rk3588: Add reset line for HDMI Receiver
>   dt-bindings: media: Document HDMI RX Controller
>   arm64: dts: rockchip: Add device tree support for HDMI RX Controller
>   media: platform: synopsys: Add support for hdmi input driver
>   MAINTAINERS: Add entry for Synopsys DesignWare HDMI RX Driver
> 
>  .../bindings/media/snps,dw-hdmi-rx.yaml       |  132 +
>  MAINTAINERS                                   |    8 +
>  .../boot/dts/rockchip/rk3588-pinctrl.dtsi     |   41 +
>  .../boot/dts/rockchip/rk3588-rock-5b.dts      |   19 +
>  arch/arm64/boot/dts/rockchip/rk3588.dtsi      |   56 +
>  drivers/clk/rockchip/rst-rk3588.c             |    1 +
>  drivers/media/platform/Kconfig                |    1 +
>  drivers/media/platform/Makefile               |    1 +
>  drivers/media/platform/synopsys/Kconfig       |    3 +
>  drivers/media/platform/synopsys/Makefile      |    2 +
>  .../media/platform/synopsys/hdmirx/Kconfig    |   18 +
>  .../media/platform/synopsys/hdmirx/Makefile   |    4 +
>  .../platform/synopsys/hdmirx/snps_hdmirx.c    | 2726 +++++++++++++++++
>  .../platform/synopsys/hdmirx/snps_hdmirx.h    |  394 +++
>  .../synopsys/hdmirx/snps_hdmirx_cec.c         |  289 ++
>  .../synopsys/hdmirx/snps_hdmirx_cec.h         |   46 +
>  .../dt-bindings/reset/rockchip,rk3588-cru.h   |    2 +
>  17 files changed, 3743 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
>  create mode 100644 drivers/media/platform/synopsys/Kconfig
>  create mode 100644 drivers/media/platform/synopsys/Makefile
>  create mode 100644 drivers/media/platform/synopsys/hdmirx/Kconfig
>  create mode 100644 drivers/media/platform/synopsys/hdmirx/Makefile
>  create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
>  create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
>  create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx_cec.c
>  create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx_cec.h
> 
> -- 
> 2.39.2
> 
> _______________________________________________
> Kernel mailing list -- kernel@mailman.collabora.com
> To unsubscribe send an email to kernel-leave@mailman.collabora.com
> This list is managed by https://mailman.collabora.com
Krzysztof Kozlowski April 3, 2024, 10:21 a.m. UTC | #2 
On 03/04/2024 11:24, Shreeya Patel wrote:
> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
> 
>> This series implements support for the Synopsys DesignWare
>> HDMI RX Controller, being compliant with standard HDMI 1.4b
>> and HDMI 2.0.
>>
> 
> Hi Mauro and Hans,
> 
> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.

Why did you put clk changes here? These go via different subsystem. That
might be one of obstacles for your patchset.

Also, you sent it just a week ago and you already ping. Please relax,
and help out by reviewing other patches on the mailing lists in order to
relieve the burden of maintainers and move your patches higher up the list.


Best regards,
Krzysztof
Shreeya Patel April 3, 2024, 11:20 a.m. UTC | #3 
On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 03/04/2024 11:24, Shreeya Patel wrote:
> > On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
> > 
> >> This series implements support for the Synopsys DesignWare
> >> HDMI RX Controller, being compliant with standard HDMI 1.4b
> >> and HDMI 2.0.
> >>
> > 
> > Hi Mauro and Hans,
> > 
> > I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
> 
> Why did you put clk changes here? These go via different subsystem. That
> might be one of obstacles for your patchset.
> 

I added clock changes in this patch series because HDMIRX driver depends on it.
I thought it is wrong to send the driver patches which don't even compile?

Since you are a more experienced developer, can you help me understand what would
be the right way to send patches in such scenarios?

Thanks,
Shreeya Patel

> Also, you sent it just a week ago and you already ping. Please relax,
> and help out by reviewing other patches on the mailing lists in order to
> relieve the burden of maintainers and move your patches higher up the list.
> 
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski April 3, 2024, 11:24 a.m. UTC | #4 
On 03/04/2024 13:20, Shreeya Patel wrote:
> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 03/04/2024 11:24, Shreeya Patel wrote:
>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
>>>
>>>> This series implements support for the Synopsys DesignWare
>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b
>>>> and HDMI 2.0.
>>>>
>>>
>>> Hi Mauro and Hans,
>>>
>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
>>
>> Why did you put clk changes here? These go via different subsystem. That
>> might be one of obstacles for your patchset.
>>
> 
> I added clock changes in this patch series because HDMIRX driver depends on it.
> I thought it is wrong to send the driver patches which don't even compile?

Hm, why HDMIRX driver depends on clock? How? This sounds really wrong.
Please get it reviewed internally first.

> 
> Since you are a more experienced developer, can you help me understand what would
> be the right way to send patches in such scenarios?

I am not the substitute for your Collabora engineers and peers. You do
not get free work from the community. First, do the work and review
internally, to solve all trivial things, like how to submit patches
upstream or how to make your driver buildable, and then ask community
for the review.

Best regards,
Krzysztof
Deborah Brouwer April 3, 2024, 9:13 p.m. UTC | #5 
On Wed, Apr 03, 2024 at 01:24:05PM +0200, Krzysztof Kozlowski wrote:
> On 03/04/2024 13:20, Shreeya Patel wrote:
> > On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > 
> >> On 03/04/2024 11:24, Shreeya Patel wrote:
> >>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
> >>>
> >>>> This series implements support for the Synopsys DesignWare
> >>>> HDMI RX Controller, being compliant with standard HDMI 1.4b
> >>>> and HDMI 2.0.
> >>>>
> >>>
> >>> Hi Mauro and Hans,
> >>>
> >>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
> >>
> >> Why did you put clk changes here? These go via different subsystem. That
> >> might be one of obstacles for your patchset.
> >>
> > 
> > I added clock changes in this patch series because HDMIRX driver depends on it.
> > I thought it is wrong to send the driver patches which don't even compile?
> 
> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong.
> Please get it reviewed internally first.
> 
> > 
> > Since you are a more experienced developer, can you help me understand what would
> > be the right way to send patches in such scenarios?
> 
> I am not the substitute for your Collabora engineers and peers. You do
> not get free work from the community. First, do the work and review
> internally, to solve all trivial things, like how to submit patches
> upstream or how to make your driver buildable, and then ask community
> for the review.

I don't think Shreeya was asking for "free" work from the community.
Her question wasn't trivial or obvious since reasonable people seem to sometimes
disagree about where to send a patch especially if it's needed to make a series compile.
I heard the issue was already resolved but had to say something since this accusation
seemed so unfair.

> 
> Best regards,
> Krzysztof
> 
>
Heiko Stuebner April 3, 2024, 10:48 p.m. UTC | #6 
Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski:
> On 03/04/2024 13:20, Shreeya Patel wrote:
> > On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> > 
> >> On 03/04/2024 11:24, Shreeya Patel wrote:
> >>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
> >>>
> >>>> This series implements support for the Synopsys DesignWare
> >>>> HDMI RX Controller, being compliant with standard HDMI 1.4b
> >>>> and HDMI 2.0.
> >>>>
> >>>
> >>> Hi Mauro and Hans,
> >>>
> >>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
> >>
> >> Why did you put clk changes here? These go via different subsystem. That
> >> might be one of obstacles for your patchset.
> >>
> > 
> > I added clock changes in this patch series because HDMIRX driver depends on it.
> > I thought it is wrong to send the driver patches which don't even compile?
> 
> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong.
> Please get it reviewed internally first.

For the change in question, the clock controller on the soc also handles
the reset controls (hence its name CRU, clock-and-reset-unit) .

There are at least 660 reset lines in the unit and it seems the hdmi-rx one
was overlooked on the initial submission, hence patches 1+2 add the
reset-line.

Of course, here only the "arm64: dts:" patch depends on the clock
change, is it references the new reset-id.


Am Mittwoch, 3. April 2024, 12:22:57 CEST schrieb Krzysztof Kozlowski:
> Please do not engage multiple subsystems in one patchset, if not
> necessary. Especially do not mix DTS into media or USB subsystems. And
> do not put DTS in the middle!

picking up your reply from patch 4/6, there seem to be different "schools
of thought" for this. Some maintainers might want to really only see
patches that are explicitly for their subsystem - I guess networking
might be a prime example for that, who will essentially apply whole series'
if nobody protests in time (including dts patches)

On the other hand I also remember seeing requests for "the full picture"
and individual maintainers then just picking and applying the patches
meant for their subsystem.

The series as it stands right now is nice in that it allows (random)
developers to just pick it up, apply it to a tree and test the actual driver
without needing to hunt for multiple dependant series.


Of course you're right, the "arm64: dts:" patch should be the last in the
series and not be in the middle of it.


Regards
Heiko
Krzysztof Kozlowski April 4, 2024, 6:15 a.m. UTC | #7 
On 04/04/2024 00:48, Heiko Stübner wrote:
> Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski:
>> On 03/04/2024 13:20, Shreeya Patel wrote:
>>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>>> On 03/04/2024 11:24, Shreeya Patel wrote:
>>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
>>>>>
>>>>>> This series implements support for the Synopsys DesignWare
>>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b
>>>>>> and HDMI 2.0.
>>>>>>
>>>>>
>>>>> Hi Mauro and Hans,
>>>>>
>>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
>>>>
>>>> Why did you put clk changes here? These go via different subsystem. That
>>>> might be one of obstacles for your patchset.
>>>>
>>>
>>> I added clock changes in this patch series because HDMIRX driver depends on it.
>>> I thought it is wrong to send the driver patches which don't even compile?
>>
>> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong.
>> Please get it reviewed internally first.
> 
> For the change in question, the clock controller on the soc also handles
> the reset controls (hence its name CRU, clock-and-reset-unit) .
> 
> There are at least 660 reset lines in the unit and it seems the hdmi-rx one
> was overlooked on the initial submission, hence patches 1+2 add the
> reset-line.
> 
> Of course, here only the "arm64: dts:" patch depends on the clock
> change, is it references the new reset-id.

Wait, that's expected, but it is not what was written. Claim was HDMIRX
driver depends *build time* ("don't even compile").

> 
> 
> Am Mittwoch, 3. April 2024, 12:22:57 CEST schrieb Krzysztof Kozlowski:
>> Please do not engage multiple subsystems in one patchset, if not
>> necessary. Especially do not mix DTS into media or USB subsystems. And
>> do not put DTS in the middle!
> 
> picking up your reply from patch 4/6, there seem to be different "schools
> of thought" for this. Some maintainers might want to really only see
> patches that are explicitly for their subsystem - I guess networking
> might be a prime example for that, who will essentially apply whole series'
> if nobody protests in time (including dts patches)

There is no school saying DTS is allowed to be in the middle.

Other schools are indeed saying that seeing DTS is good and
recommendation is to post it separate and provide a link. That's way you
avoid DTS being pulled by Greg, media or networking.

> 
> On the other hand I also remember seeing requests for "the full picture"
> and individual maintainers then just picking and applying the patches
> meant for their subsystem.
> 
> The series as it stands right now is nice in that it allows (random)
> developers to just pick it up, apply it to a tree and test the actual driver
> without needing to hunt for multiple dependant series.
> 
> 
> Of course you're right, the "arm64: dts:" patch should be the last in the
> series and not be in the middle of it.


Best regards,
Krzysztof
Krzysztof Kozlowski April 4, 2024, 6:17 a.m. UTC | #8 
On 03/04/2024 23:13, Deborah Brouwer wrote:
> On Wed, Apr 03, 2024 at 01:24:05PM +0200, Krzysztof Kozlowski wrote:
>> On 03/04/2024 13:20, Shreeya Patel wrote:
>>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>>> On 03/04/2024 11:24, Shreeya Patel wrote:
>>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
>>>>>
>>>>>> This series implements support for the Synopsys DesignWare
>>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b
>>>>>> and HDMI 2.0.
>>>>>>
>>>>>
>>>>> Hi Mauro and Hans,
>>>>>
>>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
>>>>
>>>> Why did you put clk changes here? These go via different subsystem. That
>>>> might be one of obstacles for your patchset.
>>>>
>>>
>>> I added clock changes in this patch series because HDMIRX driver depends on it.
>>> I thought it is wrong to send the driver patches which don't even compile?
>>
>> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong.
>> Please get it reviewed internally first.
>>
>>>
>>> Since you are a more experienced developer, can you help me understand what would
>>> be the right way to send patches in such scenarios?
>>
>> I am not the substitute for your Collabora engineers and peers. You do
>> not get free work from the community. First, do the work and review
>> internally, to solve all trivial things, like how to submit patches
>> upstream or how to make your driver buildable, and then ask community
>> for the review.
> 
> I don't think Shreeya was asking for "free" work from the community.
> Her question wasn't trivial or obvious since reasonable people seem to sometimes
> disagree about where to send a patch especially if it's needed to make a series compile.
> I heard the issue was already resolved but had to say something since this accusation
> seemed so unfair.

If HDMI driver does not build because of clock driver, something is
really wrong at the basics level. Therefore I am sure my statement was
fair,. based on Shreeya statement of build failure.

I am sorry, but independence of drivers and independence of DTS is a
basic thing, so to solve such you can easily get help internally from
your experienced folks (which you have).

Best regards,
Krzysztof
Shreeya Patel April 4, 2024, 8:07 a.m. UTC | #9 
On Thursday, April 04, 2024 11:45 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 04/04/2024 00:48, Heiko Stübner wrote:
> > Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski:
> >> On 03/04/2024 13:20, Shreeya Patel wrote:
> >>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >>>
> >>>> On 03/04/2024 11:24, Shreeya Patel wrote:
> >>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
> >>>>>
> >>>>>> This series implements support for the Synopsys DesignWare
> >>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b
> >>>>>> and HDMI 2.0.
> >>>>>>
> >>>>>
> >>>>> Hi Mauro and Hans,
> >>>>>
> >>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
> >>>>
> >>>> Why did you put clk changes here? These go via different subsystem. That
> >>>> might be one of obstacles for your patchset.
> >>>>
> >>>
> >>> I added clock changes in this patch series because HDMIRX driver depends on it.
> >>> I thought it is wrong to send the driver patches which don't even compile?
> >>
> >> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong.
> >> Please get it reviewed internally first.
> > 
> > For the change in question, the clock controller on the soc also handles
> > the reset controls (hence its name CRU, clock-and-reset-unit) .
> > 
> > There are at least 660 reset lines in the unit and it seems the hdmi-rx one
> > was overlooked on the initial submission, hence patches 1+2 add the
> > reset-line.
> > 
> > Of course, here only the "arm64: dts:" patch depends on the clock
> > change, is it references the new reset-id.
> 
> Wait, that's expected, but it is not what was written. Claim was HDMIRX
> driver depends *build time* ("don't even compile").
> 

For some context, when I was testing the downstream driver against the
device tree, I saw some failures because of the missing reset ( which I am trying
to add in the first two patches for this series )

...
	hdmirx_dev->rst_biu = devm_reset_control_get(hdmirx_dev->dev, "rst_biu");
	if (IS_ERR(hdmirx_dev->rst_biu)) {
		dev_err(dev, "failed to get rst_biu control\n");
		return PTR_ERR(hdmirx_dev->rst_biu);
	}

shreeya@shreeya:~/collabora/rd/rockchip/linux$ make dtbs
  DTC     arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5-evb.dtb
Error: arch/arm64/boot/dts/rockchip/rk3588.dtsi:187.23-24 syntax error
FATAL ERROR: Unable to parse input tree
make[3]: *** [scripts/Makefile.lib:419: arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5-evb.dtb] Error 1
make[2]: *** [scripts/Makefile.build:481: arch/arm64/boot/dts/rockchip] Error 2
make[1]: *** [/home/shreeya/collabora/rd/rockchip/linux/Makefile:1392: dtbs] Error 2
make: *** [Makefile:240: __sub-make] Error 2

Sorry for referring this as a driver build failure but I am sure you would 
also have not been okay with the above issues.
Ofcourse I can simply remove this dependency from the driver but maybe
that will affect the functionality and I didn't want to send a buggy patch series.

My intention here was just like Heiko said, to keep all the dependent patches
together. I didn't know that would be a trouble for Maintainers.

FWIW, HDMIRX patch series was reviewed multiple times and that is why you
see a Reviewed-by tag from a Collabora Engineer. Sometimes the things that look
problematic to one might not look the same to others and that is why I asked you
to provide some more details about the problem that you were seeing.

https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/merge_requests/21
https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/merge_requests/17


> > 
> > 
> > Am Mittwoch, 3. April 2024, 12:22:57 CEST schrieb Krzysztof Kozlowski:
> >> Please do not engage multiple subsystems in one patchset, if not
> >> necessary. Especially do not mix DTS into media or USB subsystems. And
> >> do not put DTS in the middle!
> > 
> > picking up your reply from patch 4/6, there seem to be different "schools
> > of thought" for this. Some maintainers might want to really only see
> > patches that are explicitly for their subsystem - I guess networking
> > might be a prime example for that, who will essentially apply whole series'
> > if nobody protests in time (including dts patches)
> 
> There is no school saying DTS is allowed to be in the middle.
> 
> Other schools are indeed saying that seeing DTS is good and
> recommendation is to post it separate and provide a link. That's way you
> avoid DTS being pulled by Greg, media or networking.
> 

This was my mistake and I simply forgot to remove the DTS when I was
testing the driver for the last time before sending the v3 upstream.
Adding it in the middle was incorrect, I should have added it as a separate
patch but honestly this has always been very confusing and the expectation
differs from maintainers to maintainers.

> > 
> > On the other hand I also remember seeing requests for "the full picture"
> > and individual maintainers then just picking and applying the patches
> > meant for their subsystem.
> > 
> > The series as it stands right now is nice in that it allows (random)
> > developers to just pick it up, apply it to a tree and test the actual driver
> > without needing to hunt for multiple dependant series.
> > 
> > 
> > Of course you're right, the "arm64: dts:" patch should be the last in the
> > series and not be in the middle of it.
> 

I will make sure to correct this in my v4.

Thanks,
Shreeya Patel
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski April 4, 2024, 8:17 a.m. UTC | #10 
On 04/04/2024 10:07, Shreeya Patel wrote:
> On Thursday, April 04, 2024 11:45 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 04/04/2024 00:48, Heiko Stübner wrote:
>>> Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski:
>>>> On 03/04/2024 13:20, Shreeya Patel wrote:
>>>>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>>>
>>>>>> On 03/04/2024 11:24, Shreeya Patel wrote:
>>>>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
>>>>>>>
>>>>>>>> This series implements support for the Synopsys DesignWare
>>>>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b
>>>>>>>> and HDMI 2.0.
>>>>>>>>
>>>>>>>
>>>>>>> Hi Mauro and Hans,
>>>>>>>
>>>>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
>>>>>>
>>>>>> Why did you put clk changes here? These go via different subsystem. That
>>>>>> might be one of obstacles for your patchset.
>>>>>>
>>>>>
>>>>> I added clock changes in this patch series because HDMIRX driver depends on it.
>>>>> I thought it is wrong to send the driver patches which don't even compile?
>>>>
>>>> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong.
>>>> Please get it reviewed internally first.
>>>
>>> For the change in question, the clock controller on the soc also handles
>>> the reset controls (hence its name CRU, clock-and-reset-unit) .
>>>
>>> There are at least 660 reset lines in the unit and it seems the hdmi-rx one
>>> was overlooked on the initial submission, hence patches 1+2 add the
>>> reset-line.
>>>
>>> Of course, here only the "arm64: dts:" patch depends on the clock
>>> change, is it references the new reset-id.
>>
>> Wait, that's expected, but it is not what was written. Claim was HDMIRX
>> driver depends *build time* ("don't even compile").
>>
> 
> For some context, when I was testing the downstream driver against the
> device tree, I saw some failures because of the missing reset ( which I am trying
> to add in the first two patches for this series )
> 
> ...
> 	hdmirx_dev->rst_biu = devm_reset_control_get(hdmirx_dev->dev, "rst_biu");
> 	if (IS_ERR(hdmirx_dev->rst_biu)) {
> 		dev_err(dev, "failed to get rst_biu control\n");
> 		return PTR_ERR(hdmirx_dev->rst_biu);
> 	}

That's a driver....

> shreeya@shreeya:~/collabora/rd/rockchip/linux$ make dtbs
>   DTC     arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5-evb.dtb

and that's a DTS.

> Error: arch/arm64/boot/dts/rockchip/rk3588.dtsi:187.23-24 syntax error
> FATAL ERROR: Unable to parse input tree
> make[3]: *** [scripts/Makefile.lib:419: arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5-evb.dtb] Error 1
> make[2]: *** [scripts/Makefile.build:481: arch/arm64/boot/dts/rockchip] Error 2
> make[1]: *** [/home/shreeya/collabora/rd/rockchip/linux/Makefile:1392: dtbs] Error 2
> make: *** [Makefile:240: __sub-make] Error 2

They are not related anyhow. Look above which Makefile target produced
error. Which file failed to build. This is a expressed in make[3] line.
Directory is expressed in other places.

> 
> Sorry for referring this as a driver build failure but I am sure you would 
> also have not been okay with the above issues.
> Ofcourse I can simply remove this dependency from the driver but maybe
> that will affect the functionality and I didn't want to send a buggy patch series.

What dependency?

It seems you did not understand anything from Heiko's message, so please
reach to your colleagues for explanation where is the dependency.
> 
> My intention here was just like Heiko said, to keep all the dependent patches
> together. I didn't know that would be a trouble for Maintainers.

They are not dependent.

> 
> FWIW, HDMIRX patch series was reviewed multiple times and that is why you
> see a Reviewed-by tag from a Collabora Engineer. Sometimes the things that look
> problematic to one might not look the same to others and that is why I asked you
> to provide some more details about the problem that you were seeing.
> 
> https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/merge_requests/21
> https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/merge_requests/17

Sorry, that's some third party gitlab... I don't know what does it prove.

> 
> 
>>>
>>>
>>> Am Mittwoch, 3. April 2024, 12:22:57 CEST schrieb Krzysztof Kozlowski:
>>>> Please do not engage multiple subsystems in one patchset, if not
>>>> necessary. Especially do not mix DTS into media or USB subsystems. And
>>>> do not put DTS in the middle!
>>>
>>> picking up your reply from patch 4/6, there seem to be different "schools
>>> of thought" for this. Some maintainers might want to really only see
>>> patches that are explicitly for their subsystem - I guess networking
>>> might be a prime example for that, who will essentially apply whole series'
>>> if nobody protests in time (including dts patches)
>>
>> There is no school saying DTS is allowed to be in the middle.
>>
>> Other schools are indeed saying that seeing DTS is good and
>> recommendation is to post it separate and provide a link. That's way you
>> avoid DTS being pulled by Greg, media or networking.
>>
> 
> This was my mistake and I simply forgot to remove the DTS when I was
> testing the driver for the last time before sending the v3 upstream.
> Adding it in the middle was incorrect, I should have added it as a separate
> patch but honestly this has always been very confusing and the expectation
> differs from maintainers to maintainers.

There were guidelines - presented in the conferences, mailing list and
even SoC maintainer profile explains how patches eventually end up. I
agree that it still might be confusing, but these are the basics of
submitting patches to anything touching SoC. Anyone working with SoC
will need to know them, so how about Collabora creates some internal
guideline explaining this, so such confusions could be avoided?

I know that such guidelines exist in other companies...

Best regards,
Krzysztof
Heiko Stuebner April 4, 2024, 8:19 a.m. UTC | #11 
Am Donnerstag, 4. April 2024, 08:15:50 CEST schrieb Krzysztof Kozlowski:
> On 04/04/2024 00:48, Heiko Stübner wrote:
> > Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski:
> >> On 03/04/2024 13:20, Shreeya Patel wrote:
> >>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >>>
> >>>> On 03/04/2024 11:24, Shreeya Patel wrote:
> >>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
> >>>>>
> >>>>>> This series implements support for the Synopsys DesignWare
> >>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b
> >>>>>> and HDMI 2.0.
> >>>>>>
> >>>>>
> >>>>> Hi Mauro and Hans,
> >>>>>
> >>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
> >>>>
> >>>> Why did you put clk changes here? These go via different subsystem. That
> >>>> might be one of obstacles for your patchset.
> >>>>
> >>>
> >>> I added clock changes in this patch series because HDMIRX driver depends on it.
> >>> I thought it is wrong to send the driver patches which don't even compile?
> >>
> >> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong.
> >> Please get it reviewed internally first.
> > 
> > For the change in question, the clock controller on the soc also handles
> > the reset controls (hence its name CRU, clock-and-reset-unit) .
> > 
> > There are at least 660 reset lines in the unit and it seems the hdmi-rx one
> > was overlooked on the initial submission, hence patches 1+2 add the
> > reset-line.
> > 
> > Of course, here only the "arm64: dts:" patch depends on the clock
> > change, is it references the new reset-id.
> 
> Wait, that's expected, but it is not what was written. Claim was HDMIRX
> driver depends *build time* ("don't even compile").

Trying to do a full build (kernel + dts) will fail, because the the
device-tree patch references the newly added reset-id .

So you end up with a dtc error. Same with the binding.


I think in the past to uncouple this we did reference the id by number
first:

+      resets = <&cru SRST_A_HDMIRX>, <&cru SRST_P_HDMIRX>,
+               <&cru SRST_HDMIRX_REF>, <&cru 660>;

Had the id-addition separately and then a later series for kernel x+1
to convert from 660 to SRST_A_HDMIRX_BIU .


> > Am Mittwoch, 3. April 2024, 12:22:57 CEST schrieb Krzysztof Kozlowski:
> >> Please do not engage multiple subsystems in one patchset, if not
> >> necessary. Especially do not mix DTS into media or USB subsystems. And
> >> do not put DTS in the middle!
> > 
> > picking up your reply from patch 4/6, there seem to be different "schools
> > of thought" for this. Some maintainers might want to really only see
> > patches that are explicitly for their subsystem - I guess networking
> > might be a prime example for that, who will essentially apply whole series'
> > if nobody protests in time (including dts patches)
> 
> There is no school saying DTS is allowed to be in the middle.


I think I wrote exactly that in my original reply :-)

Am Donnerstag, 4. April 2024, 00:48:38 CEST schrieb Heiko Stübner:
> Of course you're right, the "arm64: dts:" patch should be the last in the
> series and not be in the middle of it.


Heiko
Krzysztof Kozlowski April 4, 2024, 8:22 a.m. UTC | #12 
On 04/04/2024 10:19, Heiko Stübner wrote:
> Am Donnerstag, 4. April 2024, 08:15:50 CEST schrieb Krzysztof Kozlowski:
>> On 04/04/2024 00:48, Heiko Stübner wrote:
>>> Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski:
>>>> On 03/04/2024 13:20, Shreeya Patel wrote:
>>>>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>>>
>>>>>> On 03/04/2024 11:24, Shreeya Patel wrote:
>>>>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
>>>>>>>
>>>>>>>> This series implements support for the Synopsys DesignWare
>>>>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b
>>>>>>>> and HDMI 2.0.
>>>>>>>>
>>>>>>>
>>>>>>> Hi Mauro and Hans,
>>>>>>>
>>>>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
>>>>>>
>>>>>> Why did you put clk changes here? These go via different subsystem. That
>>>>>> might be one of obstacles for your patchset.
>>>>>>
>>>>>
>>>>> I added clock changes in this patch series because HDMIRX driver depends on it.
>>>>> I thought it is wrong to send the driver patches which don't even compile?
>>>>
>>>> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong.
>>>> Please get it reviewed internally first.
>>>
>>> For the change in question, the clock controller on the soc also handles
>>> the reset controls (hence its name CRU, clock-and-reset-unit) .
>>>
>>> There are at least 660 reset lines in the unit and it seems the hdmi-rx one
>>> was overlooked on the initial submission, hence patches 1+2 add the
>>> reset-line.
>>>
>>> Of course, here only the "arm64: dts:" patch depends on the clock
>>> change, is it references the new reset-id.
>>
>> Wait, that's expected, but it is not what was written. Claim was HDMIRX
>> driver depends *build time* ("don't even compile").
> 
> Trying to do a full build (kernel + dts) will fail, because the the
> device-tree patch references the newly added reset-id .
> 
> So you end up with a dtc error. Same with the binding.

Which is quite expected, nothing special, most patchsets have exactly
the same dependency. It's not a HDMIRX driver dependency. It's DTS and
clock provider on the binding header, not clock consumer.

We solved it many times and different SoC subsystems have their own
guidelines. Putting here media is not the right approach and not justified.

Best regards,
Krzysztof
Heiko Stuebner April 10, 2024, 5:15 a.m. UTC | #13 
On Thu, 28 Mar 2024 04:20:51 +0530, Shreeya Patel wrote:
> This series implements support for the Synopsys DesignWare
> HDMI RX Controller, being compliant with standard HDMI 1.4b
> and HDMI 2.0.
> 
> Features that are currently supported by the HDMI RX driver
> have been tested on rock5b board using a HDMI to micro-HDMI cable.
> It is recommended to use a good quality cable as there were
> multiple issues seen during testing the driver.
> 
> [...]

Applied, thanks!

[1/6] dt-bindings: reset: Define reset id used for HDMI Receiver
      commit: ca151fd56b5736a7adbdba5675b9d87d70f20b23
[2/6] clk: rockchip: rst-rk3588: Add reset line for HDMI Receiver
      commit: 7af67019cd78d028ef377df689ac103d51905518

Best regards,