[PATCH] media:atomisp:pci: Use error codes instead of bool variables

[PATCH] media:atomisp:pci: Use error codes instead of bool variables

[Georgiana Rodica Chelu]

Show the reason of failure by using the corresponding error code.
It helps to better understand why something failed.

---
 .../css2400/runtime/isys/src/virtual_isys.c        | 26 ++++++++++------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/virtual_isys.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/virtual_isys.c
index f1fc3b5..cf97a9e 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/virtual_isys.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/virtual_isys.c
@@ -48,7 +48,7 @@ more details.
 #ifndef ISP2401
 
 #endif
-static bool create_input_system_channel(
+static input_system_err_t create_input_system_channel(
 	input_system_cfg_t	*cfg,
 	bool			metadata,
 	input_system_channel_t	*channel);
@@ -198,7 +198,7 @@ ia_css_isys_error_t ia_css_isys_stream_create(
 		return false;
 
 	rc = create_input_system_channel(isys_stream_descr, false, &(isys_stream->channel));
-	if (rc == false) {
+	if (!rc) {
 		destroy_input_system_input_port(&isys_stream->input_port);
 		return false;
 	}
@@ -217,7 +217,7 @@ ia_css_isys_error_t ia_css_isys_stream_create(
 	/* create metadata channel */
 	if (isys_stream_descr->metadata.enable) {
 		rc = create_input_system_channel(isys_stream_descr, true, &isys_stream->md_channel);
-		if (rc == false) {
+		if (!rc) {
 			destroy_input_system_input_port(&isys_stream->input_port);
 			destroy_input_system_channel(&isys_stream->channel);
 			return false;
@@ -299,12 +299,11 @@ ia_css_isys_error_t ia_css_isys_stream_calculate_cfg(
  * Private Methods
  *
  **************************************************/
-static bool create_input_system_channel(
+static input_system_err_t create_input_system_channel(
 	input_system_cfg_t	*cfg,
 	bool			metadata,
 	input_system_channel_t	*me)
 {
-	bool rc = true;
 
 	me->dma_id = ISYS2401_DMA0_ID;
 
@@ -327,15 +326,11 @@ static bool create_input_system_channel(
 		me->ibuf_ctrl_id = IBUF_CTRL2_ID;
 		break;
 	default:
-		rc = false;
-		break;
+		goto exit;
 	}
 
-	if (!rc)
-		return false;
-
 	if (!acquire_sid(me->stream2mmio_id, &(me->stream2mmio_sid_id))) {
-		return false;
+		goto exit;
 	}
 
 	if (!acquire_ib_buffer(
@@ -346,16 +341,19 @@ static bool create_input_system_channel(
 			cfg->online,
 			&(me->ib_buffer))) {
 		release_sid(me->stream2mmio_id, &(me->stream2mmio_sid_id));
-		return false;
+		goto exit;
 	}
 
 	if (!acquire_dma_channel(me->dma_id, &(me->dma_channel))) {
 		release_sid(me->stream2mmio_id, &(me->stream2mmio_sid_id));
 		release_ib_buffer(&(me->ib_buffer));
-		return false;
+		goto exit;
 	}
 
-	return true;
+	return INPUT_SYSTEM_ERR_NO_ERROR;
+
+exit:
+	return INPUT_SYSTEM_ERR_CREATE_CHANNEL_FAIL;
 }
 
 static void destroy_input_system_channel(
-- 
2.7.4

Re: [PATCH] media:atomisp:pci: Use error codes instead of bool variables

[Georgiana Chelu]

[-- Attachment #1: Type: text/plain, Size: 4000 bytes --]

On 7 March 2017 at 23:30, Georgiana Rodica Chelu <
georgiana.chelu93@gmail.com> wrote:
> Show the reason of failure by using the corresponding error code.
> It helps to better understand why something failed.
>
> ---
>  .../css2400/runtime/isys/src/virtual_isys.c        | 26
++++++++++------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/virtual_isys.c
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/virtual_isys.c
> index f1fc3b5..cf97a9e 100644
> ---
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/virtual_isys.c
> +++
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isys/src/virtual_isys.c
> @@ -48,7 +48,7 @@ more details.
>  #ifndef ISP2401
>
>  #endif
> -static bool create_input_system_channel(
> +static input_system_err_t create_input_system_channel(
>         input_system_cfg_t      *cfg,
>         bool                    metadata,
>         input_system_channel_t  *channel);
> @@ -198,7 +198,7 @@ ia_css_isys_error_t ia_css_isys_stream_create(
>                 return false;
>
>         rc = create_input_system_channel(isys_stream_descr, false,
&(isys_stream->channel));
> -       if (rc == false) {
> +       if (!rc) {
>                 destroy_input_system_input_port(&isys_stream->input_port);
>                 return false;
>         }
> @@ -217,7 +217,7 @@ ia_css_isys_error_t ia_css_isys_stream_create(
>         /* create metadata channel */
>         if (isys_stream_descr->metadata.enable) {
>                 rc = create_input_system_channel(isys_stream_descr, true,
&isys_stream->md_channel);
> -               if (rc == false) {
> +               if (!rc) {
>
destroy_input_system_input_port(&isys_stream->input_port);
>
destroy_input_system_channel(&isys_stream->channel);
>                         return false;
> @@ -299,12 +299,11 @@ ia_css_isys_error_t
ia_css_isys_stream_calculate_cfg(
>   * Private Methods
>   *
>   **************************************************/
> -static bool create_input_system_channel(
> +static input_system_err_t create_input_system_channel(
>         input_system_cfg_t      *cfg,
>         bool                    metadata,
>         input_system_channel_t  *me)
>  {
> -       bool rc = true;
>
>         me->dma_id = ISYS2401_DMA0_ID;
>
> @@ -327,15 +326,11 @@ static bool create_input_system_channel(
>                 me->ibuf_ctrl_id = IBUF_CTRL2_ID;
>                 break;
>         default:
> -               rc = false;
> -               break;
> +               goto exit;
>         }
>
> -       if (!rc)
> -               return false;
> -
>         if (!acquire_sid(me->stream2mmio_id, &(me->stream2mmio_sid_id))) {
> -               return false;
> +               goto exit;
>         }
>
>         if (!acquire_ib_buffer(
> @@ -346,16 +341,19 @@ static bool create_input_system_channel(
>                         cfg->online,
>                         &(me->ib_buffer))) {
>                 release_sid(me->stream2mmio_id,
&(me->stream2mmio_sid_id));
> -               return false;
> +               goto exit;
>         }
>
>         if (!acquire_dma_channel(me->dma_id, &(me->dma_channel))) {
>                 release_sid(me->stream2mmio_id,
&(me->stream2mmio_sid_id));
>                 release_ib_buffer(&(me->ib_buffer));
> -               return false;
> +               goto exit;
>         }
>
> -       return true;
> +       return INPUT_SYSTEM_ERR_NO_ERROR;
> +
> +exit:
> +       return INPUT_SYSTEM_ERR_CREATE_CHANNEL_FAIL;
>  }
>
>  static void destroy_input_system_channel(
> --
> 2.7.4
>


Hi Julia,

I tried to follow your advice regarding the boolean variables.
The exploration of the code revealed me that there are some
error messages that can be used instead of the bool variables.

This patch is the improved version of this patch (link
<https://groups.google.com/forum/#%21topic/outreachy-kernel/l67ua6JYA9A>).


Regards,
Georgiana

[-- Attachment #2: Type: text/html, Size: 5241 bytes --]

Re: [Outreachy kernel] [PATCH] media:atomisp:pci: Use error codes instead of bool variables

[Julia Lawall]

> -	return true;
> +	return INPUT_SYSTEM_ERR_NO_ERROR;
> +
> +exit:
> +	return INPUT_SYSTEM_ERR_CREATE_CHANNEL_FAIL;

Oh...  I was hoping for standard kernel values like -ENOMEM, -EINVAL, etc.

Also, watch your subject lines.  Normally there is a space after each :.
In any case, you should pick what has been commonly used for this driver.

julia

Re: [Outreachy kernel] [PATCH] media:atomisp:pci: Use error codes instead of bool variables

[Georgiana Chelu]

>
> > -     return true;
> > +     return INPUT_SYSTEM_ERR_NO_ERROR;
> > +
> > +exit:
> > +     return INPUT_SYSTEM_ERR_CREATE_CHANNEL_FAIL;
>
> Oh...  I was hoping for standard kernel values like -ENOMEM, -EINVAL, etc.

I will further investigate this. At this moment, it seemed reasonable to me
to use those error codes. They were defined in the module, but never used,
which made me think that they still have a purpose. So, I chose the
error code that seemed to be related to the name of the function.

>
> Also, watch your subject lines.  Normally there is a space after each :.
> In any case, you should pick what has been commonly used for this driver.
>

I will take care of this in the future. Thanks!

> julia

Re: [Outreachy kernel] [PATCH] media:atomisp:pci: Use error codes instead of bool variables

[Julia Lawall]

On Wed, 8 Mar 2017, Georgiana Chelu wrote:

> >
> > > -     return true;
> > > +     return INPUT_SYSTEM_ERR_NO_ERROR;
> > > +
> > > +exit:
> > > +     return INPUT_SYSTEM_ERR_CREATE_CHANNEL_FAIL;
> >
> > Oh...  I was hoping for standard kernel values like -ENOMEM, -EINVAL, etc.
>
> I will further investigate this. At this moment, it seemed reasonable to me
> to use those error codes. They were defined in the module, but never used,
> which made me think that they still have a purpose. So, I chose the
> error code that seemed to be related to the name of the function.

It's a reasonable strategy, but someday the code will have to use the
kernel ones, so maybe one may as well get started on that.  The code will
be hybrid for a while, but tere is no point to try to do the whole thing
at once and make mistakes.

julia

>
> >
> > Also, watch your subject lines.  Normally there is a space after each :.
> > In any case, you should pick what has been commonly used for this driver.
> >
>
> I will take care of this in the future. Thanks!
>
> > julia
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CALta04yUrNJ2ShXUoZmHuLen%3Dj%2BrWLY7wKX2uFNXuKmCuSAiMQ%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>