Re: [PATCH] arm64: dts: qcom: add initial device-tree for Microsoft Surface Duo

[Felipe Balbi <balbi@kernel.org>]

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


Hi,

(please break lines at 80-columns)

Konrad Dybcio <konrad.dybcio@somainline.org> writes:
>> +	model = "Microsoft Surface Duo";
>> +	compatible = "microsoft,surface-duo", "qcom,sm8150-mtp";
>> +
> Please remove the -mtp compatible, as the phone is not 1:1 compatible
> with the MTP design and replace it with the platform-generic
> "qcom,sm8150" (otherwise things like CPUFreq won't work!).

will do

>> +	aliases {
>> +		serial0 = &uart2;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = "serial0:115200n8";
>> +	};
>> +
>
> Is the serial port exposed and enabled on production hardware?

nope, but we also use the same DTS for devBoards.

>> +	bq27742@55 {
>> +		compatible = "ti,bq27742";
>> +		reg = <0x55>;
>> +	};
>> +
>> +	da7280@4a {
>> +		compatible = "dlg,da7280";
>> +		reg = <0x4a>;
>> +		interrupts-extended = <&tlmm 42 IRQ_TYPE_LEVEL_LOW>;
>> +		pinctrl-names = "da7280_default";
>> +		pinctrl-0 = <&da7280_intr_default>;
>> +
>> +		dlg,actuator-type = "LRA";
>> +		dlg,dlg,const-op-mode = <1>;
>> +		dlg,dlg,periodic-op-mode = <1>;
>> +		dlg,nom-microvolt = <2000000>;
>> +		dlg,abs-max-microvolt = <2000000>;
>> +		dlg,imax-microamp = <129000>;
>> +		dlg,resonant-freq-hz = <180>;
>> +		dlg,impd-micro-ohms = <14300000>;
>> +		dlg,freq-track-enable;
>> +		dlg,bemf-sens-enable;
>> +		dlg,mem-array = <
>> +		  0x06 0x08 0x10 0x11 0x12 0x13 0x14 0x15 0x1c 0x2a
>> +		  0x33 0x3c 0x42 0x4b 0x4c 0x4e 0x17 0x19 0x27 0x29
>> +		  0x17 0x19 0x03 0x84 0x5e 0x04 0x08 0x84 0x5d 0x01
>> +		  0x84 0x5e 0x02 0x00 0xa4 0x5d 0x03 0x84 0x5e 0x06
>> +		  0x08 0x84 0x5d 0x05 0x84 0x5d 0x06 0x84 0x5e 0x08
>> +		  0x84 0x5e 0x05 0x8c 0x5e 0x24 0x84 0x5f 0x10 0x84
>> +		  0x5e 0x05 0x84 0x5e 0x08 0x84 0x5f 0x01 0x8c 0x5e
>> +		  0x04 0x84 0x5e 0x08 0x84 0x5f 0x11 0x19 0x88 0x00
>> +		  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>> +		  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>> +		>;
>> +	};
>> +
>> +	/* SMB1381 @ 0x44 */
>> +	/* MAX34417 @ 0x1c */
>> +};
>
> Please add generic labels to these peripherals, such as "vibrator: " or so.

why? They haven't been added yet, they don't even have drivers
available. What extra information will a comment bring?

>> +&i2c17 {
>> +	status = "okay";
>> +	clock-frequency = <400000>;
>> +
>> +	bq27742@55 {
>> +		compatible = "ti,bq27742";
>> +		reg = <0x55>;
>> +	};
>> +};
>
> Are there actually two of these TI ICs, presumably for the

yes

> dual-batteries? If that's the case, could you add a comment specifying
> which one is in charge (pun intended) of which cell?

that's detected, and reported, by the HW itself. Also, what's the point
if SW isn't really doing anything with the batteries other than checking
charge level and such?

-- 
balbi

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