Re: [bug report] media: i2c: Add driver for Aptina MT9V111

[jacopo mondi <jacopo@jmondi.org>]

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

Hi Dan,
    thanks for noticing this,

On Tue, Jul 31, 2018 at 09:35:54PM +0300, Dan Carpenter wrote:
> Hello Jacopo Mondi,
>
> The patch aab7ed1c3927: "media: i2c: Add driver for Aptina MT9V111"
> from Jul 25, 2018, leads to the following static checker warning:
>
> drivers/media/i2c/mt9v111.c:1163 mt9v111_probe() warn: passing zero to 'PTR_ERR'
> drivers/media/i2c/mt9v111.c:1173 mt9v111_probe() warn: passing zero to 'PTR_ERR'
> drivers/media/i2c/mt9v111.c:1184 mt9v111_probe() warn: passing zero to 'PTR_ERR'
> drivers/media/i2c/mt9v111.c:1194 mt9v111_probe() warn: passing zero to 'PTR_ERR'
>
> drivers/media/i2c/mt9v111.c
>   1155          v4l2_ctrl_handler_init(&mt9v111->ctrls, 5);
>   1156
>   1157          mt9v111->auto_awb = v4l2_ctrl_new_std(&mt9v111->ctrls,
>   1158                                                &mt9v111_ctrl_ops,
>   1159                                                V4L2_CID_AUTO_WHITE_BALANCE,
>   1160                                                0, 1, 1,
>   1161                                                V4L2_WHITE_BALANCE_AUTO);
>   1162          if (IS_ERR_OR_NULL(mt9v111->auto_awb)) {
>   1163                  ret = PTR_ERR(mt9v111->auto_awb);
>
> This just returns success because v4l2_ctrl_new_std() only return NULL

Correct, sorry, I didn't notice that.

> on error, it never returns error pointers.  I guess we should set ret to
> EINVAL?
>
> 		if (!mt9v111->auto_awb) {
> 			ret = -EINVAL;
> 			goto error_free_ctrls;
> 		}
>

We can do even better than that.
The v4l2 control handler retains errors in a flag I can inspect after
having added/created all controls here and here below.

I can return that error flag if something goes wrong.

Thanks
   j

>   1164                  goto error_free_ctrls;
>   1165          }
>   1166
>   1167          mt9v111->auto_exp = v4l2_ctrl_new_std_menu(&mt9v111->ctrls,
>   1168                                                     &mt9v111_ctrl_ops,
>   1169                                                     V4L2_CID_EXPOSURE_AUTO,
>   1170                                                     V4L2_EXPOSURE_MANUAL,
>   1171                                                     0, V4L2_EXPOSURE_AUTO);
>   1172          if (IS_ERR_OR_NULL(mt9v111->auto_exp)) {
>   1173                  ret = PTR_ERR(mt9v111->auto_exp);
>   1174                  goto error_free_ctrls;
>   1175          }
>   1176
>   1177          /* Initialize timings */
>   1178          mt9v111->hblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops,
>   1179                                              V4L2_CID_HBLANK,
>   1180                                              MT9V111_CORE_R05_MIN_HBLANK,
>   1181                                              MT9V111_CORE_R05_MAX_HBLANK, 1,
>   1182                                              MT9V111_CORE_R05_DEF_HBLANK);
>   1183          if (IS_ERR_OR_NULL(mt9v111->hblank)) {
>   1184                  ret = PTR_ERR(mt9v111->hblank);
>   1185                  goto error_free_ctrls;
>   1186          }
>   1187
>   1188          mt9v111->vblank = v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops,
>   1189                                              V4L2_CID_VBLANK,
>   1190                                              MT9V111_CORE_R06_MIN_VBLANK,
>   1191                                              MT9V111_CORE_R06_MAX_VBLANK, 1,
>   1192                                              MT9V111_CORE_R06_DEF_VBLANK);
>   1193          if (IS_ERR_OR_NULL(mt9v111->vblank)) {
>   1194                  ret = PTR_ERR(mt9v111->vblank);
>   1195                  goto error_free_ctrls;
>   1196          }
>   1197
>   1198          /* PIXEL_RATE is fixed: just expose it to user space. */
>   1199          v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops,
>
> regards,
> dan carpenter

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