| Message ID | 20200402190419.15155-3-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging: rkisp1: cap: various fixes for capture formats | expand |
On 4/2/20 4:04 PM, Dafna Hirschfeld wrote: > The value RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP should be > set to the register instead of masking with ~BIT(1) > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> Acked-by: Helen Koike <helen.koike@collabora.com> > --- > drivers/staging/media/rkisp1/rkisp1-capture.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c > index 5700d7be2819..84a3cf565106 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c > @@ -469,8 +469,8 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap) > if (cap->pix.cfg->uv_swap) { > u32 reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL); > > - rkisp1_write(rkisp1, reg & ~BIT(1), > - RKISP1_CIF_MI_XTD_FORMAT_CTRL); > + reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP; > + rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL); > } > > rkisp1_mi_config_ctrl(cap); >
Hi Dafna, Thank you for the patch. On Thu, Apr 02, 2020 at 09:04:16PM +0200, Dafna Hirschfeld wrote: > The value RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP should be > set to the register instead of masking with ~BIT(1) > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > drivers/staging/media/rkisp1/rkisp1-capture.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c > index 5700d7be2819..84a3cf565106 100644 > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c > @@ -469,8 +469,8 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap) > if (cap->pix.cfg->uv_swap) { > u32 reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL); > > - rkisp1_write(rkisp1, reg & ~BIT(1), > - RKISP1_CIF_MI_XTD_FORMAT_CTRL); > + reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP; > + rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL); This indeed simplifies the code, but I think the logic is wrong in the first place. Shouldn't this be reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL); if (cap->pix.cfg->uv_swap) reg |= RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP; else reg &= ~RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP; rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL); ? Same for patch 1/5. > } > > rkisp1_mi_config_ctrl(cap);
On 4/5/20 8:10 PM, Laurent Pinchart wrote: > Hi Dafna, > > Thank you for the patch. > > On Thu, Apr 02, 2020 at 09:04:16PM +0200, Dafna Hirschfeld wrote: >> The value RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP should be >> set to the register instead of masking with ~BIT(1) >> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >> --- >> drivers/staging/media/rkisp1/rkisp1-capture.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c >> index 5700d7be2819..84a3cf565106 100644 >> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c >> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c >> @@ -469,8 +469,8 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap) >> if (cap->pix.cfg->uv_swap) { >> u32 reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL); >> >> - rkisp1_write(rkisp1, reg & ~BIT(1), >> - RKISP1_CIF_MI_XTD_FORMAT_CTRL); >> + reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP; >> + rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL); > > This indeed simplifies the code, but I think the logic is wrong in the > first place. Shouldn't this be > > reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL); > if (cap->pix.cfg->uv_swap) > reg |= RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP; > else > reg &= ~RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP; > rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL); > > ? Same for patch 1/5. Right, I'll send v3. Thanks, Dafna > >> } >> >> rkisp1_mi_config_ctrl(cap); >
diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c index 5700d7be2819..84a3cf565106 100644 --- a/drivers/staging/media/rkisp1/rkisp1-capture.c +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c @@ -469,8 +469,8 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap) if (cap->pix.cfg->uv_swap) { u32 reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL); - rkisp1_write(rkisp1, reg & ~BIT(1), - RKISP1_CIF_MI_XTD_FORMAT_CTRL); + reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP; + rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL); } rkisp1_mi_config_ctrl(cap);
The value RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP should be set to the register instead of masking with ~BIT(1) Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- drivers/staging/media/rkisp1/rkisp1-capture.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)