[PATCH 00/14] drm/mgag200: Move model-specific code into separate functions

[Jocelyn Falempe]

On 11/07/2022 09:29, Thomas Zimmermann wrote:
> Hi Sam,
> 
> thanks for reviewing.
> 
> Am 08.07.22 um 19:22 schrieb Sam Ravnborg:
>> Hi Thomas,
>>
>> On Fri, Jul 08, 2022 at 11:39:15AM +0200, Thomas Zimmermann wrote:
>>> Mgag200 still mixes model-specific code and generic code in the same
>>> functions. Separate it into distinct helpers.
>>>
>>> As part of this effort, convert the driver from simple-KMS helpers
>>> to regular atomic helpers. The latter are way more flexible and can
>>> be adapted easily for each hardware model.
>>>
>>> Tested on Matrox G200 and G200EH hardware.
>>>
>>> Thomas Zimmermann (14):
>>>    drm/mgag200: Split mgag200_modeset_init()
>>>    drm/mgag200: Move DAC-register setup into model-specific code
>>>    dmr/mgag200: Move ER/EW3 register initializatino to per-model code
>>>    drm/mgag200: Acquire I/O-register lock in atomic_commit_tail function
>>>    drm/mgag200: Store primary plane's color format in CRTC state
>>>    drm/mgag200: Reorganize before dropping simple-KMS helpers
>>>    drm/mgag200: Replace simple-KMS with regular atomic helpers
>>>    drm/mgag200: Set SCROFF in primary-plane code
>>>    drm/mgag200: Add per-device callbacks
>>>    drm/mgag200: Provide per-device callbacks for BMC synchronization
>>>    drm/mgag200: Provide per-device callbacks for PIXPLLC
>>>    drm/mgag200: Move mode-config to model-specific code
>>>    drm/mgag200: Move CRTC atomic_enable to model-specfic code
>>>    drm/mgag200: Remove type field from struct mga_device
>>
>> I have browsed the patches and they looked good.
>> I was about to complain about several things, like bmc init, but then
>> later this is all nicely cleaned up.
>> Especially the pll setup stuff did not get much scrunity, as it like
>> most of the patch looked like code movements.
> 
> Indeed. The patch moves code and adapts the functions' interfaces to the 
> new callbacks. But the implementation remains the same.

I've read the whole serie, and it's good overall.
I agree with Sam about the duplication of the DAC-registers setup, but 
that can be solved later.

I have also tested this patchset on a G200eW, and have seen no regression.

Reviewed-by: Jocelyn Falempe <jfalempe at redhat.com>
Tested-by: Jocelyn Falempe <jfalempe at redhat.com>

> 
>>
>> All patches except "Move DAC-register setup into model-specific code"
>> are:
>> Acked-by: Sam Ravnborg <sam at ravnborg.org>
>>
>>
>>>   14 files changed, 2804 insertions(+), 1586 deletions(-)
>> This is not a diffstat that makes one go - yyipeee.
>> But the improvements makes it worthwhile.
> 
> Yeah. I accept a grow in driver size in exchange for the more cleanly 
> structured code base. In my reply to the DAC review, I outline ways to 
> re-share some of the duplicated code.
> 
> Best regards
> Thomas
> 
>>
>>     Sam
>