[PATCH 1/2] staging: media: sunxi: Change return type of cedrus_find_format()

[PATCH 1/2] staging: media: sunxi: Change return type of cedrus_find_format()

[Nishka Dasgupta]

Change return type of cedrus_find_format to bool as it is only called
once, by a function whose return value is bool, and the return value of
cedrus_find_format is returned as-is at the call-site.
Issue found with Coccinelle.

Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
---
 drivers/staging/media/sunxi/cedrus/cedrus_video.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index 9673874ece10..0ec31b9e0aea 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -55,8 +55,8 @@ static inline struct cedrus_ctx *cedrus_file2ctx(struct file *file)
 	return container_of(file->private_data, struct cedrus_ctx, fh);
 }
 
-static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions,
-						unsigned int capabilities)
+static bool cedrus_find_format(u32 pixelformat, u32 directions,
+			       unsigned int capabilities)
 {
 	struct cedrus_format *fmt;
 	unsigned int i;
@@ -70,13 +70,10 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions,
 
 		if (fmt->pixelformat == pixelformat &&
 		    (fmt->directions & directions) != 0)
-			break;
+			return true;
 	}
 
-	if (i == CEDRUS_FORMATS_COUNT)
-		return NULL;
-
-	return &cedrus_formats[i];
+	return false;
 }
 
 static bool cedrus_check_format(u32 pixelformat, u32 directions,
-- 
2.19.1

[PATCH 2/2] staging: media: sunxi: Replace function cedrus_check_format()

[Nishka Dasgupta]

Remove function cedrus_check_format as all it does is call
cedrus_find_format.
Rename cedrus_find_format to cedrus_check_format to maintain
compatibility with call sites.
Issue found with Coccinelle.

Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
---
 drivers/staging/media/sunxi/cedrus/cedrus_video.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index 0ec31b9e0aea..d5cc9ed04fd2 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -55,8 +55,8 @@ static inline struct cedrus_ctx *cedrus_file2ctx(struct file *file)
 	return container_of(file->private_data, struct cedrus_ctx, fh);
 }
 
-static bool cedrus_find_format(u32 pixelformat, u32 directions,
-			       unsigned int capabilities)
+static bool cedrus_check_format(u32 pixelformat, u32 directions,
+				unsigned int capabilities)
 {
 	struct cedrus_format *fmt;
 	unsigned int i;
@@ -76,12 +76,6 @@ static bool cedrus_find_format(u32 pixelformat, u32 directions,
 	return false;
 }
 
-static bool cedrus_check_format(u32 pixelformat, u32 directions,
-				unsigned int capabilities)
-{
-	return cedrus_find_format(pixelformat, directions, capabilities);
-}
-
 static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt)
 {
 	unsigned int width = pix_fmt->width;
-- 
2.19.1

Re: [PATCH 2/2] staging: media: sunxi: Replace function cedrus_check_format()

[Paul Kocialkowski]

Hi,

On Wed 03 Jul 19, 13:43, Nishka Dasgupta wrote:
> Remove function cedrus_check_format as all it does is call
> cedrus_find_format.
> Rename cedrus_find_format to cedrus_check_format to maintain
> compatibility with call sites.
> Issue found with Coccinelle.

Maybe we could have a !! or a bool cast to make coccinelle happy here?

Cheers,

Paul

> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus_video.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index 0ec31b9e0aea..d5cc9ed04fd2 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -55,8 +55,8 @@ static inline struct cedrus_ctx *cedrus_file2ctx(struct file *file)
>  	return container_of(file->private_data, struct cedrus_ctx, fh);
>  }
>  
> -static bool cedrus_find_format(u32 pixelformat, u32 directions,
> -			       unsigned int capabilities)
> +static bool cedrus_check_format(u32 pixelformat, u32 directions,
> +				unsigned int capabilities)
>  {
>  	struct cedrus_format *fmt;
>  	unsigned int i;
> @@ -76,12 +76,6 @@ static bool cedrus_find_format(u32 pixelformat, u32 directions,
>  	return false;
>  }
>  
> -static bool cedrus_check_format(u32 pixelformat, u32 directions,
> -				unsigned int capabilities)
> -{
> -	return cedrus_find_format(pixelformat, directions, capabilities);
> -}
> -
>  static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt)
>  {
>  	unsigned int width = pix_fmt->width;
> -- 
> 2.19.1
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH 2/2] staging: media: sunxi: Replace function cedrus_check_format()

