On 15/10/2019 14:33, Jacopo Mondi wrote:
Hi Kieran, thanks for review
<snip>
+config DRM_RCAR_CMM
- bool "R-Car DU Color Management Module (CMM) Support"
- depends on DRM && OF
- depends on DRM_RCAR_DU
DRM_RCAR_DU already depends on both DRM && OF, so I wonder if those are needed to be specified explicitly.
Doesn't hurt of course, but I see DRM_RCAR_DW_HDMI does the same, and so does DRM_RCAR_LVDS, so I don't think you need to remove it.
I did the same as it is done for HDMI and LVDS here. The extra dependencies could be dropped yes, I chose to be consistent.
Consistent is fine with me.
<snip>
+struct rcar_cmm {
- void __iomem *base;
- /*
* @lut: 1D-LUT status* @lut.enabled: 1D-LUT enabled flag*/- struct {
bool enabled;- } lut;
This used to be a more complex structure in an earlier version storing a cached version of the table. Now that the cached entry is removed, does this need to be such a complex structure rather than just say, a bool lut_enabled?
(We will soon add an equivalent clu_enabled too, but I don't know what other per-table options we'll need.)
In fact, we'll potentially have other options specific to the HGO, and CSC at some point in the future - so grouping by entity is indeed a good thing IMO.
You are right, I pondered a bit it this was worth it, but I assume the other CMM functions would have required some more complex fields so I chose to keep it separate. I have no problem to make this a lut_enabled, but I fear as soon as we support say, double buffering for the lut, having a dedicated struct would be nice.
Is it ok if I keep this the way it is?
Certainly fine for me. (That's what I tried to imply with "so grouping by entity is indeed a good thing IMO.")
<snip> -- Kieran