| Message ID | 1524634571-5914-3-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 25, 2018 at 2:36 AM, Anson Huang <Anson.Huang@nxp.com> wrote: > +&i2c3 { > + clock-frequency = <100000>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_i2c3_2>; > + status = "okay"; > + > + max7310_a: gpio@30 { > + compatible = "maxim,max7310"; > + reg = <0x30>; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + > + max7310_b: gpio@32 { > + compatible = "maxim,max7310"; > + reg = <0x32>; > + gpio-controller; > + #gpio-cells = <2>; > + }; Please also add the users for the MAX7310 pins, such as Ethernet PHY.
Hi, Fabio Anson Huang Best Regards! > -----Original Message----- > From: Fabio Estevam [mailto:festevam@gmail.com] > Sent: Thursday, April 26, 2018 5:46 AM > To: Anson Huang <anson.huang@nxp.com> > Cc: Shawn Guo <shawnguo@kernel.org>; Sascha Hauer > <kernel@pengutronix.de>; Fabio Estevam <fabio.estevam@nxp.com>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; > Russell King - ARM Linux <linux@armlinux.org.uk>; dl-linux-imx > <linux-imx@nxp.com>; moderated list:ARM/FREESCALE IMX / MXC ARM > ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; open list:OPEN > FIRMWARE AND FLATTENED DEVICE TREE BINDINGS > <devicetree@vger.kernel.org>; linux-kernel <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH V4 3/6] ARM: dts: imx6sx-sabreauto: add IO expander > max7310 support > > On Wed, Apr 25, 2018 at 2:36 AM, Anson Huang <Anson.Huang@nxp.com> > wrote: > > > +&i2c3 { > > + clock-frequency = <100000>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_i2c3_2>; > > + status = "okay"; > > + > > + max7310_a: gpio@30 { > > + compatible = "maxim,max7310"; > > + reg = <0x30>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + }; > > + > > + max7310_b: gpio@32 { > > + compatible = "maxim,max7310"; > > + reg = <0x32>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + }; > > Please also add the users for the MAX7310 pins, such as Ethernet PHY. MAX7310 pins are for PCIe, CAN, MLB. GPS etc., these modules are NOT enabled yet, so I can NOT test it, I think we should add MAX7310 users when these modules are enabled, ethernet PHY is NOT controlled by it. Anson.
On Thu, Apr 26, 2018 at 12:06 AM, Anson Huang <anson.huang@nxp.com> wrote: > MAX7310 pins are for PCIe, CAN, MLB. GPS etc., these modules are NOT enabled yet, so > I can NOT test it, I think we should add MAX7310 users when these modules are enabled, > ethernet PHY is NOT controlled by it. You said previously that MAX7310 also controls the Ethernet PHY. Is the vendor dts wrong then? If there are no MAX7310 users, why are you adding support for MAX7310 then? Confused.
Anson Huang Best Regards! > -----Original Message----- > From: Fabio Estevam [mailto:festevam@gmail.com] > Sent: Thursday, April 26, 2018 2:21 PM > To: Anson Huang <anson.huang@nxp.com> > Cc: Shawn Guo <shawnguo@kernel.org>; Sascha Hauer > <kernel@pengutronix.de>; Fabio Estevam <fabio.estevam@nxp.com>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; > Russell King - ARM Linux <linux@armlinux.org.uk>; dl-linux-imx > <linux-imx@nxp.com>; moderated list:ARM/FREESCALE IMX / MXC ARM > ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; open list:OPEN > FIRMWARE AND FLATTENED DEVICE TREE BINDINGS > <devicetree@vger.kernel.org>; linux-kernel <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH V4 3/6] ARM: dts: imx6sx-sabreauto: add IO expander > max7310 support > > On Thu, Apr 26, 2018 at 12:06 AM, Anson Huang <anson.huang@nxp.com> > wrote: > > > MAX7310 pins are for PCIe, CAN, MLB. GPS etc., these modules are NOT > > enabled yet, so I can NOT test it, I think we should add MAX7310 users > > when these modules are enabled, ethernet PHY is NOT controlled by it. > > You said previously that MAX7310 also controls the Ethernet PHY. Is the vendor > dts wrong then? > > If there are no MAX7310 users, why are you adding support for MAX7310 then? > > Confused. I searched the mail I sent, I can NOT found where I said "MAX7310 controls the ethernet phy".... There are MAX7310 users, it is just because currently those users are NOT enabled, so I can NOT verify them. MAX7310 is an independent I2C device, why we can NOT enable it first? I tested it from sysfs interface, we can control the MAX7310's IO output via echo different value to GPIO value, that means MAX7310 itself as an IO expander chip is working just fine, similar with PMIC, it is a I2C device, can be enabled independently, then consumers can be added later when they are enabled. Just my personal opinion, thanks. Anson.
On Thu, Apr 26, 2018 at 3:31 AM, Anson Huang <anson.huang@nxp.com> wrote: > I searched the mail I sent, I can NOT found where I said "MAX7310 controls the ethernet phy".... Initially you passed: pinctrl-assert-gpios = <&max7322 0 GPIO_ACTIVE_HIGH> inside the fec1 node. Then I explained that 'pinctrl-assert-gpios' is not a valid property in mainline. It only exists in NXP vendor tree. Then you explained: "Sorry, I made a mistake here, the MAX7320 IO0 is for adjusting FEC1's voltage" > There are MAX7310 users, it is just because currently those users are NOT enabled, so I can > NOT verify them. MAX7310 is an independent I2C device, why we can NOT enable it first? > I tested it from sysfs interface, we can control the MAX7310's IO output via echo different value > to GPIO value, that means MAX7310 itself as an IO expander chip is working just fine, similar with PMIC, > it is a I2C device, can be enabled independently, then consumers can be added later when they are enabled. > Just my personal opinion, thanks. Right, but if FEC needs max7322 IO0 at level 1 then better describe it in dts.
Anson Huang Best Regards! > -----Original Message----- > From: Fabio Estevam [mailto:festevam@gmail.com] > Sent: Friday, April 27, 2018 1:24 AM > To: Anson Huang <anson.huang@nxp.com> > Cc: Shawn Guo <shawnguo@kernel.org>; Sascha Hauer > <kernel@pengutronix.de>; Fabio Estevam <fabio.estevam@nxp.com>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; > Russell King - ARM Linux <linux@armlinux.org.uk>; dl-linux-imx > <linux-imx@nxp.com>; moderated list:ARM/FREESCALE IMX / MXC ARM > ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; open list:OPEN > FIRMWARE AND FLATTENED DEVICE TREE BINDINGS > <devicetree@vger.kernel.org>; linux-kernel <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH V4 3/6] ARM: dts: imx6sx-sabreauto: add IO expander > max7310 support > > On Thu, Apr 26, 2018 at 3:31 AM, Anson Huang <anson.huang@nxp.com> > wrote: > > > I searched the mail I sent, I can NOT found where I said "MAX7310 controls > the ethernet phy".... > > Initially you passed: pinctrl-assert-gpios = <&max7322 0 GPIO_ACTIVE_HIGH> > inside the fec1 node. > > Then I explained that 'pinctrl-assert-gpios' is not a valid property in mainline. It > only exists in NXP vendor tree. > > Then you explained: > > "Sorry, I made a mistake here, the MAX7320 IO0 is for adjusting FEC1's voltage" The ' pinctrl-assert-gpios ' is ONLY in NXP local tree, FEC owner added it because he wants to set this GPIO ONCE during kernel boot up, but upstream kernel does NOT support the pinctrl-assert-gpio, so we have to drop it, the MAX7322 IO0 is for adjusting FEC's voltage, I meant for FEC IO's voltage, 1.5V or 1.8V. If I made any confuse, I am sorry for that, I think it should be clear now, all we want/need is to set this GPIO ONCE to get the IO voltage we want. > > > There are MAX7310 users, it is just because currently those users are > > NOT enabled, so I can NOT verify them. MAX7310 is an independent I2C > device, why we can NOT enable it first? > > I tested it from sysfs interface, we can control the MAX7310's IO > > output via echo different value to GPIO value, that means MAX7310 > > itself as an IO expander chip is working just fine, similar with PMIC, it is a I2C > device, can be enabled independently, then consumers can be added later when > they are enabled. > > Just my personal opinion, thanks. > > Right, but if FEC needs max7322 IO0 at level 1 then better describe it in dts. Agree, since currently our FEC driver does NOT support adjusting such IO voltage in driver, and there is NO such requirement of runtime switching IO voltage for FEC, so I think it should be good here, just use the default settings of MAX7322 IO0 described in 'reg_fec', if there is any further requirement of runtime adjusting FEC IO voltage, I think FEC driver needs to add such support, and there will be new patch for it. Thanks. Anson.
diff --git a/arch/arm/boot/dts/imx6sx-sabreauto.dts b/arch/arm/boot/dts/imx6sx-sabreauto.dts index d59084f..812f40b 100644 --- a/arch/arm/boot/dts/imx6sx-sabreauto.dts +++ b/arch/arm/boot/dts/imx6sx-sabreauto.dts @@ -82,6 +82,13 @@ >; }; + pinctrl_i2c3_2: i2c3grp-2 { + fsl,pins = < + MX6SX_PAD_KEY_ROW4__I2C3_SDA 0x4001b8b1 + MX6SX_PAD_KEY_COL4__I2C3_SCL 0x4001b8b1 + >; + }; + pinctrl_uart1: uart1grp { fsl,pins = < MX6SX_PAD_GPIO1_IO04__UART1_TX 0x1b0b1 @@ -272,3 +279,24 @@ }; }; }; + +&i2c3 { + clock-frequency = <100000>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_i2c3_2>; + status = "okay"; + + max7310_a: gpio@30 { + compatible = "maxim,max7310"; + reg = <0x30>; + gpio-controller; + #gpio-cells = <2>; + }; + + max7310_b: gpio@32 { + compatible = "maxim,max7310"; + reg = <0x32>; + gpio-controller; + #gpio-cells = <2>; + }; +};
i.MX6SX Sabre Auto board has two max7310 IO expander on I2C3 bus, add support for them. Signed-off-by: Anson Huang <Anson.Huang@nxp.com> --- no changes. arch/arm/boot/dts/imx6sx-sabreauto.dts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)