| Message ID | 20200509152904.26348-1-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] media: staging: rkisp1 stats: set a measure flag with '|=' instead of '=' | expand |
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); >
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); >>
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);
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);
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(-)