[PATCH v2 23/28] drm: omapdrm: Merge the dss_features and omap_dss_features structures

[Laurent Pinchart]

Hi Tomi,

On Monday 15 May 2017 09:32:01 Tomi Valkeinen wrote:
> On 13/05/17 14:12, Laurent Pinchart wrote:
> >> Is it? You already use the dss compat string and soc_device_match to
> >> figure out some versions. Isn't that a proper way to find out about the
> >> SoC? But I agree that a more fine grained version management in each
> >> individual driver would be better.
> > 
> > For OMAP2, OMAP4 or OMAP5 it shouldn't be too much of a problem, but OMAP3
> > would be more painful to handle. The following machine names are used.
> > 
> > "AM3505"
> > "AM3517"
> > "AM437x"
> > "OMAP3430/3530"
> > "OMAP3525"
> > "OMAP3515"
> > "OMAP3503"
> > "OMAP3611"
> > "OMAP3615/AM3715"
> > "OMAP3621"
> > "OMAP3630/DM3730"
> > "AM3703"
> > "DM3725"
> 
> Don't we need all those somewhere in any case? I mean, I presume all the
> current OMAPDSS_VER_* defines are used somewhere. Which means that
> somewhere, maybe in multiple different files, we need to handle some or
> all of those.

We do, but the way we combine all the information we need in a single version 
isn't ideal. Most omapdrm sub-drivers use version checks in a way that combine 
many cases together, resulting in just a handful of different options. 
Different drivers use different checks, so we end up with a long list of 
versions that more or less describe the combination of multiple boolean (or 
short enum) parameters. By splitting the version into those parameters, we can 
simplify the code, and move the pieces where they belong to.

> For some features, perhaps we could move them to DT. If the feature is
> about how DSS is integrated into the SoC, it might make sense to have
> that in the DT, as a property for DSS, as it's not part of DSS itself.
> If the data is missing, we could inject it into the DT at boot time,
> based on the omap revision. Though I'm not sure if that would really
> help that much.

I agree in principle, but we should probably check that on a case by case 
basis.

> I think the most annoying "feature" is the blank/porch register field
> size change, that they made between OMAP3430 ES1 and ES2. And didn't
> bother to change the DSS IP version...

This could indeed be described in DT (either through a different compatible 
string or a separate DT property). Using SoC matching is a reasonable 
alternative in my opinion to handle past mistakes, as long as there's not too 
many of them.

> But back to the topic... Are you sure this version detection should not
> be centralized? If we have that many OMAP3 related devices already (btw,
> AM4 is not really OMAP3 related, even if the DSS there is)... And if we
> get one more. Does it mean we need to edit that same new device into
> many places?

Yes, I'm fairly confident, as most parameters are related to versions of the 
individual IP cores, not about how they're integrated in a particular SoC.

> Then again, maybe it's better to handle that separately in each file
> that requires the information, as sometimes all you need to know is that
> it's DSS3 based, but sometimes you need to know that it's AM4's DSS vs
> OMAP3's DSS.

Yes, that's definitely how I've implemented it (in patches I'm about to send). 
It results in simpler code in my opinion.

> > But I don't think that removing the extra platform devices is so urgent.
> > I'd rather do it when we're ready.
> > 
> > The omapdss driver needs major cleanup and refactoring, and that will
> > result in a large number of patches that will cause conflicts and be
> > sources of potential errors. I don't think that's avoidable. However,
> > lots of those cleanups will be independent from each other, so we can
> > start merging the less controversial ones as soon as they're ready. I'll
> > take care of rebasing the rest of the patches as needed.
> 
> Yes, no disagreements there. My hope is just that a series is as small
> as possible. Or, that's not correct... A series does one change at a
> time, e.g. here my problem was that the platform change was being done
> at the same time as the feature change, interleaved.
> 
> I most likely will need to backport all these to v4.9 kernel. And then
> manage new patches that will be applied on top of mainline and that v4.9
> kernel. And I will gladly take any changes to patch serieses that
> possibly make that work easier =).

-- 
Regards,

Laurent Pinchart