On 5/20/26 20:25, Kandpal, Suraj wrote:
-----Original Message-----
From: John Harrison<[email protected]>
Sent: Monday, May 4, 2026 11:15 PM
To: Kandpal, Suraj<[email protected]>;
[email protected];[email protected]; kernel-
[email protected];[email protected]; linux-
[email protected];[email protected]; intel-
[email protected]
Cc: Nautiyal, Ankit K<[email protected]>; Shankar, Uma
<[email protected]>;[email protected]; Murthy,
Arun R<[email protected]>; Nikula, Jani<[email protected]>;
[email protected];[email protected];
[email protected];[email protected];
[email protected];[email protected];[email protected];
[email protected];[email protected];
[email protected];[email protected];
[email protected];[email protected];[email protected];
[email protected];
[email protected];
[email protected];
[email protected];[email protected]
Subject: Re: [v3,2/7] drm: writeback: Modify writeback init helpers
On 3/16/26 01:30, Suraj Kandpal wrote:
Now with drm_writeback_connector moved to drm_connector it makes
more
sense use drm_connector as an argument rather than
drm_writeback_connector. The writeback connector can easily be derived
from drm_connector.
Hi John
First of all thanks for helping to move this series forward.
So this patch and all five subsequent patches are basically the same search
and replace of base_conn->wb_conn to base_conn in the DRM level helper
functions, yes? I would add a little more explanation of why "it makes more
sense". Something like: "Some of the writeback helper functions require
access to the parent drm_connector object as well as the
drm_writeback_connector object itself. So, pass in the top level object and
traverse down rather than passing in the lower level object and traversing
back up. Even where such is not the case, update to use the top level object
for consistency across the interface."
Sure will update the commit message.
Also, there could be better consistency across these 'modify' patches.
First, the subject of patches 1-5 should be 'drm/writeback: ...' not
'drm: writeback: ...'. Then you have 'modify XXX helpers', 'modify XXX params'
and 'modify params for XXX'.
Sure will keep the subject consistent
It would be cleaner to pick a single variant and
use that for all the patches. Lastly, are the final two patches really
'drm/connector:'? The header file with the function declarations being
updated is drm_modeset_helper_vtables.h. Which would make the prefix
'drm/modeset'? Although, given that the declarations are specific to
writeback support, I would just stick with 'drm/writeback'
for all seven patches.
Sure will update the prefix for last two patches as well.
Although in regard to drm/writeback after grepping the git log it seems that
drm: writeback: is the correct prefix actually the correct wording would be
more prevalent prefix. So I would like to keep that
the consistently across all my patches as well.
Just because someone else got it wrong earlier doesn't mean we should
continue to get it wrong ;).
General consensus for DRM patches is "drm/xxx/yyy: zzzz" rather than
'drm: xxx: yyy: zzz". As demonstrated:
git log --oneline | grep -E 'drm: [a-z\:\ ]+:' | wc -l
1607
git log --oneline | grep -E 'drm/[a-z\/]+:' | wc -l
67140
Not sure if there is an official statement in the style guide about it
though, and maybe it is just personal preference. So not a blocker from
me, but personally, I would go with the majority on this one.
John.
