Re: [PATCH v5 2/2] media: i2c: add Himax HM1246 image sensor driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Nov 05, 2025 at 05:59:20PM +0100, Matthias Fend wrote:
> Am 04.11.2025 um 14:28 schrieb Andy Shevchenko:
> > On Tue, Nov 04, 2025 at 11:31:34AM +0100, Matthias Fend wrote:

...

> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/gpio.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/units.h>
> > 
> > This block is semi-random.
> > First of all, no new code must use gpio.h, use the proper one.
> > Second, many are missing, e.g., bits.h, regmap.h, types.h.
> > Please, follow IWYU principle (Include What You Use).
> 
> I've noticed that you've already modified the include statements in many
> source files. I assume you've automated this somehow. May I ask how you did
> that, or is there a reliable process for verifying the include statements?

No automation is available so far (the iwyu tool needs a lot of tuning for
Linux kernel). I did it by simply reading the code. Since you are the author
you should know how to fill the inclusions much better than me.

...

> > > +	const struct cci_reg_sequence pll_regs[] = {
> > 
> > static ?
> > 
> > > +		{ HM1246_PLL1CFG_REG, pll1 },
> > > +		{ HM1246_PLL2CFG_REG, pll2 },
> > > +		{ HM1246_PLL3CFG_REG, pll3 },
> > > +		{ HM1246_SBC_CTRL_REG, HM1246_SBC_CTRL_PLL_EN },
> > > +	};
> > 
> > I would even move it outside the function. Note, static const maybe located in
> > ro memory while w/o static it's just a guarantee that compiler doesn't change
> > the values. Hence there is no guarantee it will be in ro memory.
> 
> The sequence is initialized with values ​​from the arguments, which are not
> constant. Therefore, the sequence cannot be put into a `ro` section.

Ah, indeed, you are right. It can, but it will be an intermediate (unnecessary)
step.

...

> > > +static int hm1246_pll_check_locked(struct hm1246 *hm1246)
> > > +{
> > > +	u64 boot_ref2;
> > > +	int ret;
> > > +
> > > +	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2,
> > > +		       NULL);
> > 
> > Despite being longer 80 I still would put it on one line. It will increase readability.
> > 
> > 	ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);
> > 
> > Another option is to define local regmap:
> > 
> > 	struct regmap *map = hm1246->regmap;
> > 	...
> > 	ret = cci_read(map, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);
> > 
> > which will be most readable and satisfy 80 limit.
> 
> Doing things differently is kind of a dilemma.
> Compliance with the 80-line limit is required, and local variables that are
> only used once are also undesirable.

Why? If it increases readability than it _is_ desirable change.

> The unsightly line break seems to be the most acceptable option, right?
> Or rename the local variable to 'bref2'.

Making variable cryptic wouldn't help readability.

> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return (boot_ref2 & HM1246_SBC_BOOT_REF2_PLL_LOCK) ? 0 : -EIO;
> > 
> > Think about similar improvements elsewhere in this driver.
> > 
> > > +}

...

> > > +	/* PLL lock time: tpll typ. 100us */
> > 
> > It's not a variable name, use proper English.
> 
> I think you refer to "tpll typ."? I intentionally wanted to use the same
> symbols as in the datasheet. I also used them for the other delays:
> 
> /*
>  * XSHUTDOWN to crystal clock oscillation:  tcrystal typ.  650us
>  * Sample bootstrap pin:                    tsample  max. 2000us
>  * Built in self test:                      tbist    max. 3000us
>  */
> 
> Is this acceptable from this perspective?

This is unfortunate. When it goes with max/min is much more understandable than
standalone. Try to find a compromise. Datasheet is not a golden standard, it's
just good to have something close enough to it, but it doesn't mean we have to
follow it _literally_.

> > > +	fsleep(200);

...

> > > +	const u16 RGBMIN = 0x0, RGBMAX = 0x3ff;
> > 
> > 0 is enough (no need 0x).
> > 
> > So, the MAX is 10-bit, Can we use rather (BIT(10) - 1) to show this?
> 
> Sure, I can write it that way too, even though the hexadecimal number seems
> easier for me to read.

The hexadecimal sometimes too abstract and writing the same in a slightly
different form may give a useful additional information.

...

> > > +	if (!pm_runtime_get_if_active(hm1246->dev))
> > > +		return 0;
> > 
> > Use ACQUIRE() and return directly where it makes sense.
> 
> That would indeed be a great solution. However, I haven't found any guard
> definitions for pm_runtime_get_if_active and therefore don't know how to
> adapt this functionality (increase usage only if already enabled) to use
> ACQUIRE.
> Any clue on this?

Add a patch to add this conditional guard to pm_runtime.h.

...

> > > +static int hm1246_identify_module(struct hm1246 *hm1246)
> > 
> > This is a singleton function, right?
> > 
> > Check what once.h (or even once_lite.h) provides for you for such a case,
> > and drop unneeded "identified" variable.
> 
> As I understand it, the ONCE macros create a static variable in the driver
> module. This means the function would only be called once in total, but the
> function should be called once per device.
> Therefore, I don't think that's an option here.

Good point.

...

> > > +static int __maybe_unused hm1246_g_register(struct v4l2_subdev *sd,
> > > +					    struct v4l2_dbg_register *reg)
> > 
> > If v4l2.h doesn't provide a ptr_*() macro for these cases, I recommend to add and drop these __maybe_unused.
> 
> I'm not aware of anything like that.
> Currently, it's common practice to use '#ifdef CONFIG_VIDEO_ADV_DEBUG', and
> a macro that would simplify that would probably be worthwhile.
> But I think that's for something outside the scope of this patch.

This one may be out of scope, so up to you. But just note that using
__maybe_unused is discouraged.

-- 
With Best Regards,
Andy Shevchenko