Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 18 May 2022 10:10:05 +0200
From:   Borislav Petkov <bp@alien8.de>
To:     Yazen Ghannam <yazen.ghannam@amd.com>
Cc:     linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
        Smita.KoralahalliChannabasappa@amd.com, muralidhara.mk@amd.com,
        naveenkrishna.chatradhi@amd.com
Subject: Re: [PATCH 06/18] EDAC/amd64: Add prep_chip_selects() into pvt->ops
Message-ID: <YoSp3cSoAo4SkkfQ@zn.tnic>
References: <20220509145534.44912-1-yazen.ghannam@amd.com>
 <20220509145534.44912-7-yazen.ghannam@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220509145534.44912-7-yazen.ghannam@amd.com>
Precedence: bulk

On Mon, May 09, 2022 at 02:55:22PM +0000, Yazen Ghannam wrote:
> From: Muralidhara M K <muralidhara.mk@amd.com>
> 
> GPU Nodes will need to set the number of available Chip Selects, i.e.
> Base and Mask values, differently than existing systems. A function
> pointer should be used rather than introduce another branching condition.

Yeah, it looks to me like all that detection logic should be split
eventually. Looking at read_mc_regs(), it has

        if (pvt->umc) {
                __read_mc_regs_df(pvt);

                goto skip;
        }

at the top, then a whole bunch of legacy stuff and then at the skip
label some common stuff...

Another thing you could consider is to have common functionality carved
out into helpers with "common" in the name and then call those from both
UMC and DCT paths. Perhaps that'll help keep the init paths sane. That
is, short of splitting this driver.

We did the splitting for Intel and there we have a common, librarized
code which gets linked into a couple of drivers. You don't have to do
this too - just putting it out there as an alternative.

The per-family function pointers design could be good too, if done
right. The advantage being, you have a single driver for all, yadda
yadda...

> Prepare for this by adding prep_chip_selects() to pvt->ops and set it
> as needed based on currently supported systems.
> 
> Use a "umc" prefix for modern systems, since these use Unified Memory
> Controllers (UMCs).
> 
> Use a "dct" prefix for newly-defined legacy functions, since these
> systems use DRAM Controllers (DCTs).
> 
> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>

What does Naveen's SOB mean here? Co-developed-by perhaps?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette