From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Thu, 18 Jan 2018 04:33:52 +0000 Subject: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system In-Reply-To: <26cd2102-08f4-68ef-a167-e28f549c93a0@denx.de> References: <1514351078-3487-1-git-send-email-tien.fong.chee@intel.com> <1514351078-3487-3-git-send-email-tien.fong.chee@intel.com> <26cd2102-08f4-68ef-a167-e28f549c93a0@denx.de> Message-ID: <1516250032.2704.15.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote: > On 12/27/2017 06:04 AM, tien.fong.chee at intel.com wrote: > > Whoa, this improved substantially since last time I checked. Minor > nitpicks below. > > [...] > > > > > +/* USB build is not supported yet in SPL */ > > +#ifndef CONFIG_SPL_BUILD > > +#ifdef CONFIG_USB_STORAGE > > +static int init_usb(void) > > +{ > > + int err; > > + > > + err = usb_init(); > > + if (err) > > + return err; > > + > > +#ifndef CONFIG_DM_USB > > + err = usb_stor_scan(1) < 0 ? -ENODEV : 0; > if (err) > return err; > ? > This is last line code of the function, so it's always return the result regardless error or not. > > > > +#endif > > + > > + return err; > > +} > > +#else > > +static int init_usb(void) > > +{ > > + printf("Error: Cannot load flash image: no USB > > support\n"); > debug() ? Fix globally ... > okay. > > > > + return -ENOSYS; > > +} > > +#endif > > +#endif > > + > > +#ifdef CONFIG_SATA > > +static int init_storage_sata(void) > > +{ > > + return sata_probe(0); > > +} > > +#else > > +static int init_storage_sata(void) > > +{ > > + printf("Error: Cannot load image: no SATA support\n"); > > + return -ENOSYS; > > +} > > +#endif > > + > > +#ifdef CONFIG_CMD_UBIFS > > +static int mount_ubifs(struct device_location *location) > > +{ > > + int ret; > > + char cmd[32]; > > + > > + sprintf(cmd, "ubi part %s", location->mtdpart); > snprintf() ... > okay. > > > > + ret = run_command(cmd, 0); > > + if (ret) > > + return ret; > > + > > + sprintf(cmd, "ubifsmount %s", location->ubivol); > > + > > + ret = run_command(cmd, 0); > > + > > + return ret; > > +} > > + > > +static int umount_ubifs(void) > > +{ > > + return run_command("ubifsumount", 0); > Just call the function directly ? > There are some checking like ubifs_initialized in the cmd/ubifs.c. Direct callng the function would bypass those checking. > > > > +} > > +#else > > +static int mount_ubifs(struct device_location *location) > > +{ > > + printf("Error: Cannot load image: no UBIFS support\n"); > > + return -ENOSYS; > > +} > > +#endif > > + > > +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD) > > +static int init_mmc(void) > > +{ > > + /* Just for the case MMC is not yet initialized */ > > + struct mmc *mmc = NULL; > > + int err; > > + > > + spl_mmc_find_device(&mmc, spl_boot_device()); > > + > > + err = mmc_init(mmc); > > + if (err) { > > + printf("spl: mmc init failed with error: %d\n", > > err); > > + return err; > > + } > > + > > + return err; > > +} > > +#else > > +static int init_mmc(void) > > +{ > > + /* Expect somewhere already initialize MMC */ > > + return 0; > > +} > > +#endif > > + > > +static int select_fs_dev(struct device_location *location) > > +{ > > + int ret; > > + > > + if (!strcmp("mmc", location->name)) { > > + ret = fs_set_blk_dev("mmc", location->devpart, > > FS_TYPE_ANY); > > + } else if (!strcmp("usb", location->name)) { > > + ret = fs_set_blk_dev("usb", location->devpart, > > FS_TYPE_ANY); > > + } else if (!strcmp("sata", location->name)) { > > + ret = fs_set_blk_dev("sata", location->devpart, > > FS_TYPE_ANY); > > + } else if (!strcmp("ubi", location->name)) { > > + if (location->ubivol != NULL) > > + ret = fs_set_blk_dev("ubi", NULL, > > FS_TYPE_UBIFS); > > + else > > + ret = -ENODEV; > > + } else { > > + printf("Error: unsupported location storage.\n"); > > + return -ENODEV; > > + } > > + > > + if (ret) > > + printf("Error: could not access storage.\n"); > > + > > + return ret; > > +} > > + > > +static int init_storage_device(struct device_location *location) > > +{ > > + int ret; > > + > > + if (!strcmp("mmc", location->name)) { > > + ret = init_mmc(); > > + } else if (!strcmp("sata", location->name)) { > > + ret = init_storage_sata(); > > + } else if (location->ubivol != NULL) { > > + ret = mount_ubifs(location); > > +#ifndef CONFIG_SPL_BUILD > > + /* USB build is not supported yet in SPL */ > > + } else if (!strcmp("usb", location->name)) { > > + ret = init_usb(); > > +#endif > > + } else { > > + printf("Error: no supported storage device is > > available.\n"); > > + ret = -ENODEV; > > + } > > + > > + return ret; > > +} > > + > > +static void set_storage_devpart(char *name, char *devpart) > > +{ > > + size_t i; > > + > > + for (i = 0; i < ARRAY_SIZE(default_locations); i++) { > > + if (!strcmp(default_locations[i].name, name)) > > + default_locations[i].devpart = devpart; > > + } > > +} > > + > > +/* > > + * Prepare firmware struct; > > + * return -ve if fail. > Use kerneldoc and keep it consistent. > kerneldoc doesn't has explanation for this function, and this function is not for user. Or you means i shouldn't put the comment and description to the function here? > > > > + */ > > +static int _request_firmware_prepare(struct firmware **firmware_p, > > +      const char *name, void *dbuf, > > +      size_t size, u32 offset) > > +{ > > + struct firmware *firmware; > > + struct firmware_priv *fw_priv; > > + > > + *firmware_p = NULL; > > + > > + if (!name || name[0] == '\0') > > + return -EINVAL; > > + > > + firmware = calloc(1, sizeof(*firmware)); > > + if (!firmware) { > > + printf("%s: calloc(struct firmware) failed\n", > > __func__); > If malloc fails, you're screwed anyway and printf will likely fail > too, > so drop it. > okay. > > > > + return -ENOMEM; > > + } > > + > > + fw_priv = calloc(1, sizeof(*fw_priv)); > > + if (!fw_priv) { > > + printf("%s: calloc(struct fw_priv) failed\n", > > __func__); > > + free(firmware); > > + return -ENOMEM; > > + } > > + > > + fw_priv->name = name; > > + fw_priv->offset = offset; > > + firmware->data = dbuf; > > + firmware->size = size; > > + firmware->priv = fw_priv; > > + *firmware_p = firmware; > > + > > + return 0; > > +} > [...]