[Nishka Dasgupta]

On 05/07/19 3:56 PM, Paul Kocialkowski wrote:
> Hi,
> 
> On Wed 03 Jul 19, 13:43, Nishka Dasgupta wrote:
>> Remove function cedrus_check_format as all it does is call
>> cedrus_find_format.
>> Rename cedrus_find_format to cedrus_check_format to maintain
>> compatibility with call sites.
>> Issue found with Coccinelle.
> 
> Maybe we could have a !! or a bool cast to make coccinelle happy here?

Coccinelle didn't flag the type mismatch, just the single-line 
functions. I could add the bool cast then?

Thanking you,
Nishka

> Cheers,
> 
> Paul
> 
>> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
>> ---
>>   drivers/staging/media/sunxi/cedrus/cedrus_video.c | 10 ++--------
>>   1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
>> index 0ec31b9e0aea..d5cc9ed04fd2 100644
>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
>> @@ -55,8 +55,8 @@ static inline struct cedrus_ctx *cedrus_file2ctx(struct file *file)
>>   	return container_of(file->private_data, struct cedrus_ctx, fh);
>>   }
>>   
>> -static bool cedrus_find_format(u32 pixelformat, u32 directions,
>> -			       unsigned int capabilities)
>> +static bool cedrus_check_format(u32 pixelformat, u32 directions,
>> +				unsigned int capabilities)
>>   {
>>   	struct cedrus_format *fmt;
>>   	unsigned int i;
>> @@ -76,12 +76,6 @@ static bool cedrus_find_format(u32 pixelformat, u32 directions,
>>   	return false;
>>   }
>>   
>> -static bool cedrus_check_format(u32 pixelformat, u32 directions,
>> -				unsigned int capabilities)
>> -{
>> -	return cedrus_find_format(pixelformat, directions, capabilities);
>> -}
>> -
>>   static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt)
>>   {
>>   	unsigned int width = pix_fmt->width;
>> -- 
>> 2.19.1
>>
> 

Re: [PATCH 2/2] staging: media: sunxi: Replace function cedrus_check_format()

[Paul Kocialkowski]

Hi,

On Fri 05 Jul 19, 17:43, Nishka Dasgupta wrote:
> On 05/07/19 3:56 PM, Paul Kocialkowski wrote:
> > Hi,
> > 
> > On Wed 03 Jul 19, 13:43, Nishka Dasgupta wrote:
> > > Remove function cedrus_check_format as all it does is call
> > > cedrus_find_format.
> > > Rename cedrus_find_format to cedrus_check_format to maintain
> > > compatibility with call sites.
> > > Issue found with Coccinelle.
> > 
> > Maybe we could have a !! or a bool cast to make coccinelle happy here?
> 
> Coccinelle didn't flag the type mismatch, just the single-line functions. I
> could add the bool cast then?

Looks like I failed to follow-up on this in due time, sorry.

Yes a bool cast would definitely be welcome :)

Cheers,

Paul

> Thanking you,
> Nishka
> 
> > Cheers,
> > 
> > Paul
> > 
> > > Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
> > > ---
> > >   drivers/staging/media/sunxi/cedrus/cedrus_video.c | 10 ++--------
> > >   1 file changed, 2 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > > index 0ec31b9e0aea..d5cc9ed04fd2 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> > > @@ -55,8 +55,8 @@ static inline struct cedrus_ctx *cedrus_file2ctx(struct file *file)
> > >   	return container_of(file->private_data, struct cedrus_ctx, fh);
> > >   }
> > > -static bool cedrus_find_format(u32 pixelformat, u32 directions,
> > > -			       unsigned int capabilities)
> > > +static bool cedrus_check_format(u32 pixelformat, u32 directions,
> > > +				unsigned int capabilities)
> > >   {
> > >   	struct cedrus_format *fmt;
> > >   	unsigned int i;
> > > @@ -76,12 +76,6 @@ static bool cedrus_find_format(u32 pixelformat, u32 directions,
> > >   	return false;
> > >   }
> > > -static bool cedrus_check_format(u32 pixelformat, u32 directions,
> > > -				unsigned int capabilities)
> > > -{
> > > -	return cedrus_find_format(pixelformat, directions, capabilities);
> > > -}
> > > -
> > >   static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt)
> > >   {
> > >   	unsigned int width = pix_fmt->width;
> > > -- 
> > > 2.19.1
> > > 
> > 
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH 1/2] staging: media: sunxi: Change return type of cedrus_find_format()

