[1/2] media: staging: rkisp1 stats: set a measure flag with '|=' instead of '='
diff mbox series

Message ID 20200509152904.26348-1-dafna.hirschfeld@collabora.com
State New
Headers show
Series
  • [1/2] media: staging: rkisp1 stats: set a measure flag with '|=' instead of '='
Related show

Commit Message

Dafna Hirschfeld May 9, 2020, 3:29 p.m. UTC
The flag RKISP1_CIF_ISP_STAT_AFM_FIN that indicates a type of
statistics is mistakenly set with '=' instead of '|='
this might set off previous flags.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-stats.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Helen Koike May 19, 2020, 4:39 p.m. UTC | #1
Hi Dafna,

Thanks for the patch.

On 5/9/20 12:29 PM, Dafna Hirschfeld wrote:
> The flag RKISP1_CIF_ISP_STAT_AFM_FIN that indicates a type of
> statistics is mistakenly set with '=' instead of '|='
> this might set off previous flags.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-stats.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 6dfcbdc3deb8..8351bda0be03 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -253,7 +253,7 @@ static void rkisp1_stats_get_afc_meas(struct rkisp1_stats *stats,
>  	struct rkisp1_device *rkisp1 = stats->rkisp1;
>  	struct rkisp1_cif_isp_af_stat *af;
>  
> -	pbuf->meas_type = RKISP1_CIF_ISP_STAT_AFM_FIN;
> +	pbuf->meas_type |= RKISP1_CIF_ISP_STAT_AFM_FIN;

It seems there is a redundancy, the place that calls this function already sets this bit.
It seems there is a similar issue in all rkisp1_stats_get_* functions.

Could you please check and do the proper cleanup?

Thanks
Helen

>  
>  	af = &pbuf->params.af;
>  	af->window[0].sum = rkisp1_read(rkisp1, RKISP1_CIF_ISP_AFM_SUM_A);
>
Helen Koike May 19, 2020, 4:41 p.m. UTC | #2
On 5/19/20 1:39 PM, Helen Koike wrote:
> Hi Dafna,
> 
> Thanks for the patch.
> 
> On 5/9/20 12:29 PM, Dafna Hirschfeld wrote:
>> The flag RKISP1_CIF_ISP_STAT_AFM_FIN that indicates a type of
>> statistics is mistakenly set with '=' instead of '|='
>> this might set off previous flags.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>  drivers/staging/media/rkisp1/rkisp1-stats.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
>> index 6dfcbdc3deb8..8351bda0be03 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
>> @@ -253,7 +253,7 @@ static void rkisp1_stats_get_afc_meas(struct rkisp1_stats *stats,
>>  	struct rkisp1_device *rkisp1 = stats->rkisp1;
>>  	struct rkisp1_cif_isp_af_stat *af;
>>  
>> -	pbuf->meas_type = RKISP1_CIF_ISP_STAT_AFM_FIN;
>> +	pbuf->meas_type |= RKISP1_CIF_ISP_STAT_AFM_FIN;
> 
> It seems there is a redundancy, the place that calls this function already sets this bit.
> It seems there is a similar issue in all rkisp1_stats_get_* functions.
> 
> Could you please check and do the proper cleanup?

Please ignore my comment, for some reason I didn't see the second patch.

Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> 
> Thanks
> Helen
> 
>>  
>>  	af = &pbuf->params.af;
>>  	af->window[0].sum = rkisp1_read(rkisp1, RKISP1_CIF_ISP_AFM_SUM_A);
>>
Laurent Pinchart May 20, 2020, 12:15 a.m. UTC | #3
Hi Dafna,

Thank you for the patch.

On Sat, May 09, 2020 at 05:29:03PM +0200, Dafna Hirschfeld wrote:
> The flag RKISP1_CIF_ISP_STAT_AFM_FIN that indicates a type of
> statistics is mistakenly set with '=' instead of '|='
> this might set off previous flags.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/staging/media/rkisp1/rkisp1-stats.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 6dfcbdc3deb8..8351bda0be03 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -253,7 +253,7 @@ static void rkisp1_stats_get_afc_meas(struct rkisp1_stats *stats,
>  	struct rkisp1_device *rkisp1 = stats->rkisp1;
>  	struct rkisp1_cif_isp_af_stat *af;
>  
> -	pbuf->meas_type = RKISP1_CIF_ISP_STAT_AFM_FIN;
> +	pbuf->meas_type |= RKISP1_CIF_ISP_STAT_AFM_FIN;
>  
>  	af = &pbuf->params.af;
>  	af->window[0].sum = rkisp1_read(rkisp1, RKISP1_CIF_ISP_AFM_SUM_A);

Patch
diff mbox series

diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 6dfcbdc3deb8..8351bda0be03 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -253,7 +253,7 @@  static void rkisp1_stats_get_afc_meas(struct rkisp1_stats *stats,
 	struct rkisp1_device *rkisp1 = stats->rkisp1;
 	struct rkisp1_cif_isp_af_stat *af;
 
-	pbuf->meas_type = RKISP1_CIF_ISP_STAT_AFM_FIN;
+	pbuf->meas_type |= RKISP1_CIF_ISP_STAT_AFM_FIN;
 
 	af = &pbuf->params.af;
 	af->window[0].sum = rkisp1_read(rkisp1, RKISP1_CIF_ISP_AFM_SUM_A);