Hi Simon,
On Thu 13 Oct 2022 at 17:51, Simon Glass <s...@chromium.org> wrote:
> HI Julien,
>
> On Wed, 12 Oct 2022 at 05:38, Julien Masson <jmas...@baylibre.com> wrote:
>>
>> The user has now the choice to specify the splash location in the MMC
>> as a raw storage.
>>
>> Signed-off-by: Julien Masson <jmas...@baylibre.com>
>> ---
>> common/splash.c | 6 ++++++
>> common/splash_source.c | 29 +++++++++++++++++++++++++++++
>> 2 files changed, 35 insertions(+)
>>
>> diff --git a/common/splash.c b/common/splash.c
>> index 0e520cc103..5206e35f74 100644
>> --- a/common/splash.c
>> +++ b/common/splash.c
>> @@ -39,6 +39,12 @@ static struct splash_location default_splash_locations[]
>> = {
>> .flags = SPLASH_STORAGE_FS,
>> .devpart = "0:1",
>> },
>> + {
>> + .name = "mmc_raw",
>> + .storage = SPLASH_STORAGE_MMC,
>> + .flags = SPLASH_STORAGE_RAW,
>> + .devpart = "0:1",
>> + },
>> {
>> .name = "usb_fs",
>> .storage = SPLASH_STORAGE_USB,
>> diff --git a/common/splash_source.c b/common/splash_source.c
>> index 87e55a54f8..b4bf6f1336 100644
>> --- a/common/splash_source.c
>> +++ b/common/splash_source.c
>> @@ -65,6 +65,33 @@ static int splash_nand_read_raw(u32 bmp_load_addr, int
>> offset, size_t read_size)
>> }
>> #endif
>>
>> +#ifdef CONFIG_CMD_MMC
>> +static int splash_mmc_read_raw(u32 bmp_load_addr, struct splash_location
>> *location,
>> + size_t read_size)
>> +{
>> + struct disk_partition partition;
>> + struct blk_desc *desc;
>> + lbaint_t blkcnt;
>> + int ret, n;
>> +
>> + ret = part_get_info_by_dev_and_name_or_num("mmc", location->devpart,
>> &desc,
>> + &partition, 1);
>> + if (ret < 0)
>> + return ret;
>> +
>> + blkcnt = DIV_ROUND_UP(read_size, partition.blksz);
>> + n = blk_dread(desc, partition.start, blkcnt, (void
>> *)(uintptr_t)bmp_load_addr);
>> +
>> + return (n == blkcnt) ? 0 : -EINVAL;
>
> -EIO would be better
Yes I will change this in V2.
>
>> +}
>> +#else
>> +static int splash_mmc_read_raw(u32 bmp_load_addr, int offset, size_t
>> read_size)
I just discovered that my the second argument type is wrong.
I will fix that in V2, sorry.
>> +{
>> + debug("%s: mmc support not available\n", __func__);
>> + return -ENOSYS;
>
> Rather than the #ifdef you should be able to put these two lines in
> the first function. Does patman not warn you about this sort of thing?
Yes that is true I can place the #ifdef inside the function but I've
"voluntary" done that to stay consistent with other functions:
splash_(sf|nand).*
No I did not use patman, I've run checkpatch first and git send-email:
$ ./scripts/checkpatch.pl --strict --u-boot
But even with patman I don't see the warning you are talking:
$ ./tools/patman/patman -n
Cleaned 2 patches
0 errors, 1 warnings, 0 checks for 0002-splash-support-raw-image-from-MMC.patch:
common/splash_source.c:68: warning: Use 'if (IS_ENABLED(CONFIG...))' instead of
'#if or #ifdef' where possible
checkpatch.pl found 0 error(s), 1 warning(s), 0 checks(s)
Alias 'splash' not found
Alias 'splash' not found
Not sending emails due to errors/warnings
Dry run, so not doing much. But I would do this:
...
What command do you use to have this warning ?
Thanks
>
>> +}
>> +#endif
>> +
>> static int splash_storage_read_raw(struct splash_location *location,
>> u32 bmp_load_addr, size_t read_size)
>> {
>> @@ -75,6 +102,8 @@ static int splash_storage_read_raw(struct splash_location
>> *location,
>>
>> offset = location->offset;
>> switch (location->storage) {
>> + case SPLASH_STORAGE_MMC:
>> + return splash_mmc_read_raw(bmp_load_addr, location,
>> read_size);
>> case SPLASH_STORAGE_NAND:
>> return splash_nand_read_raw(bmp_load_addr, offset, read_size);
>> case SPLASH_STORAGE_SF:
>> --
>> 2.37.3
>>
>
> Regards,
> SImon
--
Julien Masson
