devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Andreas Färber" <afaerber-l3A5Bk7waGM@public.gmane.org>
To: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: linux-samsung-soc
	<linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Stephan van Schaik
	<stephan-zGZ7W/ttz6xN8Ch2cx6nig@public.gmane.org>,
	Vincent Palatin
	<vpalatin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>,
	Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>,
	Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>,
	Ian Campbell
	<ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>,
	Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>,
	Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>,
	Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>,
	Kukjin Kim <kgene.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>,
	"OPEN FIRMWARE AND..."
	<devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	ARM PORT
	<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
	LKML <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH v2 4/4] ARM: dts: exynos5250: Add Spring device tree
Date: Wed, 30 Jul 2014 15:06:36 +0200	[thread overview]
Message-ID: <53D8EDDC.7010407@suse.de> (raw)
In-Reply-To: <CAD=FV=Xu9b6433pV=bqi3jZZR03G6nyi2iNgbSXwAcRY4Y=4yA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

Hi Doug,

Am 25.07.2014 18:02, schrieb Doug Anderson:
> Andreas,
> 
> On Fri, Jul 18, 2014 at 10:20 AM, Andreas Färber <afaerber@suse.de> wrote:
>> +       gpio-keys {
>> +               compatible = "gpio-keys";
>> +
>> +               power {
>> +                       label = "Power";
>> +                       gpios = <&gpx1 3 1>;
>> +                       linux,code = <116>; /* KEY_POWER */
>> +                       gpio-key,wakeup;
>> +               };
> 
> Is the lid switch not working?

No, I don't see any reaction at least. I notice that 3.8 has pinctrls on
gpio-keys directly though...

> Where is your pinctrl?

Maybe that's what you mean here?

> Can you match this in sort order with snow to make diffs easier?

Done.

>> +       };
>> +};
>> +
>> +&i2c_0 {
>> +       status = "okay";
>> +       samsung,i2c-sda-delay = <100>;
>> +       samsung,i2c-max-bus-freq = <378000>;
>> +
>> +       s5m8767_pmic@66 {
>> +               compatible = "samsung,s5m8767-pmic";
>> +               reg = <0x66>;
>> +               interrupt-parent = <&gpx3>;
>> +               interrupts = <2 0>;
>> +               pinctrl-names = "default";
>> +               pinctrl-0 = <&s5m8767_irq &s5m8767_dvs &s5m8767_ds>;
>> +               wakeup-source;
>> +
>> +               s5m8767,pmic-buck-dvs-gpios = <&gpd1 0 1>, /* DVS1 */
>> +                                             <&gpd1 1 1>, /* DVS2 */
>> +                                             <&gpd1 2 1>; /* DVS3 */
>> +
>> +               s5m8767,pmic-buck-ds-gpios = <&gpx2 3 1>, /* SET1 */
>> +                                            <&gpx2 4 1>, /* SET2 */
>> +                                            <&gpx2 5 1>; /* SET3 */
>> +
>> +               /*
>> +                * The following arrays of DVS voltages are not used, since we are
>> +                * not using GPIOs to control PMIC bucks, but they must be defined
>> +                * to please the driver.
>> +                */
>> +               s5m8767,pmic-buck2-dvs-voltage = <1350000>, <1300000>,
>> +                                                <1250000>, <1200000>,
>> +                                                <1150000>, <1100000>,
>> +                                                <1000000>, <950000>;
>> +
>> +               s5m8767,pmic-buck3-dvs-voltage = <1100000>, <1100000>,
>> +                                                <1100000>, <1100000>,
>> +                                                <1000000>, <1000000>,
>> +                                                <1000000>, <1000000>;
>> +
>> +               s5m8767,pmic-buck4-dvs-voltage = <1200000>, <1200000>,
>> +                                                <1200000>, <1200000>,
>> +                                                <1200000>, <1200000>,
>> +                                                <1200000>, <1200000>;
>> +
>> +               clocks {
>> +                       compatible = "samsung,s5m8767-clk";
>> +                       #clock-cells = <1>;
>> +                       clock-output-names = "en32khz_ap",
>> +                                            "en32khz_cp",
>> +                                            "en32khz_bt";
>> +               };
>> +
>> +               regulators {
>> +                       s5m_ldo4_reg: LDO4 {
>> +                               regulator-name = "P1.0V_LDO_OUT4";
>> +                               regulator-min-microvolt = <1000000>;
>> +                               regulator-max-microvolt = <1000000>;
>> +                               regulator-always-on;
>> +                               op_mode = <0>;
>> +                       };
>> +
>> +                       s5m_ldo5_reg: LDO5 {
>> +                               regulator-name = "P1.0V_LDO_OUT5";
>> +                               regulator-min-microvolt = <1000000>;
>> +                               regulator-max-microvolt = <1000000>;
>> +                               regulator-always-on;
>> +                               op_mode = <0>;
>> +                       };
>> +
>> +                       s5m_ldo6_reg: LDO6 {
>> +                               regulator-name = "vdd_mydp";
>> +                               regulator-min-microvolt = <1000000>;
>> +                               regulator-max-microvolt = <1000000>;
>> +                               regulator-always-on;
>> +                               op_mode = <3>;
>> +                       };
>> +
>> +                       s5m_ldo7_reg: LDO7 {
>> +                               regulator-name = "P1.1V_LDO_OUT7";
>> +                               regulator-min-microvolt = <1100000>;
>> +                               regulator-max-microvolt = <1100000>;
>> +                               regulator-always-on;
>> +                               op_mode = <3>;
>> +                       };
>> +
>> +                       s5m_ldo8_reg: LDO8 {
>> +                               regulator-name = "P1.0V_LDO_OUT8";
>> +                               regulator-min-microvolt = <1000000>;
>> +                               regulator-max-microvolt = <1000000>;
>> +                               regulator-always-on;
>> +                               op_mode = <3>;
>> +                       };
>> +
>> +                       s5m_ldo10_reg: LDO10 {
>> +                               regulator-name = "P1.8V_LDO_OUT10";
>> +                               regulator-min-microvolt = <1800000>;
>> +                               regulator-max-microvolt = <1800000>;
>> +                               regulator-always-on;
>> +                               op_mode = <3>;
>> +                       };
>> +
>> +                       s5m_ldo11_reg: LDO11 {
>> +                               regulator-name = "P1.8V_LDO_OUT11";
>> +                               regulator-min-microvolt = <1800000>;
>> +                               regulator-max-microvolt = <1800000>;
>> +                               regulator-always-on;
>> +                               op_mode = <0>;
>> +                       };
>> +
>> +                       s5m_ldo12_reg: LDO12 {
>> +                               regulator-name = "P3.0V_LDO_OUT12";
>> +                               regulator-min-microvolt = <3000000>;
>> +                               regulator-max-microvolt = <3000000>;
>> +                               regulator-always-on;
>> +                               op_mode = <3>;
>> +                       };
>> +
>> +                       s5m_ldo13_reg: LDO13 {
>> +                               regulator-name = "P1.8V_LDO_OUT13";
>> +                               regulator-min-microvolt = <1800000>;
>> +                               regulator-max-microvolt = <1800000>;
>> +                               regulator-always-on;
>> +                               op_mode = <0>;
>> +                       };
>> +
>> +                       s5m_ldo14_reg: LDO14 {
>> +                               regulator-name = "P1.8V_LDO_OUT14";
>> +                               regulator-min-microvolt = <1800000>;
>> +                               regulator-max-microvolt = <1800000>;
>> +                               regulator-always-on;
>> +                               op_mode = <3>;
>> +                       };
>> +
>> +                       s5m_ldo15_reg: LDO15 {
>> +                               regulator-name = "P1.0V_LDO_OUT15";
>> +                               regulator-min-microvolt = <1000000>;
>> +                               regulator-max-microvolt = <1000000>;
>> +                               regulator-always-on;
>> +                               op_mode = <3>;
>> +                       };
>> +
>> +                       s5m_ldo16_reg: LDO16 {
>> +                               regulator-name = "P1.8V_LDO_OUT16";
>> +                               regulator-min-microvolt = <1800000>;
>> +                               regulator-max-microvolt = <1800000>;
>> +                               regulator-always-on;
>> +                               op_mode = <3>;
>> +                       };
>> +
>> +                       s5m_ldo17_reg: LDO17 {
>> +                               regulator-name = "P2.8V_LDO_OUT17";
>> +                               regulator-min-microvolt = <2800000>;
>> +                               regulator-max-microvolt = <2800000>;
>> +                               regulator-always-on;
>> +                               op_mode = <0>;
>> +                       };
>> +
>> +                       s5m_ldo25_reg: LDO25 {
>> +                               regulator-name = "vdd_bridge";
>> +                               regulator-min-microvolt = <1200000>;
>> +                               regulator-max-microvolt = <1200000>;
>> +                               regulator-always-on;
>> +                               op_mode = <1>;
>> +                       };
>> +
>> +                       BUCK1 {
>> +                               regulator-name = "vdd_mif";
>> +                               regulator-min-microvolt = <950000>;
>> +                               regulator-max-microvolt = <1300000>;
>> +                               regulator-always-on;
>> +                               regulator-boot-on;
>> +                               op_mode = <3>;
>> +                       };
>> +
>> +                       BUCK2 {
>> +                               regulator-name = "vdd_arm";
>> +                               regulator-min-microvolt = <850000>;
>> +                               regulator-max-microvolt = <1350000>;
>> +                               regulator-always-on;
>> +                               regulator-boot-on;
>> +                               op_mode = <3>;
>> +                       };
>> +
>> +                       BUCK3 {
>> +                               regulator-name = "vdd_int";
>> +                               regulator-min-microvolt = <900000>;
>> +                               regulator-max-microvolt = <1200000>;
>> +                               regulator-always-on;
>> +                               regulator-boot-on;
>> +                               op_mode = <3>;
>> +                       };
>> +
>> +                       BUCK4 {
>> +                               regulator-name = "vdd_g3d";
>> +                               regulator-min-microvolt = <850000>;
>> +                               regulator-max-microvolt = <1300000>;
>> +                               regulator-boot-on;
>> +                               op_mode = <3>;
>> +                       };
>> +
>> +                       BUCK5 {
>> +                               regulator-name = "P1.8V_BUCK_OUT5";
>> +                               regulator-min-microvolt = <1800000>;
>> +                               regulator-max-microvolt = <1800000>;
>> +                               regulator-always-on;
>> +                               regulator-boot-on;
>> +                               op_mode = <1>;
>> +                       };
>> +
>> +                       BUCK6 {
>> +                               regulator-name = "P1.2V_BUCK_OUT6";
>> +                               regulator-min-microvolt = <1200000>;
>> +                               regulator-max-microvolt = <1200000>;
>> +                               regulator-always-on;
>> +                               regulator-boot-on;
>> +                               op_mode = <0>;
>> +                       };
>> +
>> +                       BUCK9 {
>> +                               regulator-name = "vdd_ummc";
>> +                               regulator-min-microvolt = <950000>;
>> +                               regulator-max-microvolt = <3000000>;
>> +                               regulator-always-on;
>> +                               regulator-boot-on;
>> +                               op_mode = <3>;
>> +                       };
>> +               };
>> +       };
>> +};
>> +
>> +&i2c_1 {
>> +       status = "okay";
>> +       samsung,i2c-sda-delay = <100>;
>> +       samsung,i2c-max-bus-freq = <378000>;
>> +};
>> +
>> +&i2c_2 {
>> +       status = "okay";
>> +       samsung,i2c-sda-delay = <100>;
>> +       samsung,i2c-max-bus-freq = <66000>;
>> +
>> +       hdmiddc@50 {
>> +               compatible = "samsung,exynos4210-hdmiddc";
>> +               reg = <0x50>;
>> +       };
>> +};
>> +
>> +&i2c_3 {
>> +       status = "okay";
>> +       samsung,i2c-sda-delay = <100>;
>> +       samsung,i2c-max-bus-freq = <66000>;
>> +};
>> +
>> +&i2c_4 {
>> +       status = "okay";
>> +       samsung,i2c-sda-delay = <100>;
>> +       samsung,i2c-max-bus-freq = <66000>;
>> +
>> +       cros_ec: embedded-controller {
>> +               compatible = "google,cros-ec-i2c";
>> +               reg = <0x1e>;
>> +               interrupts = <6 0>;
>> +               interrupt-parent = <&gpx1>;
> 
> I think you want a pinctrl for gpx1-6, like Snow's "ec_irq".

Done. On v1 you had asked about a pinctrl for the keyboard, which has
none, but cros_ec itself does in fact.

>> +               wakeup-source;
>> +       };
>> +};
>> +
>> +&i2c_5 {
>> +       status = "okay";
>> +       samsung,i2c-sda-delay = <100>;
>> +       samsung,i2c-max-bus-freq = <66000>;
>> +};
>> +
>> +&i2c_7 {
>> +       status = "okay";
>> +       samsung,i2c-sda-delay = <100>;
>> +       samsung,i2c-max-bus-freq = <66000>;
>> +};
>> +
>> +&i2c_8 {
>> +       status = "okay";
>> +       samsung,i2c-sda-delay = <100>;
>> +       samsung,i2c-max-bus-freq = <378000>;
>> +
>> +       hdmiphy: hdmiphy@38 {
>> +               compatible = "samsung,exynos4212-hdmiphy";
>> +               reg = <0x38>;
>> +       };
>> +};
>> +
>> +&mmc_0 {
>> +       status = "okay";
>> +       num-slots = <1>;
>> +       supports-highspeed;
>> +       broken-cd;
>> +       card-detect-delay = <200>;
>> +       samsung,dw-mshc-ciu-div = <3>;
>> +       samsung,dw-mshc-sdr-timing = <2 3>;
>> +       samsung,dw-mshc-ddr-timing = <1 2>;
>> +       pinctrl-names = "default";
>> +       pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_cd &sd0_bus4 &sd0_bus8>;
>> +
>> +       slot@0 {
>> +               reg = <0>;
>> +               bus-width = <8>;
>> +       };
>> +};
>> +
>> +&mmc_1 {
>> +       status = "okay";
>> +       num-slots = <1>;
>> +       supports-highspeed;
>> +       broken-cd;
>> +       card-detect-delay = <200>;
>> +       samsung,dw-mshc-ciu-div = <3>;
>> +       samsung,dw-mshc-sdr-timing = <2 3>;
>> +       samsung,dw-mshc-ddr-timing = <1 2>;
>> +       pinctrl-names = "default";
>> +       pinctrl-0 = <&sd1_clk &sd1_cmd &sd1_cd &sd1_bus4>;
>> +
>> +       slot@0 {
>> +               reg = <0>;
>> +               bus-width = <4>;
>> +       };
>> +};
> 
> mmc_1 on Spring matches with mmc_3 on Snow.  That means you need to
> make sure you've got the reduced drive strengths, too.  I don't think
> your device tree has these.  Specifically you need local overrides of
> sd1_clk, sd1_cmd, etc to lower drive strengths.

