On 07/12/2023 06:20, Zhi Mao wrote: > Add a V4L2 sub-device driver for Galaxycore GC08A3 image sensor. > > Reviewed-By: yunkec@xxxxxxxxxxxx I don't see review given here: https://lore.kernel.org/linux-media/20231123115104.32094-1-zhi.mao@xxxxxxxxxxxx/ This does not look like real review. Where was it performed? How thorough was it? How many review iterations did it include? Why there is no name but anonymous review? > Signed-off-by: Zhi Mao <zhi.mao@xxxxxxxxxxxx> > --- > drivers/media/i2c/Kconfig | 14 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/gc08a3.c | 1888 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 1903 insertions(+) > create mode 100644 drivers/media/i2c/gc08a3.c > ... > +static inline struct gc08a3 *to_gc08a3(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct gc08a3, sd); > +} > + > +static int gc08a3_power_on(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct gc08a3 *gc08a3 = to_gc08a3(sd); > + int ret; > + > + gpiod_set_value_cansleep(gc08a3->enable_gpio, 0); Why do you put make enable GPIO low? That's not how enable GPIO is supposed to work... > + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > + > + ret = regulator_bulk_enable(GC08A3_NUM_SUPPLIES, gc08a3->supplies); > + if (ret < 0) { > + dev_err(gc08a3->dev, "failed to enable regulators: %d\n", ret); > + return ret; > + } > + > + ret = clk_prepare_enable(gc08a3->xclk); > + if (ret < 0) { > + regulator_bulk_disable(GC08A3_NUM_SUPPLIES, gc08a3->supplies); > + dev_err(gc08a3->dev, "clk prepare enable failed\n"); > + return ret; > + } > + > + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > + > + gpiod_set_value_cansleep(gc08a3->enable_gpio, 1); > + usleep_range(GC08A3_MIN_SLEEP_US, GC08A3_MAX_SLEEP_US); > + > + return 0; > +} > + > +static int gc08a3_power_off(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct gc08a3 *gc08a3 = to_gc08a3(sd); > + > + gpiod_set_value_cansleep(gc08a3->enable_gpio, 0); How making enable GPIO low is related to power off? Enable means you turn on some feature, not shutdown. Look at common GPIO consumer bindings in the kernel. ... > +static int gc08a3_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct gc08a3 *gc08a3; > + int ret; > + > + ret = gc08a3_parse_fwnode(dev); > + if (ret) > + return ret; > + > + gc08a3 = devm_kzalloc(dev, sizeof(*gc08a3), GFP_KERNEL); > + if (!gc08a3) > + return -ENOMEM; > + > + gc08a3->dev = dev; > + > + gc08a3->xclk = devm_clk_get(dev, NULL); > + if (IS_ERR(gc08a3->xclk)) > + return dev_err_probe(dev, PTR_ERR(gc08a3->xclk), > + "failed to get xclk\n"); Misaligned indentation > + > + ret = clk_set_rate(gc08a3->xclk, GC08A3_DEFAULT_CLK_FREQ); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to set xclk frequency\n"); Misaligned indentation > + > + ret = gc08a3_get_regulators(dev, gc08a3); > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "failed to get regulators\n"); Misaligned indentation > + > + gc08a3->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); > + if (IS_ERR(gc08a3->enable_gpio)) > + return dev_err_probe(dev, PTR_ERR(gc08a3->enable_gpio), > + "failed to get gpio\n"); Misaligned indentation... probably entire code is misaligned. Best regards, Krzysztof
