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

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]

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 <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Acked-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]