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
