Re: [U-Boot] [PATCH v2 02/11] efi_loader: return efi_status_t from efi_gop_register

2018-03-23 Thread Heinrich Schuchardt 
On 03/23/2018 03:29 PM, Simon Glass wrote:
> Hi Heinrich,
> 
> On 15 February 2018 at 00:31, Heinrich Schuchardt  wrote:
>> All initialization routines should return a status code instead of
>> a boolean.
>>
>> Signed-off-by: Heinrich Schuchardt 
>> ---
>> v2
>> new patch
>> ---
>>  include/efi_loader.h |  2 +-
>>  lib/efi_loader/efi_gop.c | 34 ++
>>  2 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 72c83fd5033..779b8bde2e3 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -179,7 +179,7 @@ int efi_disk_create_partitions(efi_handle_t parent, 
>> struct blk_desc *desc,
>>const char *if_typename, int diskid,
>>const char *pdevname);
>>  /* Called by bootefi to make GOP (graphical) interface available */
>> -int efi_gop_register(void);
>> +efi_status_t efi_gop_register(void);
>>  /* Called by bootefi to make the network interface available */
>>  int efi_net_register(void);
>>  /* Called by bootefi to make the watchdog available */
>> diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
>> index 3caddd5f844..91b0b6a064b 100644
>> --- a/lib/efi_loader/efi_gop.c
>> +++ b/lib/efi_loader/efi_gop.c
>> @@ -125,8 +125,13 @@ efi_status_t EFIAPI gop_blt(struct efi_gop *this, void 
>> *buffer,
>> return EFI_EXIT(EFI_SUCCESS);
>>  }
>>
>> -/* This gets called from do_bootefi_exec(). */
>> -int efi_gop_register(void)
>> +/*
>> + * Install graphical output protocol.
>> + *
>> + * If no supported video device exists this is not considered as an
>> + * error.
>> + */
> 
> This comment should be in the header file so people can see the API in
> one place.
> 
> It's unfortunate that U-Boot error codes get lost here. Perhaps it
> does not make sense to return them and have the caller report the
> error? I'm not sure what is best, but one symptom of the current
> approach is the use of printf() to report (and suppress) the error.

I understand why using log() is a better choice than printf(). We
already added a log category for the EFI subsystem without using it yet.

Do I get your idea right:

You would return an error code to the caller. And if the caller thinks
that the GOP protocol is not needed he should output a log message and
pass on.

As Alex has already picked this patch I would prefer to put such a
change into an extra patch.

Regards

Heinrich
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 02/11] efi_loader: return efi_status_t from efi_gop_register

2018-03-23 Thread Simon Glass 
Hi Heinrich,

On 15 February 2018 at 00:31, Heinrich Schuchardt  wrote:
> All initialization routines should return a status code instead of
> a boolean.
>
> Signed-off-by: Heinrich Schuchardt 
> ---
> v2
> new patch
> ---
>  include/efi_loader.h |  2 +-
>  lib/efi_loader/efi_gop.c | 34 ++
>  2 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 72c83fd5033..779b8bde2e3 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -179,7 +179,7 @@ int efi_disk_create_partitions(efi_handle_t parent, 
> struct blk_desc *desc,
>const char *if_typename, int diskid,
>const char *pdevname);
>  /* Called by bootefi to make GOP (graphical) interface available */
> -int efi_gop_register(void);
> +efi_status_t efi_gop_register(void);
>  /* Called by bootefi to make the network interface available */
>  int efi_net_register(void);
>  /* Called by bootefi to make the watchdog available */
> diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
> index 3caddd5f844..91b0b6a064b 100644
> --- a/lib/efi_loader/efi_gop.c
> +++ b/lib/efi_loader/efi_gop.c
> @@ -125,8 +125,13 @@ efi_status_t EFIAPI gop_blt(struct efi_gop *this, void 
> *buffer,
> return EFI_EXIT(EFI_SUCCESS);
>  }
>
> -/* This gets called from do_bootefi_exec(). */
> -int efi_gop_register(void)
> +/*
> + * Install graphical output protocol.
> + *
> + * If no supported video device exists this is not considered as an
> + * error.
> + */

This comment should be in the header file so people can see the API in
one place.

