On Tue, 2020-06-30 at 16:40 +0200, Tomasz Figa wrote: > On Tue, Jun 30, 2020 at 4:37 PM Andy Shevchenko > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > > > On Tue, Jun 30, 2020 at 04:21:31PM +0200, Tomasz Figa wrote: > > > On Tue, Jun 30, 2020 at 4:19 PM Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote: > > > > On Tue, Jun 30, 2020 at 11:54 AM Sakari Ailus > > > > <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > > > ... > > > > > > > > + ov02a10->rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > > > > > > > > > > > > > > > Shouldn't this be GPIOD_OUT_HIGH? That's the state it'll be after powering > > > > > the sensor off the first time. Alternatively make reset signal high in > > > > > runtime suspend callback. > > > > > > > > We might want to keep the signals low when the regulators are powered > > > > down, to reduce the leakage. > > > > > > Ah, I actually recall that the reset pin was physically active low, so > > > we would indeed better keep it at HIGH. Sorry for the noise. > > > > Here HIGH means "asserted", so in the code above it's LOW, means "deasserted", > > i.o.w. non-reset state. I dunno what is better, it depends to the firmware / > > driver expectations of the device configuration (from the power point of view, > > HIGH makes sense here, and not LOW, i.o.w. asserted reset line). > > > > And nice of the logical state that it doesn't depend to the active low / high > > (the latter just transparent to the user). > > Yeah, after reading through the GPIO subsystem documentation and > discussing with a bunch of people on how to interpret it, we concluded > that the driver needs to deal only with the logical state of the > signal. > > Actually, I later realized that the problem of leakage should likely > be solved by pinctrl suspend settings, based on given hardware > conditions, rather than the driver explicitly. > Thank you for all your explanation. Fixed in next release. > Best regards, > Tomasz
