diff mbox series

[v7,2/5] media: rkisp1: stats: remove a wrong cast to u8

Message ID 20210120164446.1220-3-dafna.hirschfeld@collabora.com (mailing list archive)
State New
Headers show
Series Fix the rkisp1 userspace API for later IP versions | expand

Commit Message

Dafna Hirschfeld Jan. 20, 2021, 4:44 p.m. UTC
hist_bins is an array of type __u32. Each entry represent
a 20 bit fixed point value as documented inline.
The cast to u8 when setting the values is wrong. Remove it.

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

Comments

Heiko Stuebner Jan. 21, 2021, 9:31 a.m. UTC | #1
Am Mittwoch, 20. Januar 2021, 17:44:43 CET schrieb Dafna Hirschfeld:
> hist_bins is an array of type __u32. Each entry represent
> a 20 bit fixed point value as documented inline.
> The cast to u8 when setting the values is wrong. Remove it.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> ---
>  drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> index 3ddab8fa8f2d..4cdb180fa64d 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> @@ -235,8 +235,7 @@ static void rkisp1_stats_get_hst_meas(struct rkisp1_stats *stats,
>  	pbuf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
>  	for (i = 0; i < RKISP1_CIF_ISP_HIST_BIN_N_MAX; i++)
>  		pbuf->params.hist.hist_bins[i] =
> -			(u8)rkisp1_read(rkisp1,
> -					RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
> +			rkisp1_read(rkisp1, RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
>  }
>  
>  static void rkisp1_stats_get_bls_meas(struct rkisp1_stats *stats,
>
Tomasz Figa Jan. 21, 2021, 10:07 a.m. UTC | #2
Hi Dafna,

On Thu, Jan 21, 2021 at 1:45 AM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
> hist_bins is an array of type __u32. Each entry represent
> a 20 bit fixed point value as documented inline.
> The cast to u8 when setting the values is wrong. Remove it.

Thanks for the patch. See my comment inline.

>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> index 3ddab8fa8f2d..4cdb180fa64d 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> @@ -235,8 +235,7 @@ static void rkisp1_stats_get_hst_meas(struct rkisp1_stats *stats,
>         pbuf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
>         for (i = 0; i < RKISP1_CIF_ISP_HIST_BIN_N_MAX; i++)
>                 pbuf->params.hist.hist_bins[i] =
> -                       (u8)rkisp1_read(rkisp1,
> -                                       RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
> +                       rkisp1_read(rkisp1, RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);

Is the register guaranteed to return 0 for the upper unused 12 bits?
Should we mask them instead?

Best regards,
Tomasz
Dafna Hirschfeld Jan. 21, 2021, 11:47 a.m. UTC | #3
Am 21.01.21 um 11:07 schrieb Tomasz Figa:
> Hi Dafna,
> 
> On Thu, Jan 21, 2021 at 1:45 AM Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> wrote:
>>
>> hist_bins is an array of type __u32. Each entry represent
>> a 20 bit fixed point value as documented inline.
>> The cast to u8 when setting the values is wrong. Remove it.
> 
> Thanks for the patch. See my comment inline.
> 
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
>> index 3ddab8fa8f2d..4cdb180fa64d 100644
>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
>> @@ -235,8 +235,7 @@ static void rkisp1_stats_get_hst_meas(struct rkisp1_stats *stats,
>>          pbuf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
>>          for (i = 0; i < RKISP1_CIF_ISP_HIST_BIN_N_MAX; i++)
>>                  pbuf->params.hist.hist_bins[i] =
>> -                       (u8)rkisp1_read(rkisp1,
>> -                                       RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
>> +                       rkisp1_read(rkisp1, RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
> 
> Is the register guaranteed to return 0 for the upper unused 12 bits?
> Should we mask them instead?

I printed the values and I always see 0 for the last 12 bits. But the datasheet
does not say that explicitly.
I can add a mask to be on the safe side.

Thanks,
Dafna

> 
> Best regards,
> Tomasz
>
diff mbox series

Patch

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
index 3ddab8fa8f2d..4cdb180fa64d 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
@@ -235,8 +235,7 @@  static void rkisp1_stats_get_hst_meas(struct rkisp1_stats *stats,
 	pbuf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
 	for (i = 0; i < RKISP1_CIF_ISP_HIST_BIN_N_MAX; i++)
 		pbuf->params.hist.hist_bins[i] =
-			(u8)rkisp1_read(rkisp1,
-					RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
+			rkisp1_read(rkisp1, RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
 }
 
 static void rkisp1_stats_get_bls_meas(struct rkisp1_stats *stats,