mbox series

[v5?,0/6] Tweaked basic Synopsys DW HDMI QP TX driver for Rockchip RK3588

Message ID 20240830152132.8894-1-shimrrashai@gmail.com (mailing list archive)
Headers show
Series Tweaked basic Synopsys DW HDMI QP TX driver for Rockchip RK3588 | expand

Message

Shimrra Shai Aug. 30, 2024, 3:21 p.m. UTC
Hi,

I saw Cristian Ciocaltea's proposed basic driver for the Synopsys DW
HDMI QP transmit (TX) facility on the Rockchip RK3588 and noticed that
it had seen some critique and thought I'd help it along a little by
making some of the changes that others had suggested in the discussion
thread. This package is mostly like his(?) original but features the
following changes suggested by Conor Dooley and Heiko Stuebner:

 * Documentation for the device tree bindings specifies the various
   clocks explicitly in both the general (synopsys,dw-hdmi-qp.yaml)
   and Rockchip-specific (rockchip,rk3588-dw-hdmi-qp.yaml) files.
 * Changed the compatibles for the RK3588 VO0 and VO1 GRFs in the
   Device Trees (rk3588-base.dtsi) to reflect their different natures.

and some of my own changes:

 * Tweaked the driver code slightly - mostly organizational, but also
   added a mutex around device access in the dw_hdmi_qp_... method
   that was present in the downstream BSP driver which might have been
   necessary to prevent thread bugs.
 * Improved grammar & punctuation in some of the English on the
   Kconfigs and output messages.

Let me know how you like it. I hope this is suitable enough for kernel
integration as I'd really like to be able to get some of the newest
kernels having video bringup out of the box. I'm testing and
developing on the Firefly ITX-3588J board; not sure if this will also
work on the Rock 5B too, as I don't have one, but since little was
done to the original driver I don't think it should break anything.
Moreover, I wanna get the device tree bindings nailed on this point
because I'd like to prepare a UEFI firmware package for this board
that would be capable of booting mainline kernels from this (or a
soon-to-be) version onward, without needing a separate device tree
file with the kernel, like a "proper" PC. That naturally implies
having at least all basic desktop functionality down, including video
output, before the DT is "baked" into the firmware.

- Shimrra Shai

Signed-off-by: Shimrra Shai <shimrrashai@gmail.com>
---
Link to Cristian's original w/previous versions' history: https://lore.kernel.org/linux-rockchip/20240819-b4-rk3588-bridge-upstream-v4-0-6417c72a2749@collabora.com/

Total changes:
 Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi-qp.yaml          |  89 +++++++++++
 Documentation/devicetree/bindings/display/rockchip/rockchip,rk3588-dw-hdmi-qp.yaml | 171 ++++++++++++++++++++
 Documentation/devicetree/bindings/soc/rockchip/grf.yaml                            |   6 +-
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi                                      |  44 ++++-
 drivers/gpu/drm/bridge/synopsys/Kconfig                                            |   8 +
 drivers/gpu/drm/bridge/synopsys/Makefile                                           |   1 +
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c                                       | 780 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.h                                       | 834 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/rockchip/Kconfig                                                   |   9 ++
 drivers/gpu/drm/rockchip/Makefile                                                  |   1 +
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c                                     | 431 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c                                        |   2 +
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h                                        |   1 +
 include/drm/bridge/dw_hdmi_qp.h                                                    |  36 +++++
 14 files changed, 2409 insertions(+), 4 deletions(-)

Comments

Shimrra Shai Aug. 30, 2024, 3:33 p.m. UTC | #1
Geez, my email system seems to have screwed up this submission by
duplicate-sending messages for some reason. Just FYI.

Shimrra
Cristian Ciocaltea Aug. 30, 2024, 5:06 p.m. UTC | #2
Hi Shimrra,

On 8/30/24 6:21 PM, Shimrra Shai wrote:
> Hi,
> 
> I saw Cristian Ciocaltea's proposed basic driver for the Synopsys DW
> HDMI QP transmit (TX) facility on the Rockchip RK3588 and noticed that
> it had seen some critique and thought I'd help it along a little by
> making some of the changes that others had suggested in the discussion
> thread. This package is mostly like his(?) original but features the
> following changes suggested by Conor Dooley and Heiko Stuebner:

Please stop doing this!  

I appreciate your intention to help, but this is not the proper way of
doing it.  This is a work-in-progress series and you should have asked
before taking over.  Please do not interfere with other people's work
without having a preliminary agreement with the author(s).

Additionally, before submitting any other patches, you should get 
familiar with the process - see [1] for a starting point.

