Re: [PATCH v2 1/6] ARM: tegra: Add device-tree for Acer Iconia Tab A500

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3611 bytes --]

On Mon, Apr 06, 2020 at 10:41:05PM +0300, Dmitry Osipenko wrote:
[...]
> diff --git a/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts b/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
[...]
> +	host1x@50000000 {
> +		dc@54200000 {
> +			rgb {
> +				status = "okay";
> +				nvidia,panel = <&panel>;
> +
> +				port@0 {
> +					lvds_output: endpoint {
> +						remote-endpoint = <&lvds_encoder_input>;
> +						bus-width = <18>;
> +					};
> +				};
> +			};
> +		};

This seems a little strange to me, though, admittedly, I've never worked
with these types of bridges before, so I may be misunderstanding this. I
was under the impression that we could obtain the panel by traversing an
OF graph, so that we didn't have to have that extra nvidia,panel
property. As it is, you seem to describe two different paths, one that
goes from the RGB output to the panel directly, and another that goes
from the RGB output to the LVDS encoder and then to the panel.

It doesn't seem to me like a direct link from RGB output to panel does
actually exist in this setup.

[...]
> +	pwm: pwm@7000a000 {
> +		status = "okay";
> +		power-supply = <&vdd_3v3_sys>;
> +	};

I don't see power-supply defined as a property for the PWM controller.
Why do you need this?

[...]
> +	sdhci@c8000000 {
> +		status = "okay";
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		max-frequency = <25000000>;
> +		keep-power-in-suspend;
> +		bus-width = <4>;
> +		non-removable;
> +
> +		mmc-pwrseq = <&brcm_wifi_pwrseq>;
> +		vmmc-supply = <&vdd_3v3_sys>;
> +		vqmmc-supply = <&vdd_3v3_sys>;
> +
> +		/* Azurewave AW-NH611 BCM4329 */
> +		WiFi@1 {

I think these names are supposed to be lowercase.

> +			reg = <1>;
> +			compatible = "brcm,bcm4329-fmac";
> +			interrupt-parent = <&gpio>;
> +			interrupts = <TEGRA_GPIO(S, 0) IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "host-wake";
> +		};
> +	};
[...]
> +	clocks {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		clk32k_in: clock@0 {
> +			compatible = "fixed-clock";
> +			reg = <0>;
> +			#clock-cells = <0>;
> +			clock-frequency = <32768>;
> +			clock-output-names = "tps658621-out32k";
> +		};
> +
> +		rtc_32k_wifi: clock@1 {
> +			compatible = "fixed-clock";
> +			reg = <1>;
> +			#clock-cells = <0>;
> +			clock-frequency = <32768>;
> +			clock-output-names = "kk3270032";
> +		};
> +	};

Are these clocks going to the PMIC and RTC, or are they generated by the
chips? If they are generated by the chips, which sounds like they might
be, wouldn't it be better to represent them as children of the
corresponding chips?

There's probably no infrastructure to do this, so maybe that would be
overkill. But for clarity it might be worth documenting here where
exactly these clocks come from.

[...]
> +	memory-controller@7000f400 {
> +		nvidia,use-ram-code;
> +
> +		emc-tables@elpida-8gb {

I don't think unit-addresses are supposed to be freeform text like
above. These should always reflect the value of the "reg" property,
though in this case we don't have one...

> +			nvidia,ram-code = <0>;

In retrospect it might have been better to just reuse the reg property
for this.

I think in this case it might be best to reflect the RAM code in the
unit-address. At least that way we conceptually get things right since
it's the RAM code that selects which of these tables is used, much like
a register, I2C slave address, or SPI chip select would select which of
the subdevices are targetted.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]