Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Cao, Bingbu" <bingbu.cao@intel.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "Qiu, Tian Shu" <tian.shu.qiu@intel.com>,
        "Tu, ShawnX" <shawnx.tu@intel.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>
Subject: RE: [PATCH v1 2/8] media: ov2740: Replace voodoo coding with
 understandle flow
Thread-Topic: [PATCH v1 2/8] media: ov2740: Replace voodoo coding with
 understandle flow
Thread-Index: AQHYoOgS6EJ7U7sB80m4WsxYIgWSA62R+J8w
Date:   Wed, 27 Jul 2022 09:52:08 +0000
Message-ID: <DM8PR11MB5653E47793F38ADA5174BFF399979@DM8PR11MB5653.namprd11.prod.outlook.com>
References: <20220726120556.2881-1-andriy.shevchenko@linux.intel.com>
 <20220726120556.2881-2-andriy.shevchenko@linux.intel.com>
In-Reply-To: <20220726120556.2881-2-andriy.shevchenko@linux.intel.com>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.6.500.17
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?2HwyQVmWCGDCNj4/KReCfRlAHlVqt/qDS+IZkwSKX4PJd2xxN6BDftjZU56d?=
 =?us-ascii?Q?cEcvngzYz7N+Az7i/FFp4i3faEnhLs640EHXQXEOQasf/Ye2xDa2dDDK+fD6?=
 =?us-ascii?Q?1ZUtjxFGpOuogVFkor8OeG5v+uCWgGUl3kQClMG59DwvnBjz1bk0Ir/ekDUk?=
 =?us-ascii?Q?5gnMBURwVRsxM31EPljTSZcKvoTm3V3e/ahTZ6qHF/cHSL5IJdGjLi4nkUZ2?=
 =?us-ascii?Q?GaTm9/YsAXKDktpK5Xg4EHM8bEU+bySuDwhloJDaaTO/6w2y0qu1ehzTkfKV?=
 =?us-ascii?Q?nCket/lvMvV8fCAk1hZ0jx6+DwBNP8GiYF21/Rl5xHDNyCBj6E/IIe0vWAOL?=
 =?us-ascii?Q?usSlq96FATtY4x2qAY/7+WHx0h7DRXv93WvPwlqvTVT5e1fFEb0Es+57qLhB?=
 =?us-ascii?Q?luqWT6F4dTPHEy8tTg5ZONgdJuGzPjd2VAeP0QvE44/avq9O0eFEjgxtHQr9?=
 =?us-ascii?Q?Yo2Xr0W8gTHtjPg8Jkp+cdg0A8AmIXp23VgWzyBD1LL1EJi6wLLTz93C1Yqc?=
 =?us-ascii?Q?pFfohW/dHYNygYwkcGuUv8tRPtnWMV5MPqOw3XTuOTWTzB3bdSzsGxZa1JMX?=
 =?us-ascii?Q?bqfkcTNu0S0BG6Skz28rqNDib4JLpF1OeUUMJ2XF7y16BQa9mPJmYd0mdSPH?=
 =?us-ascii?Q?Tve9wtVTzmR6COyhKDTHuVsn+O0YuuBFsUh9gnRntbNrBgISuckGNS8AMA3r?=
 =?us-ascii?Q?4Oe6DoPaQB6e2ONXEutezlVKNe9w7T5/g4D2kjLUxziWxyLNFt9YtSvvj1R5?=
 =?us-ascii?Q?s9D136bpN8xK1UH9vRtD7tW/fdjWWbLmzuh39rzmfgdwaKQ+8lOYKNJwKEbG?=
 =?us-ascii?Q?5hz0fPCJZrDqTuUy3LNwNs2PTROTiBjUnN7g1U0sCHLJ590juioiozcL/8aE?=
 =?us-ascii?Q?uxNTjHawdVxuW1HhIaZrnVwlS6/DAcj1i+AzazgRTQyPtG848M/c0hFZJ/ov?=
 =?us-ascii?Q?Kcjp1/Ouy0mgQ6yQJiOd1uJoUS74UpaD8z/cdHVoTTTf0HwqRkN/wwR5pIgo?=
 =?us-ascii?Q?U7erc3rKPqMasK/GoZ9C+qrQaAK1eiBAQG0GaUwSnFJByuYBOdXqmwLZ/kKa?=
 =?us-ascii?Q?NtVOtVRWMzX3FBkSj/i5fcfUY7+9vO+Q1M9dMMLULY1vn+T1RUa6BR2RDyxX?=
 =?us-ascii?Q?Xc6v4iySr1njGd0sCGcSVNQNQs9QrjJ0cV7MgJlCCQxvMNMqLh5+U7vqKztk?=
 =?us-ascii?Q?xh8iodwmEXQq97Z/KJ6PMg1650PPm0+hKxa0TwF1Eg2CkyURfu3KkXvi29Ne?=
 =?us-ascii?Q?rpV9aUYZdYiRR4P8s8B//IdoRZyHGSf8USRpMBda5bDdqnYHu/VlwW7p5zx/?=
 =?us-ascii?Q?3duHAIjIBfjyLwCbQ/XjeRV8fmxzn5O3BMY7TekKLSdySi0D1SbcgtNmU8SH?=
 =?us-ascii?Q?tAlRVTRmoLrI02exyp1f+/O7pVU7OkywCA9+izB8JDVMhi44WcD754dvQmi4?=
 =?us-ascii?Q?ZNpkupB/hY6XZDuBT30LTm9C4f4NcWGg3KC7VKSQ6JyYyTJMFkEhxX/eQ1FJ?=
 =?us-ascii?Q?OH1N5aBXfNb2B+N9n3j143rh9fArSXurvXjC4WyycxMpaq3CbUGn6uZhnFOr?=
 =?us-ascii?Q?6Ovg7FXFfa6gEP/5l80m08FQtWLWzLs0WiF56CoK?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM8PR11MB5653.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ebfc38ce-c6d1-487a-026e-08da6fb5abdd
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Jul 2022 09:52:08.9029
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 6I0fva6YCiN/ohDkoBUBdTMR6rzCUR2aVDcvkFtq6D0NWZ9YsjQyVIordIt427S3Aca36COVEUkKDnMoTCn8uA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR11MB3315
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Andy,=20

