Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 6 Sep 2021 14:05:34 +0200
From:   Robert Richter <rrichter@amd.com>
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Terry Bowman <Terry.Bowman@amd.com>,
        linux-watchdog@vger.kernel.org, wim@linux-watchdog.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Watchdog: sp5100_tco: Replace watchdog cd6h/cd7h port
 I/O accesses with MMIO accesses
Message-ID: <YTYEDvTTrE5JXlk4@rric.localdomain>
References: <20210813213216.54780-1-Terry.Bowman@amd.com>
 <416a67a7-646b-eb8d-b617-80cbbbc028c6@roeck-us.net>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <416a67a7-646b-eb8d-b617-80cbbbc028c6@roeck-us.net>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?1ruli8hpGtbQAmS7F4fNncDnTM+GiT5+0pzV73MITZx3noZVNJyv8C/vS2iL?=
 =?us-ascii?Q?ko8V7JyfQ/KfyZ0EYScLnglf1fVdvkLEzrAU8SBU0uBNMZMsQ8/aTFrXddLk?=
 =?us-ascii?Q?/7KbzcajZSsWw1j2fUPWqxOflX3uD12sMZN/lo5B4OUlrFp1dEoUk5Z7+r1A?=
 =?us-ascii?Q?y9VKNeAj0XcFiffLGzZusv1LshMRhXbJdsLYCC9eiPVsMe5a11QnQ0xVzFSz?=
 =?us-ascii?Q?ytmi+PD55KdPTkv+llAquIZmiTbnALS8xSxFODepyPDncWNBKFTSzV/I60cq?=
 =?us-ascii?Q?c2+kC0AJjA214Vk4PifXxxgnw4uTR5sRj4V3MiTAu060BEcte/7orYfnMj+B?=
 =?us-ascii?Q?8ICmYI1+EmQpDW1y6lD1ejurYcmlP8EbWVB1miUXyjwmGvHMbQkhw+6CCR7X?=
 =?us-ascii?Q?/lPtGwybA692pzARWZess5Z3Bye+uQed6mDu7fi93eS8E49N1zrmvneH+CLD?=
 =?us-ascii?Q?CFgyZiXs5pmm0vrSl5y/8rpp/HFxnXVpc8mv9pFquGEQEpb7SIDNGTXvD+9q?=
 =?us-ascii?Q?I/vHHb69tZDSDRjSsWl/h623iQr4Y9KrtXDyZwvIujzz5uwP9zwkaXDg7fC1?=
 =?us-ascii?Q?woCJBN3BBCpXofaXyUiAqj5H88cuIL5md8liD5G6priIjbzzyitzl+tpQgcq?=
 =?us-ascii?Q?bn78brDzyobSp0m5Hia32zClboS4ZAvMXzBCcN5V17c9r/n8X3H0upvbRAdW?=
 =?us-ascii?Q?KY3MSD0ZDcd56mkuab1kA7N9cPQZRdhtiyMaULfo8qCYQ/dMKTYPCVeLQHP5?=
 =?us-ascii?Q?k7WJ0dgNmIWuFIMEEbmXdfLW4tAs5vGNR1MmKmel/PSxAQ/ZX+T1tqjvzAtT?=
 =?us-ascii?Q?sIPTZEjxwOjaTkd0rs2dQ5kFq8IfFmtXqCakQVyfneXyUT+TlDsU/RfFxBNV?=
 =?us-ascii?Q?91mClk7ost6fCUzZjmrMVmwr8Oscn4X/YWg4H2S8+Vf/SPX1oshsLynXN8zb?=
 =?us-ascii?Q?nB8Z55fj4XDGdhs2+oBJFsLT1PXtivD0I/onJY/cCoRe1wwJgF3tbDxbynui?=
 =?us-ascii?Q?9JMfA6Yd5J1p6nRSsdxiZD+4mBuoELQLe4YX6/VJsch+IRikLaWb0g+VWU1O?=
 =?us-ascii?Q?y1uQq4YpB5Wlxb54DTQZPr3uXKUHWaueXN3YuK2ZhlfsshM9q173gVPJNjQi?=
 =?us-ascii?Q?Jb69XF8ZYTMIhS4NQy8tAXnxhAyQrXdH3eJlfuYvg7tl+t/94PqSoE4XJOHa?=
 =?us-ascii?Q?zmj29Pi841qZgz57In0PARwao4EBfbpzhOzS15PVgQNLi+J40nzYb5UR961e?=
 =?us-ascii?Q?yU6dbhdRASGpIEowLfkBd5fVKr+J6xbKBFj30DlQjHoAXiee5IH3qWXqs9b4?=
 =?us-ascii?Q?1Gk+6ASHmV2JMM6zMNJhW2Sw?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bd271f7b-e6c5-4805-b6f3-08d9712ea62b
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3657.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Sep 2021 12:05:42.1921
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ape4yhYMxV7RTrWRZbBmVnuDH9TQhwO8yqwWIDGUHaxY6gOq2iWpaodPtX2ruXz+fCREqwjbFMz7IZvpey2joA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1628
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Guenter,

On 13.08.21 15:37:30, Guenter Roeck wrote:
> I am sorry, I don't understand why the new code can not use devm functions,

devm was not chosen here as the iomem is only used during
initialization. The resource is released at the end of it. Using devm
would keep it open and carry it around including its data until the
device is released which we wanted to avoid. This would also add
unnecessary data to struct sp5100_tco and/or the device's data.

> why the new data structure is necessary in the first place,

Instead we used struct efch_cfg to carry all that temporary data
required during initialization. This is destroyed at the end of
sp5100_tco_setupdevice().

> and why it is
> not possible to improve alignment with the existing code.

The driver serves 3 to 4 platforms now. To refactor it a core driver
is needed plus sub-drivers for each platform. We decided not to change
existing code here for older platforms and keep it as it is for them.

Moreover, the efch driver only needs changes to its initialization
code (using mmio instead of ioports), everything else remains the
same.

> This will require
> a substantial amount of time to review to ensure that the changes are not
> excessive (at first glance it for sure looks like that to me).

This patch implements mostly helper functions and adds alternative
code paths for the mmio case. For an easier review we could split the
patch in about 3-4 patches, would that help?

Thanks,

-Robert