linux-kernel - Re: [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VciCJuoOwC8ozanWYqSCM=vWpiaqymJ2-gQfrSt5Ts6fQ@mail.gmail.com>
Date: Tue, 27 Feb 2024 14:46:54 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Zhi Mao <zhi.mao@...iatek.com>
Cc: mchehab@...nel.org, robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, 
	sakari.ailus@...ux.intel.com, laurent.pinchart@...asonboard.com, 
	shengnan.wang@...iatek.com, yaya.chang@...iatek.com, 10572168@...com, 
	Project_Global_Chrome_Upstream_Group@...iatek.com, yunkec@...omium.org, 
	conor+dt@...nel.org, matthias.bgg@...il.com, 
	angelogioacchino.delregno@...labora.com, jacopo.mondi@...asonboard.com, 
	hverkuil-cisco@...all.nl, heiko@...ech.de, jernej.skrabec@...il.com, 
	macromorgan@...mail.com, linus.walleij@...aro.org, hdegoede@...hat.com, 
	tomi.valkeinen@...asonboard.com, gerald.loacker@...fvision.net, 
	bingbu.cao@...el.com, dan.scally@...asonboard.com, 
	linux-media@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v6 2/2] media: i2c: Add GC08A3 image sensor driver

On Tue, Feb 27, 2024 at 3:33 AM Zhi Mao <zhi.mao@...iatek.com> wrote:
>
> Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor.

..

> +/*
> + * gc08a3.c - gc08a3 sensor driver

Drop the filename from the file, it's impractical (esp. if the file
will be renamed for some reason in the future).

> + *
> + * Copyright 2023 MediaTek
> + *
> + * Zhi Mao <zhi.mao@...iatek.com>
> + */

..

> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>

This is a semi-random list of inclusions. Please, follow the IWYU
(Include What You Use) principle. *At least* this list misses the
following:  array_size.h, bits.h, container_of.h, device.h, err.h,
i2c.h, math64.h, module.h, mod_devicetable.h, property.h, types.h.

..

> +       /*update crop info to subdev state*/

Missing spaces.

..

> +       /*update fmt info to subdev state*/

Ditto.

..

> +static int gc08a3_test_pattern(struct gc08a3 *gc08a3, u32 pattern_menu)
> +{
> +       u32 pattern = 0;

No, please use the default case for this assignment.

> +       int ret;
> +
> +       if (pattern_menu) {
> +               switch (pattern_menu) {
> +               case 1:
> +                       pattern = 0x00;
> +                       break;
> +               case 2:
> +                       pattern = 0x10;
> +                       break;
> +               case 3:
> +               case 4:
> +               case 5:
> +               case 6:
> +               case 7:
> +                       pattern = pattern_menu + 1;
> +                       break;
> +               }
> +
> +               ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_IDX,
> +                               pattern, NULL);
> +               if (ret)
> +                       return ret;
> +

> +               ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN,
> +                               GC08A3_TEST_PATTERN_EN, NULL);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               ret = cci_write(gc08a3->regmap, GC08A3_REG_TEST_PATTERN_EN,
> +                               0x00, NULL);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;

'else' is redundant, but you can deduplicate code above with

  return cci_write(...);
} else {
  return cci_write(...);
}

Of course you can go even further, but I think with the above it will
be balanced to the way that it's easy to understand how branches
behave ('else' in this case helps to indent semantically coupled
lines).

> +}


> +       /*
> +        * Applying V4L2 control value only happens
> +        * when power is on for streaming
> +        */

Respect English grammar and punctuation, i.e. don't forget periods at
the end of sentences in multi-line comments.

..

> +       endpoint =
> +               fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> +                                               FWNODE_GRAPH_ENDPOINT_NEXT);
> +       if (!endpoint) {

Strictly speaking dev_fwnode(dev) might be NULL or an error pointer. I
dunno how the graph is implemented there and if it's possible to get
an error pointer out of it. At least this probably needs to be aligned
there at some point.

> +               dev_err(dev, "endpoint node not found\n");
> +               return -EINVAL;
> +       }

..

> +static const struct dev_pm_ops gc08a3_pm_ops = {
> +       RUNTIME_PM_OPS(gc08a3_power_off, gc08a3_power_on, NULL)
> +};

There is a DEFINE_* PM macro, use it.

..

> +

Redundant blank line.

> +module_i2c_driver(gc08a3_i2c_driver);

-- 
With Best Regards,
Andy Shevchenko