lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 6 May 2020 13:29:43 +0200
From:   Robert Foss <robert.foss@...aro.org>
To:     Dongchun Zhu <dongchun.zhu@...iatek.com>
Cc:     Tomasz Figa <tfiga@...omium.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Sakari Ailus <sakari.ailus@....fi>,
        Marco Felsch <m.felsch@...gutronix.de>,
        Maxime Ripard <maxime@...no.tech>,
        linux-media <linux-media@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        Fabio Estevam <festevam@...il.com>,
        Ben Kao <ben.kao@...el.com>, Maxime Ripard <mripard@...nel.org>
Subject: Re: [PATCH v10 1/3] media: dt-bindings: ov8856: Document YAML bindings

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.

Looking into devm_gpiod_get_optional(), the flag argument
GPIOD_OUT_LOW or HIGH for that matter is actually not used initialize
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@...r.kernel.org
> >  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@...il.com>
>

Powered by blists - more mailing lists