[Paul Kocialkowski]

Hi,

On Wed 03 Jul 19, 13:43, Nishka Dasgupta wrote:
> Change return type of cedrus_find_format to bool as it is only called
> once, by a function whose return value is bool, and the return value of
> cedrus_find_format is returned as-is at the call-site.
> Issue found with Coccinelle.

The purpose of this function (although definitely under-used at this point),
was to return the pointer to the element structure, not to indicate whether
the format format is part of the list or not.

In spite of that, this change reduces the use case for the function, so I do
not think it is beneficial, sorry.

Cheers,

Paul

> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus_video.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index 9673874ece10..0ec31b9e0aea 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -55,8 +55,8 @@ static inline struct cedrus_ctx *cedrus_file2ctx(struct file *file)
>  	return container_of(file->private_data, struct cedrus_ctx, fh);
>  }
>  
> -static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions,
> -						unsigned int capabilities)
> +static bool cedrus_find_format(u32 pixelformat, u32 directions,
> +			       unsigned int capabilities)
>  {
>  	struct cedrus_format *fmt;
>  	unsigned int i;
> @@ -70,13 +70,10 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions,
>  
>  		if (fmt->pixelformat == pixelformat &&
>  		    (fmt->directions & directions) != 0)
> -			break;
> +			return true;
>  	}
>  
> -	if (i == CEDRUS_FORMATS_COUNT)
> -		return NULL;
> -
> -	return &cedrus_formats[i];
> +	return false;
>  }
>  
>  static bool cedrus_check_format(u32 pixelformat, u32 directions,
> -- 
> 2.19.1
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH 1/2] staging: media: sunxi: Change return type of cedrus_find_format()

[Nishka Dasgupta]

On 05/07/19 3:56 PM, Paul Kocialkowski wrote:
> Hi,
> 
> On Wed 03 Jul 19, 13:43, Nishka Dasgupta wrote:
>> Change return type of cedrus_find_format to bool as it is only called
>> once, by a function whose return value is bool, and the return value of
>> cedrus_find_format is returned as-is at the call-site.
>> Issue found with Coccinelle.
> 
> The purpose of this function (although definitely under-used at this point),
> was to return the pointer to the element structure, not to indicate whether
> the format format is part of the list or not.
> 
> In spite of that, this change reduces the use case for the function, so I do
> not think it is beneficial, sorry.

Okay, thank you for the clarification.

Nishka

> Cheers,
> 
> Paul
> 
>> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
>> ---
>>   drivers/staging/media/sunxi/cedrus/cedrus_video.c | 11 ++++-------
>>   1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
>> index 9673874ece10..0ec31b9e0aea 100644
>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
>> @@ -55,8 +55,8 @@ static inline struct cedrus_ctx *cedrus_file2ctx(struct file *file)
>>   	return container_of(file->private_data, struct cedrus_ctx, fh);
>>   }
>>   
>> -static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions,
>> -						unsigned int capabilities)
>> +static bool cedrus_find_format(u32 pixelformat, u32 directions,
>> +			       unsigned int capabilities)
>>   {
>>   	struct cedrus_format *fmt;
>>   	unsigned int i;
>> @@ -70,13 +70,10 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions,
>>   
>>   		if (fmt->pixelformat == pixelformat &&
>>   		    (fmt->directions & directions) != 0)
>> -			break;
>> +			return true;
>>   	}
>>   
>> -	if (i == CEDRUS_FORMATS_COUNT)
>> -		return NULL;
>> -
>> -	return &cedrus_formats[i];
>> +	return false;
>>   }
>>   
>>   static bool cedrus_check_format(u32 pixelformat, u32 directions,
>> -- 
>> 2.19.1
>>
>