Re: [PATCH v4 02/14] media: ov772x: correct setting of banding filter

[jacopo mondi]

Hi Akinobu,
   thanks for the patch

On Mon, Apr 30, 2018 at 02:13:01AM +0900, Akinobu Mita wrote:
> The banding filter ON/OFF is controlled via bit 5 of COM8 register.  It
> is attempted to be enabled in ov772x_set_params() by the following line.
>
>   ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, 1);
>
> But this unexpectedly results disabling the banding filter, because the
> mask and set bits are exclusive.
>
> On the other hand, ov772x_s_ctrl() correctly sets the bit by:
>
>   ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, BNDF_ON_OFF);
>
> Cc: Jacopo Mondi 
> Cc: Laurent Pinchart 
> Cc: Hans Verkuil 
> Cc: Sakari Ailus 
> Cc: Mauro Carvalho Chehab 
> Signed-off-by: Akinobu Mita 

Acked-by: Jacopo Mondi 

One unrelated note: the fixes you have added in v4 are very welcome.
For another time, maybe you want to send incremental patches instead
of adding them to a series already in review, as increasing the series
size may slow down its inclusion due to review latencies.
V1 was 6 patches, v2 and v3 10, and this is one 14. This is fine, but
to speed up things, maybe send fixes like this one separately and
clearly state they have some dependency on an already sent series.
That said, I'm not collecting patches, so that's just how I see that,
maybe Sakari, who usually picks sensor driver contributions prefers the way
you sent this.

Thanks
   j

> ---
> * v4
> - New patch
>
>  drivers/media/i2c/ov772x.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index b62860c..e255070 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -1035,7 +1035,7 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>
>   /* Set COM8. */
>   if (priv->band_filter) {
> - ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, 1);
> + ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, BNDF_ON_OFF);
>   if (!ret)
>   ret = ov772x_mask_set(client, BDBASE,
> 0xff, 256 - priv->band_filter);
> --
> 2.7.4
>


signature.asc
Description: PGP signature

[PATCH v4 02/14] media: ov772x: correct setting of banding filter

[Akinobu Mita]

The banding filter ON/OFF is controlled via bit 5 of COM8 register.  It
is attempted to be enabled in ov772x_set_params() by the following line.

ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, 1);

But this unexpectedly results disabling the banding filter, because the
mask and set bits are exclusive.

On the other hand, ov772x_s_ctrl() correctly sets the bit by:

ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, BNDF_ON_OFF);

Cc: Jacopo Mondi 
Cc: Laurent Pinchart 
Cc: Hans Verkuil 
Cc: Sakari Ailus 
Cc: Mauro Carvalho Chehab 
Signed-off-by: Akinobu Mita 
---
* v4
- New patch

 drivers/media/i2c/ov772x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index b62860c..e255070 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1035,7 +1035,7 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 
/* Set COM8. */
if (priv->band_filter) {
-   ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, 1);
+   ret = ov772x_mask_set(client, COM8, BNDF_ON_OFF, BNDF_ON_OFF);
if (!ret)
ret = ov772x_mask_set(client, BDBASE,
  0xff, 256 - priv->band_filter);
-- 
2.7.4