Thanks for your patch.

Although I am not familiar with the voodoo programming, it looks
good for me.

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

________________________
BRs, =20
Bingbu Cao=20

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Tuesday, July 26, 2022 8:06 PM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Qiu, Tian Shu <tian.shu.qiu@intel.com>; Tu, ShawnX
> <shawnx.tu@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>; Mauro Carvalho
> Chehab <mchehab@kernel.org>
> Subject: [PATCH v1 2/8] media: ov2740: Replace voodoo coding with
> understandle flow
>=20
> Besides not being understandable at the first glance, the code might
> provoke a compiler or a static analyser tool to warn about out-of-bound
> access (when len =3D=3D 0).
>=20
> Replace it with clear flow an understandable intention.
>=20
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/media/i2c/ov2740.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>=20
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c inde=
x
> c975db1bbe8c..81c0ab220339 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -377,10 +377,10 @@ static int ov2740_read_reg(struct ov2740 *ov2740,
> u16 reg, u16 len, u32 *val)
>  	struct i2c_client *client =3D v4l2_get_subdevdata(&ov2740->sd);
>  	struct i2c_msg msgs[2];
>  	u8 addr_buf[2];
> -	u8 data_buf[4] =3D {0};
> +	u8 data_buf[4];
>  	int ret =3D 0;
>=20
> -	if (len > sizeof(data_buf))
> +	if (len > 4)
>  		return -EINVAL;
>=20
>  	put_unaligned_be16(reg, addr_buf);
> @@ -391,13 +391,22 @@ static int ov2740_read_reg(struct ov2740 *ov2740,
> u16 reg, u16 len, u32 *val)
>  	msgs[1].addr =3D client->addr;
>  	msgs[1].flags =3D I2C_M_RD;
>  	msgs[1].len =3D len;
> -	msgs[1].buf =3D &data_buf[sizeof(data_buf) - len];
> +	msgs[1].buf =3D data_buf;
>=20
>  	ret =3D i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>  	if (ret !=3D ARRAY_SIZE(msgs))
>  		return ret < 0 ? ret : -EIO;
>=20
> -	*val =3D get_unaligned_be32(data_buf);
> +	if (len =3D=3D 4)
> +		*val =3D get_unaligned_be32(data_buf);
> +	else if (len =3D=3D 3)
> +		*val =3D get_unaligned_be24(data_buf);
> +	else if (len =3D=3D 2)
> +		*val =3D get_unaligned_be16(data_buf);
> +	else if (len =3D=3D 1)
> +		*val =3D data_buf[0];
> +	else
> +		return -EINVAL;
>=20
>  	return 0;
>  }
> @@ -412,7 +421,16 @@ static int ov2740_write_reg(struct ov2740 *ov2740,
> u16 reg, u16 len, u32 val)
>  		return -EINVAL;
>=20
>  	put_unaligned_be16(reg, buf);
> -	put_unaligned_be32(val << 8 * (4 - len), buf + 2);
> +	if (len =3D=3D 4)
> +		put_unaligned_be32(val, buf + 2);
> +	else if (len =3D=3D 3)
> +		put_unaligned_be24(val, buf + 2);
> +	else if (len =3D=3D 2)
> +		put_unaligned_be16(val, buf + 2);
> +	else if (len =3D=3D 1)
> +		buf[2] =3D val;
> +	else
> +		return -EINVAL;
>=20
>  	ret =3D i2c_master_send(client, buf, len + 2);
>  	if (ret !=3D len + 2)
> --
> 2.35.1