On Thu, 2023-12-07 at 09:18 +0100, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > 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? > [mtk]: Sorry, I removed this "Reviewed-By:" messge. please review patch:v3 > > > > 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... > [mtk]: fixed in patch:v3 > > +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. > [mtk]:Here, we rename it to "reset-pin", according to sensor power off sequence in sepc, we should pull low this pin. > > ... > > > +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 > [mtk]: fixed in patch:v3 > > + > > +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 [mtk]: fixed in patch:v3 > > > + > > +ret = gc08a3_get_regulators(dev, gc08a3); > > +if (ret < 0) > > +return dev_err_probe(dev, ret, > > + "failed to get regulators\n"); > > Misaligned indentation > [mtk]: fixed in patch:v3 > > + > > +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. > [mtk]: fixed in patch:v3 > > Best regards, > Krzysztof >
