Re: [PATCH] arm64: dts: qcom: Add device tree for Samsung J5 2015 (samsung-j5)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Bjorn,

Thanks a lot for your review!

On Thu, Nov 18, 2021 at 09:31:39PM -0600, Bjorn Andersson wrote:
> On Tue 16 Nov 14:07 CST 2021, Julian Ribbeck wrote:
> 
> > Samsung J5 2015 is a MSM8916 based Smartphone. It is similar to some of the
> > other MSM8916 devices, especially the Samsung ones.
> > 
> > With this patch initial support for the following is added:
> >   - eMMC/SD card
> >   - Buttons
> >   - USB (although no suiting MUIC driver currently)
> >   - UART (untested for lack of equipment)
> >   - WiFi/Bluetooth (WCNSS)
> > 
> > It is worth noting that Samsung J5 with MSM8916 exists in different
> > generations (e.g Samsung J5 2015 and Samsung J5 2016) which each have
> > different models (e.g. samsung-j5nlte, samsung-j5xnlte, etc). This patch
> > is only regarding the 2015 generation, but should work with all of it's
> > models, as far as we could test.
> > 
> > Co-developed-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> > Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> > Signed-off-by: Julian Ribbeck <julian.ribbeck@xxxxxx>
> > ---
> >  arch/arm64/boot/dts/qcom/Makefile             |   1 +
> >  .../boot/dts/qcom/msm8916-samsung-j5.dts      | 209 ++++++++++++++++++
> >  2 files changed, 210 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dts
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> > index 6b816eb33309..08bfccb0daeb 100644
> > --- a/arch/arm64/boot/dts/qcom/Makefile
> > +++ b/arch/arm64/boot/dts/qcom/Makefile
> > @@ -15,6 +15,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-longcheer-l8910.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-mtp.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-samsung-a3u-eur.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-samsung-a5u-eur.dtb
> > +dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-samsung-j5.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-samsung-serranove.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-wingtech-wt88047.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)	+= msm8992-bullhead-rev-101.dtb
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dts b/arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dts
> > new file mode 100644
> > index 000000000000..687bea438a57
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dts
> > @@ -0,0 +1,209 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> 
> If you authored this, could we please get it under BSD license?
> 

I'm afraid the same problem applies to all MSM8916-related device trees:
https://lore.kernel.org/linux-arm-msm/YMIznk4scPv1qOzP@xxxxxxxxxxx/

Given the similarities between the devices it's easiest to take portions
from existing device trees and just change some properties. But this
means that many people were involved and I'm not sure if it is
appropriate to apply a different license without asking all of them.

It's an unfortunate situation that will likely also apply to more
MSM8916 device trees submitted in the future. I hope it's still
acceptable even with the GPL-2.0-only license. :)

