On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
On 12/27/2017 06:04 AM, tien.fong.chee@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);
elseret = -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 isavailable.\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;
+}
[...]