Done. Wasn't sure about _cd, so I included it.

>> +
>> +&mmc_2 {
>> +       /* MMC2 pins are used as GPIO for eDP bridge control. */
>> +       status = "disabled";
>> +};
> 
> Remove the mmc_2 reference.  It's disabled in exynos5250.dtsi already.

Done. That drops the documentation of why, too, though.

>> +
>> +&pinctrl_0 {
>> +       /*
>> +        * Disabled pullups since external part has its own pullups and
>> +        * double-pulling gets us out of spec in some cases.
>> +        */
>> +       i2c2_bus: i2c2-bus {
>> +               samsung,pin-pud = <0>;
>> +       };
>> +
>> +       s5m8767_dvs: s5m8767-dvs {
>> +               samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2";
>> +               samsung,pin-function = <0>;
>> +               samsung,pin-pud = <1>;
>> +               samsung,pin-drv = <0>;
>> +       };
>> +
>> +       s5m8767_ds: s5m8767-ds {
>> +               samsung,pins = "gpx2-3", "gpx2-4", "gpx2-5";
>> +               samsung,pin-function = <0>;
>> +               samsung,pin-pud = <1>;
>> +               samsung,pin-drv = <0>;
>> +       };
>> +
>> +       s5m8767_irq: s5m8767-irq {
>> +               samsung,pins = "gpx3-2";
>> +               samsung,pin-function = <0>;
>> +               samsung,pin-pud = <0>;
>> +               samsung,pin-drv = <0>;
>> +       };
>> +
>> +       hdmi_hpd_irq: hdmi-hpd-irq {
>> +               samsung,pins = "gpx3-7";
>> +               samsung,pin-function = <0>;
>> +               samsung,pin-pud = <1>;
>> +               samsung,pin-drv = <0>;
>> +       };
> 
> Ideally you could do one more cleanup patch to snow first to make
> diffs easier between the two dts files.  Moving things in the snow
> file from "pinctrl@11400000" to the pinctrl_0 section would help.
> ...and other cleanups you could do to snow to make diffs similar would
> also be really great.

Done.

>> +};
>> +
>> +&pinctrl_1 {
>> +       hsic_reset: hsic-reset {
>> +               samsung,pins = "gpe1-0";
>> +               samsung,pin-function = <1>;
>> +               samsung,pin-pud = <0>;
>> +               samsung,pin-drv = <0>;
>> +       };
>> +};
>> +
>> +&spi_1 {
>> +       status = "okay";
>> +       samsung,spi-src-clk = <0>;
>> +       num-cs = <1>;
>> +};
>> +
>> +&usbdrd_phy {
>> +       vbus-supply = <&usb3_vbus_reg>;
>> +};
>> +
>> +#include "cros-ec-keyboard.dtsi"
>> --
>> 1.9.3