> > +
> > +/dts-v1/;
> > +
> > +#include "msm8916-pm8916.dtsi"
> > +#include <dt-bindings/gpio/gpio.h>
> > +
> > +/ {
> > +	model = "Samsung Galaxy J5 (2015)";
> > +	compatible = "samsung,j5", "qcom,msm8916";
> > +	chassis-type = "handset";
> > +
> > +	aliases {
> > +		serial0 = &blsp1_uart2;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial0";
> > +	};
> > +
> > +	reserved-memory {
> > +		/* Additional memory used by Samsung firmware modifications */
> > +		tz-apps@85500000 {
> > +			reg = <0x0 0x85500000 0x0 0xb00000>;
> > +			no-map;
> > +		};
> > +	};
> > +
> > +	gpio-keys {
> > +		compatible = "gpio-keys";
> > +
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&gpio_keys_default>;
> > +
> > +		label = "GPIO Buttons";
> > +
> > +		volume-up {
> > +			label = "Volume Up";
> > +			gpios = <&msmgpio 107 GPIO_ACTIVE_LOW>;
> > +			linux,code = <KEY_VOLUMEUP>;
> > +		};
> > +
> > +		home-key {
> > +			lable = "Home Key";
> > +			gpios = <&msmgpio 109 GPIO_ACTIVE_LOW>;
> > +			linux,code = <KEY_HOMEPAGE>;
> > +		};
> > +	};
> > +};
> > +
> > +&blsp1_uart2 {
> 
> Can you please sort these nodes alphabetically?
> 

It looks mostly alphabetically ordered to me, could you clarify which
nodes you are referring to exactly?

The exceptions are &smd_rpm_regulators and &msmgpio, which are explicitly
at the end of the file (consistent with all other MSM8916 device trees).
I think it's easier to focus on the main interesting part of the device
tree that way (the device definitions). If you prefer to have strict
alphebtical order I can prepare a bulk patch that changes the order in
all the existing MSM8916 device trees. The most important thing for me
is that they are consistent.

Thanks,
Stephan

> 
> > +	status = "okay";
> > +};
> > +
> > +&pm8916_resin {
> > +	status = "okay";
> > +	linux,code = <KEY_VOLUMEDOWN>;
> > +};
> > +
> > +/* FIXME: Replace with SM5703 MUIC when driver is available */
> > +&pm8916_usbin {
> > +	status = "okay";
> > +};
> > +
> > +&pronto {
> > +	status = "okay";
> > +};
> > +
> > +&sdhc_1 {
> > +	status = "okay";
> > +
> > +	pinctrl-names = "default", "sleep";
> > +	pinctrl-0 = <&sdc1_clk_on &sdc1_cmd_on &sdc1_data_on>;
> > +	pinctrl-1 = <&sdc1_clk_off &sdc1_cmd_off &sdc1_data_off>;
> > +};
> > +
> > +&sdhc_2 {
> > +	status = "okay";
> > +
> > +	pinctrl-names = "default", "sleep";
> > +	pinctrl-0 = <&sdc2_clk_on &sdc2_cmd_on &sdc2_data_on &sdc2_cd_on>;
> > +	pinctrl-1 = <&sdc2_clk_off &sdc2_cmd_off &sdc2_data_off &sdc2_cd_off>;
> > +
> > +	cd-gpios = <&msmgpio 38 GPIO_ACTIVE_LOW>;
> > +};
> > +
> > +&usb {
> > +	status = "okay";
> > +	dr_mode = "peripheral";
> > +	extcon = <&pm8916_usbin>;
> > +};
> > +
> > +&usb_hs_phy {
> > +	extcon = <&pm8916_usbin>;
> > +	qcom,init-seq = /bits/ 8 <0x1 0x19 0x2 0x0b>;
> > +};
> > +
> > +&smd_rpm_regulators {
> > +	vdd_l1_l2_l3-supply = <&pm8916_s3>;
> > +	vdd_l4_l5_l6-supply = <&pm8916_s4>;
> > +	vdd_l7-supply = <&pm8916_s4>;
> > +
> > +	s3 {
> > +		regulator-min-microvolt = <1200000>;
> > +		regulator-max-microvolt = <1300000>;
> > +	};
> > +
> > +	s4 {
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <2100000>;
> > +	};
> > +
> > +	l1 {
> > +		regulator-min-microvolt = <1225000>;
> > +		regulator-max-microvolt = <1225000>;
> > +	};
> > +
> > +	l2 {
> > +		regulator-min-microvolt = <1200000>;
> > +		regulator-max-microvolt = <1200000>;
> > +	};
> > +
> > +	l4 {
> > +		regulator-min-microvolt = <2050000>;
> > +		regulator-max-microvolt = <2050000>;
> > +	};
> > +
> > +	l5 {
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <1800000>;
> > +	};
> > +
> > +	l6 {
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <1800000>;
> > +	};
> > +
> > +	l7 {
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <1800000>;
> > +	};
> > +
> > +	l8 {
> > +		regulator-min-microvolt = <2850000>;
> > +		regulator-max-microvolt = <2900000>;
> > +	};
> > +
> > +	l9 {
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +	};
> > +
> > +	l10 {
> > +		regulator-min-microvolt = <2700000>;
> > +		regulator-max-microvolt = <2800000>;
> > +	};
> > +
> > +	l11 {
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <2950000>;
> > +		regulator-allow-set-load;
> > +		regulator-system-load = <200000>;
> > +	};
> > +
> > +	l12 {
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <2950000>;
> > +	};
> > +
> > +	l13 {
> > +		regulator-min-microvolt = <3075000>;
> > +		regulator-max-microvolt = <3075000>;
> > +	};
> > +
> > +	l14 {
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <3300000>;
> > +	};
> > +
> > +	l15 {
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <3300000>;
> > +	};
> > +
> > +	l16 {
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <3300000>;
> > +	};
> > +
> > +	l17 {
> > +		regulator-min-microvolt = <3000000>;
> > +		regulator-max-microvolt = <3000000>;
> > +	};
> > +
> > +	l18 {
> > +		regulator-min-microvolt = <2700000>;
> > +		regulator-max-microvolt = <2700000>;
> > +	};
> > +};
> > +
> > +&msmgpio {
> > +	gpio_keys_default: gpio-keys-default {
> > +		pins = "gpio107", "gpio109";
> > +		function = "gpio";
> > +
> > +		drive-strength = <2>;
> > +		bias-pull-up;
> > +	};
> > +};
> > --
> > 2.33.1
> > 



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux