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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Feb 27, 2024 at 3:33 AM Zhi Mao <zhi.mao@mediatek.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@mediatek.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