Also found some unresolved review comments from v1 that I incorporated.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
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

      parent reply	other threads:[~2014-07-30 13:06 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1405704009-8430-1-git-send-email-afaerber@suse.de>
2014-07-18 17:20 ` [PATCH v2 1/4] ARM: dts: exynos5250: max77686 is Snow only Andreas Färber
2014-07-25  5:28   ` Kukjin Kim
2014-07-25 15:08     ` Doug Anderson
2014-07-25 15:13       ` Doug Anderson
2014-07-25 15:30         ` Andreas Färber
2014-07-25 16:02           ` Doug Anderson
2014-07-29 16:15   ` Doug Anderson
2014-07-18 17:20 ` [PATCH v2 2/4] ARM: dts: exynos5250: cypress,cyapa trackpad " Andreas Färber
2014-07-29 16:16   ` Doug Anderson
2014-07-29 23:06     ` Kukjin Kim
2014-07-30  0:21       ` Andreas Färber
2014-07-18 17:20 ` [PATCH v2 3/4] ARM: dts: exynos5250: Fold common ChromeOS parts into Snow Andreas Färber
2014-07-25 16:02   ` Doug Anderson
2014-07-29 12:45     ` Andreas Färber
2014-07-29 15:11       ` Doug Anderson
2014-07-25 16:35   ` Javier Martinez Canillas
2014-07-25 16:43     ` Doug Anderson
2014-07-25 17:02       ` Javier Martinez Canillas
2014-07-29 13:00         ` Andreas Färber
2014-07-29 14:39           ` Javier Martinez Canillas
2014-07-29 15:27           ` Doug Anderson
2014-07-18 17:20 ` [PATCH v2 4/4] ARM: dts: exynos5250: Add Spring device tree Andreas Färber
2014-07-25 16:02   ` Doug Anderson
     [not found]     ` <CAD=FV=Xu9b6433pV=bqi3jZZR03G6nyi2iNgbSXwAcRY4Y=4yA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-07-30 13:06       ` Andreas Färber [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53D8EDDC.7010407@suse.de \
    --to=afaerber-l3a5bk7wagm@public.gmane.org \
    --cc=ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org \
    --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \
    --cc=galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org \
    --cc=ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org \
    --cc=kgene.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
    --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org \
    --cc=linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=mark.rutland-5wv7dgnIgG8@public.gmane.org \
    --cc=pawel.moll-5wv7dgnIgG8@public.gmane.org \
    --cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
    --cc=stephan-zGZ7W/ttz6xN8Ch2cx6nig@public.gmane.org \
    --cc=vpalatin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).