[PATCH u-boot-marvell 2/3] arm: mvebu: turris_omnia: Use the i2c_eeprom misc driver for EEPROM reading in U-Boot proper

[Stefan Roese]

On 9/3/24 10:04, Marek Behún wrote:
> On Tue, Sep 03, 2024 at 08:57:24AM +0200, Stefan Roese wrote:
>> On 8/29/24 10:08, Marek Behún wrote:
>>> Use the i2c_eeprom miscellaneous driver for reading Turris Omnia EEPROM
>>> in U-Boot proper. Keep using dm_i2c_read() in SPL build, since adding
>>> the i2c_eeprom driver to SPL build increases the image by 1.5 KiB.
>>>
>>> Signed-off-by: Marek Behún <kabel at kernel.org>
>>> ---
>>>    arch/arm/mach-mvebu/Kconfig              | 1 +
>>>    board/CZ.NIC/turris_omnia/turris_omnia.c | 9 +++++++--
>>>    configs/turris_omnia_defconfig           | 1 -
>>>    3 files changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
>>> index 4a8328760eb..c1a1a333e6c 100644
>>> --- a/arch/arm/mach-mvebu/Kconfig
>>> +++ b/arch/arm/mach-mvebu/Kconfig
>>> @@ -149,6 +149,7 @@ config TARGET_TURRIS_OMNIA
>>>    	select SPL_SYS_MALLOC_SIMPLE
>>>    	select SYS_I2C_MVTWSI
>>>    	select ATSHA204A
>>> +	select I2C_EEPROM
>>>    	select ARMADA_38X_SUPPORT_OLD_DDR3_TRAINING
>>>    config TARGET_TURRIS_MOX
>>> diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
>>> index 392df53a6d8..46f20f05c05 100644
>>> --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
>>> +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
>>> @@ -23,6 +23,7 @@
>>>    #include <dt-bindings/gpio/gpio.h>
>>>    #include <fdt_support.h>
>>>    #include <hexdump.h>
>>> +#include <i2c_eeprom.h>
>>>    #include <time.h>
>>>    #include <turris-omnia-mcu-interface.h>
>>>    #include <linux/bitops.h>
>>> @@ -473,9 +474,13 @@ static bool omnia_read_eeprom(struct omnia_eeprom *oep)
>>>    	if (!eeprom)
>>>    		return false;
>>> -	ret = dm_i2c_read(eeprom, 0, (void *)oep, sizeof(*oep));
>>> +	if (IS_ENABLED(CONFIG_SPL_BUILD))
>>> +		ret = dm_i2c_read(eeprom, 0, (void *)oep, sizeof(*oep));
>>> +	else
>>> +		ret = i2c_eeprom_read(eeprom, 0, (void *)oep, sizeof(*oep));
>>
>> So whats the advantage using i2c_eeprom_read() here in U-Boot proper?
>> The only thing that stands out to me is that the code get more complex
>> because of the SPL/non-SPL distinction.
> 
> The reasons are as follows:
> - it's "more correct" semantically
> 
> - the i2c_eeprom driver was already enabled for U-Boot proper in
>    defconfig (with this commit selected by the board), because of the
>    `eeprom` commmand, so I thought "why not use it?"
>    (looking at the code now, it goes
>       i2c_eeprom_read()
>         dereference of i2c_eeprom_ops->read method
>         i2c_eeprom_std_read()
>           dm_i2c_read()
>     so the complication is trivial, IMO)
> 
> - the following commit adds code that writes the eeprom, and this cannot
>    be done with simple dm_i2c_write(), because each eeprom page has to be
>    written by another transaction, which is what i2c_eeprom_write()
>    does.
> 
>    So I knew that I was going to add i2c_eeprom_write(), and I thought
>    that it would seem weird writing with i2c_eeprom_write() but reading
>    with dm_i2c_read().

That's what I thought after reviewing patch 3/3. Okay, makes sense.
Let's see, if we can pull this into the upcoming release.

Reviewed-by: Stefan Roese <sr at denx.de>

Thanks,
Stefan