Re: [GIT PULL FOR 6.8] Some fixes and new drivers, too

From: Sakari Ailus <sakari.ailus@iki.fi>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: linux-media@vger.kernel.org
Subject: Re: [GIT PULL FOR 6.8] Some fixes and new drivers, too
Date: Tue, 12 Dec 2023 11:05:48 +0000	[thread overview]
Message-ID: <ZXg-jP23tEaj_RJk@valkosipuli.retiisi.eu> (raw)
In-Reply-To: <03b41071-e477-44c0-83f8-c90ce545c6de@xs4all.nl>

Hi Hans,

On Tue, Dec 12, 2023 at 10:09:31AM +0100, Hans Verkuil wrote:
> On 12/12/2023 09:16, Sakari Ailus wrote:
> > Hi Hans, Mauro,
> > 
> > Here's a nice set of some fixes as well as new drivers that some of which,
> > in turn, depend on the fixes. In particular, there are a few sensors
> > (imx290 in particular) that have little endian registers, either just some
> > or all, and this was missed in V4L2 CCI conversion earlier on. Another
> > driver also needs these CCI_*LE() macros later in the set.
> > 
> > There are new drivers for Galaxycore gc0308, Omnivision ov64a30 camera
> > sensor as well as Avnet Alvium camera modules.
> > 
> > Miscellaneous fixes exist for imx335, imx214, imx415, ov2740 as well as the
> > IPU bridge.
> > 
> > Documentation improvements for camera sensors are included, too.
> > 
> > Please pull.
> 
> Running the build script gives me 3 sparse warnings:
> 
> drivers/media/i2c/imx415.c:402:30: warning: decimal constant 2376000000 is between LONG_MAX and ULONG_MAX. For C99 that means long long, C90 compilers are very likely to produce unsigned long (and a
> warning) here
> drivers/media/i2c/imx415.c:417:30: warning: decimal constant 2376000000 is between LONG_MAX and ULONG_MAX. For C99 that means long long, C90 compilers are very likely to produce unsigned long (and a
> warning) here
> drivers/media/i2c/imx415.c:432:30: warning: decimal constant 2376000000 is between LONG_MAX and ULONG_MAX. For C99 that means long long, C90 compilers are very likely to produce unsigned long (and a
> warning) here
> 
> (2376000000 should be 2376000000ULL)

But the C standard used here is C99, not C90. So there doesn't seem to be
an actual problem here.

Although I agree with you in explicitly specifying the type.

> 
> and one smatch warning:
> 
> drivers/media/i2c/alvium-csi2.c:2429 alvium_probe() warn: passing zero to 'dev_err_probe'

There seems to be an issue not catched during the review. The ret
assignment is one line too late, so dev_err_probe() still gets the wrong
value. This indeed should be fixed.

> 
> checkpatch.pl --strict also complains about a few things:
> 
> 0001-media-atmel-isi-Fix-crash-due-to-missing-subdev-in-s.patch:
> 
> ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit bc0e8d91feec ("media: v4l: subdev: Switch to stream-aware state functions")'
> #8:
> As a result of converting to the stream-aware state functions, commit

This is a warning from checkpatch.pl which is apparently produced by commit
being on the previous line. I.e. a false positive.

> 
> 0009-media-i2c-Add-support-for-alvium-camera.patch
> 
> A bunch of:
> 
> CHECK: Assignment operator '=' should be on the previous line
> #1120: FILE: drivers/media/i2c/alvium-csi2.c:1027:
> +       alvium->is_mipi_fmt_avail[ALVIUM_BIT_YUV420_8_LEG]
> +                                 = avail_fmt->yuv420_8_leg;
> 
> and:
> 
> CHECK: Unnecessary parentheses around '!alvium_csi2_fmts[fmt].is_raw'
> #1174: FILE: drivers/media/i2c/alvium-csi2.c:1081:
> +               if ((!alvium_csi2_fmts[fmt].is_raw) ||
> +                   (alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit]))
> 
> CHECK: Unnecessary parentheses around '!alvium_csi2_fmts[fmt].is_raw'
> #1191: FILE: drivers/media/i2c/alvium-csi2.c:1098:
> +               if ((!alvium_csi2_fmts[fmt].is_raw) ||
> +                   (alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit])) {
> 
> WARNING: Improper SPDX comment style for 'drivers/media/i2c/alvium-csi2.h', please use '/*' instead
> #2630: FILE: drivers/media/i2c/alvium-csi2.h:1:
> +// SPDX-License-Identifier: GPL-2.0
> 
> WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> #2630: FILE: drivers/media/i2c/alvium-csi2.h:1:
> +// SPDX-License-Identifier: GPL-2.0
> 
> 0037-media-dt-bindings-Add-OmniVision-OV64A40.patch:
> 
> WARNING: Co-developed-by and Signed-off-by: name/email do not match
> #11:
> Co-developed-by: Lee Jackson <lee.jackson@arducam.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> 0038-media-i2c-Add-driver-for-OmniVision-OV64A40.patch:
> 
> WARNING: Co-developed-by and Signed-off-by: name/email do not match
> #11:
> Co-developed-by: Lee Jackson <lee.jackson@arducam.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> 0039-media-i2c-imx415-Convert-to-new-CCI-register-access-.patch:

I'll ask Jacopo to fix these.

But overall, apart from the missing SoB lines (I missed this while looking
at the long list of warnings the patches produce), is there something that
prevents merging this? I agree there are bugs, but this all is still found
at the time the PR has been sent which makes remedying them very difficult.

> 
> CHECK: Alignment should match open parenthesis
> #461: FILE: drivers/media/i2c/imx415.c:462:
> +               cci_write(sensor->regmap, IMX415_BLKLEVEL,
> +                                  IMX415_BLKLEVEL_DEFAULT, &ret);

-- 
Regards,

Sakari Ailus