It's unfortunate that U-Boot error codes get lost here. Perhaps it
does not make sense to return them and have the caller report the
error? I'm not sure what is best, but one symptom of the current
approach is the use of printf() to report (and suppress) the error.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v2 02/11] efi_loader: return efi_status_t from efi_gop_register

2018-02-14 Thread Heinrich Schuchardt 
All initialization routines should return a status code instead of
a boolean.

Signed-off-by: Heinrich Schuchardt 
---
v2
new patch
---
 include/efi_loader.h |  2 +-
 lib/efi_loader/efi_gop.c | 34 ++
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 72c83fd5033..779b8bde2e3 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -179,7 +179,7 @@ int efi_disk_create_partitions(efi_handle_t parent, struct 
blk_desc *desc,
   const char *if_typename, int diskid,
   const char *pdevname);
 /* Called by bootefi to make GOP (graphical) interface available */
-int efi_gop_register(void);
+efi_status_t efi_gop_register(void);
 /* Called by bootefi to make the network interface available */
 int efi_net_register(void);
 /* Called by bootefi to make the watchdog available */
diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
index 3caddd5f844..91b0b6a064b 100644
--- a/lib/efi_loader/efi_gop.c
+++ b/lib/efi_loader/efi_gop.c
@@ -125,8 +125,13 @@ efi_status_t EFIAPI gop_blt(struct efi_gop *this, void 
*buffer,
return EFI_EXIT(EFI_SUCCESS);
 }
 
-/* This gets called from do_bootefi_exec(). */
-int efi_gop_register(void)
+/*
+ * Install graphical output protocol.
+ *
+ * If no supported video device exists this is not considered as an
+ * error.
+ */
+efi_status_t efi_gop_register(void)
 {
struct efi_gop_obj *gopobj;
u32 bpix, col, row;
@@ -136,12 +141,15 @@ int efi_gop_register(void)
 
 #ifdef CONFIG_DM_VIDEO
struct udevice *vdev;
+   struct video_priv *priv;
 
/* We only support a single video output device for now */
-   if (uclass_first_device(UCLASS_VIDEO, &vdev) || !vdev)
-   return -1;
+   if (uclass_first_device(UCLASS_VIDEO, &vdev) || !vdev) {
+   printf("WARNING: No video device\n");
+   return EFI_SUCCESS;
+   }
 
-   struct video_priv *priv = dev_get_uclass_priv(vdev);
+   priv = dev_get_uclass_priv(vdev);
bpix = priv->bpix;
col = video_get_xsize(vdev);
row = video_get_ysize(vdev);
@@ -170,13 +178,14 @@ int efi_gop_register(void)
break;
default:
/* So far, we only work in 16 or 32 bit mode */
-   return -1;
+   printf("WARNING: Unsupported video mode\n");
+   return EFI_SUCCESS;
}
 
gopobj = calloc(1, sizeof(*gopobj));
if (!gopobj) {
printf("ERROR: Out of memory\n");
-   return 1;
+   return EFI_OUT_OF_RESOURCES;
}
 
/* Hook up to the device list */
@@ -186,8 +195,8 @@ int efi_gop_register(void)
ret = efi_add_protocol(gopobj->parent.handle, &efi_gop_guid,
   &gopobj->ops);
if (ret != EFI_SUCCESS) {
-   printf("ERROR: Out of memory\n");
-   return 1;
+   printf("ERROR: Failure adding gop protocol\n");
+   return ret;
}
gopobj->ops.query_mode = gop_query_mode;
gopobj->ops.set_mode = gop_set_mode;
@@ -199,10 +208,11 @@ int efi_gop_register(void)
gopobj->mode.info_size = sizeof(gopobj->info);
 
 #ifdef CONFIG_DM_VIDEO
-   if (bpix == VIDEO_BPP32) {
+   if (bpix == VIDEO_BPP32)
 #else
-   if (bpix == LCD_COLOR32) {
+   if (bpix == LCD_COLOR32)
 #endif
+   {
/* With 32bit color space we can directly expose the fb */
gopobj->mode.fb_base = fb_base;
gopobj->mode.fb_size = fb_size;
@@ -217,5 +227,5 @@ int efi_gop_register(void)
gopobj->bpix = bpix;
gopobj->fb = fb;
 
-   return 0;
+   return EFI_SUCCESS;
 }
-- 
2.15.1

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot