On 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
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.
You are rewriting the true error code with -ENODEV instead of propagating it.
+#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.
Then factor those out into a function you can all and call that function.
+} +#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?
Kerneldoc specifies the format of the comment, so that documentation can be generated from it. If you document functions, use that format.
- */
+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;
+}
[...]