* Re: [PATCH v2 1/4] net: hix5hd2_gmac: add generic compatible string
From: Dongpo Li @ 2016-12-12 11:16 UTC (permalink / raw)
To: Rob Herring
Cc: mark.rutland-5wv7dgnIgG8, mturquette-rdvid1DuHRBWk0Htik3J/w,
sboyd-sgV2jX0FEOL9JmXXK+q4OQ, linux-I+IVW8TIWO2tmTQ+vhA3Yw,
zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A,
yisen.zhuang-hv44wF8Li93QT0dZR+AlfA,
salil.mehta-hv44wF8Li93QT0dZR+AlfA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
arnd-r2nGTMty4D4, andrew-g2DYL2Zd6BY,
xuejiancheng-C8/M+/jPZTeaMJb+Lgu22Q,
benjamin.chenhao-C8/M+/jPZTeaMJb+Lgu22Q,
caizhiyong-C8/M+/jPZTeaMJb+Lgu22Q, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161209223521.5dnj7l44e663sntl@rob-hp-laptop>
Hi Rob,
On 2016/12/10 6:35, Rob Herring wrote:
> On Mon, Dec 05, 2016 at 09:27:58PM +0800, Dongpo Li wrote:
>> The "hix5hd2" is SoC name, add the generic ethernet driver name.
>> The "hisi-gemac-v1" is the basic version and "hisi-gemac-v2" adds
>> the SG/TXCSUM/TSO/UFO features.
>>
>> Signed-off-by: Dongpo Li <lidongpo-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
>> ---
>> .../devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt | 9 +++++++--
>> drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 15 +++++++++++----
>> 2 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt b/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
>> index 75d398b..75920f0 100644
>> --- a/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
>> +++ b/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
>> @@ -1,7 +1,12 @@
>> Hisilicon hix5hd2 gmac controller
>>
>> Required properties:
>> -- compatible: should be "hisilicon,hix5hd2-gmac".
>> +- compatible: should contain one of the following SoC strings:
>> + * "hisilicon,hix5hd2-gemac"
>> + * "hisilicon,hi3798cv200-gemac"
>> + and one of the following version string:
>> + * "hisilicon,hisi-gemac-v1"
>> + * "hisilicon,hisi-gemac-v2"
>
> What combinations are valid? I assume both chips don't have both v1 and
> v2. 2 SoCs and 2 versions so far, I don't think there is much point to
> have the v1 and v2 compatible strings.
>
The v1 and v2 are generic MAC compatible strings, many HiSilicon SoCs may
use the same MAC version. For example,
hix5hd2, hi3716cv200 SoCs use the v1 MAC version,
hi3798cv200, hi3516a SoCs use the v2 MAC version,
and there may be more SoCs added in future.
So I think the generic compatible strings are okay here.
Should I add the hi3716cv200, hi3516a SoCs compatible here?
Do you have any good advice?
>> - reg: specifies base physical address(s) and size of the device registers.
>> The first region is the MAC register base and size.
>> The second region is external interface control register.
>> @@ -20,7 +25,7 @@ Required properties:
>>
>> Example:
>> gmac0: ethernet@f9840000 {
>> - compatible = "hisilicon,hix5hd2-gmac";
>> + compatible = "hisilicon,hix5hd2-gemac", "hisilicon,hisi-gemac-v1";
>
> You can't just change compatible strings.
>
Okay, maybe I should name all the compatible string with the suffix "-gmac" instead of
"-gemac". This can keep the compatible strings with the same suffix. Is this okay?
Can I just add the generic compatible string without changing the SoCs compatible string?
Like following:
gmac0: ethernet@f9840000 {
- compatible = "hisilicon,hix5hd2-gmac";
+ compatible = "hisilicon,hix5hd2-gmac", "hisilicon,hisi-gmac-v1";
>> reg = <0xf9840000 0x1000>,<0xf984300c 0x4>;
>> interrupts = <0 71 4>;
>> #address-cells = <1>;
>
> .
>
Regards,
Dongpo
.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/3] dts: hisi: add dts files for Hi3516CV300 demo board
From: Jiancheng Xue @ 2016-12-12 11:34 UTC (permalink / raw)
To: Pan Wen, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, xuwei5-C8/M+/jPZTeaMJb+Lgu22Q,
Arnd Bergmann
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
howell.yang-C8/M+/jPZTeaMJb+Lgu22Q,
jalen.hsu-C8/M+/jPZTeaMJb+Lgu22Q,
lvkuanliang-C8/M+/jPZTeaMJb+Lgu22Q,
suwenping-C8/M+/jPZTeaMJb+Lgu22Q, raojun-C8/M+/jPZTeaMJb+Lgu22Q,
kevin.lixu-C8/M+/jPZTeaMJb+Lgu22Q
In-Reply-To: <20161017120705.3726-4-wenpan-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
On 2016/10/17 20:07, Pan Wen wrote:
> Add dts files for Hi3516CV300 demo board.
>
> Signed-off-by: Pan Wen <wenpan-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
> ---
Could you help to review this patch, please?
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/hi3516cv300-demb.dts | 148 ++++++++++++
> arch/arm/boot/dts/hi3516cv300.dtsi | 397 +++++++++++++++++++++++++++++++++
> 3 files changed, 546 insertions(+)
> create mode 100644 arch/arm/boot/dts/hi3516cv300-demb.dts
> create mode 100644 arch/arm/boot/dts/hi3516cv300.dtsi
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index befcd26..1f25530 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -171,6 +171,7 @@ dtb-$(CONFIG_ARCH_HIP01) += \
> dtb-$(CONFIG_ARCH_HIP04) += \
> hip04-d01.dtb
> dtb-$(CONFIG_ARCH_HISI) += \
> + hi3516cv300-demb.dtb \
> hi3519-demb.dtb
> dtb-$(CONFIG_ARCH_HIX5HD2) += \
> hisi-x5hd2-dkb.dtb
> diff --git a/arch/arm/boot/dts/hi3516cv300-demb.dts b/arch/arm/boot/dts/hi3516cv300-demb.dts
> new file mode 100644
> index 0000000..6a75cd6
> --- /dev/null
> +++ b/arch/arm/boot/dts/hi3516cv300-demb.dts
> @@ -0,0 +1,148 @@
> +/*
> + * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +
> +/dts-v1/;
> +#include "hi3516cv300.dtsi"
> +
> +/ {
> + model = "Hisilicon Hi3516CV300 DEMO Board";
> + compatible = "hisilicon,hi3516cv300";
> +
> + aliases {
> + serial0 = &uart0;
> + serial1 = &uart1;
> + serial2 = &uart2;
> + i2c0 = &i2c_bus0;
> + i2c1 = &i2c_bus1;
> + spi0 = &spi_bus0;
> + spi1 = &spi_bus1;
> + };
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + memory {
> + device_type = "memory";
> + reg = <0x80000000 0x10000000>;
> + };
> +};
> +
> +&dual_timer0 {
> + status = "okay";
> +};
> +
> +&watchdog {
> + status = "okay";
> +};
> +
> +&pwm {
> + status = "okay";
> +};
> +
> +&uart0 {
> + status = "okay";
> +};
> +
> +&i2c_bus0 {
> + status = "okay";
> + clock-frequency = <100000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c0_pmux>;
> +};
> +
> +&i2c_bus1 {
> + status = "okay";
> + clock-frequency = <100000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c1_pmux>;
> +};
> +
> +&spi_bus0{
> + status = "disabled";
> + num-cs = <1>;
> + cs-gpios = <&gpio_chip0 6 0>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&spi0_pmux>;
> +};
> +
> +&spi_bus1{
> + status = "okay";
> + num-cs = <2>;
> + cs-gpios = <&gpio_chip5 3 0>, <&gpio_chip5 4 0>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&spi1_pmux>;
> +};
> +
> +&fmc {
> + spi-nor@0 {
> + compatible = "jedec,spi-nor";
> + reg = <0>;
> + spi-max-frequency = <160000000>;
> + m25p,fast-read;
> + };
> +};
> +
> +&mdio {
> + phy0: phy@1 {
> + reg = <1>;
> + };
> +};
> +
> +&hisi_femac {
> + mac-address = [00 00 00 00 00 00];
> + phy-mode = "rmii";
> + phy-handle = <&phy0>;
> + hisilicon,phy-reset-delays-us = <10000 10000 150000>;
> +};
> +
> +&dmac {
> + status = "okay";
> +};
> +
> +&pmux {
> + i2c0_pmux: i2c0_pmux {
> + pinctrl-single,pins = <
> + 0x2c 0x3
> + 0x30 0x3>;
> + };
> +
> + i2c1_pmux: i2c1_pmux {
> + pinctrl-single,pins = <
> + 0x20 0x1
> + 0x24 0x1>;
> + };
> +
> + spi0_pmux: spi0_pmux {
> + pinctrl-single,pins = <
> + 0x28 0x1
> + 0x2c 0x1
> + 0x30 0x1
> + 0x34 0x1>;
> + };
> +
> + spi1_pmux: spi1_pmux {
> + pinctrl-single,pins = <
> + 0xc4 0x1
> + 0xc8 0x1
> + 0xcc 0x1
> + 0xd0 0x1
> + 0xd4 0x1>;
> + };
> +};
> diff --git a/arch/arm/boot/dts/hi3516cv300.dtsi b/arch/arm/boot/dts/hi3516cv300.dtsi
> new file mode 100644
> index 0000000..1da41ab
> --- /dev/null
> +++ b/arch/arm/boot/dts/hi3516cv300.dtsi
> @@ -0,0 +1,397 @@
> +/*
> + * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "skeleton.dtsi"
> +#include <dt-bindings/clock/hi3516cv300-clock.h>
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,arm926ej-s";
> + reg = <0>;
> + };
> + };
> +
> + vic: interrupt-controller@10040000 {
> + compatible = "arm,pl190-vic";
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + reg = <0x10040000 0x1000>;
> + };
> +
> + soc {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "simple-bus";
> + interrupt-parent = <&vic>;
> + ranges;
> +
> + clk_3m: clk_3m {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <3000000>;
> + };
> +
> + clk_apb: clk_apb {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <50000000>;
> + };
> +
> + crg: clock-reset-controller@12010000 {
> + compatible = "hisilicon,hi3516cv300-crg";
> + reg = <0x12010000 0x1000>;
> + #clock-cells = <1>;
> + #reset-cells = <2>;
> + };
> +
> + sysctrl: system-controller@12020000 {
> + compatible = "hisilicon,hi3516cv300-sysctrl", "syscon";
> + reg = <0x12020000 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + reboot {
> + compatible = "syscon-reboot";
> + regmap = <&sysctrl>;
> + offset = <0x4>;
> + mask = <0xdeadbeef>;
> + };
> +
> + dual_timer0: dual_timer@12000000 {
> + compatible = "arm,sp804", "arm,primecell";
> + reg = <0x12000000 0x1000>;
> + interrupts = <3>;
> + clocks = <&clk_3m>, <&clk_3m>, <&clk_apb>;
> + clock-names = "timer0", "timer1", "apb_pclk";
> + status = "disabled";
> + };
> +
> + dual_timer1: dual_timer@12001000 {
> + compatible = "arm,sp804", "arm,primecell";
> + reg = <0x12001000 0x1000>;
> + interrupts = <4>;
> + clocks = <&clk_3m>, <&clk_3m>, <&clk_apb>;
> + clock-names = "timer0", "timer1", "apb_pclk";
> + status = "disabled";
> + };
> +
> + watchdog: watchdog@12080000 {
> + compatible = "arm,sp805-wdt", "arm,primecell";
> + arm,primecell-periphid = <0x00141805>;
> + reg = <0x12080000 0x1000>;
> + clocks = <&sysctrl HI3516CV300_WDT_CLK>,
> + <&crg HI3516CV300_APB_CLK>;
> + clock-names = "wdog_clk", "apb_pclk";
> + status = "disabled";
> + };
> +
> + pwm: pwm@12130000 {
> + compatible = "hisilicon,hi3516cv300-pwm",
> + "hisilicon,hibvt-pwm";
> + reg = <0x12130000 0x10000>;
> + clocks = <&crg HI3516CV300_PWM_CLK>;
> + resets = <&crg 0x38 0>;
> + #pwm-cells = <2>;
> + status = "disabled";
> + };
> +
> + uart0: uart@12100000 {
> + compatible = "arm,pl011", "arm,primecell";
> + reg = <0x12100000 0x1000>;
> + interrupts = <5>;
> + clocks = <&crg HI3516CV300_UART0_CLK>;
> + clock-names = "apb_pclk";
> + status = "disabled";
> + };
> +
> + uart1: uart@12101000 {
> + compatible = "arm,pl011", "arm,primecell";
> + reg = <0x12101000 0x1000>;
> + interrupts = <30>;
> + clocks = <&crg HI3516CV300_UART1_CLK>;
> + clock-names = "apb_pclk";
> + status = "disabled";
> + };
> +
> + uart2: uart@12102000 {
> + compatible = "arm,pl011", "arm,primecell";
> + reg = <0x12102000 0x1000>;
> + interrupts = <25>;
> + clocks = <&crg HI3516CV300_UART2_CLK>;
> + clock-names = "apb_pclk";
> + status = "disabled";
> + };
> +
> + i2c_bus0: i2c@12110000 {
> + compatible = "hisilicon,hi3516cv300-i2c",
> + "hisilicon,hibvt-i2c";
> + reg = <0x12110000 0x1000>;
> + clocks = <&crg HI3516CV300_APB_CLK>;
> + status = "disabled";
> + };
> +
> + i2c_bus1: i2c@12112000 {
> + compatible = "hisilicon,hi3516cv300-i2c",
> + "hisilicon,hibvt-i2c";
> + reg = <0x12112000 0x1000>;
> + clocks = <&crg HI3516CV300_APB_CLK>;
> + status = "disabled";
> + };
> +
> + spi_bus0: spi@12120000 {
> + compatible = "arm,pl022", "arm,primecell";
> + reg = <0x12120000 0x1000>;
> + interrupts = <6>;
> + clocks = <&crg HI3516CV300_SPI0_CLK>;
> + clock-names = "apb_pclk";
> + dmas = <&dmac 12 1>, <&dmac 13 2>;
> + dma-names = "rx", "tx";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
> +
> + spi_bus1: spi@12121000 {
> + compatible = "arm,pl022", "arm,primecell";
> + reg = <0x12121000 0x1000>, <0x12030000 0x4>;
> + interrupts = <7>;
> + clocks = <&crg HI3516CV300_SPI1_CLK>;
> + clock-names = "apb_pclk";
> + dmas = <&dmac 14 1>, <&dmac 15 2>;
> + dma-names = "rx", "tx";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
> +
> + fmc: spi-nor-controller@10000000 {
> + compatible = "hisilicon,fmc-spi-nor";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x10000000 0x1000>, <0x14000000 0x1000000>;
> + reg-names = "control", "memory";
> + clocks = <&crg HI3516CV300_FMC_CLK>;
> + assigned-clocks = <&crg HI3516CV300_FMC_CLK>;
> + assigned-clock-rates = <24000000>;
> + };
> +
> + mdio: mdio@10051100 {
> + compatible = "hisilicon,hisi-femac-mdio";
> + reg = <0x10051100 0x10>;
> + clocks = <&crg HI3516CV300_ETH_CLK>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> +
> + hisi_femac: ethernet@10090000 {
> + compatible = "hisilicon,hi3516cv300-femac",
> + "hisilicon,hisi-femac-v2";
> + reg = <0x10050000 0x1000>,<0x10051300 0x200>;
> + interrupts = <12>;
> + clocks = <&crg HI3516CV300_ETH_CLK>;
> + resets = <&crg 0xec 0>, <&crg 0xec 3>;
> + reset-names = "mac","phy";
> + };
> +
> + dmac: dma-controller@10030000 {
> + compatible = "arm,pl080", "arm,primecell";
> + reg = <0x10030000 0x1000>;
> + interrupts = <14>;
> + clocks = <&crg HI3516CV300_DMAC_CLK>;
> + clock-names = "apb_pclk";
> + lli-bus-interface-ahb1;
> + lli-bus-interface-ahb2;
> + mem-bus-interface-ahb1;
> + mem-bus-interface-ahb2;
> + memcpy-burst-size = <256>;
> + memcpy-bus-width = <32>;
> + #dma-cells = <2>;
> + status = "disabled";
> + };
> +
> + gpio_chip0: gpio@12140000 {
> + compatible = "arm,pl061", "arm,primecell";
> + reg = <0x12140000 0x1000>;
> + interrupts = <31>;
> + clocks = <&crg HI3516CV300_APB_CLK>;
> + clock-names = "apb_pclk";
> + #gpio-cells = <2>;
> + gpio-ranges = <&pmux 0 61 2>,
> + <&pmux 4 11 1>,
> + <&pmux 5 10 1>,
> + <&pmux 6 13 2>;
> +
> + status = "disabled";
> + };
> +
> + gpio_chip1: gpio@12141000 {
> + compatible = "arm,pl061", "arm,primecell";
> + reg = <0x12141000 0x1000>;
> + interrupts = <31>;
> + clocks = <&crg HI3516CV300_APB_CLK>;
> + clock-names = "apb_pclk";
> + #gpio-cells = <2>;
> + gpio-ranges = <&pmux 0 16 7>,
> + <&pmux 7 0 1>;
> + status = "disabled";
> + };
> +
> + gpio_chip2: gpio@12142000 {
> + compatible = "arm,pl061", "arm,primecell";
> + reg = <0x12142000 0x1000>;
> + interrupts = <31>;
> + clocks = <&crg HI3516CV300_APB_CLK>;
> + clock-names = "apb_pclk";
> + #gpio-cells = <2>;
> + gpio-ranges = <&pmux 0 46 1>,
> + <&pmux 1 45 1>,
> + <&pmux 2 44 1>,
> + <&pmux 3 43 1>,
> + <&pmux 4 39 1>,
> + <&pmux 5 38 1>,
> + <&pmux 6 40 1>,
> + <&pmux 7 48 1>;
> + status = "disabled";
> + };
> +
> + gpio_chip3: gpio@12143000 {
> + compatible = "arm,pl061", "arm,primecell";
> + reg = <0x12143000 0x1000>;
> + interrupts = <31>;
> + clocks = <&crg HI3516CV300_APB_CLK>;
> + clock-names = "apb_pclk";
> + #gpio-cells = <2>;
> + gpio-ranges = <&pmux 0 37 1>,
> + <&pmux 1 36 1>,
> + <&pmux 2 35 1>,
> + <&pmux 3 34 1>,
> + <&pmux 4 23 2>,
> + <&pmux 6 8 2>;
> + status = "disabled";
> + };
> +
> + gpio_chip4: gpio@12144000 {
> + compatible = "arm,pl061", "arm,primecell";
> + reg = <0x12144000 0x1000>;
> + interrupts = <31>;
> + clocks = <&crg HI3516CV300_APB_CLK>;
> + clock-names = "apb_pclk";
> + #gpio-cells = <2>;
> + gpio-ranges = <&pmux 0 27 1>,
> + <&pmux 1 26 1>,
> + <&pmux 2 31 1>,
> + <&pmux 3 30 1>,
> + <&pmux 4 28 2>,
> + <&pmux 6 33 1>,
> + <&pmux 7 32 1>;
> + status = "disabled";
> + };
> +
> + gpio_chip5: gpio@12145000 {
> + compatible = "arm,pl061", "arm,primecell";
> + reg = <0x12145000 0x1000>;
> + interrupts = <31>;
> + clocks = <&crg HI3516CV300_APB_CLK>;
> + clock-names = "apb_pclk";
> + #gpio-cells = <2>;
> + gpio-ranges = <&pmux 0 53 1>,
> + <&pmux 1 51 2>,
> + <&pmux 3 50 1>,
> + <&pmux 4 49 1>,
> + <&pmux 5 47 1>,
> + <&pmux 6 40 2>;
> + status = "disabled";
> + };
> +
> + gpio_chip6: gpio@12146000 {
> + compatible = "arm,pl061", "arm,primecell";
> + reg = <0x12146000 0x1000>;
> + interrupts = <31>;
> + clocks = <&crg HI3516CV300_APB_CLK>;
> + clock-names = "apb_pclk";
> + #gpio-cells = <2>;
> + gpio-ranges = <&pmux 0 7 1>,
> + <&pmux 1 6 1>,
> + <&pmux 2 4 1>,
> + <&pmux 3 5 1>,
> + <&pmux 4 15 1>,
> + <&pmux 5 1 3>;
> + status = "disabled";
> + };
> +
> + gpio_chip7: gpio@12147000 {
> + compatible = "arm,pl061", "arm,primecell";
> + reg = <0x12147000 0x1000>;
> + interrupts = <31>;
> + clocks = <&crg HI3516CV300_APB_CLK>;
> + clock-names = "apb_pclk";
> + #gpio-cells = <2>;
> + gpio-ranges = <&pmux 1 55 6>,
> + <&pmux 7 25 1>;
> + status = "disabled";
> + };
> +
> + gpio_chip8: gpio@12148000 {
> + compatible = "arm,pl061", "arm,primecell";
> + reg = <0x12148000 0x1000>;
> + interrupts = <31>;
> + clocks = <&crg HI3516CV300_APB_CLK>;
> + clock-names = "apb_pclk";
> + #gpio-cells = <2>;
> + gpio-ranges = <&pmux 0 63 3>,
> + <&pmux 3 12 1>;
> + status = "disabled";
> + };
> +
> + pmux: pinmux@12040000 {
> + compatible = "pinctrl-single";
> + reg = <0x12040000 0x108>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + #gpio-range-cells = <3>;
> + ranges;
> +
> + pinctrl-single,register-width = <32>;
> + pinctrl-single,function-mask = <7>;
> + /* pin base, nr pins & gpio function */
> + pinctrl-single,gpio-range = <&range 0 54 0
> + &range 55 6 1 &range 61 5 0>;
> +
> + range: gpio-range {
> + #pinctrl-single,gpio-range-cells = <3>;
> + };
> + };
> +
> + pconf: pinconf@12040800 {
> + compatible = "pinconf-single";
> + reg = <0x12040800 0x130>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + pinctrl-single,register-width = <32>;
> + };
> + };
> +};
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 2/2] Add support for OV5647 sensor
From: Ramiro Oliveira @ 2016-12-12 11:39 UTC (permalink / raw)
To: Sakari Ailus, Ramiro Oliveira
Cc: mchehab-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
dheitmueller-eb9eJ82Ua7k9XoPSrs7Ehg,
slongerbeam-Re5JQEeQqe8AvxtiuMwx3w, lars-Qo5EllUWu/uELgA04lAiVw,
robert.jarzmik-GANU6spQydw, pavel-+ZI9xUNit7I,
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w,
sakari.ailus-VuQAYsv1563Yd54FQh9/CA, mark.rutland-5wv7dgnIgG8,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <20161207230138.GA16630-S+BSfZ9RZZmRSg0ZkenSGLdO1Tsj/99ntUK59QYPAWc@public.gmane.org>
Hi Sakari
On 12/7/2016 11:01 PM, Sakari Ailus wrote:
> Hi Ramiro,
>
> On Mon, Dec 05, 2016 at 05:36:34PM +0000, Ramiro Oliveira wrote:
>> Add support for OV5647 sensor.
>>
>> Modes supported:
>> - 640x480 RAW 8
>>
>> Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>> ---
>> MAINTAINERS | 7 +
>> drivers/media/i2c/Kconfig | 12 +
>> drivers/media/i2c/Makefile | 1 +
>> drivers/media/i2c/ov5647.c | 866 +++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 886 insertions(+)
>> create mode 100644 drivers/media/i2c/ov5647.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 52cc077..72e828a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8923,6 +8923,13 @@ M: Harald Welte <laforge-TgoAw6mPHtdg9hUCZPvPmw@public.gmane.org>
>> S: Maintained
>> F: drivers/char/pcmcia/cm4040_cs.*
>>
>> +OMNIVISION OV5647 SENSOR DRIVER
>> +M: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>> +L: linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> +T: git git://linuxtv.org/media_tree.git
>> +S: Maintained
>> +F: drivers/media/i2c/ov5647.c
>> +
>> OMNIVISION OV7670 SENSOR DRIVER
>> M: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
>> L: linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index b31fa6f..c1b78e5 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -531,6 +531,18 @@ config VIDEO_OV2659
>> To compile this driver as a module, choose M here: the
>> module will be called ov2659.
>>
>> +config VIDEO_OV5647
>> + tristate "OmniVision OV5647 sensor support"
>> + depends on OF
>
> How does this driver depend on OF, other than matching the compatible
> string?
>
It doesn't, should I proceed diferently?
>> + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>> + depends on MEDIA_CAMERA_SUPPORT
>> + ---help---
>> + This is a Video4Linux2 sensor-level driver for the OmniVision
>> + OV5647 camera.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called ov5647.
>> +
>> config VIDEO_OV7640
>> tristate "OmniVision OV7640 sensor support"
>> depends on I2C && VIDEO_V4L2
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index 92773b2..0d9014c 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -82,3 +82,4 @@ obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o
>> obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o
>> obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
>> obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
>> +obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
>> new file mode 100644
>> index 0000000..2aae806
>> --- /dev/null
>> +++ b/drivers/media/i2c/ov5647.c
>> @@ -0,0 +1,866 @@
>> +/*
>> + * A V4L2 driver for OmniVision OV5647 cameras.
>> + *
>> + * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driver
>> + * Copyright (C) 2011 Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> + *
>> + * Based on Omnivision OV7670 Camera Driver
>> + * Copyright (C) 2006-7 Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
>> + *
>> + * Copyright (C) 2016, Synopsys, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + *
>> + * This program is distributed .as is. WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/i2c.h>
>> +#include <linux/delay.h>
>> +#include <linux/videodev2.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-mediabus.h>
>> +#include <media/v4l2-image-sizes.h>
>> +#include <media/v4l2-of.h>
>> +#include <linux/io.h>
>
> Alphabetical order, please.
>
>> +
>> +#define SENSOR_NAME "ov5647"
>> +
>> +#define OV5647_SW_RESET 0x1003
>> +#define OV5647_REG_CHIPID_H 0x300A
>> +#define OV5647_REG_CHIPID_L 0x300B
>> +
>> +#define REG_TERM 0xfffe
>> +#define VAL_TERM 0xfe
>> +#define REG_DLY 0xffff
>> +
>> +#define OV5647_ROW_START 0x01
>> +#define OV5647_ROW_START_MIN 0
>> +#define OV5647_ROW_START_MAX 2004
>> +#define OV5647_ROW_START_DEF 54
>> +
>> +#define OV5647_COLUMN_START 0x02
>> +#define OV5647_COLUMN_START_MIN 0
>> +#define OV5647_COLUMN_START_MAX 2750
>> +#define OV5647_COLUMN_START_DEF 16
>> +
>> +#define OV5647_WINDOW_HEIGHT 0x03
>> +#define OV5647_WINDOW_HEIGHT_MIN 2
>> +#define OV5647_WINDOW_HEIGHT_MAX 2006
>> +#define OV5647_WINDOW_HEIGHT_DEF 1944
>> +
>> +#define OV5647_WINDOW_WIDTH 0x04
>> +#define OV5647_WINDOW_WIDTH_MIN 2
>> +#define OV5647_WINDOW_WIDTH_MAX 2752
>> +#define OV5647_WINDOW_WIDTH_DEF 2592
>> +
>> +struct regval_list {
>> + u16 addr;
>> + u8 data;
>> +};
>> +
>> +struct cfg_array {
>> + struct regval_list *regs;
>> + int size;
>> +};
>> +
>> +struct sensor_win_size {
>> + int width;
>> + int height;
>> + unsigned int hoffset;
>> + unsigned int voffset;
>> + unsigned int hts;
>> + unsigned int vts;
>> + unsigned int pclk;
>> + unsigned int mipi_bps;
>> + unsigned int fps_fixed;
>> + unsigned int bin_factor;
>> + unsigned int intg_min;
>> + unsigned int intg_max;
>> + void *regs;
>> + int regs_size;
>> + int (*set_size)(struct v4l2_subdev *sd);
>> +};
>> +
>> +
>> +struct ov5647 {
>> + struct device *dev;
>> + struct v4l2_subdev sd;
>> + struct media_pad pad;
>> + struct mutex lock;
>> + struct v4l2_mbus_framefmt format;
>> + struct sensor_format_struct *fmt;
>> + unsigned int width;
>> + unsigned int height;
>> + unsigned int capture_mode;
>> + int hue;
>
> At least capture_mode and hue are unused. Please remove unused fields.
>
>> + struct v4l2_fract tpf;
>> + struct sensor_win_size *current_wins;
>> +};
>> +
>> +static inline struct ov5647 *to_state(struct v4l2_subdev *sd)
>> +{
>> + return container_of(sd, struct ov5647, sd);
>> +}
>> +
>> +static struct regval_list sensor_oe_disable_regs[] = {
>> + {0x3000, 0x00},
>> + {0x3001, 0x00},
>> + {0x3002, 0x00},
>> +};
>> +
>> +static struct regval_list sensor_oe_enable_regs[] = {
>> + {0x3000, 0x0f},
>> + {0x3001, 0xff},
>> + {0x3002, 0xe4},
>> +};
>> +
>> +static struct regval_list ov5647_640x480[] = {
>
> Does this list expect a certain external clock frequency? If it does, should
> you check that the actual frequency matches with the expectation?
>
Yes. But like I said in the DT patch the external clock has a fixed frequency. I
can add a check if you thinks it's better.
>> + {0x0100, 0x00},
>> + {0x0103, 0x01},
>> + {0x3034, 0x08},
>> + {0x3035, 0x21},
>> + {0x3036, 0x46},
>> + {0x303c, 0x11},
>> + {0x3106, 0xf5},
>> + {0x3821, 0x07},
>> + {0x3820, 0x41},
>> + {0x3827, 0xec},
>> + {0x370c, 0x0f},
>> + {0x3612, 0x59},
>> + {0x3618, 0x00},
>> + {0x5000, 0x06},
>> + {0x5001, 0x01},
>> + {0x5002, 0x41},
>> + {0x5003, 0x08},
>> + {0x5a00, 0x08},
>> + {0x3000, 0x00},
>> + {0x3001, 0x00},
>> + {0x3002, 0x00},
>> + {0x3016, 0x08},
>> + {0x3017, 0xe0},
>> + {0x3018, 0x44},
>> + {0x301c, 0xf8},
>> + {0x301d, 0xf0},
>> + {0x3a18, 0x00},
>> + {0x3a19, 0xf8},
>> + {0x3c01, 0x80},
>> + {0x3b07, 0x0c},
>> + {0x380c, 0x07},
>> + {0x380d, 0x68},
>> + {0x380e, 0x03},
>> + {0x380f, 0xd8},
>> + {0x3814, 0x31},
>> + {0x3815, 0x31},
>> + {0x3708, 0x64},
>> + {0x3709, 0x52},
>> + {0x3808, 0x02},
>> + {0x3809, 0x80},
>> + {0x380a, 0x01},
>> + {0x380b, 0xE0},
>> + {0x3801, 0x00},
>> + {0x3802, 0x00},
>> + {0x3803, 0x00},
>> + {0x3804, 0x0a},
>> + {0x3805, 0x3f},
>> + {0x3806, 0x07},
>> + {0x3807, 0xa1},
>> + {0x3811, 0x08},
>> + {0x3813, 0x02},
>> + {0x3630, 0x2e},
>> + {0x3632, 0xe2},
>> + {0x3633, 0x23},
>> + {0x3634, 0x44},
>> + {0x3636, 0x06},
>> + {0x3620, 0x64},
>> + {0x3621, 0xe0},
>> + {0x3600, 0x37},
>> + {0x3704, 0xa0},
>> + {0x3703, 0x5a},
>> + {0x3715, 0x78},
>> + {0x3717, 0x01},
>> + {0x3731, 0x02},
>> + {0x370b, 0x60},
>> + {0x3705, 0x1a},
>> + {0x3f05, 0x02},
>> + {0x3f06, 0x10},
>> + {0x3f01, 0x0a},
>> + {0x3a08, 0x01},
>> + {0x3a09, 0x27},
>> + {0x3a0a, 0x00},
>> + {0x3a0b, 0xf6},
>> + {0x3a0d, 0x04},
>> + {0x3a0e, 0x03},
>> + {0x3a0f, 0x58},
>> + {0x3a10, 0x50},
>> + {0x3a1b, 0x58},
>> + {0x3a1e, 0x50},
>> + {0x3a11, 0x60},
>> + {0x3a1f, 0x28},
>> + {0x4001, 0x02},
>> + {0x4004, 0x02},
>> + {0x4000, 0x09},
>> + {0x4837, 0x24},
>> + {0x4050, 0x6e},
>> + {0x4051, 0x8f},
>> + {0x0100, 0x01},
>> +};
>> +
>> +struct sensor_format_struct;
>> +
>> +/**
>> + * @short I2C Write operation
>> + * @param[in] i2c_client I2C client
>> + * @param[in] reg register address
>> + * @param[in] val value to write
>> + * @return Error code
>> + */
>> +static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
>> +{
>> + int ret;
>> + unsigned char data[3] = { reg >> 8, reg & 0xff, val};
>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> + ret = i2c_master_send(client, data, 3);
>> + if (ret != 3) {
>> + dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
>> + __func__, reg);
>> + return ret < 0 ? ret : -EIO;
>> + }
>> + return 0;
>> +}
>> +
>> +/**
>> + * @short I2C Read operation
>> + * @param[in] i2c_client I2C client
>> + * @param[in] reg register address
>> + * @param[out] val value read
>> + * @return Error code
>> + */
>> +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
>> +{
>> + int ret;
>> + unsigned char data_w[2] = { reg >> 8, reg & 0xff };
>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> +
>> + ret = i2c_master_send(client, data_w, 2);
>> +
>> + if (ret < 2) {
>> + dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
>> + __func__, reg);
>> + return ret < 0 ? ret : -EIO;
>> + }
>> +
>> +
>> + ret = i2c_master_recv(client, val, 1);
>> +
>> + if (ret < 1) {
>> + dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
>> + __func__, reg);
>> + return ret < 0 ? ret : -EIO;
>> + }
>> + return 0;
>> +}
>> +
>> +static int ov5647_write_array(struct v4l2_subdev *sd,
>> + struct regval_list *regs, int array_size)
>> +{
>> + int i = 0;
>> + int ret = 0;
>> +
>> + if (!regs)
>> + return -EINVAL;
>> +
>> + while (i < array_size) {
>> + ret = ov5647_write(sd, regs->addr, regs->data);
>> + if (ret < 0)
>> + return ret;
>> + i++;
>> + regs++;
>> + }
>> + return 0;
>> +}
>> +
>> +static void ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
>> +{
>> + u8 channel_id;
>> +
>> + ov5647_read(sd, 0x4814, &channel_id);
>> + channel_id &= ~(3 << 6);
>> + ov5647_write(sd, 0x4814, channel_id | (channel << 6));
>> +}
>> +
>> +void ov5647_stream_on(struct v4l2_subdev *sd)
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> + ov5647_write(sd, 0x4202, 0x00);
>> + dev_dbg(&client->dev, "Stream on");
>> + ov5647_write(sd, 0x300D, 0x00);
>> +}
>> +
>> +void ov5647_stream_off(struct v4l2_subdev *sd)
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> + ov5647_write(sd, 0x4202, 0x0f);
>> + dev_dbg(&client->dev, "Stream off");
>> + ov5647_write(sd, 0x300D, 0x01);
>> +}
>> +
>> +/****************************************************************************/
>> +
>> +/**
>> + * @short Set SW standby
>> + * @param[in] sd v4l2 sd
>> + * @param[in] stanby standby mode status (on or off)
>> + * @return Error code
>> + */
>> +static int set_sw_standby(struct v4l2_subdev *sd, bool standby)
>> +{
>> + int ret;
>> + unsigned char rdval;
>> +
>> + ret = ov5647_read(sd, 0x0100, &rdval);
>> + if (ret != 0)
>> + return ret;
>> +
>> + if (standby)
>> + rdval &= 0xfe;
>> + else
>> + rdval |= 0x01;
>> +
>> + ret = ov5647_write(sd, 0x0100, rdval);
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * @short Store information about the video data format.
>> + */
>> +static struct sensor_format_struct {
>> + u8 *desc;
>> + u32 mbus_code;
>> + struct regval_list *regs;
>> + int regs_size;
>> + int bpp;
>
> At least desc and bpp are unused.
>
>> +} sensor_formats[] = {
>> + {
>> + .desc = "Raw RGB Bayer",
>> + .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
>> + .regs = ov5647_640x480,
>> + .regs_size = ARRAY_SIZE(ov5647_640x480),
>> + .bpp = 1
>> + },
>> +};
>> +#define N_FMTS ARRAY_SIZE(sensor_formats)
>> +
>> +/* ----------------------------------------------------------------------- */
>> +
>> +/**
>> + * @short Initialize sensor
>> + * @param[in] sd v4l2 subdev
>> + * @param[in] val not used
>> + * @return Error code
>> + */
>> +static int __sensor_init(struct v4l2_subdev *sd)
>> +{
>> + int ret;
>> + u8 resetval;
>> + u8 rdval;
>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> + dev_dbg(&client->dev, "sensor init\n");
>> +
>> + ret = ov5647_read(sd, 0x0100, &rdval);
>> + if (ret != 0)
>> + return ret;
>> +
>> + ov5647_write(sd, 0x4800, 0x25);
>> + ov5647_stream_off(sd);
>> +
>> + ret = ov5647_write_array(sd, ov5647_640x480,
>> + ARRAY_SIZE(ov5647_640x480));
>> + if (ret < 0) {
>> + dev_err(&client->dev, "write sensor_default_regs error\n");
>> + return ret;
>> + }
>> +
>> + ov5647_set_virtual_channel(sd, 0);
>> +
>> + ov5647_read(sd, 0x0100, &resetval);
>> + if (!(resetval & 0x01)) {
>> + dev_err(&client->dev, "Device was in SW standby");
>> + ov5647_write(sd, 0x0100, 0x01);
>> + }
>> +
>> + ov5647_write(sd, 0x4800, 0x04);
>> + ov5647_stream_on(sd);
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * @short Control sensor power state
>> + * @param[in] sd v4l2 subdev
>> + * @param[in] on Sensor power
>> + * @return Error code
>> + */
>> +static int sensor_power(struct v4l2_subdev *sd, int on)
>> +{
>> + int ret;
>> + struct ov5647 *ov5647 = to_state(sd);
>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> + ret = 0;
>> + mutex_lock(&ov5647->lock);
>> +
>> + if (on) {
>> + dev_dbg(&client->dev, "OV5647 power on\n");
>> +
>> + ret = ov5647_write_array(sd, sensor_oe_enable_regs,
>> + ARRAY_SIZE(sensor_oe_enable_regs));
>> +
>> + ret = __sensor_init(sd);
>> +
>> + if (ret < 0)
>> + dev_err(&client->dev,
>> + "Camera not available, check Power\n");
>> + } else {
>> + dev_dbg(&client->dev, "OV5647 power off\n");
>> +
>> + dev_dbg(&client->dev, "disable oe\n");
>> + ret = ov5647_write_array(sd, sensor_oe_disable_regs,
>> + ARRAY_SIZE(sensor_oe_disable_regs));
>> +
>> + if (ret < 0)
>> + dev_dbg(&client->dev, "disable oe failed\n");
>> +
>> + ret = set_sw_standby(sd, true);
>> +
>> + if (ret < 0)
>> + dev_dbg(&client->dev, "soft stby failed\n");
>> +
>> + }
>> +
>> + mutex_unlock(&ov5647->lock);
>> +
>> + return ret;
>> +}
>> +
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> +/**
>> + * @short Get register value
>> + * @param[in] sd v4l2 subdev
>> + * @param[in] reg register struct
>> + * @return Error code
>> + */
>> +static int sensor_get_register(struct v4l2_subdev *sd,
>> + struct v4l2_dbg_register *reg)
>> +{
>> + unsigned char val = 0;
>> + int ret;
>> +
>> + ret = ov5647_read(sd, reg->reg & 0xff, &val);
>> + if (ret != 0)
>> + return ret;
>> +
>> + reg->val = val;
>> + reg->size = 1;
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * @short Set register value
>> + * @param[in] sd v4l2 subdev
>> + * @param[in] reg register struct
>> + * @return Error code
>> + */
>> +static int sensor_set_register(struct v4l2_subdev *sd,
>> + const struct v4l2_dbg_register *reg)
>> +{
>> + return ov5647_write(sd, reg->reg & 0xff, reg->val & 0xff);
>> +}
>> +#endif
>> +
>> +/* ----------------------------------------------------------------------- */
>> +
>> +/**
>> + * @short Subdev core operations registration
>> + */
>> +static const struct v4l2_subdev_core_ops sensor_core_ops = {
>> + .s_power = sensor_power,
>
> The s_power() op will be called by the bridge (ISP) driver as well. You
> should expect that it may be enabled more than once before being disabled
> equal number of times.
>
Should I add an IF check to verify if the sensor is powered on before running
the power on/off routine.
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> + .g_register = sensor_get_register,
>> + .s_register = sensor_set_register,
>> +#endif
>> +};
>> +
>> +/* ----------------------------------------------------------------------- */
>> +
>> +
>> +
>> +/**
>> + * @short Enumerate available image formats
>> + * @param[in] sd v4l2 subdev
>> + * @param[in] index index
>> + * @param[in] code MBUS Pixel code
>> + * @return Error code
>> + */
>> +static int sensor_enum_fmt(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> + if (code->pad || code->index >= N_FMTS)
>> + return -EINVAL;
>> +
>> + code->code = sensor_formats[code->index].mbus_code;
>> + return 0;
>> +}
>> +
>> +/**
>> + * @short Try frame format internal function
>> + * @param[in] sd v4l2 subdev
>> + * @param[in] fmt frame format
>> + * @return Error code
>> + */
>> +static int sensor_try_fmt_internal(struct v4l2_subdev *sd,
>> + struct v4l2_mbus_framefmt *fmt, struct sensor_format_struct **ret_fmt,
>> + struct sensor_win_size **ret_wsize)
>> +{
>> + int index;
>> +
>> + for (index = 0; index < N_FMTS; index++)
>> + if (sensor_formats[index].mbus_code == fmt->code)
>> + break;
>> +
>> + if (index >= N_FMTS)
>> + return -EINVAL;
>> +
>> + if (ret_fmt != NULL)
>> + *ret_fmt = sensor_formats + index;
>> +
>> + fmt->field = V4L2_FIELD_NONE;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * @short Set frame format
>> + * @param[in] sd v4l2 subdev
>> + * @param[in] fmt frame format
>> + * @return Error code
>> + */
>> +static int sensor_s_fmt(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_format *fmt)
>> +{
>> + int ret;
>> + struct sensor_format_struct *sensor_fmt;
>> + struct sensor_win_size *wsize;
>> + struct ov5647 *info = to_state(sd);
>> +
>> + ov5647_write_array(sd, sensor_oe_disable_regs,
>> + ARRAY_SIZE(sensor_oe_disable_regs));
>
> Should you check the error code here?
>
>> +
>> + ret = sensor_try_fmt_internal(sd, &fmt->format,
>> + &sensor_fmt, &wsize);
>
> Do you set wsize somewhere?
>
>> + if (ret)
>> + return ret;
>> +
>> + ov5647_write_array(sd, sensor_fmt->regs, sensor_fmt->regs_size);
>
> And here.
>
>> +
>> + ret = 0;
>
> ret was already zero.
>
>> +
>> + if (wsize->regs)
>> + ov5647_write_array(sd, wsize->regs, wsize->regs_size);
>> +
>> + if (wsize->set_size)
>> + wsize->set_size(sd);
>> +
>> + info->fmt = sensor_fmt;
>> + info->width = wsize->width;
>> + info->height = wsize->height;
>> +
>> + ov5647_write_array(sd, sensor_oe_enable_regs,
>> + ARRAY_SIZE(sensor_oe_enable_regs));
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * @short Set stream parameters
>> + * @param[in] sd v4l2 subdev
>> + * @param[in] parms stream parameters
>> + * @return Error code
>> + */
>> +static int sensor_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
>> +{
>> + struct v4l2_captureparm *cp = &parms->parm.capture;
>> + struct ov5647 *info = to_state(sd);
>> +
>> + if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> + return -EINVAL;
>> +
>> + if (info->tpf.numerator == 0)
>> + return -EINVAL;
>> +
>> + info->capture_mode = cp->capturemode;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * @short Get stream parameters
>> + * @param[in] sd v4l2 subdev
>> + * @param[in] parms stream parameters
>> + * @return Error code
>> + */
>> +static int sensor_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
>> +{
>> + struct v4l2_captureparm *cp = &parms->parm.capture;
>> + struct ov5647 *info = to_state(sd);
>> +
>> + if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> + return -EINVAL;
>> +
>> + memset(cp, 0, sizeof(struct v4l2_captureparm));
>> + cp->capability = V4L2_CAP_TIMEPERFRAME;
>> + cp->capturemode = info->capture_mode;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * @short Subdev video operations registration
>> + *
>> + */
>> +static const struct v4l2_subdev_video_ops sensor_video_ops = {
>> + .s_parm = sensor_s_parm,
>> + .g_parm = sensor_g_parm,
>
> Please use the g/s_frame_interval() instead. That's what sub-device drivers
> generally use for the purpose.
>
>> +};
>> +
>> +/* ----------------------------------------------------------------------- */
>> +
>> +/**
>> + * @short Subdev operations registration
>> + *
>> + */
>> +static const struct v4l2_subdev_ops subdev_ops = {
>> + .core = &sensor_core_ops,
>> + .video = &sensor_video_ops,
>> +};
>> +
>> +/* -----------------------------------------------------------------------------
>> + * V4L2 subdev internal operations
>> + */
>> +
>> +/**
>> + * @short Detect camera version and model
>> + * @param[in] sd v4l2 subdev
>> + * @return Error code
>> + */
>> +int ov5647_detect(struct v4l2_subdev *sd)
>> +{
>> + unsigned char v;
>> + int ret;
>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> + ret = ov5647_write(sd, OV5647_SW_RESET, 0x01);
>> + if (ret < 0)
>> + return ret;
>> + ret = ov5647_read(sd, OV5647_REG_CHIPID_H, &v);
>> + if (ret < 0)
>> + return ret;
>> + if (v != 0x56) {
>> + dev_err(&client->dev, "Wrong model version detected");
>> + return -ENODEV;
>> + }
>> + ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &v);
>> + if (ret < 0)
>> + return ret;
>> + if (v != 0x47) {
>> + dev_err(&client->dev, "Wrong model version detected");
>> + return -ENODEV;
>> + }
>> +
>> + ret = ov5647_write(sd, OV5647_SW_RESET, 0x00);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * @short Detect if camera is registered
>> + * @param[in] sd v4l2 subdev
>> + * @return Error code
>> + */
>> +static int ov5647_registered(struct v4l2_subdev *sd)
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> + dev_info(&client->dev, "OV5647 detected at address 0x%02x\n",
>> + client->addr);
>
> I might omit this. If there's a need to debug this then the information
> should be printed by the framework instead using debug level messages.
>
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * @short Open device
>> + * @param[in] sd v4l2 subdev
>> + * @param[in] fh v4l2 file handler
>> + * @return Error code
>> + */
>> +static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>> +{
>> + struct v4l2_mbus_framefmt *format =
>> + v4l2_subdev_get_try_format(sd, fh->pad, 0);
>> + struct v4l2_rect *crop =
>> + v4l2_subdev_get_try_crop(sd, fh->pad, 0);
>> +
>> + crop->left = OV5647_COLUMN_START_DEF;
>> + crop->top = OV5647_ROW_START_DEF;
>> + crop->width = OV5647_WINDOW_WIDTH_DEF;
>> + crop->height = OV5647_WINDOW_HEIGHT_DEF;
>> +
>> + format->code = MEDIA_BUS_FMT_SBGGR8_1X8;
>> +
>> + format->width = OV5647_WINDOW_WIDTH_DEF;
>> + format->height = OV5647_WINDOW_HEIGHT_DEF;
>> + format->field = V4L2_FIELD_NONE;
>> + format->colorspace = V4L2_COLORSPACE_SRGB;
>> +
>> + return sensor_power(sd, true);
>> +}
>> +
>> +/**
>> + * @short Open device
>> + * @param[in] sd v4l2 subdev
>> + * @param[in] fh v4l2 file handler
>> + * @return Error code
>> + */
>> +static int ov5647_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>> +{
>> + return sensor_power(sd, false);
>> +}
>> +
>> +/**
>> + * @short Subdev internal operations registration
>> + *
>> + */
>> +static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
>> + .registered = ov5647_registered,
>> + .open = ov5647_open,
>> + .close = ov5647_close,
>> +};
>> +
>> +/**
>> + * @short Initialization routine - Entry point of the driver
>> + * @param[in] client pointer to the i2c client structure
>> + * @param[in] id pointer to the i2c device id structure
>> + * @return 0 on success and a negative number on failure
>> + */
>> +static int ov5647_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct device *dev = &client->dev;
>> + struct ov5647 *sensor;
>> + int ret = 0;
>
> No need to initialise ret here.
>
>> + struct v4l2_subdev *sd;
>> +
>> + dev_info(&client->dev, "Installing OmniVision OV5647 camera driver\n");
>> +
>> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
>> + if (sensor == NULL)
>> + return -ENOMEM;
>> +
>> + mutex_init(&sensor->lock);
>> + sensor->dev = dev;
>> +
>> + sd = &sensor->sd;
>> + v4l2_i2c_subdev_init(sd, client, &subdev_ops);
>> + sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +
>> + sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
>> + sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
>> + ret = media_entity_pads_init(&sd->entity, 1, &sensor->pad);
>> + if (ret < 0)
>> + goto mutex_remove;
>> +
>> + ret = ov5647_detect(sd);
>> + if (ret < 0)
>> + goto error;
>> +
>> + ret = v4l2_async_register_subdev(sd);
>> + if (ret < 0)
>> + goto error;
>> +
>> + return 0;
>> +error:
>> + media_entity_cleanup(&sd->entity);
>> +mutex_remove:
>> + mutex_destroy(&sensor->lock);
>> + return ret;
>> +}
>> +
>> +/**
>> + * @short Exit routine - Exit point of the driver
>> + * @param[in] client pointer to the i2c client structure
>> + * @return 0 on success and a negative number on failure
>> + */
>> +static int ov5647_remove(struct i2c_client *client)
>> +{
>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> + struct ov5647 *ov5647 = to_state(sd);
>> +
>> + v4l2_async_unregister_subdev(&ov5647->sd);
>> + media_entity_cleanup(&ov5647->sd.entity);
>> + v4l2_device_unregister_subdev(sd);
>> + mutex_destroy(&ov5647->lock);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id ov5647_id[] = {
>> + { "ov5647", 0 },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ov5647_id);
>> +
>> +#if IS_ENABLED(CONFIG_OF)
>> +static const struct of_device_id ov5647_of_match[] = {
>> + { .compatible = "ovti,ov5647" },
>> + { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, ov5647_of_match);
>> +#endif
>> +
>> +/**
>> + * @short i2c driver structure
>> + */
>> +static struct i2c_driver ov5647_driver = {
>> + .driver = {
>> + .of_match_table = of_match_ptr(ov5647_of_match),
>> + .owner = THIS_MODULE,
>> + .name = "ov5647",
>> + },
>> + .probe = ov5647_probe,
>> + .remove = ov5647_remove,
>> + .id_table = ov5647_id,
>> +};
>> +
>> +module_i2c_driver(ov5647_driver);
>> +
>> +MODULE_AUTHOR("Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>");
>> +MODULE_DESCRIPTION("A low-level driver for OmniVision ov5647 sensors");
>> +MODULE_LICENSE("GPL v2");
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 1/2] Add OV5647 device tree documentation
From: Ramiro Oliveira @ 2016-12-12 11:39 UTC (permalink / raw)
To: Sakari Ailus, Ramiro Oliveira
Cc: mchehab, linux-kernel, linux-media, robh+dt, devicetree, davem,
gregkh, geert+renesas, akpm, linux, hverkuil, dheitmueller,
slongerbeam, lars, robert.jarzmik, pavel, pali.rohar,
sakari.ailus, mark.rutland, CARLOS.PALMINHA
In-Reply-To: <20161207223319.GZ16630@valkosipuli.retiisi.org.uk>
Hi Sakari,
Thank you for the feedback.
On 12/7/2016 10:33 PM, Sakari Ailus wrote:
> Hi Ramiro,
>
> Thank you for the patch.
>
> On Mon, Dec 05, 2016 at 05:36:33PM +0000, Ramiro Oliveira wrote:
>> Add device tree documentation.
>>
>> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
>> ---
>> .../devicetree/bindings/media/i2c/ov5647.txt | 19 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>> new file mode 100644
>> index 0000000..4c91b3b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>> @@ -0,0 +1,19 @@
>> +Omnivision OV5647 raw image sensor
>> +---------------------------------
>> +
>> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces
>> +and CCI (I2C compatible) control bus.
>> +
>> +Required properties:
>> +
>> +- compatible : "ovti,ov5647";
>> +- reg : I2C slave address of the sensor;
>> +
>> +The common video interfaces bindings (see video-interfaces.txt) should be
>> +used to specify link to the image data receiver. The OV5647 device
>> +node should contain one 'port' child node with an 'endpoint' subnode.
>> +
>> +Following properties are valid for the endpoint node:
>> +
>> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
>> + video-interfaces.txt. The sensor supports only two data lanes.
>
> Doesn't this sensor require a external clock, a reset GPIO and / or a
> regulator or a few? Do you need data-lanes, unless you can change the order
> or the number?
In the setup I'm using, I'm not aware of any reset GPIO or regulator. I do use a
external clock but it's fixed and not controlled by SW. Should I add a property
for this?
>
> An example DT snippet wouldn't hurt.
Sure, I can add a example snippet.
>
^ permalink raw reply
* Re: [PATCH 3/3] power: supply: bq24735-charger: allow chargers to share the ac-detect gpio
From: Peter Rosin @ 2016-12-12 11:39 UTC (permalink / raw)
To: linux-kernel
Cc: Sebastian Reichel, Rob Herring, Mark Rutland, linux-pm,
devicetree
In-Reply-To: <1481540424-19293-4-git-send-email-peda@axentia.se>
On 2016-12-12 12:00, Peter Rosin wrote:
> If several parallel bq24735 chargers have their ac-detect gpios wired
> together (or if only one of the parallel bq24735 chargers have its
> ac-detect pin wired to a gpio, and the others are assumed to react the
> same), then all driver instances need to check the same gpio. But the
> gpio subsystem does not allow sharing gpios, so handle that locally.
>
> However, only do this for the polling case, sharing is not supported if
> the ac detection is handled with interrupts.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> drivers/power/supply/bq24735-charger.c | 101 +++++++++++++++++++++++++++++----
> 1 file changed, 90 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
> index 3765806d5d46..3b21a064a9a7 100644
> --- a/drivers/power/supply/bq24735-charger.c
> +++ b/drivers/power/supply/bq24735-charger.c
> @@ -25,6 +25,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_gpio.h>
> #include <linux/gpio/consumer.h>
> #include <linux/power_supply.h>
> #include <linux/slab.h>
> @@ -43,12 +44,24 @@
> #define BQ24735_MANUFACTURER_ID 0xfe
> #define BQ24735_DEVICE_ID 0xff
>
> +struct bq24735;
> +
> +struct bq24735_shared {
> + struct list_head list;
> + struct bq24735 *owner;
> + struct gpio_desc *status_gpio;
> +};
> +
> +static struct mutex shared_lock;
Aww crap, that should of course be
static DEFINE_MUTEX(shared_lock);
Will fix in v2, but I'll wait for other feedback first. Why is it
impossible to see these things before hitting send?
Cheers,
peda
^ permalink raw reply
* Re: [PATCH v5 1/2] Add OV5647 device tree documentation
From: Sakari Ailus @ 2016-12-12 11:49 UTC (permalink / raw)
To: Ramiro Oliveira
Cc: mchehab-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
dheitmueller-eb9eJ82Ua7k9XoPSrs7Ehg,
slongerbeam-Re5JQEeQqe8AvxtiuMwx3w, lars-Qo5EllUWu/uELgA04lAiVw,
robert.jarzmik-GANU6spQydw, pavel-+ZI9xUNit7I,
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w,
sakari.ailus-VuQAYsv1563Yd54FQh9/CA, mark.rutland-5wv7dgnIgG8,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <cc5229b9-0705-4189-39d5-7c3e0a96c008-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
Hi Ramiro,
On Mon, Dec 12, 2016 at 11:39:31AM +0000, Ramiro Oliveira wrote:
> Hi Sakari,
>
> Thank you for the feedback.
>
> On 12/7/2016 10:33 PM, Sakari Ailus wrote:
> > Hi Ramiro,
> >
> > Thank you for the patch.
> >
> > On Mon, Dec 05, 2016 at 05:36:33PM +0000, Ramiro Oliveira wrote:
> >> Add device tree documentation.
> >>
> >> Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> >> ---
> >> .../devicetree/bindings/media/i2c/ov5647.txt | 19 +++++++++++++++++++
> >> 1 file changed, 19 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
> >> new file mode 100644
> >> index 0000000..4c91b3b
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
> >> @@ -0,0 +1,19 @@
> >> +Omnivision OV5647 raw image sensor
> >> +---------------------------------
> >> +
> >> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces
> >> +and CCI (I2C compatible) control bus.
> >> +
> >> +Required properties:
> >> +
> >> +- compatible : "ovti,ov5647";
> >> +- reg : I2C slave address of the sensor;
> >> +
> >> +The common video interfaces bindings (see video-interfaces.txt) should be
> >> +used to specify link to the image data receiver. The OV5647 device
> >> +node should contain one 'port' child node with an 'endpoint' subnode.
> >> +
> >> +Following properties are valid for the endpoint node:
> >> +
> >> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
> >> + video-interfaces.txt. The sensor supports only two data lanes.
> >
> > Doesn't this sensor require a external clock, a reset GPIO and / or a
> > regulator or a few? Do you need data-lanes, unless you can change the order
> > or the number?
>
> In the setup I'm using, I'm not aware of any reset GPIO or regulator. I do use a
> external clock but it's fixed and not controlled by SW. Should I add a property
> for this?
The sensor datasheet defines a power-up and power-down sequence for the
device. If you don't implement these sequences in the driver on a DT based
system, nothing suggests that they're implemented correctly. Could it be
that the boot loader simply enables the regulators or another device
requires them to be enabled?
I presume at least the reset GPIO should be controlled explicitly in order
to ensure correct function. Although hardware can be surprising: I have one
production system that has no reset GPIO for the sensor albeit the sensor
datasheet says that's part of the power up sequence.
>
> >
> > An example DT snippet wouldn't hurt.
>
> Sure, I can add a example snippet.
>
> >
>
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2] PCI: rockchip: Add quirk to disable RC's ASPM L0s
From: Shawn Lin @ 2016-12-12 11:51 UTC (permalink / raw)
To: Bjorn Helgaas, Rob Herring
Cc: linux-pci-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Wenrui Li,
Brian Norris, Jeffy Chen, devicetree-u79uwXL29TY76Z2rM5mHXA,
Shawn Lin
Rockchip's RC outputs 100MHz reference clock but there are
two methods for PHY to generate it.
(1)One of them is to use system PLL to generate 100MHz clock and
the PHY will relock it and filter signal noise then outputs the
reference clock.
(2)Another way is to share Soc's 24MHZ crystal oscillator with
PHY and force PHY's DLL to generate 100MHz internally.
When using case(2), the exit from L0s doesn't work fine occasionally
due to the broken design of RC receiver's logical circuit. So even if
we use extended-synch, it still fails for PHY to relock the bits from
FTS sometimes. This will hang the system.
Maybe we could argue that why not use case(1) to avoid it? The reason
is that as we could see the reference clock is derived from system PLL
and the path from it to PHY isn't so clean which means there are some
noise introduced by power-domain and other buses can't be filterd out
by PHY and we could see noise from the frequency spectrum by
oscilloscope. This makes the TX compatibility test a little difficult
to pass the spec. So case(1) and case(2) are both used indeed now. If
using case(2), we should disable RC's L0s support, and that is why we
need this property to indicate this quirk.
Also after checking quirk.c, I noticed there is already a quirk for
disabling L0s unconditionally, quirk_disable_aspm_l0s. But obviously we
shouldn't do that as mentioned above that case(1) could still works fine
with L0s.
Reported-by: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Cc: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v2:
- drop the quirk prefix
Documentation/devicetree/bindings/pci/rockchip-pcie.txt | 2 ++
drivers/pci/host/pcie-rockchip.c | 9 +++++++++
2 files changed, 11 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
index 71aeda1..1453a73 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
@@ -43,6 +43,8 @@ Required properties:
- interrupt-map-mask and interrupt-map: standard PCI properties
Optional Property:
+- aspm-no-l0s: RC won't support ASPM L0s. This property is needed if
+ using 24MHz OSC for RC's PHY.
- ep-gpios: contain the entry for pre-reset gpio
- num-lanes: number of lanes to use
- vpcie3v3-supply: The phandle to the 3.3v regulator to use for PCIe.
diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index f2dca7b..35988fc 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -140,6 +140,8 @@
#define PCIE_RC_CONFIG_DCR_CSPL_SHIFT 18
#define PCIE_RC_CONFIG_DCR_CSPL_LIMIT 0xff
#define PCIE_RC_CONFIG_DCR_CPLS_SHIFT 26
+#define PCIE_RC_CONFIG_LINK_CAP (PCIE_RC_CONFIG_BASE + 0xcc)
+#define PCIE_RC_CONFIG_LINK_CAP_L0S BIT(10)
#define PCIE_RC_CONFIG_LCS (PCIE_RC_CONFIG_BASE + 0xd0)
#define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)
#define PCIE_RC_CONFIG_THP_CAP (PCIE_RC_CONFIG_BASE + 0x274)
@@ -653,6 +655,13 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
status &= ~PCIE_RC_CONFIG_THP_CAP_NEXT_MASK;
rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_THP_CAP);
+ /* Clear L0s from RC's link cap */
+ if (of_property_read_bool(dev->of_node, "apsm-no-l0s")) {
+ status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LINK_CAP);
+ status &= ~PCIE_RC_CONFIG_LINK_CAP_L0S;
+ rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LINK_CAP);
+ }
+
rockchip_pcie_write(rockchip, 0x0, PCIE_RC_BAR_CONF);
rockchip_pcie_write(rockchip,
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v5 2/2] Add support for OV5647 sensor
From: Sakari Ailus @ 2016-12-12 11:54 UTC (permalink / raw)
To: Ramiro Oliveira
Cc: mchehab-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
dheitmueller-eb9eJ82Ua7k9XoPSrs7Ehg,
slongerbeam-Re5JQEeQqe8AvxtiuMwx3w, lars-Qo5EllUWu/uELgA04lAiVw,
robert.jarzmik-GANU6spQydw, pavel-+ZI9xUNit7I,
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w,
sakari.ailus-VuQAYsv1563Yd54FQh9/CA, mark.rutland-5wv7dgnIgG8,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <936d4f19-9a1a-0873-121e-b9471449f70d-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
Hi Ramiro,
On Mon, Dec 12, 2016 at 11:39:13AM +0000, Ramiro Oliveira wrote:
> Hi Sakari
>
> On 12/7/2016 11:01 PM, Sakari Ailus wrote:
> > Hi Ramiro,
> >
> > On Mon, Dec 05, 2016 at 05:36:34PM +0000, Ramiro Oliveira wrote:
> >> Add support for OV5647 sensor.
> >>
> >> Modes supported:
> >> - 640x480 RAW 8
> >>
> >> Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> >> ---
> >> MAINTAINERS | 7 +
> >> drivers/media/i2c/Kconfig | 12 +
> >> drivers/media/i2c/Makefile | 1 +
> >> drivers/media/i2c/ov5647.c | 866 +++++++++++++++++++++++++++++++++++++++++++++
> >> 4 files changed, 886 insertions(+)
> >> create mode 100644 drivers/media/i2c/ov5647.c
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 52cc077..72e828a 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -8923,6 +8923,13 @@ M: Harald Welte <laforge-TgoAw6mPHtdg9hUCZPvPmw@public.gmane.org>
> >> S: Maintained
> >> F: drivers/char/pcmcia/cm4040_cs.*
> >>
> >> +OMNIVISION OV5647 SENSOR DRIVER
> >> +M: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> >> +L: linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> +T: git git://linuxtv.org/media_tree.git
> >> +S: Maintained
> >> +F: drivers/media/i2c/ov5647.c
> >> +
> >> OMNIVISION OV7670 SENSOR DRIVER
> >> M: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
> >> L: linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> >> index b31fa6f..c1b78e5 100644
> >> --- a/drivers/media/i2c/Kconfig
> >> +++ b/drivers/media/i2c/Kconfig
> >> @@ -531,6 +531,18 @@ config VIDEO_OV2659
> >> To compile this driver as a module, choose M here: the
> >> module will be called ov2659.
> >>
> >> +config VIDEO_OV5647
> >> + tristate "OmniVision OV5647 sensor support"
> >> + depends on OF
> >
> > How does this driver depend on OF, other than matching the compatible
> > string?
> >
>
> It doesn't, should I proceed diferently?
You should drop the dependency if you don't need it. But I bet you will
actually need it.
>
> >> + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> >> + depends on MEDIA_CAMERA_SUPPORT
> >> + ---help---
> >> + This is a Video4Linux2 sensor-level driver for the OmniVision
> >> + OV5647 camera.
> >> +
> >> + To compile this driver as a module, choose M here: the
> >> + module will be called ov5647.
> >> +
> >> config VIDEO_OV7640
> >> tristate "OmniVision OV7640 sensor support"
> >> depends on I2C && VIDEO_V4L2
> >> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> >> index 92773b2..0d9014c 100644
> >> --- a/drivers/media/i2c/Makefile
> >> +++ b/drivers/media/i2c/Makefile
> >> @@ -82,3 +82,4 @@ obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o
> >> obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o
> >> obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
> >> obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
> >> +obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
> >> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> >> new file mode 100644
> >> index 0000000..2aae806
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/ov5647.c
> >> @@ -0,0 +1,866 @@
> >> +/*
> >> + * A V4L2 driver for OmniVision OV5647 cameras.
> >> + *
> >> + * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driver
> >> + * Copyright (C) 2011 Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> >> + *
> >> + * Based on Omnivision OV7670 Camera Driver
> >> + * Copyright (C) 2006-7 Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
> >> + *
> >> + * Copyright (C) 2016, Synopsys, Inc.
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU General Public License as
> >> + * published by the Free Software Foundation version 2.
> >> + *
> >> + * This program is distributed .as is. WITHOUT ANY WARRANTY of any
> >> + * kind, whether express or implied; without even the implied warranty
> >> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> + * GNU General Public License for more details.
> >> + */
> >> +#include <linux/init.h>
> >> +#include <linux/module.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/i2c.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/videodev2.h>
> >> +#include <media/v4l2-device.h>
> >> +#include <media/v4l2-ctrls.h>
> >> +#include <media/v4l2-mediabus.h>
> >> +#include <media/v4l2-image-sizes.h>
> >> +#include <media/v4l2-of.h>
> >> +#include <linux/io.h>
> >
> > Alphabetical order, please.
> >
> >> +
> >> +#define SENSOR_NAME "ov5647"
> >> +
> >> +#define OV5647_SW_RESET 0x1003
> >> +#define OV5647_REG_CHIPID_H 0x300A
> >> +#define OV5647_REG_CHIPID_L 0x300B
> >> +
> >> +#define REG_TERM 0xfffe
> >> +#define VAL_TERM 0xfe
> >> +#define REG_DLY 0xffff
> >> +
> >> +#define OV5647_ROW_START 0x01
> >> +#define OV5647_ROW_START_MIN 0
> >> +#define OV5647_ROW_START_MAX 2004
> >> +#define OV5647_ROW_START_DEF 54
> >> +
> >> +#define OV5647_COLUMN_START 0x02
> >> +#define OV5647_COLUMN_START_MIN 0
> >> +#define OV5647_COLUMN_START_MAX 2750
> >> +#define OV5647_COLUMN_START_DEF 16
> >> +
> >> +#define OV5647_WINDOW_HEIGHT 0x03
> >> +#define OV5647_WINDOW_HEIGHT_MIN 2
> >> +#define OV5647_WINDOW_HEIGHT_MAX 2006
> >> +#define OV5647_WINDOW_HEIGHT_DEF 1944
> >> +
> >> +#define OV5647_WINDOW_WIDTH 0x04
> >> +#define OV5647_WINDOW_WIDTH_MIN 2
> >> +#define OV5647_WINDOW_WIDTH_MAX 2752
> >> +#define OV5647_WINDOW_WIDTH_DEF 2592
> >> +
> >> +struct regval_list {
> >> + u16 addr;
> >> + u8 data;
> >> +};
> >> +
> >> +struct cfg_array {
> >> + struct regval_list *regs;
> >> + int size;
> >> +};
> >> +
> >> +struct sensor_win_size {
> >> + int width;
> >> + int height;
> >> + unsigned int hoffset;
> >> + unsigned int voffset;
> >> + unsigned int hts;
> >> + unsigned int vts;
> >> + unsigned int pclk;
> >> + unsigned int mipi_bps;
> >> + unsigned int fps_fixed;
> >> + unsigned int bin_factor;
> >> + unsigned int intg_min;
> >> + unsigned int intg_max;
> >> + void *regs;
> >> + int regs_size;
> >> + int (*set_size)(struct v4l2_subdev *sd);
> >> +};
> >> +
> >> +
> >> +struct ov5647 {
> >> + struct device *dev;
> >> + struct v4l2_subdev sd;
> >> + struct media_pad pad;
> >> + struct mutex lock;
> >> + struct v4l2_mbus_framefmt format;
> >> + struct sensor_format_struct *fmt;
> >> + unsigned int width;
> >> + unsigned int height;
> >> + unsigned int capture_mode;
> >> + int hue;
> >
> > At least capture_mode and hue are unused. Please remove unused fields.
> >
> >> + struct v4l2_fract tpf;
> >> + struct sensor_win_size *current_wins;
> >> +};
> >> +
> >> +static inline struct ov5647 *to_state(struct v4l2_subdev *sd)
> >> +{
> >> + return container_of(sd, struct ov5647, sd);
> >> +}
> >> +
> >> +static struct regval_list sensor_oe_disable_regs[] = {
> >> + {0x3000, 0x00},
> >> + {0x3001, 0x00},
> >> + {0x3002, 0x00},
> >> +};
> >> +
> >> +static struct regval_list sensor_oe_enable_regs[] = {
> >> + {0x3000, 0x0f},
> >> + {0x3001, 0xff},
> >> + {0x3002, 0xe4},
> >> +};
> >> +
> >> +static struct regval_list ov5647_640x480[] = {
> >
> > Does this list expect a certain external clock frequency? If it does, should
> > you check that the actual frequency matches with the expectation?
> >
>
> Yes. But like I said in the DT patch the external clock has a fixed frequency. I
> can add a check if you thinks it's better.
>
> >> + {0x0100, 0x00},
> >> + {0x0103, 0x01},
> >> + {0x3034, 0x08},
> >> + {0x3035, 0x21},
> >> + {0x3036, 0x46},
> >> + {0x303c, 0x11},
> >> + {0x3106, 0xf5},
> >> + {0x3821, 0x07},
> >> + {0x3820, 0x41},
> >> + {0x3827, 0xec},
> >> + {0x370c, 0x0f},
> >> + {0x3612, 0x59},
> >> + {0x3618, 0x00},
> >> + {0x5000, 0x06},
> >> + {0x5001, 0x01},
> >> + {0x5002, 0x41},
> >> + {0x5003, 0x08},
> >> + {0x5a00, 0x08},
> >> + {0x3000, 0x00},
> >> + {0x3001, 0x00},
> >> + {0x3002, 0x00},
> >> + {0x3016, 0x08},
> >> + {0x3017, 0xe0},
> >> + {0x3018, 0x44},
> >> + {0x301c, 0xf8},
> >> + {0x301d, 0xf0},
> >> + {0x3a18, 0x00},
> >> + {0x3a19, 0xf8},
> >> + {0x3c01, 0x80},
> >> + {0x3b07, 0x0c},
> >> + {0x380c, 0x07},
> >> + {0x380d, 0x68},
> >> + {0x380e, 0x03},
> >> + {0x380f, 0xd8},
> >> + {0x3814, 0x31},
> >> + {0x3815, 0x31},
> >> + {0x3708, 0x64},
> >> + {0x3709, 0x52},
> >> + {0x3808, 0x02},
> >> + {0x3809, 0x80},
> >> + {0x380a, 0x01},
> >> + {0x380b, 0xE0},
> >> + {0x3801, 0x00},
> >> + {0x3802, 0x00},
> >> + {0x3803, 0x00},
> >> + {0x3804, 0x0a},
> >> + {0x3805, 0x3f},
> >> + {0x3806, 0x07},
> >> + {0x3807, 0xa1},
> >> + {0x3811, 0x08},
> >> + {0x3813, 0x02},
> >> + {0x3630, 0x2e},
> >> + {0x3632, 0xe2},
> >> + {0x3633, 0x23},
> >> + {0x3634, 0x44},
> >> + {0x3636, 0x06},
> >> + {0x3620, 0x64},
> >> + {0x3621, 0xe0},
> >> + {0x3600, 0x37},
> >> + {0x3704, 0xa0},
> >> + {0x3703, 0x5a},
> >> + {0x3715, 0x78},
> >> + {0x3717, 0x01},
> >> + {0x3731, 0x02},
> >> + {0x370b, 0x60},
> >> + {0x3705, 0x1a},
> >> + {0x3f05, 0x02},
> >> + {0x3f06, 0x10},
> >> + {0x3f01, 0x0a},
> >> + {0x3a08, 0x01},
> >> + {0x3a09, 0x27},
> >> + {0x3a0a, 0x00},
> >> + {0x3a0b, 0xf6},
> >> + {0x3a0d, 0x04},
> >> + {0x3a0e, 0x03},
> >> + {0x3a0f, 0x58},
> >> + {0x3a10, 0x50},
> >> + {0x3a1b, 0x58},
> >> + {0x3a1e, 0x50},
> >> + {0x3a11, 0x60},
> >> + {0x3a1f, 0x28},
> >> + {0x4001, 0x02},
> >> + {0x4004, 0x02},
> >> + {0x4000, 0x09},
> >> + {0x4837, 0x24},
> >> + {0x4050, 0x6e},
> >> + {0x4051, 0x8f},
> >> + {0x0100, 0x01},
> >> +};
> >> +
> >> +struct sensor_format_struct;
> >> +
> >> +/**
> >> + * @short I2C Write operation
> >> + * @param[in] i2c_client I2C client
> >> + * @param[in] reg register address
> >> + * @param[in] val value to write
> >> + * @return Error code
> >> + */
> >> +static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
> >> +{
> >> + int ret;
> >> + unsigned char data[3] = { reg >> 8, reg & 0xff, val};
> >> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> +
> >> + ret = i2c_master_send(client, data, 3);
> >> + if (ret != 3) {
> >> + dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
> >> + __func__, reg);
> >> + return ret < 0 ? ret : -EIO;
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * @short I2C Read operation
> >> + * @param[in] i2c_client I2C client
> >> + * @param[in] reg register address
> >> + * @param[out] val value read
> >> + * @return Error code
> >> + */
> >> +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
> >> +{
> >> + int ret;
> >> + unsigned char data_w[2] = { reg >> 8, reg & 0xff };
> >> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> +
> >> +
> >> + ret = i2c_master_send(client, data_w, 2);
> >> +
> >> + if (ret < 2) {
> >> + dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
> >> + __func__, reg);
> >> + return ret < 0 ? ret : -EIO;
> >> + }
> >> +
> >> +
> >> + ret = i2c_master_recv(client, val, 1);
> >> +
> >> + if (ret < 1) {
> >> + dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
> >> + __func__, reg);
> >> + return ret < 0 ? ret : -EIO;
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +static int ov5647_write_array(struct v4l2_subdev *sd,
> >> + struct regval_list *regs, int array_size)
> >> +{
> >> + int i = 0;
> >> + int ret = 0;
> >> +
> >> + if (!regs)
> >> + return -EINVAL;
> >> +
> >> + while (i < array_size) {
> >> + ret = ov5647_write(sd, regs->addr, regs->data);
> >> + if (ret < 0)
> >> + return ret;
> >> + i++;
> >> + regs++;
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +static void ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
> >> +{
> >> + u8 channel_id;
> >> +
> >> + ov5647_read(sd, 0x4814, &channel_id);
> >> + channel_id &= ~(3 << 6);
> >> + ov5647_write(sd, 0x4814, channel_id | (channel << 6));
> >> +}
> >> +
> >> +void ov5647_stream_on(struct v4l2_subdev *sd)
> >> +{
> >> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> +
> >> + ov5647_write(sd, 0x4202, 0x00);
> >> + dev_dbg(&client->dev, "Stream on");
> >> + ov5647_write(sd, 0x300D, 0x00);
> >> +}
> >> +
> >> +void ov5647_stream_off(struct v4l2_subdev *sd)
> >> +{
> >> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> +
> >> + ov5647_write(sd, 0x4202, 0x0f);
> >> + dev_dbg(&client->dev, "Stream off");
> >> + ov5647_write(sd, 0x300D, 0x01);
> >> +}
> >> +
> >> +/****************************************************************************/
> >> +
> >> +/**
> >> + * @short Set SW standby
> >> + * @param[in] sd v4l2 sd
> >> + * @param[in] stanby standby mode status (on or off)
> >> + * @return Error code
> >> + */
> >> +static int set_sw_standby(struct v4l2_subdev *sd, bool standby)
> >> +{
> >> + int ret;
> >> + unsigned char rdval;
> >> +
> >> + ret = ov5647_read(sd, 0x0100, &rdval);
> >> + if (ret != 0)
> >> + return ret;
> >> +
> >> + if (standby)
> >> + rdval &= 0xfe;
> >> + else
> >> + rdval |= 0x01;
> >> +
> >> + ret = ov5647_write(sd, 0x0100, rdval);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +/**
> >> + * @short Store information about the video data format.
> >> + */
> >> +static struct sensor_format_struct {
> >> + u8 *desc;
> >> + u32 mbus_code;
> >> + struct regval_list *regs;
> >> + int regs_size;
> >> + int bpp;
> >
> > At least desc and bpp are unused.
> >
> >> +} sensor_formats[] = {
> >> + {
> >> + .desc = "Raw RGB Bayer",
> >> + .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> >> + .regs = ov5647_640x480,
> >> + .regs_size = ARRAY_SIZE(ov5647_640x480),
> >> + .bpp = 1
> >> + },
> >> +};
> >> +#define N_FMTS ARRAY_SIZE(sensor_formats)
> >> +
> >> +/* ----------------------------------------------------------------------- */
> >> +
> >> +/**
> >> + * @short Initialize sensor
> >> + * @param[in] sd v4l2 subdev
> >> + * @param[in] val not used
> >> + * @return Error code
> >> + */
> >> +static int __sensor_init(struct v4l2_subdev *sd)
> >> +{
> >> + int ret;
> >> + u8 resetval;
> >> + u8 rdval;
> >> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> +
> >> + dev_dbg(&client->dev, "sensor init\n");
> >> +
> >> + ret = ov5647_read(sd, 0x0100, &rdval);
> >> + if (ret != 0)
> >> + return ret;
> >> +
> >> + ov5647_write(sd, 0x4800, 0x25);
> >> + ov5647_stream_off(sd);
> >> +
> >> + ret = ov5647_write_array(sd, ov5647_640x480,
> >> + ARRAY_SIZE(ov5647_640x480));
> >> + if (ret < 0) {
> >> + dev_err(&client->dev, "write sensor_default_regs error\n");
> >> + return ret;
> >> + }
> >> +
> >> + ov5647_set_virtual_channel(sd, 0);
> >> +
> >> + ov5647_read(sd, 0x0100, &resetval);
> >> + if (!(resetval & 0x01)) {
> >> + dev_err(&client->dev, "Device was in SW standby");
> >> + ov5647_write(sd, 0x0100, 0x01);
> >> + }
> >> +
> >> + ov5647_write(sd, 0x4800, 0x04);
> >> + ov5647_stream_on(sd);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * @short Control sensor power state
> >> + * @param[in] sd v4l2 subdev
> >> + * @param[in] on Sensor power
> >> + * @return Error code
> >> + */
> >> +static int sensor_power(struct v4l2_subdev *sd, int on)
> >> +{
> >> + int ret;
> >> + struct ov5647 *ov5647 = to_state(sd);
> >> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> +
> >> + ret = 0;
> >> + mutex_lock(&ov5647->lock);
> >> +
> >> + if (on) {
> >> + dev_dbg(&client->dev, "OV5647 power on\n");
> >> +
> >> + ret = ov5647_write_array(sd, sensor_oe_enable_regs,
> >> + ARRAY_SIZE(sensor_oe_enable_regs));
> >> +
> >> + ret = __sensor_init(sd);
> >> +
> >> + if (ret < 0)
> >> + dev_err(&client->dev,
> >> + "Camera not available, check Power\n");
> >> + } else {
> >> + dev_dbg(&client->dev, "OV5647 power off\n");
> >> +
> >> + dev_dbg(&client->dev, "disable oe\n");
> >> + ret = ov5647_write_array(sd, sensor_oe_disable_regs,
> >> + ARRAY_SIZE(sensor_oe_disable_regs));
> >> +
> >> + if (ret < 0)
> >> + dev_dbg(&client->dev, "disable oe failed\n");
> >> +
> >> + ret = set_sw_standby(sd, true);
> >> +
> >> + if (ret < 0)
> >> + dev_dbg(&client->dev, "soft stby failed\n");
> >> +
> >> + }
> >> +
> >> + mutex_unlock(&ov5647->lock);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> >> +/**
> >> + * @short Get register value
> >> + * @param[in] sd v4l2 subdev
> >> + * @param[in] reg register struct
> >> + * @return Error code
> >> + */
> >> +static int sensor_get_register(struct v4l2_subdev *sd,
> >> + struct v4l2_dbg_register *reg)
> >> +{
> >> + unsigned char val = 0;
> >> + int ret;
> >> +
> >> + ret = ov5647_read(sd, reg->reg & 0xff, &val);
> >> + if (ret != 0)
> >> + return ret;
> >> +
> >> + reg->val = val;
> >> + reg->size = 1;
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +/**
> >> + * @short Set register value
> >> + * @param[in] sd v4l2 subdev
> >> + * @param[in] reg register struct
> >> + * @return Error code
> >> + */
> >> +static int sensor_set_register(struct v4l2_subdev *sd,
> >> + const struct v4l2_dbg_register *reg)
> >> +{
> >> + return ov5647_write(sd, reg->reg & 0xff, reg->val & 0xff);
> >> +}
> >> +#endif
> >> +
> >> +/* ----------------------------------------------------------------------- */
> >> +
> >> +/**
> >> + * @short Subdev core operations registration
> >> + */
> >> +static const struct v4l2_subdev_core_ops sensor_core_ops = {
> >> + .s_power = sensor_power,
> >
> > The s_power() op will be called by the bridge (ISP) driver as well. You
> > should expect that it may be enabled more than once before being disabled
> > equal number of times.
> >
>
> Should I add an IF check to verify if the sensor is powered on before running
> the power on/off routine.
You need a counter. One way to do that is in the driver itself, or you can
do what I recently did in the smiapp driver, using runtime PM so you don't
explicitly need to manage it:
<URL:https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media/i2c/smiapp/smiapp-core.c?h=smiapp-pm&id=c29df33f9ec94226eab8ee92d8c66ab83c76659a>
Or, before the runtime PM conversion:
<URL:https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media/i2c/smiapp/smiapp-core.c?h=smiapp-pm&id=c8d2bc9bc39ebea8437fd974fdbc21847bb897a3>
The use of runtime PM in sub-device drivers is new and I'm not entirely
happy how it's implemented in the smiapp driver right now, hence it might be
better not to use it (yet). Up to you.
>
> >> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> >> + .g_register = sensor_get_register,
> >> + .s_register = sensor_set_register,
> >> +#endif
> >> +};
> >> +
> >> +/* ----------------------------------------------------------------------- */
> >> +
> >> +
> >> +
> >> +/**
> >> + * @short Enumerate available image formats
> >> + * @param[in] sd v4l2 subdev
> >> + * @param[in] index index
> >> + * @param[in] code MBUS Pixel code
> >> + * @return Error code
> >> + */
> >> +static int sensor_enum_fmt(struct v4l2_subdev *sd,
> >> + struct v4l2_subdev_pad_config *cfg,
> >> + struct v4l2_subdev_mbus_code_enum *code)
> >> +{
> >> + if (code->pad || code->index >= N_FMTS)
> >> + return -EINVAL;
> >> +
> >> + code->code = sensor_formats[code->index].mbus_code;
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * @short Try frame format internal function
> >> + * @param[in] sd v4l2 subdev
> >> + * @param[in] fmt frame format
> >> + * @return Error code
> >> + */
> >> +static int sensor_try_fmt_internal(struct v4l2_subdev *sd,
> >> + struct v4l2_mbus_framefmt *fmt, struct sensor_format_struct **ret_fmt,
> >> + struct sensor_win_size **ret_wsize)
> >> +{
> >> + int index;
> >> +
> >> + for (index = 0; index < N_FMTS; index++)
> >> + if (sensor_formats[index].mbus_code == fmt->code)
> >> + break;
> >> +
> >> + if (index >= N_FMTS)
> >> + return -EINVAL;
> >> +
> >> + if (ret_fmt != NULL)
> >> + *ret_fmt = sensor_formats + index;
> >> +
> >> + fmt->field = V4L2_FIELD_NONE;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * @short Set frame format
> >> + * @param[in] sd v4l2 subdev
> >> + * @param[in] fmt frame format
> >> + * @return Error code
> >> + */
> >> +static int sensor_s_fmt(struct v4l2_subdev *sd,
> >> + struct v4l2_subdev_pad_config *cfg,
> >> + struct v4l2_subdev_format *fmt)
> >> +{
> >> + int ret;
> >> + struct sensor_format_struct *sensor_fmt;
> >> + struct sensor_win_size *wsize;
> >> + struct ov5647 *info = to_state(sd);
> >> +
> >> + ov5647_write_array(sd, sensor_oe_disable_regs,
> >> + ARRAY_SIZE(sensor_oe_disable_regs));
> >
> > Should you check the error code here?
> >
> >> +
> >> + ret = sensor_try_fmt_internal(sd, &fmt->format,
> >> + &sensor_fmt, &wsize);
> >
> > Do you set wsize somewhere?
> >
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ov5647_write_array(sd, sensor_fmt->regs, sensor_fmt->regs_size);
> >
> > And here.
> >
> >> +
> >> + ret = 0;
> >
> > ret was already zero.
> >
> >> +
> >> + if (wsize->regs)
> >> + ov5647_write_array(sd, wsize->regs, wsize->regs_size);
> >> +
> >> + if (wsize->set_size)
> >> + wsize->set_size(sd);
> >> +
> >> + info->fmt = sensor_fmt;
> >> + info->width = wsize->width;
> >> + info->height = wsize->height;
> >> +
> >> + ov5647_write_array(sd, sensor_oe_enable_regs,
> >> + ARRAY_SIZE(sensor_oe_enable_regs));
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * @short Set stream parameters
> >> + * @param[in] sd v4l2 subdev
> >> + * @param[in] parms stream parameters
> >> + * @return Error code
> >> + */
> >> +static int sensor_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> >> +{
> >> + struct v4l2_captureparm *cp = &parms->parm.capture;
> >> + struct ov5647 *info = to_state(sd);
> >> +
> >> + if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >> + return -EINVAL;
> >> +
> >> + if (info->tpf.numerator == 0)
> >> + return -EINVAL;
> >> +
> >> + info->capture_mode = cp->capturemode;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * @short Get stream parameters
> >> + * @param[in] sd v4l2 subdev
> >> + * @param[in] parms stream parameters
> >> + * @return Error code
> >> + */
> >> +static int sensor_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> >> +{
> >> + struct v4l2_captureparm *cp = &parms->parm.capture;
> >> + struct ov5647 *info = to_state(sd);
> >> +
> >> + if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >> + return -EINVAL;
> >> +
> >> + memset(cp, 0, sizeof(struct v4l2_captureparm));
> >> + cp->capability = V4L2_CAP_TIMEPERFRAME;
> >> + cp->capturemode = info->capture_mode;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * @short Subdev video operations registration
> >> + *
> >> + */
> >> +static const struct v4l2_subdev_video_ops sensor_video_ops = {
> >> + .s_parm = sensor_s_parm,
> >> + .g_parm = sensor_g_parm,
> >
> > Please use the g/s_frame_interval() instead. That's what sub-device drivers
> > generally use for the purpose.
> >
> >> +};
> >> +
> >> +/* ----------------------------------------------------------------------- */
> >> +
> >> +/**
> >> + * @short Subdev operations registration
> >> + *
> >> + */
> >> +static const struct v4l2_subdev_ops subdev_ops = {
> >> + .core = &sensor_core_ops,
> >> + .video = &sensor_video_ops,
> >> +};
> >> +
> >> +/* -----------------------------------------------------------------------------
> >> + * V4L2 subdev internal operations
> >> + */
> >> +
> >> +/**
> >> + * @short Detect camera version and model
> >> + * @param[in] sd v4l2 subdev
> >> + * @return Error code
> >> + */
> >> +int ov5647_detect(struct v4l2_subdev *sd)
> >> +{
> >> + unsigned char v;
> >> + int ret;
> >> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> +
> >> + ret = ov5647_write(sd, OV5647_SW_RESET, 0x01);
> >> + if (ret < 0)
> >> + return ret;
> >> + ret = ov5647_read(sd, OV5647_REG_CHIPID_H, &v);
> >> + if (ret < 0)
> >> + return ret;
> >> + if (v != 0x56) {
> >> + dev_err(&client->dev, "Wrong model version detected");
> >> + return -ENODEV;
> >> + }
> >> + ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &v);
> >> + if (ret < 0)
> >> + return ret;
> >> + if (v != 0x47) {
> >> + dev_err(&client->dev, "Wrong model version detected");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + ret = ov5647_write(sd, OV5647_SW_RESET, 0x00);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * @short Detect if camera is registered
> >> + * @param[in] sd v4l2 subdev
> >> + * @return Error code
> >> + */
> >> +static int ov5647_registered(struct v4l2_subdev *sd)
> >> +{
> >> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> +
> >> + dev_info(&client->dev, "OV5647 detected at address 0x%02x\n",
> >> + client->addr);
> >
> > I might omit this. If there's a need to debug this then the information
> > should be printed by the framework instead using debug level messages.
> >
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * @short Open device
> >> + * @param[in] sd v4l2 subdev
> >> + * @param[in] fh v4l2 file handler
> >> + * @return Error code
> >> + */
> >> +static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> >> +{
> >> + struct v4l2_mbus_framefmt *format =
> >> + v4l2_subdev_get_try_format(sd, fh->pad, 0);
> >> + struct v4l2_rect *crop =
> >> + v4l2_subdev_get_try_crop(sd, fh->pad, 0);
> >> +
> >> + crop->left = OV5647_COLUMN_START_DEF;
> >> + crop->top = OV5647_ROW_START_DEF;
> >> + crop->width = OV5647_WINDOW_WIDTH_DEF;
> >> + crop->height = OV5647_WINDOW_HEIGHT_DEF;
> >> +
> >> + format->code = MEDIA_BUS_FMT_SBGGR8_1X8;
> >> +
> >> + format->width = OV5647_WINDOW_WIDTH_DEF;
> >> + format->height = OV5647_WINDOW_HEIGHT_DEF;
> >> + format->field = V4L2_FIELD_NONE;
> >> + format->colorspace = V4L2_COLORSPACE_SRGB;
> >> +
> >> + return sensor_power(sd, true);
> >> +}
> >> +
> >> +/**
> >> + * @short Open device
> >> + * @param[in] sd v4l2 subdev
> >> + * @param[in] fh v4l2 file handler
> >> + * @return Error code
> >> + */
> >> +static int ov5647_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> >> +{
> >> + return sensor_power(sd, false);
> >> +}
> >> +
> >> +/**
> >> + * @short Subdev internal operations registration
> >> + *
> >> + */
> >> +static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
> >> + .registered = ov5647_registered,
> >> + .open = ov5647_open,
> >> + .close = ov5647_close,
> >> +};
> >> +
> >> +/**
> >> + * @short Initialization routine - Entry point of the driver
> >> + * @param[in] client pointer to the i2c client structure
> >> + * @param[in] id pointer to the i2c device id structure
> >> + * @return 0 on success and a negative number on failure
> >> + */
> >> +static int ov5647_probe(struct i2c_client *client,
> >> + const struct i2c_device_id *id)
> >> +{
> >> + struct device *dev = &client->dev;
> >> + struct ov5647 *sensor;
> >> + int ret = 0;
> >
> > No need to initialise ret here.
> >
> >> + struct v4l2_subdev *sd;
> >> +
> >> + dev_info(&client->dev, "Installing OmniVision OV5647 camera driver\n");
> >> +
> >> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> >> + if (sensor == NULL)
> >> + return -ENOMEM;
> >> +
> >> + mutex_init(&sensor->lock);
> >> + sensor->dev = dev;
> >> +
> >> + sd = &sensor->sd;
> >> + v4l2_i2c_subdev_init(sd, client, &subdev_ops);
> >> + sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >> +
> >> + sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> >> + sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
> >> + ret = media_entity_pads_init(&sd->entity, 1, &sensor->pad);
> >> + if (ret < 0)
> >> + goto mutex_remove;
> >> +
> >> + ret = ov5647_detect(sd);
> >> + if (ret < 0)
> >> + goto error;
> >> +
> >> + ret = v4l2_async_register_subdev(sd);
> >> + if (ret < 0)
> >> + goto error;
> >> +
> >> + return 0;
> >> +error:
> >> + media_entity_cleanup(&sd->entity);
> >> +mutex_remove:
> >> + mutex_destroy(&sensor->lock);
> >> + return ret;
> >> +}
> >> +
> >> +/**
> >> + * @short Exit routine - Exit point of the driver
> >> + * @param[in] client pointer to the i2c client structure
> >> + * @return 0 on success and a negative number on failure
> >> + */
> >> +static int ov5647_remove(struct i2c_client *client)
> >> +{
> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> + struct ov5647 *ov5647 = to_state(sd);
> >> +
> >> + v4l2_async_unregister_subdev(&ov5647->sd);
> >> + media_entity_cleanup(&ov5647->sd.entity);
> >> + v4l2_device_unregister_subdev(sd);
> >> + mutex_destroy(&ov5647->lock);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct i2c_device_id ov5647_id[] = {
> >> + { "ov5647", 0 },
> >> + { }
> >> +};
> >> +MODULE_DEVICE_TABLE(i2c, ov5647_id);
> >> +
> >> +#if IS_ENABLED(CONFIG_OF)
> >> +static const struct of_device_id ov5647_of_match[] = {
> >> + { .compatible = "ovti,ov5647" },
> >> + { /* sentinel */ },
> >> +};
> >> +MODULE_DEVICE_TABLE(of, ov5647_of_match);
> >> +#endif
> >> +
> >> +/**
> >> + * @short i2c driver structure
> >> + */
> >> +static struct i2c_driver ov5647_driver = {
> >> + .driver = {
> >> + .of_match_table = of_match_ptr(ov5647_of_match),
> >> + .owner = THIS_MODULE,
> >> + .name = "ov5647",
> >> + },
> >> + .probe = ov5647_probe,
> >> + .remove = ov5647_remove,
> >> + .id_table = ov5647_id,
> >> +};
> >> +
> >> +module_i2c_driver(ov5647_driver);
> >> +
> >> +MODULE_AUTHOR("Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>");
> >> +MODULE_DESCRIPTION("A low-level driver for OmniVision ov5647 sensors");
> >> +MODULE_LICENSE("GPL v2");
> >
--
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 1/2] Add OV5647 device tree documentation
From: Ramiro Oliveira @ 2016-12-12 12:15 UTC (permalink / raw)
To: Sakari Ailus, Ramiro Oliveira
Cc: mchehab-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
dheitmueller-eb9eJ82Ua7k9XoPSrs7Ehg,
slongerbeam-Re5JQEeQqe8AvxtiuMwx3w, lars-Qo5EllUWu/uELgA04lAiVw,
robert.jarzmik-GANU6spQydw, pavel-+ZI9xUNit7I,
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w,
sakari.ailus-VuQAYsv1563Yd54FQh9/CA, mark.rutland-5wv7dgnIgG8,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <20161212114900.GS16630-S+BSfZ9RZZmRSg0ZkenSGLdO1Tsj/99ntUK59QYPAWc@public.gmane.org>
Hi Sakari
On 12/12/2016 11:49 AM, Sakari Ailus wrote:
> Hi Ramiro,
>
> On Mon, Dec 12, 2016 at 11:39:31AM +0000, Ramiro Oliveira wrote:
>> Hi Sakari,
>>
>> Thank you for the feedback.
>>
>> On 12/7/2016 10:33 PM, Sakari Ailus wrote:
>>> Hi Ramiro,
>>>
>>> Thank you for the patch.
>>>
>>> On Mon, Dec 05, 2016 at 05:36:33PM +0000, Ramiro Oliveira wrote:
>>>> Add device tree documentation.
>>>>
>>>> Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>> .../devicetree/bindings/media/i2c/ov5647.txt | 19 +++++++++++++++++++
>>>> 1 file changed, 19 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>> new file mode 100644
>>>> index 0000000..4c91b3b
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>> @@ -0,0 +1,19 @@
>>>> +Omnivision OV5647 raw image sensor
>>>> +---------------------------------
>>>> +
>>>> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces
>>>> +and CCI (I2C compatible) control bus.
>>>> +
>>>> +Required properties:
>>>> +
>>>> +- compatible : "ovti,ov5647";
>>>> +- reg : I2C slave address of the sensor;
>>>> +
>>>> +The common video interfaces bindings (see video-interfaces.txt) should be
>>>> +used to specify link to the image data receiver. The OV5647 device
>>>> +node should contain one 'port' child node with an 'endpoint' subnode.
>>>> +
>>>> +Following properties are valid for the endpoint node:
>>>> +
>>>> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
>>>> + video-interfaces.txt. The sensor supports only two data lanes.
>>>
>>> Doesn't this sensor require a external clock, a reset GPIO and / or a
>>> regulator or a few? Do you need data-lanes, unless you can change the order
>>> or the number?
>>
>> In the setup I'm using, I'm not aware of any reset GPIO or regulator. I do use a
>> external clock but it's fixed and not controlled by SW. Should I add a property
>> for this?
>
> The sensor datasheet defines a power-up and power-down sequence for the
> device. If you don't implement these sequences in the driver on a DT based
> system, nothing suggests that they're implemented correctly. Could it be
> that the boot loader simply enables the regulators or another device
> requires them to be enabled?
>
> I presume at least the reset GPIO should be controlled explicitly in order
> to ensure correct function. Although hardware can be surprising: I have one
> production system that has no reset GPIO for the sensor albeit the sensor
> datasheet says that's part of the power up sequence.
>
Sorry for the misunderstanding. I wanted to say that, there is no SW controlled
reset. In the board we're using to connect the sensor to our D-PHY we have a
GPIO controller that when it receives power, it removes the sensor from reset,
so I have no control over that.
Regarding the clock, should I create a new property?
And also, regarding the data-lanes, AFAIK it isn't possible to change the order
of the data and clock lanes so should I remove that property?
>>
>>>
>>> An example DT snippet wouldn't hurt.
>>
>> Sure, I can add a example snippet.
>>
>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 4/9] dt-bindings: iio: iio-mux: document iio-mux bindings
From: Peter Rosin @ 2016-12-12 12:18 UTC (permalink / raw)
To: Jonathan Cameron, Rob Herring
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Wolfram Sang, Mark Rutland,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Jonathan Corbet, Arnd Bergmann, Greg Kroah-Hartman,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <d2726499-4034-8d5d-4cff-61da86af5add-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On 2016-12-10 19:21, Jonathan Cameron wrote:
> On 06/12/16 09:18, Peter Rosin wrote:
>> On 2016-12-06 00:26, Rob Herring wrote:
>>> On Wed, Nov 30, 2016 at 09:16:58AM +0100, Peter Rosin wrote:
>>>> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>>>> ---
>>>> .../bindings/iio/multiplexer/iio-mux.txt | 40 ++++++++++++++++++++++
>>>> MAINTAINERS | 6 ++++
>>>> 2 files changed, 46 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
>>>
>>> I'm still not convinced about this binding, but don't really have more
>>> comments ATM. Sending 6 versions in 2 weeks or so doesn't really help
>>> either.
>>
>> Sorry about the noise, I'll try to be more careful going forward. On
>> the flip side, I haven't touched the code since v6.
>>
>> I don't see how bindings that are as flexible as the current (and
>> original) phandle link between the mux consumer and the mux controller
>> would look, and at the same time be simpler to understand. You need
>> to be able to refer to a mux controller from several mux consumers, and
>> you need to support several mux controllers in one node (the ADG792A
>> case). And, AFAICT, the complex case wasn't really the problem, it was
>> that it is overly complex to describe the simple case of one mux
>> consumer and one mux controller. But in your comment for v2 [1] you
>> said that I was working around limitations with shared GPIO pins. But
>> solving that in the GPIO subsystem would not solve all that the
>> phandle approach is solving, since you would not have support for
>> ADG792A (or other non-GPIO controlled muxes). So, I think listing
>> the gpio pins inside the mux consumer node is a non-starter, the mux
>> controller has to live in its own node with its own compatible.
>>
>> Would you be happier if I managed to marry the phandle approach with
>> the option of having the mux controller as a child node of the mux
>> consumer for the simple case?
>>
>> I added an example at the end of this message (the same as the first
>> example in v4 [2], at least in principle) for easy comparison between
>> the phandle and the controller-in-child-node approaches. I can't say
>> that I personally find the difference all that significant, and do not
>> think it is worth it. As I see it, the "simple option" would just muddy
>> the waters...
>>
>> [1] http://marc.info/?l=linux-kernel&m=147948334204795&w=2
>> [2] http://marc.info/?l=linux-kernel&m=148001364904240&w=2
>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt b/Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
>>>> new file mode 100644
>>>> index 000000000000..8080cf790d82
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
>>>> @@ -0,0 +1,40 @@
>>>> +IIO multiplexer bindings
>>>> +
>>>> +If a multiplexer is used to select which hardware signal is fed to
>>>> +e.g. an ADC channel, these bindings describe that situation.
>>>> +
>>>> +Required properties:
>>>> +- compatible : "iio-mux"
>>>
>>> This is a Linuxism. perhaps "adc-mux".
>>
>> No, that's not general enough, it could just as well be used to mux a
>> temperature sensor. Or whatever. Hmmm, given that "iio-mux" is bad, perhaps
>> "io-channel-mux" is better? That matches the io-channels property used to
>> refer to the parent channel.
> analog-mux maybe? Makes more sense out of context (though with io-channels defined on
> the next line you have plenty of context here ;)
Not that I care all that much about the name, but that doesn't really
fit if you take e.g. an IIO_INDEX channel. That sounds entirely non-analog
to me, but what do I know? Maybe that example doesn't make sense for some
reason, but I can't help but think that there will be some non-analog
channel in the future, if there isn't one already.
So, my preference is io-channel-mux, as that matches the previous dt
naming for what is muxed. But that's just my opinion, if I'm told that
it should be something else, then that's ok.
I'm more worried about other aspects, such as how to get reviewers and who
is going to take the core mux patches and what tree they are going to be
merged into etc. That is, if this series is going anywhere at all or if
someone is going to put up a road-block for some reason...
Cheers,
peda
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 1/2] Add OV5647 device tree documentation
From: Sakari Ailus @ 2016-12-12 12:19 UTC (permalink / raw)
To: Ramiro Oliveira
Cc: mchehab-DgEjT+Ai2ygdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
dheitmueller-eb9eJ82Ua7k9XoPSrs7Ehg,
slongerbeam-Re5JQEeQqe8AvxtiuMwx3w, lars-Qo5EllUWu/uELgA04lAiVw,
robert.jarzmik-GANU6spQydw, pavel-+ZI9xUNit7I,
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w,
sakari.ailus-VuQAYsv1563Yd54FQh9/CA, mark.rutland-5wv7dgnIgG8,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <0f72309f-ec5e-4252-f6d7-7a7f7a9dc4c5-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
Hi Ramiro,
On Mon, Dec 12, 2016 at 12:15:04PM +0000, Ramiro Oliveira wrote:
> Hi Sakari
>
> On 12/12/2016 11:49 AM, Sakari Ailus wrote:
> > Hi Ramiro,
> >
> > On Mon, Dec 12, 2016 at 11:39:31AM +0000, Ramiro Oliveira wrote:
> >> Hi Sakari,
> >>
> >> Thank you for the feedback.
> >>
> >> On 12/7/2016 10:33 PM, Sakari Ailus wrote:
> >>> Hi Ramiro,
> >>>
> >>> Thank you for the patch.
> >>>
> >>> On Mon, Dec 05, 2016 at 05:36:33PM +0000, Ramiro Oliveira wrote:
> >>>> Add device tree documentation.
> >>>>
> >>>> Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> >>>> ---
> >>>> .../devicetree/bindings/media/i2c/ov5647.txt | 19 +++++++++++++++++++
> >>>> 1 file changed, 19 insertions(+)
> >>>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
> >>>> new file mode 100644
> >>>> index 0000000..4c91b3b
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
> >>>> @@ -0,0 +1,19 @@
> >>>> +Omnivision OV5647 raw image sensor
> >>>> +---------------------------------
> >>>> +
> >>>> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces
> >>>> +and CCI (I2C compatible) control bus.
> >>>> +
> >>>> +Required properties:
> >>>> +
> >>>> +- compatible : "ovti,ov5647";
> >>>> +- reg : I2C slave address of the sensor;
> >>>> +
> >>>> +The common video interfaces bindings (see video-interfaces.txt) should be
> >>>> +used to specify link to the image data receiver. The OV5647 device
> >>>> +node should contain one 'port' child node with an 'endpoint' subnode.
> >>>> +
> >>>> +Following properties are valid for the endpoint node:
> >>>> +
> >>>> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
> >>>> + video-interfaces.txt. The sensor supports only two data lanes.
> >>>
> >>> Doesn't this sensor require a external clock, a reset GPIO and / or a
> >>> regulator or a few? Do you need data-lanes, unless you can change the order
> >>> or the number?
> >>
> >> In the setup I'm using, I'm not aware of any reset GPIO or regulator. I do use a
> >> external clock but it's fixed and not controlled by SW. Should I add a property
> >> for this?
> >
> > The sensor datasheet defines a power-up and power-down sequence for the
> > device. If you don't implement these sequences in the driver on a DT based
> > system, nothing suggests that they're implemented correctly. Could it be
> > that the boot loader simply enables the regulators or another device
> > requires them to be enabled?
> >
> > I presume at least the reset GPIO should be controlled explicitly in order
> > to ensure correct function. Although hardware can be surprising: I have one
> > production system that has no reset GPIO for the sensor albeit the sensor
> > datasheet says that's part of the power up sequence.
> >
>
> Sorry for the misunderstanding. I wanted to say that, there is no SW controlled
> reset. In the board we're using to connect the sensor to our D-PHY we have a
> GPIO controller that when it receives power, it removes the sensor from reset,
> so I have no control over that.
Do you mean to say that there's a GPIO controller but there's not (yet?) a
driver for that or that the reset line is actually hard-wired to something
else?
>
> Regarding the clock, should I create a new property?
Yes. The sensor does require a clock.
>
> And also, regarding the data-lanes, AFAIK it isn't possible to change the order
> of the data and clock lanes so should I remove that property?
Sounds good to me.
>
> >>
> >>>
> >>> An example DT snippet wouldn't hurt.
> >>
> >> Sure, I can add a example snippet.
> >>
> >>>
> >>
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v7 1/8] Documentation: arm: define DT cpu capacity-dmips-mhz bindings
From: Lorenzo Pieralisi @ 2016-12-12 12:40 UTC (permalink / raw)
To: Juri Lelli
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, peterz-wEGCiKHe2LqWVfeAwA7xHQ,
vincent.guittot-QSEj5FYQhm4dnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-lFZ/pmaqli7XmaaqVzeoHQ, sudeep.holla-5wv7dgnIgG8,
catalin.marinas-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
morten.rasmussen-5wv7dgnIgG8, dietmar.eggemann-5wv7dgnIgG8,
broonie-DgEjT+Ai2ygdnm+yROfE0A, Pawel Moll, Ian Campbell,
Kumar Gala, Maxime Ripard, Olof Johansson, Gregory CLEMENT,
Paul Walmsley, Linus Walleij, Chen-Yu Tsai, Thomas Petazzoni
In-Reply-To: <1473085372-2840-2-git-send-email-juri.lelli-5wv7dgnIgG8@public.gmane.org>
On Mon, Sep 05, 2016 at 03:22:45PM +0100, Juri Lelli wrote:
[...]
> +===========================================
> +5 - References
> +===========================================
> +
> +[1] ARM Linux Kernel documentation - CPUs bindings
> + Documentation/devicetree/bindings/arm/cpus.txt
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index e6782d50cbcd..c1dcf4cade2e 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -241,6 +241,14 @@ nodes to be present and contain the properties described below.
> # List of phandles to idle state nodes supported
> by this cpu [3].
>
> + - capacity-dmips-mhz
> + Usage: Optional
> + Value type: <u32>
> + Definition:
> + # u32 value representing CPU capacity [3] in
> + DMIPS/MHz, relative to highest capacity-dmips-mhz
> + in the system.
> +
> - rockchip,pmu
> Usage: optional for systems that have an "enable-method"
> property value of "rockchip,rk3066-smp"
> @@ -464,3 +472,5 @@ cpus {
> [2] arm/msm/qcom,kpss-acc.txt
> [3] ARM Linux kernel documentation - idle states bindings
> Documentation/devicetree/bindings/arm/idle-states.txt
> +[3] ARM Linux kernel documentation - cpu capacity bindings
> + Documentation/devicetree/bindings/arm/cpu-capacity.txt
Trivia: bumped into this while reviewing something else, too many
threes, you will have to fix the added reference up.
Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: usb:xhci: support disable usb2 LPM Remote Wakeup
From: Mathias Nyman @ 2016-12-12 13:00 UTC (permalink / raw)
To: Thang Q. Nguyen, Rob Herring
Cc: Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Mathias Nyman,
Greg Kroah-Hartman, devicetree, linux-kernel, linux-usb, Phong Vo,
Loc Ho, Vu Nguyen, patches
In-Reply-To: <CAKrQpStd4+ZZjdYyq93zq-wnqQCiQmSi5jJrYbuiZX5kVH74YA@mail.gmail.com>
On 12.12.2016 06:00, Thang Q. Nguyen wrote:
> On Sat, Dec 10, 2016 at 4:36 AM, Rob Herring <robh@kernel.org> wrote:
>> On Sun, Dec 04, 2016 at 07:42:01PM +0700, Thang Q. Nguyen wrote:
>>> From: Thang Nguyen <tqnguyen@apm.com>
>>>
>>> As per USB 2.0 link power management addendum ECN, table 1-2, page 4,
>>> device or host initiated via resume signaling; device-initiated resumes
>>> can be optionally enabled/disabled by software. This patch adds support
>>> to control enabling the USB2 RWE feature via DT/ACPI attribute.
>>>
>>> Signed-off-by: Vu Nguyen <vnguyen@apm.com>
>>> Signed-off-by: Thang Nguyen <tqnguyen@apm.com>
>>> ---
>>> Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
>>> drivers/usb/host/xhci-plat.c | 3 +++
>>> drivers/usb/host/xhci.c | 5 ++++-
>>> drivers/usb/host/xhci.h | 1 +
>>> 4 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
>>> index 966885c..9b4cd14 100644
>>> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
>>> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
>>> @@ -25,6 +25,7 @@ Required properties:
>>>
>>> Optional properties:
>>> - clocks: reference to a clock
>>> + - usb2-rwe-disable: disable USB2 LPM Remote Wakeup capable
>>
>> Remote wakeup has been around since USB 1.0 days. Does this need to be
>> USB2 or XHCI specific?
> This is XHCI specific. Per XHCI specification 1.1, remote wakeup is
> optional for XHCI 1.0 and required for XHCI 1.1. This patch provides
> ability for software to disable RWE for USB2 in XHCI1.0 controller.
I think I understand what's going on.
USB:
The good old USB2 suspend is called L2. Device enters it after 3ms if there is no link activity.
If a device can remote wakeup (RWE) it's stated in the descriptor. RWE can be turned on
of off using standard SET/CLEAR Fature requests
The LPM L1 USB2 state again is entered with a LPM extended transaction to avoid the
3ms wait before powersaving. L1 state is exit can be done with a simialr RWE as L2 resume.
The RWE from L1 can turned on/off using a bit in the LPM extended transaction.
XHCI:
Specs say that if the device supports RWE we should enable it for LPM L1 exit as well.
This is done by setting the RWE (LPM L1) bit in PORTPMSC register. This bit only affect LPM L1 remote
wake. see 4.23.5.1.1.1
The issue might be that xhci driver never check if the device actually supports RWE, we always
set the PORTPMSC RWE (for LPM L1) bit.
How about checking something like udev->do_remote_wakeup and setting and setting the bit
based on that.
The function that you are changing, xhci_set_usb2_hardware_lpm() should only be used if
host has Hardware LPM Cabaility bit (HLC) set for that USB2 port in the
USB 2.0 xHCI Supported Protocol Capability.
Host that don't supprt LPM won't have that set. See xhci 7.2.2.1.3.2
-Mathias
^ permalink raw reply
* [PATCH v6 0/5] ARM: dts: da850: tilcdc related DT changes
From: Bartosz Golaszewski @ 2016-12-12 13:05 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand,
Mark Rutland, Laurent Pinchart, Peter Ujfalusi, Russell King
Cc: LKML, arm-soc, linux-drm, linux-devicetree, Bartosz Golaszewski
This series contains the last DT changes required for LCDC support
on da850-lcdk. The first one adds the dumb-vga-dac nodes, the second
limits the maximum pixel clock rate.
v1 -> v2:
- drop patch 3/3 (already merged)
- use max-pixelclock instead of max-bandwidth for display mode limiting
v2 -> v3:
- make the commit message in patch [2/2] more detailed
- move the max-pixelclock property to da850.dtsi as the limit
affects all da850-based boards
v3 -> v4:
- remove the input port from the display node
- move the display ports node to da850-lcdk.dts
- rename the vga_bridge node to vga-bridge
- move the LCDC pins to the LCDC node (from the vga bridge node)
v4 -> v5:
- rename the display label to lcdc
- instead of using the 'dumb-vga-dac' compatible, add bindings for
ti,ths8135 and use it as the vga-bridge node compatible
v5 -> v6:
- drop the endpoint numbers for nodes with single endpoints
- split patch 2/4 from v5 into two separate patches: one adding DT
bindings and one adding actual support for ths8135
Bartosz Golaszewski (5):
ARM: dts: da850: rename the display node label
drm: bridge: add DT bindings for TI ths8135
drm: bridge: add support for TI ths8135
ARM: dts: da850-lcdk: add the vga-bridge node
ARM: dts: da850: specify the maximum pixel clock rate for tilcdc
.../bindings/display/bridge/ti,ths8135.txt | 52 +++++++++++++++++
arch/arm/boot/dts/da850-lcdk.dts | 67 ++++++++++++++++++++++
arch/arm/boot/dts/da850.dtsi | 3 +-
drivers/gpu/drm/bridge/dumb-vga-dac.c | 1 +
4 files changed, 122 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v6 1/5] ARM: dts: da850: rename the display node label
From: Bartosz Golaszewski @ 2016-12-12 13:05 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand,
Mark Rutland, Laurent Pinchart, Peter Ujfalusi, Russell King
Cc: linux-devicetree, linux-drm, LKML, arm-soc, Bartosz Golaszewski
In-Reply-To: <1481547942-24775-1-git-send-email-bgolaszewski@baylibre.com>
The tilcdc node name is 'display' as per the ePAPR 1.1 recommendation.
The label is also 'display', but change it to 'lcdc' to make it clear
what the underlying hardware is.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
arch/arm/boot/dts/da850.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index ffc6e1a..3f51e59 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -448,7 +448,7 @@
dma-names = "tx", "rx";
};
- display: display@213000 {
+ lcdc: display@213000 {
compatible = "ti,da850-tilcdc";
reg = <0x213000 0x1000>;
interrupts = <52>;
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v6 2/5] drm: bridge: add DT bindings for TI ths8135
From: Bartosz Golaszewski @ 2016-12-12 13:05 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand,
Mark Rutland, Laurent Pinchart, Peter Ujfalusi, Russell King
Cc: LKML, arm-soc, linux-drm, linux-devicetree, Bartosz Golaszewski
In-Reply-To: <1481547942-24775-1-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
THS8135 is a configurable video DAC. Add DT bindings for this chip.
Signed-off-by: Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
.../bindings/display/bridge/ti,ths8135.txt | 52 ++++++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt b/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
new file mode 100644
index 0000000..87aff90
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,ths8135.txt
@@ -0,0 +1,52 @@
+THS8135 Video DAC
+-----------------
+
+This is the binding for Texas Instruments THS8135 Video DAC bridge.
+
+Required properties:
+
+- compatible: Must be "ti,ths8135"
+
+Required nodes:
+
+This device has two video ports. Their connections are modelled using the OF
+graph bindings specified in Documentation/devicetree/bindings/graph.txt.
+
+- Video port 0 for RGB input
+- Video port 1 for VGA output
+
+Example
+-------
+
+vga-bridge {
+ compatible = "ti,ths8135";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+
+ vga_bridge_in: endpoint {
+ reg = <0>;
+ remote-endpoint = <&lcdc_out_vga>;
+ };
+ };
+
+ port@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ vga_bridge_out: endpoint {
+ reg = <0>;
+ remote-endpoint = <&vga_con_in>;
+ };
+ };
+ };
+};
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v6 3/5] drm: bridge: add support for TI ths8135
From: Bartosz Golaszewski @ 2016-12-12 13:05 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand,
Mark Rutland, Laurent Pinchart, Peter Ujfalusi, Russell King
Cc: linux-devicetree, linux-drm, LKML, arm-soc, Bartosz Golaszewski
In-Reply-To: <1481547942-24775-1-git-send-email-bgolaszewski@baylibre.com>
THS8135 is a configurable video DAC, but no configuration is actually
necessary to make it work.
For now use the dumb-vga-dac driver to support it.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/gpu/drm/bridge/dumb-vga-dac.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index afec232..498fa75 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -204,6 +204,7 @@ static int dumb_vga_remove(struct platform_device *pdev)
static const struct of_device_id dumb_vga_match[] = {
{ .compatible = "dumb-vga-dac" },
+ { .compatible = "ti,ths8135" },
{},
};
MODULE_DEVICE_TABLE(of, dumb_vga_match);
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v6 4/5] ARM: dts: da850-lcdk: add the vga-bridge node
From: Bartosz Golaszewski @ 2016-12-12 13:05 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand,
Mark Rutland, Laurent Pinchart, Peter Ujfalusi, Russell King
Cc: linux-devicetree, linux-drm, LKML, arm-soc, Bartosz Golaszewski
In-Reply-To: <1481547942-24775-1-git-send-email-bgolaszewski@baylibre.com>
Add the vga-bridge node to the board DT together with corresponding
ports and vga connector. This allows to retrieve the edid info from
the display automatically.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
arch/arm/boot/dts/da850-lcdk.dts | 67 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 67 insertions(+)
diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
index afcb482..e003111 100644
--- a/arch/arm/boot/dts/da850-lcdk.dts
+++ b/arch/arm/boot/dts/da850-lcdk.dts
@@ -51,6 +51,51 @@
system-clock-frequency = <24576000>;
};
};
+
+ vga-bridge {
+ compatible = "ti,ths8135";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+
+ vga_bridge_in: endpoint {
+ reg = <0>;
+ remote-endpoint = <&lcdc_out_vga>;
+ };
+ };
+
+ port@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ vga_bridge_out: endpoint {
+ reg = <0>;
+ remote-endpoint = <&vga_con_in>;
+ };
+ };
+ };
+ };
+
+ vga {
+ compatible = "vga-connector";
+
+ ddc-i2c-bus = <&i2c0>;
+
+ port {
+ vga_con_in: endpoint {
+ remote-endpoint = <&vga_bridge_out>;
+ };
+ };
+ };
};
&pmx_core {
@@ -236,3 +281,25 @@
&memctrl {
status = "okay";
};
+
+&lcdc {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&lcd_pins>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ lcdc_out: port@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ lcdc_out_vga: endpoint {
+ reg = <0>;
+ remote-endpoint = <&vga_bridge_in>;
+ };
+ };
+ };
+};
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH v6 5/5] ARM: dts: da850: specify the maximum pixel clock rate for tilcdc
From: Bartosz Golaszewski @ 2016-12-12 13:05 UTC (permalink / raw)
To: Jyri Sarha, Tomi Valkeinen, David Airlie, Kevin Hilman,
Michael Turquette, Sekhar Nori, Rob Herring, Frank Rowand,
Mark Rutland, Laurent Pinchart, Peter Ujfalusi, Russell King
Cc: linux-devicetree, linux-drm, LKML, arm-soc, Bartosz Golaszewski
In-Reply-To: <1481547942-24775-1-git-send-email-bgolaszewski@baylibre.com>
At maximum CPU frequency of 300 MHz the maximum pixel clock frequency
is 37.5 MHz[1]. We must filter out any mode for which the calculated
pixel clock rate would exceed this value.
Specify the max-pixelclock property for the display node for
da850-lcdk.
[1] http://processors.wiki.ti.com/index.php/OMAP-L1x/C674x/AM1x_LCD_Controller_(LCDC)_Throughput_and_Optimization_Techniques
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
arch/arm/boot/dts/da850.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 3f51e59..ba5bf80 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -452,6 +452,7 @@
compatible = "ti,da850-tilcdc";
reg = <0x213000 0x1000>;
interrupts = <52>;
+ max-pixelclock = <37500>;
status = "disabled";
};
};
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* Re: [PATCH v5 1/2] Add OV5647 device tree documentation
From: Ramiro Oliveira @ 2016-12-12 13:07 UTC (permalink / raw)
To: Sakari Ailus, Ramiro Oliveira
Cc: mchehab, linux-kernel, linux-media, robh+dt, devicetree, davem,
gregkh, geert+renesas, akpm, linux, hverkuil, dheitmueller,
slongerbeam, lars, robert.jarzmik, pavel, pali.rohar,
sakari.ailus, mark.rutland, CARLOS.PALMINHA
In-Reply-To: <20161212121947.GU16630@valkosipuli.retiisi.org.uk>
Hi Sakari
On 12/12/2016 12:19 PM, Sakari Ailus wrote:
> Hi Ramiro,
>
> On Mon, Dec 12, 2016 at 12:15:04PM +0000, Ramiro Oliveira wrote:
>> Hi Sakari
>>
>> On 12/12/2016 11:49 AM, Sakari Ailus wrote:
>>> Hi Ramiro,
>>>
>>> On Mon, Dec 12, 2016 at 11:39:31AM +0000, Ramiro Oliveira wrote:
>>>> Hi Sakari,
>>>>
>>>> Thank you for the feedback.
>>>>
>>>> On 12/7/2016 10:33 PM, Sakari Ailus wrote:
>>>>> Hi Ramiro,
>>>>>
>>>>> Thank you for the patch.
>>>>>
>>>>> On Mon, Dec 05, 2016 at 05:36:33PM +0000, Ramiro Oliveira wrote:
>>>>>> Add device tree documentation.
>>>>>>
>>>>>> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
>>>>>> ---
>>>>>> .../devicetree/bindings/media/i2c/ov5647.txt | 19 +++++++++++++++++++
>>>>>> 1 file changed, 19 insertions(+)
>>>>>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..4c91b3b
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>>>>> @@ -0,0 +1,19 @@
>>>>>> +Omnivision OV5647 raw image sensor
>>>>>> +---------------------------------
>>>>>> +
>>>>>> +OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces
>>>>>> +and CCI (I2C compatible) control bus.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +
>>>>>> +- compatible : "ovti,ov5647";
>>>>>> +- reg : I2C slave address of the sensor;
>>>>>> +
>>>>>> +The common video interfaces bindings (see video-interfaces.txt) should be
>>>>>> +used to specify link to the image data receiver. The OV5647 device
>>>>>> +node should contain one 'port' child node with an 'endpoint' subnode.
>>>>>> +
>>>>>> +Following properties are valid for the endpoint node:
>>>>>> +
>>>>>> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
>>>>>> + video-interfaces.txt. The sensor supports only two data lanes.
>>>>>
>>>>> Doesn't this sensor require a external clock, a reset GPIO and / or a
>>>>> regulator or a few? Do you need data-lanes, unless you can change the order
>>>>> or the number?
>>>>
>>>> In the setup I'm using, I'm not aware of any reset GPIO or regulator. I do use a
>>>> external clock but it's fixed and not controlled by SW. Should I add a property
>>>> for this?
>>>
>>> The sensor datasheet defines a power-up and power-down sequence for the
>>> device. If you don't implement these sequences in the driver on a DT based
>>> system, nothing suggests that they're implemented correctly. Could it be
>>> that the boot loader simply enables the regulators or another device
>>> requires them to be enabled?
>>>
>>> I presume at least the reset GPIO should be controlled explicitly in order
>>> to ensure correct function. Although hardware can be surprising: I have one
>>> production system that has no reset GPIO for the sensor albeit the sensor
>>> datasheet says that's part of the power up sequence.
>>>
>>
>> Sorry for the misunderstanding. I wanted to say that, there is no SW controlled
>> reset. In the board we're using to connect the sensor to our D-PHY we have a
>> GPIO controller that when it receives power, it removes the sensor from reset,
>> so I have no control over that.
>
> Do you mean to say that there's a GPIO controller but there's not (yet?) a
> driver for that or that the reset line is actually hard-wired to something
> else?
>
The reset line is hardwired to a GPIO controller that when powered-on removes
the sensor from reset. However I have no control over the GPIO controller.
>>
>> Regarding the clock, should I create a new property?
>
> Yes. The sensor does require a clock.
>
>>
>> And also, regarding the data-lanes, AFAIK it isn't possible to change the order
>> of the data and clock lanes so should I remove that property?
>
> Sounds good to me.
>
>>
>>>>
>>>>>
>>>>> An example DT snippet wouldn't hurt.
>>>>
>>>> Sure, I can add a example snippet.
>>>>
>>>>>
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v7 1/8] Documentation: arm: define DT cpu capacity-dmips-mhz bindings
From: Juri Lelli @ 2016-12-12 13:33 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, peterz-wEGCiKHe2LqWVfeAwA7xHQ,
vincent.guittot-QSEj5FYQhm4dnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-lFZ/pmaqli7XmaaqVzeoHQ, sudeep.holla-5wv7dgnIgG8,
catalin.marinas-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
morten.rasmussen-5wv7dgnIgG8, dietmar.eggemann-5wv7dgnIgG8,
broonie-DgEjT+Ai2ygdnm+yROfE0A, Pawel Moll, Ian Campbell,
Kumar Gala, Maxime Ripard, Olof Johansson, Gregory CLEMENT,
Paul Walmsley, Linus Walleij, Chen-Yu Tsai, Thomas Petazzoni
In-Reply-To: <20161212124005.GA19177@red-moon>
On 12/12/16 12:40, Lorenzo Pieralisi wrote:
> On Mon, Sep 05, 2016 at 03:22:45PM +0100, Juri Lelli wrote:
>
> [...]
>
> > +===========================================
> > +5 - References
> > +===========================================
> > +
> > +[1] ARM Linux Kernel documentation - CPUs bindings
> > + Documentation/devicetree/bindings/arm/cpus.txt
> > diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> > index e6782d50cbcd..c1dcf4cade2e 100644
> > --- a/Documentation/devicetree/bindings/arm/cpus.txt
> > +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> > @@ -241,6 +241,14 @@ nodes to be present and contain the properties described below.
> > # List of phandles to idle state nodes supported
> > by this cpu [3].
> >
> > + - capacity-dmips-mhz
> > + Usage: Optional
> > + Value type: <u32>
> > + Definition:
> > + # u32 value representing CPU capacity [3] in
> > + DMIPS/MHz, relative to highest capacity-dmips-mhz
> > + in the system.
> > +
> > - rockchip,pmu
> > Usage: optional for systems that have an "enable-method"
> > property value of "rockchip,rk3066-smp"
> > @@ -464,3 +472,5 @@ cpus {
> > [2] arm/msm/qcom,kpss-acc.txt
> > [3] ARM Linux kernel documentation - idle states bindings
> > Documentation/devicetree/bindings/arm/idle-states.txt
> > +[3] ARM Linux kernel documentation - cpu capacity bindings
> > + Documentation/devicetree/bindings/arm/cpu-capacity.txt
>
> Trivia: bumped into this while reviewing something else, too many
> threes, you will have to fix the added reference up.
Argh! Fixed locally, thanks for catching it.
Best,
- Juri
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 1/4] net: hix5hd2_gmac: add generic compatible string
From: Rob Herring @ 2016-12-12 14:21 UTC (permalink / raw)
To: Dongpo Li
Cc: Mark Rutland, Michael Turquette, Stephen Boyd, Russell King,
Zhangfei Gao, Yisen Zhuang, salil.mehta, David Miller,
Arnd Bergmann, Andrew Lunn, Jiancheng Xue, benjamin.chenhao,
caizhiyong, netdev, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <584E871D.7060200@hisilicon.com>
On Mon, Dec 12, 2016 at 5:16 AM, Dongpo Li <lidongpo@hisilicon.com> wrote:
> Hi Rob,
>
> On 2016/12/10 6:35, Rob Herring wrote:
>> On Mon, Dec 05, 2016 at 09:27:58PM +0800, Dongpo Li wrote:
>>> The "hix5hd2" is SoC name, add the generic ethernet driver name.
>>> The "hisi-gemac-v1" is the basic version and "hisi-gemac-v2" adds
>>> the SG/TXCSUM/TSO/UFO features.
>>>
>>> Signed-off-by: Dongpo Li <lidongpo@hisilicon.com>
>>> ---
>>> .../devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt | 9 +++++++--
>>> drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 15 +++++++++++----
>>> 2 files changed, 18 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt b/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
>>> index 75d398b..75920f0 100644
>>> --- a/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
>>> +++ b/Documentation/devicetree/bindings/net/hisilicon-hix5hd2-gmac.txt
>>> @@ -1,7 +1,12 @@
>>> Hisilicon hix5hd2 gmac controller
>>>
>>> Required properties:
>>> -- compatible: should be "hisilicon,hix5hd2-gmac".
>>> +- compatible: should contain one of the following SoC strings:
>>> + * "hisilicon,hix5hd2-gemac"
>>> + * "hisilicon,hi3798cv200-gemac"
>>> + and one of the following version string:
>>> + * "hisilicon,hisi-gemac-v1"
>>> + * "hisilicon,hisi-gemac-v2"
>>
>> What combinations are valid? I assume both chips don't have both v1 and
>> v2. 2 SoCs and 2 versions so far, I don't think there is much point to
>> have the v1 and v2 compatible strings.
>>
> The v1 and v2 are generic MAC compatible strings, many HiSilicon SoCs may
> use the same MAC version. For example,
> hix5hd2, hi3716cv200 SoCs use the v1 MAC version,
> hi3798cv200, hi3516a SoCs use the v2 MAC version,
> and there may be more SoCs added in future.
> So I think the generic compatible strings are okay here.
> Should I add the hi3716cv200, hi3516a SoCs compatible here?
Yes.
> Do you have any good advice?
>
>>> - reg: specifies base physical address(s) and size of the device registers.
>>> The first region is the MAC register base and size.
>>> The second region is external interface control register.
>>> @@ -20,7 +25,7 @@ Required properties:
>>>
>>> Example:
>>> gmac0: ethernet@f9840000 {
>>> - compatible = "hisilicon,hix5hd2-gmac";
>>> + compatible = "hisilicon,hix5hd2-gemac", "hisilicon,hisi-gemac-v1";
>>
>> You can't just change compatible strings.
>>
> Okay, maybe I should name all the compatible string with the suffix "-gmac" instead of
> "-gemac". This can keep the compatible strings with the same suffix. Is this okay?
> Can I just add the generic compatible string without changing the SoCs compatible string?
> Like following:
> gmac0: ethernet@f9840000 {
> - compatible = "hisilicon,hix5hd2-gmac";
> + compatible = "hisilicon,hix5hd2-gmac", "hisilicon,hisi-gmac-v1";
Yes, this is fine.
Rob
^ permalink raw reply
* Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer + regulator support
From: Rob Herring @ 2016-12-12 14:47 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Dmitry Torokhov, Doug Anderson, Brian Norris,
Caesar Wang, open list:ARM/Rockchip SoC...,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Mark Rutland
In-Reply-To: <20161212100110.GA13907-/m+UfqrgI5QNLKR9yMNcA1aTQe2KTcn/@public.gmane.org>
On Mon, Dec 12, 2016 at 4:01 AM, Benjamin Tissoires
<benjamin.tissoires-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Dec 12 2016 or thereabouts, Jiri Kosina wrote:
>> Given the timing (merge window being open) and given then NACK given by
>> Rob, I've now unapplied the patches (the for-4.10/i2c-hid branch is now
>> obsolete, and has been superseded by for-4.10/i2c-hid-nopower).
>>
>> However, this is mostly done in order to provide more time for discussion;
>> I still disagree with the reasoning behind the NACK.
>>
>
> To hopefully make things going forward a little bit, I was wondering
> over the week-end if we should not solve this particular issue by adding
> an intermediate platform DT node:
>
> instead of having:
> ---
> i2c-hid-dev@2c {
> compatible = "hid-over-i2c";
> reg = <0x2c>;
> hid-descr-addr = <0x0020>;
> interrupt-parent = <&gpx3>;
> interrupts = <3 2>;
> vdd-supply = <sth>;
> init-delay-ms = <100>;
> };
> ---
>
> we would have:
> ---
> platform-i2c-hid@01 {
> compatible = "very-special-board-that-needs-firmware-quirks-and-delay-of-100ms";
> vdd-supply = <sth>;
> i2c-hid-dev@2c {
> compatible = "hid-over-i2c";
> reg = <0x2c>;
> hid-descr-addr = <0x0020>;
> interrupt-parent = <&gpx3>;
> interrupts = <3 2>;
> };
> };
> ---
>
> If I am not wrong, the platform device should be initialized before
> i2c-hid get called, which allows to setup properly the vdd supply.
> On resume/suspend, the tree should be respected and we should be able to
> enable/disable power in the same fashion this patch provides.
>
> We could then extend this platform device at will without tinkering in
> i2c-hid and we could also handle the GPIOs, reset or whatever is
> required in the future through compatibles.
>
> Thoughts? yes? no? bullshit?
That is structuring the DT match your driver structure, not what the
h/w looks like. This would make sense if the device was multi-function
of which HID-over-I2C was one function. You could do what you describe
with a single node and the platform driver creates the hid-over-i2c
device. DT is not the only way to create devices.
Anyway, we're only debating this:
i2c-hid-dev@2c {
compatible = "wacom,w9013", "hid-over-i2c";
reg = <0x2c>;
hid-descr-addr = <0x0020>;
interrupt-parent = <&gpx3>;
interrupts = <3 2>;
vdd-supply = <sth>;
init-delay-ms = <100>;
};
vs.
i2c-hid-dev@2c {
compatible = "hid-over-i2c";
reg = <0x2c>;
hid-descr-addr = <0x0020>;
interrupt-parent = <&gpx3>;
interrupts = <3 2>;
vdd-supply = <sth>;
init-delay-ms = <100>;
};
My only other nit is use "post-power-on-delay-ms" which is already a
defined property name rather than "init-delay-ms".
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2 0/2] Add support for the DW IP Prototyping Kits for MIPI CSI-2 Host
From: Ramiro Oliveira @ 2016-12-12 15:00 UTC (permalink / raw)
To: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
mchehab-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-0h96xk9xTtrk1uMJSBkQmQ, hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw, arnd-r2nGTMty4D4,
sudipm.mukherjee-Re5JQEeQqe8AvxtiuMwx3w,
tiffany.lin-NuS5LvNUpcJWk0Htik3J/w,
minghsiu.tsai-NuS5LvNUpcJWk0Htik3J/w,
jean-christophe.trotin-qxv4g6HH51o,
andrew-ct.chen-NuS5LvNUpcJWk0Htik3J/w,
simon.horman-wFxRvT7yatFl57MIdRCFDg,
songjun.wu-UWL1GkI3JZL3oGB3hsPCZA, bparrot-l0cyMroinI0,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w,
Ramiro.Oliveira-HKixBCOQz3hWk0Htik3J/w
This patchset adds support for the DW CSI-2 Host IPK. These kits are intended
to help in the bringup of IP titles developed by Synopsys.
This is the second version of this patchset.
v2:
- Add more detailed descriptions in the DT documentation
- Add binding examples to DT documentation
- Remove unnecessary debug structures
- Remove unused fields in structures
- Change variable types
- Remove unused functions
- Declare functions as static
- Remove some prints
- Add missing newlines.
Ramiro Oliveira (2):
Add Documentation for Media Device, Video Device, and Synopsys DW MIPI
CSI-2 Host
Support for basic DW CSI-2 Host IPK funcionality
.../devicetree/bindings/media/snps,dw-mipi-csi.txt | 27 +
.../devicetree/bindings/media/snps,plat-ipk.txt | 9 +
.../bindings/media/snps,video-device.txt | 12 +
MAINTAINERS | 7 +
drivers/media/platform/Kconfig | 1 +
drivers/media/platform/Makefile | 2 +
drivers/media/platform/dwc/Kconfig | 36 +
drivers/media/platform/dwc/Makefile | 3 +
drivers/media/platform/dwc/dw_mipi_csi.c | 647 ++++++++++++++++
drivers/media/platform/dwc/dw_mipi_csi.h | 180 +++++
drivers/media/platform/dwc/plat_ipk.c | 818 +++++++++++++++++++++
drivers/media/platform/dwc/plat_ipk.h | 101 +++
drivers/media/platform/dwc/plat_ipk_video.h | 97 +++
drivers/media/platform/dwc/video_device.c | 707 ++++++++++++++++++
drivers/media/platform/dwc/video_device.h | 85 +++
15 files changed, 2732 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/snps,dw-mipi-csi.txt
create mode 100644 Documentation/devicetree/bindings/media/snps,plat-ipk.txt
create mode 100644 Documentation/devicetree/bindings/media/snps,video-device.txt
create mode 100644 drivers/media/platform/dwc/Kconfig
create mode 100644 drivers/media/platform/dwc/Makefile
create mode 100644 drivers/media/platform/dwc/dw_mipi_csi.c
create mode 100644 drivers/media/platform/dwc/dw_mipi_csi.h
create mode 100644 drivers/media/platform/dwc/plat_ipk.c
create mode 100644 drivers/media/platform/dwc/plat_ipk.h
create mode 100644 drivers/media/platform/dwc/plat_ipk_video.h
create mode 100644 drivers/media/platform/dwc/video_device.c
create mode 100644 drivers/media/platform/dwc/video_device.h
--
2.10.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2 1/2] Add Documentation for Media Device, Video Device, and Synopsys DW MIPI CSI-2 Host
From: Ramiro Oliveira @ 2016-12-12 15:00 UTC (permalink / raw)
To: robh+dt, mark.rutland, mchehab, devicetree, linux-kernel,
linux-media
Cc: davem, gregkh, geert+renesas, akpm, linux, hverkuil,
laurent.pinchart+renesas, arnd, sudipm.mukherjee, tiffany.lin,
minghsiu.tsai, jean-christophe.trotin, andrew-ct.chen,
simon.horman, songjun.wu, bparrot, CARLOS.PALMINHA,
Ramiro.Oliveira
In-Reply-To: <cover.1481548484.git.roliveir@synopsys.com>
Create device tree bindings documentation for Media and Video Device, as well
as the DW MIPI CSI-2 Host.
Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
---
.../devicetree/bindings/media/snps,dw-mipi-csi.txt | 37 ++++++++
.../devicetree/bindings/media/snps,plat-ipk.txt | 105 +++++++++++++++++++++
2 files changed, 142 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/snps,dw-mipi-csi.txt
create mode 100644 Documentation/devicetree/bindings/media/snps,plat-ipk.txt
diff --git a/Documentation/devicetree/bindings/media/snps,dw-mipi-csi.txt b/Documentation/devicetree/bindings/media/snps,dw-mipi-csi.txt
new file mode 100644
index 0000000..1caa652
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/snps,dw-mipi-csi.txt
@@ -0,0 +1,37 @@
+Synopsys DesignWare CSI-2 Host controller
+
+Description
+-----------
+
+This HW block is used to receive image coming from an MIPI CSI-2 compatible
+camera.
+
+Required properties:
+- compatible: shall be "snps,dw-mipi-csi"
+- reg : physical base address and size of the device memory mapped
+ registers;
+- interrupts : CSI-2 Host interrupt
+- data-lanes : Number of lanes to be used
+- output-type : Core output to be used (IPI-> 0 or IDI->1 or BOTH->2) These
+ values choose which of the Core outputs will be used, it can be Image Data
+ Interface or Image Pixel Interface.
+- phys, phy-names: List of one PHY specifier and identifier string (as defined
+ in Documentation/devicetree/bindings/phy/phy-bindings.txt). This PHY is a MIPI
+ DPHY working in RX mode.
+
+Optional properties(if in IPI mode):
+- ipi-mode : Mode to be used when in IPI(Camera -> 0 or Automatic -> 1)
+ This property defines if the controller will use the video timings available
+ in the video stream or if it will use pre-defined ones.
+- ipi-color-mode: Bus depth to be used in IPI (48 bits -> 0 or 16 bits -> 1)
+ This property defines the width of the IPI bus.
+- ipi-auto-flush: Data auto-flush (1 -> Yes or 0 -> No). This property defines
+ if the data is automatically flushed in each vsync or if this process is done
+ manually
+- virtual-channel: Virtual channel where data is present when in IPI mode. This
+ property chooses the virtual channel which IPI will use to retrieve the video
+ stream.
+
+The per-board settings:
+ - port sub-node describing a single endpoint connected to the dw-mipi-csi
+ as described in video-interfaces.txt[1].
diff --git a/Documentation/devicetree/bindings/media/snps,plat-ipk.txt b/Documentation/devicetree/bindings/media/snps,plat-ipk.txt
new file mode 100644
index 0000000..50e9279
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/snps,plat-ipk.txt
@@ -0,0 +1,105 @@
+Synopsys DesignWare CSI-2 Host IPK Media Device
+
+The Synopsys DesignWare CSI-2 Host IPK subsystem comprises of multiple
+sub-devices represented by separate device tree nodes. Currently this includes:
+plat-ipk, video-device, and dw-mipi-csi.
+
+The sub-subdevices are defined as child nodes of the common 'camera' node which
+also includes common properties of the whole subsystem not really specific to
+any single sub-device.
+
+Common 'camera' node
+--------------------
+
+Required properties:
+
+- compatible: must be "snps,plat-ipk", "simple-bus"
+
+The 'camera' node must include at least one 'video-device' and one 'dw-mipi-csi'
+child node.
+
+'video-device' device nodes
+-------------------
+
+Required properties:
+
+- compatible: "snps,video-device"
+- dmas, dma-names: List of one DMA specifier and identifier string (as defined
+ in Documentation/devicetree/bindings/dma/dma.txt) per port. Each port
+ requires a DMA channel with the identifier string set to "port" followed by
+ the port index.
+
+Image sensor nodes
+------------------
+
+The sensor device nodes should be added to their control bus controller (e.g.
+I2C0) nodes and linked to a port node in the dw-mipi-csi,using the common video
+interfaces bindings, defined in video-interfaces.txt.
+
+Example:
+
+ i2c@0x02000 {
+ compatible = "snps,designware-i2c";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x02000 0x100>;
+ clock-frequency = <400000>;
+ clocks = <&i2cclk>;
+ interrupts =<0>;
+ ov: camera@0x36 {
+ compatible = "ovti,ov5647";
+ reg = <0x36>;
+ port {
+ camera_1: endpoint {
+ remote-endpoint = <&csi1_ep1>;
+ clock-lanes = <0>;
+ data-lanes = <1 2 >;
+ };
+ };
+ };
+ };
+
+
+ camera {
+ compatible = "snps,plat-ipk", "simple-bus";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+ video_device: video-device@0x10000 {
+ compatible = "snps,video-device";
+ dmas = <&axi_vdma_0 0>;
+ dma-names = "vdma0";
+ };
+
+
+ csi2_1: csi2@0x03000 {
+ compatible = "snps,dw-mipi-csi";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = < 0x03000 0x7FF>;
+ interrupts = <2>;
+ data-lanes = <2>;
+ output-type = <2>;
+
+ phys = <&mipi_phy_ctrl1 0>;
+ phy-names = "csi2-dphy";
+
+ /*IPI Related Configurations*/
+ ipi-mode = <0>;
+ ipi-color-mode = <0>;
+ ipi-auto-flush = <1>;
+ virtual-channel = <0>;
+
+ /* Camera MIPI CSI-2 (CSI1) */
+ port@1 {
+ reg = <1>;
+ csi1_ep1: endpoint {
+ remote-endpoint = <&camera_1>;
+ data-lanes = <1 2>;
+ };
+ };
+ };
+ };
+ };
+
+The dw-mipi-csi device binding is defined in snps,dw-mipi-csi.txt.
--
2.10.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox