Re: [net-next PATCH v2 1/2] net: phy: aquantia: add firmware load support

[Heiner Kallweit <hkallweit1@gmail.com>]

On 01.11.2023 13:57, Christian Marangi wrote:
> On Wed, Nov 01, 2023 at 02:01:33PM +0100, Heiner Kallweit wrote:
>> On 01.11.2023 13:36, Christian Marangi wrote:
>>> From: Robert Marko <robimarko@gmail.com>
>>>
>>> Aquantia PHY-s require firmware to be loaded before they start operating.
>>> It can be automatically loaded in case when there is a SPI-NOR connected
>>> to Aquantia PHY-s or can be loaded from the host via MDIO.
>>>
>>> This patch adds support for loading the firmware via MDIO as in most cases
>>> there is no SPI-NOR being used to save on cost.
>>> Firmware loading code itself is ported from mainline U-boot with cleanups.
>>>
>>> The firmware has mixed values both in big and little endian.
>>> PHY core itself is big-endian but it expects values to be in little-endian.
>>> The firmware is little-endian but CRC-16 value for it is stored at the end
>>> of firmware in big-endian.
>>>
>>> It seems the PHY does the conversion internally from firmware that is
>>> little-endian to the PHY that is big-endian on using the mailbox
>>> but mailbox returns a big-endian CRC-16 to verify the written data
>>> integrity.
>>>
>>> Co-developed-by: Christian Marangi <ansuelsmth@gmail.com>
>>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>>> ---
>>> Changes v2:
>>> - Move out of RFC
>>> - Address sanity check for offsets
>>> - Add additional comments on firmware load check
>>> - Fix some typo
>>> - Capitalize CRC in comments
>>> - Rename load_sysfs to load_fs
>>>
>>
>> To make the driver better maintainable: can the firmware handling code
>> be placed in a separate source code file, similar to what has been done
>> for the hwmon part?
>> If yes, then this could also be the right time to move the aquantia
>> driver to an own subdirectory.
>>
> 
> Sure! Np for me just is it really worth it? hwmod is a bigger one but
> this is really a few functions.
> 
r8169_firmware.c is even smaller and I've never regretted having it factored
out. Whether it makes sense depends on how much you share with the main module
and how the API is structured that you provide to the main module.
So I don't say you have to do it, I'm just saying it's worth considering it.

> Anyway if requested, I will move in v3 the driver to a dedicated
> directory and move the function to a separate file!
>