| Message ID | 20230524230459.120681-1-guptarud@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] ARM: Add label to sleep_clk in qcom-msm8960.dtsi | expand |
On Wed, May 24, 2023 at 07:04:54PM -0400, Rudraksha Gupta wrote: Thank you for your patches Rudraksha! Please use: git log --oneline -- arch/arm/boot/dts/qcom-smm8960.dtsi And look at how other patches to this file has prefixed their subject line. Then please follow this in your patches (it differs between patches...). > Allows msm8960 DTS files to reference the sleep_clk node. > Please make the purpose clear in your commit message. Specifically, which part of msm8960 dts needs to reference the sleep clock? Please also include a cover letter (git format-patch --cover-letter) when you send a series of patches. Regards, Bjorn > Signed-off-by: Rudraksha Gupta <guptarud@gmail.com> > --- > arch/arm/boot/dts/qcom-msm8960.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom-msm8960.dtsi > index 2a668cd535cc..a4d8dd2d24a6 100644 > --- a/arch/arm/boot/dts/qcom-msm8960.dtsi > +++ b/arch/arm/boot/dts/qcom-msm8960.dtsi > @@ -71,7 +71,7 @@ pxo_board: pxo_board { > clock-output-names = "pxo_board"; > }; > > - sleep_clk { > + sleep_clk: sleep_clk { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <32768>; > -- > 2.34.1 >
> Where did the "att" come from in the compatible. Is this some carrier
specific model of the phone?
This is the code name for the device. Since there are usually multiple
variants of a device, using the code name differentiates between those
variants. For example, if I left this as "samsung,express", it would be
unclear if I am referring to the GT-I8730 (code name: expresslte) or the
SGH-I437 model. This is typically done in postmarketOS:
https://wiki.postmarketos.org/wiki/Devices and XDA developers. I believe
it is a carrier specific model of the Samsung Galaxy Express.
On Mon May 29, 2023 at 3:41 AM EDT, Konrad Dybcio wrote: > > > On 28.05.2023 15:36, Rudraksha Gupta wrote: > >> Where did the "att" come from in the compatible. Is this some carrier specific model of the phone? > > > > This is the code name for the device. Since there are usually multiple variants of a device, using the code name differentiates between those variants. For example, if I left this as "samsung,express", it would be unclear if I am referring to the GT-I8730 (code name: expresslte) or the SGH-I437 model. This is typically done in postmarketOS: https://wiki.postmarketos.org/wiki/Devices and XDA developers. I believe it is a carrier specific model of the Samsung Galaxy Express. > > > > Please fix your email client: > > - wrap each line at about 80 characters > - don't trim messages unless they're very long or > there's some other good reason > - Don't send v(n+1) as a reply to v(n), send it in a > separate thread. > Gotcha, I will do that next time. Should I still send v2 in a separate thread? Not sure if I should as I don't want to spam anyone. > > > Konrad
On Mon May 29, 2023 at 6:37 PM EDT, Conor Dooley wrote: > On Mon, May 29, 2023 at 06:28:05PM -0400, Rudraksha Gupta wrote: > > > Gotcha, I will do that next time. Should I still send v2 in a separate > > thread? > > That'd then be either [PATCH v3], or [RESEND PATCH v2]. Ideally v3 since > you will have added the tags you received on v2. > AFAIK Bjorn uses b4, which should be able to deal with the series as-is, > but sending a v3 with the Acks etc would not be a bad idea. You > should probably give it a couple days before doing that to see if it > gets picked up before doing that. > > > Not sure if I should as I don't want to spam anyone. > > Sending new revisions of patches as a new thread is not spamming, we > signed up to be maintainers, or get to email from the lists, after all. > Sending several revisions of the same patches in a day would be spamming > though, but that is not what we are talking about here ;) > > Cheers, > Conor. Ah, I see. Thank you for being patient with me. I'm really excited to see my patch go in! :)
On Sat, 27 May 2023 20:10:06 -0400, Rudraksha Gupta wrote: > Add a compatible for Samsung Galaxy Express SGH-I437. > > Signed-off-by: Rudraksha Gupta <guptarud@gmail.com> > --- > Documentation/devicetree/bindings/arm/qcom.yaml | 1 + > 1 file changed, 1 insertion(+) > Running 'make dtbs_check' with the schema in this patch gives the following warnings. Consider if they are expected or the schema is incorrect. These may not be new warnings. Note that it is not yet a requirement to have 0 warnings for dtbs_check. This will change in the future. Full log is available here: https://patchwork.ozlabs.org/patch/1786779 /: qcom,board-id: False schema does not allow [[8, 0]] arch/arm/boot/dts/qcom-msm8974pro-oneplus-bacon.dtb /: qcom,msm-id: False schema does not allow [[194, 65536]] arch/arm/boot/dts/qcom-msm8974pro-oneplus-bacon.dtb
On 28/05/2023 02:10, Rudraksha Gupta wrote: > This patch series adds support for the Samsung Galaxy Express SGH-I437. > Currently the following things work on this phone: UART, eMMC, SD Card, and USB. > > version 2: > - Combined patch 1 into patch 4, as the sleep_clk label is specifically needed for the USB node. > - Reformatted the commit messages to align with the style used in other commit messages that modify the same files. > - Included a cover letter to provide an overview of the patch series. > - Slight refactoring of the device tree source (DTS) file. > Do not attach (thread) your patchsets to some other threads (unrelated or older versions). This buries them deep in the mailbox and might interfere with applying entire sets. Best regards, Krzysztof
On 28/05/2023 02:10, Rudraksha Gupta wrote: > Add the required nodes to support USB on the MSM8960 SoC. As it's very > similar to the APQ8064 SoC, the nodes are almost identical > > Add a label to sleep_clk for the USB node to reference > > Signed-off-by: Rudraksha Gupta <guptarud@gmail.com> > + usb1: usb@12500000 { > + compatible = "qcom,ci-hdrc"; > + reg = <0x12500000 0x200>, > + <0x12500200 0x200>; > + interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&gcc USB_HS1_XCVR_CLK>, <&gcc USB_HS1_H_CLK>; > + clock-names = "core", "iface"; > + assigned-clocks = <&gcc USB_HS1_XCVR_CLK>; > + assigned-clock-rates = <60000000>; > + resets = <&gcc USB_HS1_RESET>; > + reset-names = "core"; > + phy_type = "ulpi"; > + ahb-burst-config = <0>; > + phys = <&usb_hs1_phy>; > + phy-names = "usb-phy"; > + status = "disabled"; status is the last > + #reset-cells = <1>; > + > + ulpi { > + usb_hs1_phy: phy { > + compatible = "qcom,usb-hs-phy-msm8960", > + "qcom,usb-hs-phy"; > + clocks = <&sleep_clk>, <&cxo_board>; > + clock-names = "sleep", "ref"; > + resets = <&usb1 0>; > + reset-names = "por"; > + #phy-cells = <0>; > + }; > + }; > + }; > + Don't add stray blank lines. Best regards, Krzysztof
Thank you for your patch. There is something to discuss/improve. On 28/05/2023 02:10, Rudraksha Gupta wrote: > This adds a very basic device tree file for the Samsung Galaxy Express Please do not use "This commit/patch", but imperative mood. See longer explanation here: https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > SGH-I437. Currently, the following things work: UART, eMMC, SD Card, and > USB. > > Signed-off-by: Rudraksha Gupta <guptarud@gmail.com> > --- > arch/arm/boot/dts/Makefile | 1 + > .../dts/qcom-msm8960-samsung-expressatt.dts | 334 ++++++++++++++++++ > 2 files changed, 335 insertions(+) > create mode 100644 arch/arm/boot/dts/qcom-msm8960-samsung-expressatt.dts > > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile > index 59829fc90315..12c90f263142 100644 > --- a/arch/arm/boot/dts/Makefile > +++ b/arch/arm/boot/dts/Makefile > @@ -1081,6 +1081,7 @@ dtb-$(CONFIG_ARCH_QCOM) += \ > qcom-msm8916-samsung-grandmax.dtb \ > qcom-msm8916-samsung-serranove.dtb \ > qcom-msm8960-cdp.dtb \ > + qcom-msm8960-samsung-expressatt.dtb \ > qcom-msm8974-lge-nexus5-hammerhead.dtb \ > qcom-msm8974-sony-xperia-rhine-amami.dtb \ > qcom-msm8974-sony-xperia-rhine-honami.dtb \ > diff --git a/arch/arm/boot/dts/qcom-msm8960-samsung-expressatt.dts b/arch/arm/boot/dts/qcom-msm8960-samsung-expressatt.dts > new file mode 100644 > index 000000000000..a1ee9c272558 > --- /dev/null > +++ b/arch/arm/boot/dts/qcom-msm8960-samsung-expressatt.dts > @@ -0,0 +1,334 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <dt-bindings/input/input.h> > + > +#include "qcom-msm8960.dtsi" > +#include <dt-bindings/reset/qcom,gcc-msm8960.h> > + > +/ { > + model = "Samsung Galaxy Express SGH-I437"; > + compatible = "samsung,expressatt", "qcom,msm8960"; > + chassis-type = "handset"; > + > + aliases { > + serial0 = &gsbi5_serial; > + mmc0 = &sdcc1; /* SDCC1 eMMC slot */ > + mmc1 = &sdcc3; /* SDCC3 SD card slot */ > + }; > + > + chosen { > + stdout-path = "serial0:115200n8"; > + }; > +}; > + > +&gsbi5 { > + qcom,mode = <GSBI_PROT_I2C_UART>; > + status = "okay"; > +}; > + > +&gsbi5_serial { > + status = "okay"; > +}; > + > +&sdcc1 { > + vmmc-supply = <&pm8921_l5>; > + status = "okay"; > +}; > + > +&sdcc3 { > + vmmc-supply = <&pm8921_l6>; > + vqmmc-supply = <&pm8921_l7>; > + status = "okay"; > +}; > + > +&gsbi1 { > + qcom,mode = <GSBI_PROT_SPI>; > + pinctrl-0 = <&spi1_default>; > + pinctrl-names = "default"; > + status = "okay"; > +}; > + > +&gsbi1_spi { > + status = "okay"; > +}; > + > +&msmgpio { > + spi1_default: spi1_default { No underscores in node names, missing proper suffix. It does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). > + mux { It does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). > + pins = "gpio6", "gpio7", "gpio9"; > + function = "gsbi1"; > + }; > + > + mosi { It does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). > + pins = "gpio6"; > + drive-strength = <12>; > + bias-disable; > + }; > + > + miso { > + pins = "gpio7"; > + drive-strength = <12>; > + bias-disable; > + }; > + > + cs { > + pins = "gpio8"; > + drive-strength = <12>; > + bias-disable; > + output-low; > + }; > + > + clk { > + pins = "gpio9"; > + drive-strength = <12>; > + bias-disable; > + }; > + }; > +}; > + > + One blank line, not two. ... > +&usb1 { > + dr_mode = "otg"; > + status = "okay"; > +}; > + No improvements here. Best regards, Krzysztof
On Tue May 30, 2023 at 9:22 AM EDT, Krzysztof Kozlowski wrote: > On 28/05/2023 02:10, Rudraksha Gupta wrote: > > Adds qcom,usb-hs-phy-msm8960 compatible > > > > Please use subject prefixes matching the subsystem. You can get them for > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > your patch is touching. > > Best regards, > Krzysztof It seems like "dt-bindings:" is the subject prefix for this file. Would you like me to use another prefix instead?
On Wed, 31 May 2023 at 05:49, Rudraksha Gupta <guptarud@gmail.com> wrote: > > On Tue May 30, 2023 at 9:22 AM EDT, Krzysztof Kozlowski wrote: > > On 28/05/2023 02:10, Rudraksha Gupta wrote: > > > Adds qcom,usb-hs-phy-msm8960 compatible > > > > > > > Please use subject prefixes matching the subsystem. You can get them for > > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > > your patch is touching. > > > > Best regards, > > Krzysztof > > It seems like "dt-bindings:" is the subject prefix for this file. Would > you like me to use another prefix instead? At least it should be "dt-bindings: phy: ". However as this change covers existing file, it probably should be "dt-bindings: phy: qcom,usb-hs-phy: "
diff --git a/arch/arm/boot/dts/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom-msm8960.dtsi index 2a668cd535cc..a4d8dd2d24a6 100644 --- a/arch/arm/boot/dts/qcom-msm8960.dtsi +++ b/arch/arm/boot/dts/qcom-msm8960.dtsi @@ -71,7 +71,7 @@ pxo_board: pxo_board { clock-output-names = "pxo_board"; }; - sleep_clk { + sleep_clk: sleep_clk { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <32768>;
Allows msm8960 DTS files to reference the sleep_clk node. Signed-off-by: Rudraksha Gupta <guptarud@gmail.com> --- arch/arm/boot/dts/qcom-msm8960.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)