Hi Tomasz, On Thu, Sep 03, 2020 at 12:59:20AM +0200, Tomasz Figa wrote: > Hi Sakari, > > On Mon, Aug 31, 2020 at 7:41 PM Sakari Ailus > <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > > > Hi Xingyu, > > > > Thanks for the update. I've got a few more comments below. > > > > Do you happen to have some insight on what the OTP data contains and what > > does the driver do with it? > > > > At least in principle the OTP data may be programmed for the customer so > > the same sensor could contain something else what the driver expects to > > find there. > > > > Thanks for the review. For anything without my reply, assume fixed. :) > > As far as I can see, the data is being read from an area that is > supposed to be reserved for Galaxycore, so I'd assume it doesn't > depend on the customer. Sounds good. > > [snip] > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > > index da11036..aeaf594 100644 > > > --- a/drivers/media/i2c/Kconfig > > > +++ b/drivers/media/i2c/Kconfig > > > @@ -712,6 +712,18 @@ config VIDEO_APTINA_PLL > > > config VIDEO_SMIAPP_PLL > > > tristate > > > > > > +config VIDEO_GC5035 > > > + tristate "Galaxycore GC5035 sensor support" > > > + depends on I2C && VIDEO_V4L2 > > > + select MEDIA_CONTROLLER > > > + select VIDEO_V4L2_SUBDEV_API > > > > Add: > > > > V4L2_FWNODE > > OF > > This driver doesn't depend on OF. It uses the firmware-independent > property access API. (v4 I sent actually uses device_property_*()). Yes, this is even better. > > [snip] > > > +static int __gc5035_power_on(struct gc5035 *gc5035) > > > +{ > > > + struct device *dev = &gc5035->client->dev; > > > + int i, ret; > > > + > > > + ret = clk_prepare_enable(gc5035->xvclk); > > > + if (ret < 0) { > > > + dev_err(dev, "Failed to enable xvclk\n"); > > > + return ret; > > > + } > > > + > > > + gpiod_set_value_cansleep(gc5035->reset_gpio, 1); > > > + > > > + for (i = 0; i < GC5035_NUM_SUPPLIES; i++) { > > > + ret = regulator_enable(gc5035->supplies[i].consumer); > > > + if (ret) { > > > + dev_err(dev, "Failed to enable %s: %d\n", > > > + gc5035->supplies[i].supply, ret); > > > + goto disable_reg_clk; > > > > Please use regulator_bulk_enable() here, and regulator_bulk_disable() > > below. > > > > This actually needs to have one of the regulators (iovdd) enabled > before the other ones, but regulator_bulk_enable() is async. In v4 I > used regulator_enable() for iovdd and regulator_bulk_enable() for the > other two for optimal sequencing. Ack. -- Sakari Ailus