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