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

[Laurent Pinchart]

Hi Tomi,

On Wednesday 10 May 2017 10:43:07 Tomi Valkeinen wrote:
> On 10/05/17 01:09, Laurent Pinchart wrote:
> > On Tuesday 09 May 2017 15:02:36 Tomi Valkeinen wrote:
> >> On 08/05/17 14:32, Laurent Pinchart wrote:
> >>> Both structures describe features of a particular OMAP DSS version,
> >>> there's no reason to keep them separate. Merge them together, allowing
> >>> initialization of the features based on the DSS compatible string
> >>> instead of the OMAP SoC version.
> >> 
> >> I don't think this is the correct way. As I mentioned earlier,
> >> dss_features should go. We should work on moving the parts from
> >> dss_features to each individual driver. I think there are probably only
> >> a very few dss-features that need to be accessed by multiple files, and
> >> those features could have a function of their own to query them.
> > 
> > I don't disagree with that, but can't we do so on top of this series ? I
> > don't think that these patches make the situation worse.
> > 
> >> But that may also be a bit bigger work, so... I thought about it already
> >> earlier in this series: wouldn't it be easier to first reconstruct the
> >> current OMAPDSS_VER_* with soc_device_match(), at module init time,
> >> which would allow us to keep most of the omapdss side unchanged. Then
> >> continue removing the arch/arm/ stuff.
> > 
> > I considered that to start with, but decided it was really a hack. I
> > instead
>
> 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"

> > went for cleaning things up where possible, and keeping hacks where a
> > proper rework would require a set of new patch series. The result is a
> > balance between a few hacks and lots of cleanup patches, which I
> > considered was right when I realized that the series was already 28
> > patches long.
> 
> Well, my concern is that these patches change a lot of things, moving
> stuff from here to there. And I think they would be moved or changed
> again later. So lots of room for conflicts and errors.
> 
> For example, in this patch you move things from dss.c to dss_features.c,
> but I think the dss.c is the right place for them (at least most of
> them), so we'd just end up moving them back. And if instead we'd move
> things from dss_features.c to dss.c, well, many of the things don't
> belong to dss.c and would be moved again later.

I agree with that. I've been working on patches to deconstruct dss_features 
and move all features to the right place. I've achieved a v1 that I still need 
to cleanup a bit.

> Then the HDMI PLL/PHY changes, well, as I mentioned, I don't fully agree
> with those.

I'll reply to that mail separately.

> So, while I think recreating the omapdss version in the dss driver would
> clearly be a temporary measure, still afaics it would be not that many
> lines of code and in just one place, and would allow us to easily move
> on to remove the extra platform devices and dependencies to arch/arm/.
> 
> After that we could start cleaning up the version handling inside the
> driver.
> 
> It is just the omapdss version and the dsi pinmuxing we get from the
> platform data, so getting rid of just those two should allow removal of
> the platform device. So, correct me if I'm wrong, but all the hdmi, dss
> "model" and dss_feature changes could as well be done in a separate
> series later.

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.

-- 
Regards,

Laurent Pinchart