linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Felipe Balbi <balbi@kernel.org>
To: Konrad Dybcio <konrad.dybcio@somainline.org>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>,
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
	Felipe Balbi <felipe.balbi@microsoft.com>
Subject: Re: [PATCH] arm64: dts: qcom: add initial device-tree for Microsoft Surface Duo
Date: Fri, 28 May 2021 13:00:15 +0300	[thread overview]
Message-ID: <871r9rb2kw.fsf@kernel.org> (raw)
In-Reply-To: <397b7def-99fb-1084-f0bd-6da81066af17@somainline.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 --]

      reply	other threads:[~2021-05-28 10:00 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-10 12:05 Felipe Balbi
2021-05-11  4:43 ` Bjorn Andersson
2021-05-11  8:07   ` Felipe Balbi
2021-05-25 17:32     ` Bjorn Andersson
2021-05-28  9:57       ` Felipe Balbi
2021-05-28 12:03         ` Felipe Balbi
2021-05-29 16:56           ` Bjorn Andersson
2021-06-03 10:19             ` Felipe Balbi
2021-05-18 20:57 ` Konrad Dybcio
2021-05-28 10:00   ` Felipe Balbi [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=871r9rb2kw.fsf@kernel.org \
    --to=balbi@kernel.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=felipe.balbi@microsoft.com \
    --cc=konrad.dybcio@somainline.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH] arm64: dts: qcom: add initial device-tree for Microsoft Surface Duo' \
    /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

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).