Re: [PATCH] ARM: dts: mvebu: add support for SolidRun Clearfog GTR

[Baruch Siach <baruch@tkos.co.il>]

Hi Andrew,

Thanks for your review.

On Fri, Nov 29 2019, Andrew Lunn wrote:

> On Thu, Nov 28, 2019 at 07:00:06PM +0200, Baruch Siach wrote:
>> SolidRun Clearfog GTR L8 and S4 SBCs are based on Armada 385. They
>> features 8 (L8) or 4 (S4) switched Ethernet ports, 1 1Gb Ethernet port,
>> 1 directly connected SFP port, 1 SFP port behind the switch (not
>> currently described in DT),
>
> Hi Baruch
>
> Did you try to add the SFP on the switch? What problems did you have?
> The general support for it should be there.

I have not tested this SFP yet.

>> +			i2c@11100 { /* SFP */
>> +				pinctrl-0 = <&cf_gtr_i2c1_pins>;
>> +				pinctrl-names = "default";
>> +				status = "okay";
>> +			};
>
> Since there are two SFPs, it would be a good idea to indicate which
> one this is.

This port is marked CON5/CON6 on the PCB. I'll extend the comment in v2.

>> +	sfp: sfp {
>> +		compatible = "sff,sfp";
>> +		i2c-bus = <&i2c1>;
>> +		los-gpio = <&gpio1 22 GPIO_ACTIVE_HIGH>;
>> +		mod-def0-gpio = <&gpio0 25 GPIO_ACTIVE_LOW>;
>> +		tx-disable-gpio = <&gpio1 14 GPIO_ACTIVE_HIGH>;
>> +	};
>
> when you get the second sfp working you are going to have naming
> issue. So maybe call this sfp0?

Good idea.

>> +		led1 {
>> +			label = "led1";
>> +			gpios = <&gpio1 10 GPIO_ACTIVE_HIGH>;
>> +		};
>> +
>> +		led2 {
>> +			label = "led2";
>> +			gpios = <&gpio1 20 GPIO_ACTIVE_HIGH>;
>> +		};
>
> There is a naming convention of LEDS labels. This does not fit it.

Indeed. The 'label' property is deprecated since commit
c5d18dd6b64. These are general propose GPIOs. Not sure what 'function'
property fits here. Also, these GPIOs go out to an optional front-panel
connector. The 'color' property does not make much sense in that
case. I'll stick LED_COLOR_ID_GREEN for the no front-panel option.

>> +&gpio0 {
>> +	pinctrl-0 = <&cf_gtr_fan_pwm>;
>> +	pinctrl-names = "default";
>> +
>> +	wifi-disable {
>> +		gpio-hog;
>> +		gpios = <30 GPIO_ACTIVE_LOW>, <31 GPIO_ACTIVE_LOW>;
>> +		output-low;
>> +		line-name = "wifi-disable";
>> +	};
>> +};
>
> Isn't there a generic rfkill GPIO driver? Never looked, but it seems
> like it should exist.

Over the years there were attempts to add DT binding to rfkill-regulator
or rfkill-gpio:

  https://patchwork.kernel.org/patch/9407171/

  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/247328.html

  https://www.spinics.net/lists/linux-wireless/msg84462.html

Not sure what is the right way to let userspace know about GPIO that
serves as rfkill.

>> +	sar-isolation {
>> +		gpio-hog;
>> +		gpios = <15 GPIO_ACTIVE_LOW>;
>> +		output-low;
>> +		line-name = "sar-isolation";
>> +	};
>
> What is SAR?

Sample at Reset. That's the Armada 38x datasheet term. This GPIO is
active at reset time to isolate SAR signals from interference of
external devices. We deassert isolation after reset to make these signal
usable.

baruch

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel