On 20-05-06 13:29, Robert Foss wrote:
> Hey Dongchun,
>
> Thanks for having a look at this series.
>
> > > +examples:
> > > + - |
> > > + #include <dt-bindings/gpio/gpio.h>
> > > +
> > > + i2c {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + ov8856: camera@10 {
> > > + compatible = "ovti,ov8856";
> > > + reg = <0x10>;
> > > +
> > > + reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>;
> >
> > Apologies for missing to follow the earlier discussion related to this.
> > I noticed the GPIO flag para and __ov8856_power_on() are aligned using
> > ACTIVE_LOW.
> >
> > But from the datasheet, XSHUTDN pin is active-high for OV8856.
> > It means devm_gpiod_get API (in probe func) should use GPIOD_OUT_LOW to
> > initialize the GPIO as output with a value of 0.
> > Otherwise it should use GPIOD_OUT_HIGH.
> >
> > There is one case for GPIO_ACTIVE_LOW setting:
> > https://patchwork.linuxtv.org/patch/63460/
> > https://patchwork.linuxtv.org/patch/63461/
>
> We went back and forth about this a few times, and I switched to this
> gpio setting after having worked through the device probing reset gpio
> toggling. Semantically it seemed easier to understand in the driver,
> since the gpio is called reset and not !shutdown.
IMHO you can keep your version. DTs are part of the system integration.
What if one system has a invert logic infront of the gpio input.. The
system integrator needs to read and to understand the schematic
correctly to pick the correct value.
> Looking into devm_gpiod_get_optional(), the flag argument
> GPIOD_OUT_LOW or HIGH for that matter is actually not used initialize
The good think about gpiod is that it care about logic values not
physical/electrical values. If you set GPIOD_OUT_HIGH then the reset is
asserted, whatever asserted means electrical.
Regards,
Marco
> the output, but only used for an exclusivity check.
> https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-devres.c#L109
>
> If you prefer, I can invert the logic again. To me making the reset
> gpio active resulting in the device being actually reset seems like
> the most intuitive and easy to understand option.
> The different OmniVision drivers seem to have different approaches to
> this. The ov9640 driver for example is doing what this series
> currently is doing.
>
> >
> > Sakari, Tomasz, am I right?
> >
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&clk_24m_cam>;
> > > +
> > > + clocks = <&cam_osc>;
> > > + clock-names = "xvclk";
> > > + clock-frequency = <19200000>;
> > > +
> > > + avdd-supply = <&mt6358_vcama2_reg>;
> > > + dvdd-supply = <&mt6358_vcamd_reg>;
> > > + dovdd-supply = <&mt6358_vcamio_reg>;
> > > +
> > > + port {
> > > + wcam_out: endpoint {
> > > + remote-endpoint = <&mipi_in_wcam>;
> > > + data-lanes = <1 2 3 4>;
> > > + link-frequencies = /bits/ 64 <360000000>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +...
> > > \ No newline at end of file
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 26f281d9f32a..84b262afd13d 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -12489,6 +12489,7 @@ L: linux-media@xxxxxxxxxxxxxxx
> > > S: Maintained
> > > T: git git://linuxtv.org/media_tree.git
> > > F: drivers/media/i2c/ov8856.c
> > > +F: Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> > >
> >
> > Had you run parse-maintainers.pl?
> > The new item is supposed to be arranged in alphabetical order.
>
> No, I have not. But upon running it now, it doesn't make suggest any
> changes. But let me order the files manually in the next revision.
>
> However, I noticed I removed the wrong person from the maintainers
> file in this revision.
> So, I'll correct that and add you Dongchun as the maintainer if that's ok.
>
> >
> > > OMNIVISION OV9640 SENSOR DRIVER
> > > M: Petr Cvek <petrcvekcz@xxxxxxxxx>
> >
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
