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

[Tomi Valkeinen]

On 10/05/17 01:09, Laurent Pinchart wrote:
> Hi Tomi,
> 
> 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.

> 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.

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

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.

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170510/69420877/attachment.sig>