>  * Documentation for the device tree bindings specifies the various
>    clocks explicitly in both the general (synopsys,dw-hdmi-qp.yaml)
>    and Rockchip-specific (rockchip,rk3588-dw-hdmi-qp.yaml) files.

Why? Did you read [2]?

>  * Changed the compatibles for the RK3588 VO0 and VO1 GRFs in the
>    Device Trees (rk3588-base.dtsi) to reflect their different natures.

This has been already handled - see [3].

> and some of my own changes:
> 
>  * Tweaked the driver code slightly - mostly organizational, but also
>    added a mutex around device access in the dw_hdmi_qp_... method
>    that was present in the downstream BSP driver which might have been
>    necessary to prevent thread bugs.
>  * Improved grammar & punctuation in some of the English on the
>    Kconfigs and output messages.
> 
> Let me know how you like it. I hope this is suitable enough for kernel
> integration as I'd really like to be able to get some of the newest
> kernels having video bringup out of the box. 

That's definitely not suitable as you made lots of other mistakes while
preparing the patches, i.e. preserving authorship, missing commit
descriptions, SoB tags, etc.

Regards,
Cristian

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html
[2] https://lore.kernel.org/lkml/038073d0-d4b9-4938-9a51-ea2aeb4530f6@collabora.com/
[3] https://lore.kernel.org/lkml/20240828-rk3588-vo-grf-compat-v2-0-4db2f791593f@collabora.com/
Shimrra Shai Aug. 30, 2024, 5:54 p.m. UTC | #3
Cristian Ciocaltea wrote:
> Please stop doing this!
>
> I appreciate your intention to help, but this is not the proper way of
> doing it.  This is a work-in-progress series and you should have asked
> before taking over.  Please do not interfere with other people's work
> without having a preliminary agreement with the author(s).
>
> Additionally, before submitting any other patches, you should get
> familiar with the process - see [1] for a starting point.
>

Hi Cristian,

Sorry, I did not know what the rules/norms/customs were around this
kind of thing here as I figured it was an open contribution space. I
did not know that I should have asked for agreement with you
beforehand. So go ahead and ignore this patch series if it goes
against the rules/customs. Even more if these points have already been
addressed, as redundant work is obviously not helpful.

That said, if there is some way to help along this project "the right
way", I would like to for sure! Just tell me what you'd _really_ need
help/assistance with to get this moved ahead and I'll see if I can
give it.

Thank you!

- Shimrra

(BTW, I thought I read a lot of that stuff in your [1], but I guess I
glossed over some of the finest details; unfortunately my mind tends
to do that a lot [drop details], so I will not contest your complaint
about the substance of the submission as containing mistakes, either.

And I only think I submitted one other patch before so I am not very
experienced at this group, despite being much more experienced with
coding. So again I apologize.)
Cristian Ciocaltea Aug. 30, 2024, 7:51 p.m. UTC | #4
Hi Shimrra,

On 8/30/24 8:54 PM, Shimrra Shai wrote:
> Cristian Ciocaltea wrote:
>> Please stop doing this!
>>
>> I appreciate your intention to help, but this is not the proper way of
>> doing it.  This is a work-in-progress series and you should have asked
>> before taking over.  Please do not interfere with other people's work
>> without having a preliminary agreement with the author(s).
>>
>> Additionally, before submitting any other patches, you should get
>> familiar with the process - see [1] for a starting point.
>>
> 
> Hi Cristian,
> 
> Sorry, I did not know what the rules/norms/customs were around this
> kind of thing here as I figured it was an open contribution space. I
> did not know that I should have asked for agreement with you
> beforehand. So go ahead and ignore this patch series if it goes
> against the rules/customs. Even more if these points have already been
> addressed, as redundant work is obviously not helpful.

This is an open community and, obviously, anyone is free to contribute
without asking for permission.  However, taking over an ongoing work is
nothing but a source of confusion.  You may do this for work that is known
to be abandoned, e.g. the author(s) explicitly mentioned that in one of
their last posts, or when there is no sign of activity for long enough
time (usually months, for sure not days or weeks).  In the latter case, I
find it reasonable to still ask for a confirmation that the submitter has
no intention to continue or, at least, discuss the possibilities to join
forces and help moving further.

> That said, if there is some way to help along this project "the right
> way", I would like to for sure! Just tell me what you'd _really_ need
> help/assistance with to get this moved ahead and I'll see if I can
> give it.

Getting more testing on the series, reporting back the findings and/or
providing fixes, would be much appreciated, for sure!  The goal is to land
the basic functionality first, hence any new features should be submitted
afterwards.

Regards,
Cristian