Hi Julien,
On Thu, 13 Oct 2022 at 10:08, Julien Masson <jmas...@baylibre.com> wrote:
>
> 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
^ That is the warning
> 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
Regards,
Simon
