| Message ID | 20250225093051.58406-1-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: qcom: x1e80100-slim7x: Drop incorrect qcom,ath12k-calibration-variant | expand |
On Tue, Feb 25, 2025 at 10:30:51AM +0100, Krzysztof Kozlowski wrote: > There is no such property as qcom,ath12k-calibration-variant: neither in > the bindings nor in the driver. See dtbs_check: > > x1e80100-lenovo-yoga-slim7x.dtb: wifi@0: 'qcom,ath12k-calibration-variant' does not match any of the regexes: 'pinctrl-[0-9]+' > Adding Jeff and ath12k@ to the cc list. Is the driver able to find the calibration variant in case it is not running on the ACPI system? I see that it uses dmi_walk. Does it work in the non-ACPI case? > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts b/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts > index a3d53f2ba2c3..9aff5a1f044d 100644 > --- a/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts > +++ b/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts > @@ -674,8 +674,6 @@ &pcie4_port0 { > wifi@0 { > compatible = "pci17cb,1107"; > reg = <0x10000 0x0 0x0 0x0 0x0>; > - > - qcom,ath12k-calibration-variant = "LES790"; > }; > }; > > -- > 2.43.0 >
On 25/02/2025 10:50, Dmitry Baryshkov wrote: > On Tue, Feb 25, 2025 at 10:30:51AM +0100, Krzysztof Kozlowski wrote: >> There is no such property as qcom,ath12k-calibration-variant: neither in >> the bindings nor in the driver. See dtbs_check: >> >> x1e80100-lenovo-yoga-slim7x.dtb: wifi@0: 'qcom,ath12k-calibration-variant' does not match any of the regexes: 'pinctrl-[0-9]+' >> > > Adding Jeff and ath12k@ to the cc list. Is the driver able to find the > calibration variant in case it is not running on the ACPI system? I see > that it uses dmi_walk. Does it work in the non-ACPI case? But nothing parses such string as 'qcom,ath12k-calibration-variant' (see git grep), so how would driver use it? Best regards, Krzysztof
On Tue, Feb 25, 2025 at 11:22:25AM +0100, Krzysztof Kozlowski wrote: > On 25/02/2025 10:50, Dmitry Baryshkov wrote: > > On Tue, Feb 25, 2025 at 10:30:51AM +0100, Krzysztof Kozlowski wrote: > >> There is no such property as qcom,ath12k-calibration-variant: neither in > >> the bindings nor in the driver. See dtbs_check: > >> > >> x1e80100-lenovo-yoga-slim7x.dtb: wifi@0: 'qcom,ath12k-calibration-variant' does not match any of the regexes: 'pinctrl-[0-9]+' > >> > > > > Adding Jeff and ath12k@ to the cc list. Is the driver able to find the > > calibration variant in case it is not running on the ACPI system? I see > > that it uses dmi_walk. Does it work in the non-ACPI case? > > > But nothing parses such string as 'qcom,ath12k-calibration-variant' (see > git grep), so how would driver use it? That's what I'm asking: is the property redundant or is it correct and it is a driver that needs to be fixed?
On 25/02/2025 12:45, Dmitry Baryshkov wrote: > On Tue, Feb 25, 2025 at 11:22:25AM +0100, Krzysztof Kozlowski wrote: >> On 25/02/2025 10:50, Dmitry Baryshkov wrote: >>> On Tue, Feb 25, 2025 at 10:30:51AM +0100, Krzysztof Kozlowski wrote: >>>> There is no such property as qcom,ath12k-calibration-variant: neither in >>>> the bindings nor in the driver. See dtbs_check: >>>> >>>> x1e80100-lenovo-yoga-slim7x.dtb: wifi@0: 'qcom,ath12k-calibration-variant' does not match any of the regexes: 'pinctrl-[0-9]+' >>>> >>> >>> Adding Jeff and ath12k@ to the cc list. Is the driver able to find the >>> calibration variant in case it is not running on the ACPI system? I see >>> that it uses dmi_walk. Does it work in the non-ACPI case? >> >> >> But nothing parses such string as 'qcom,ath12k-calibration-variant' (see >> git grep), so how would driver use it? > > That's what I'm asking: is the property redundant or is it correct and > it is a driver that needs to be fixed? I assume driver will need something like that property, but that's not a reason to accept incorrect one in DTS. One cannot add properties to DTS without bindings, so bypassing bindings review, and then claim "but my driver needs them". Send proper patches for driver first which will get a review. This could be instead renamed to final correct property, but since there is no user, no indication it is needed or correct, I cannot prepare such patch. I would not know what to write (e.g. "rename to qcom,foo-bar, because I have no clue"). Best regards, Krzysztof
On 2/25/2025 4:14 AM, Krzysztof Kozlowski wrote: > On 25/02/2025 12:45, Dmitry Baryshkov wrote: >> On Tue, Feb 25, 2025 at 11:22:25AM +0100, Krzysztof Kozlowski wrote: >>> On 25/02/2025 10:50, Dmitry Baryshkov wrote: >>>> On Tue, Feb 25, 2025 at 10:30:51AM +0100, Krzysztof Kozlowski wrote: >>>>> There is no such property as qcom,ath12k-calibration-variant: neither in >>>>> the bindings nor in the driver. See dtbs_check: >>>>> >>>>> x1e80100-lenovo-yoga-slim7x.dtb: wifi@0: 'qcom,ath12k-calibration-variant' does not match any of the regexes: 'pinctrl-[0-9]+' >>>>> >>>> >>>> Adding Jeff and ath12k@ to the cc list. Is the driver able to find the >>>> calibration variant in case it is not running on the ACPI system? I see >>>> that it uses dmi_walk. Does it work in the non-ACPI case? >>> >>> >>> But nothing parses such string as 'qcom,ath12k-calibration-variant' (see >>> git grep), so how would driver use it? >> >> That's what I'm asking: is the property redundant or is it correct and >> it is a driver that needs to be fixed? > > I assume driver will need something like that property, but that's not a > reason to accept incorrect one in DTS. One cannot add properties to DTS > without bindings, so bypassing bindings review, and then claim "but my > driver needs them". Send proper patches for driver first which will get > a review. We definitely need a calibration variant entry. I've pinged the development team to get the driver patch. I'm also verifying internally that there are no issues with your renaming proposal: qcom,ath1*k-calibration-variant => qcom,calibration-variant https://msgid.link/20250225-b-wifi-qcom-calibration-variant-v1-0-3b2aa3f89c53@linaro.org /jeff
On 25/02/2025 17:44, Jeff Johnson wrote: >>>> >>>> But nothing parses such string as 'qcom,ath12k-calibration-variant' (see >>>> git grep), so how would driver use it? >>> >>> That's what I'm asking: is the property redundant or is it correct and >>> it is a driver that needs to be fixed? >> >> I assume driver will need something like that property, but that's not a >> reason to accept incorrect one in DTS. One cannot add properties to DTS >> without bindings, so bypassing bindings review, and then claim "but my >> driver needs them". Send proper patches for driver first which will get >> a review. > > We definitely need a calibration variant entry. > I've pinged the development team to get the driver patch. The patches were on the lists but were not accepted. Therefore DTS property cannot get into the kernel. I am sorry, but this is not somehow fluid or flexible that internal team can squeeze something into the kernel. Also post factum reasoning is not correct, because this would open the gate to bypass any sort of review. Just squeeze your stuff into the DTS and then you can bypass all DT maintainers :/ All properties must be documented and bindings must be accepted *before* DTS patch is applied. Best regards, Krzysztof
On Tue, Feb 25, 2025 at 08:44:57AM -0800, Jeff Johnson wrote: > On 2/25/2025 4:14 AM, Krzysztof Kozlowski wrote: > > On 25/02/2025 12:45, Dmitry Baryshkov wrote: > >> On Tue, Feb 25, 2025 at 11:22:25AM +0100, Krzysztof Kozlowski wrote: > >>> On 25/02/2025 10:50, Dmitry Baryshkov wrote: > >>>> On Tue, Feb 25, 2025 at 10:30:51AM +0100, Krzysztof Kozlowski wrote: > >>>>> There is no such property as qcom,ath12k-calibration-variant: neither in > >>>>> the bindings nor in the driver. See dtbs_check: > >>>>> > >>>>> x1e80100-lenovo-yoga-slim7x.dtb: wifi@0: 'qcom,ath12k-calibration-variant' does not match any of the regexes: 'pinctrl-[0-9]+' > >>>>> > >>>> > >>>> Adding Jeff and ath12k@ to the cc list. Is the driver able to find the > >>>> calibration variant in case it is not running on the ACPI system? I see > >>>> that it uses dmi_walk. Does it work in the non-ACPI case? > >>> > >>> > >>> But nothing parses such string as 'qcom,ath12k-calibration-variant' (see > >>> git grep), so how would driver use it? > >> > >> That's what I'm asking: is the property redundant or is it correct and > >> it is a driver that needs to be fixed? > > > > I assume driver will need something like that property, but that's not a > > reason to accept incorrect one in DTS. One cannot add properties to DTS > > without bindings, so bypassing bindings review, and then claim "but my > > driver needs them". Send proper patches for driver first which will get > > a review. > > We definitely need a calibration variant entry. > I've pinged the development team to get the driver patch. I think we need a confirmation from sobody using Slim7x if the driver can read info from DMI or if it can not. In the end, DMI != ACPI. > > I'm also verifying internally that there are no issues with your renaming > proposal: qcom,ath1*k-calibration-variant => qcom,calibration-variant > https://msgid.link/20250225-b-wifi-qcom-calibration-variant-v1-0-3b2aa3f89c53@linaro.org > > /jeff
On 2/25/2025 9:07 AM, Krzysztof Kozlowski wrote: > On 25/02/2025 17:44, Jeff Johnson wrote: >>>>> >>>>> But nothing parses such string as 'qcom,ath12k-calibration-variant' (see >>>>> git grep), so how would driver use it? >>>> >>>> That's what I'm asking: is the property redundant or is it correct and >>>> it is a driver that needs to be fixed? >>> >>> I assume driver will need something like that property, but that's not a >>> reason to accept incorrect one in DTS. One cannot add properties to DTS >>> without bindings, so bypassing bindings review, and then claim "but my >>> driver needs them". Send proper patches for driver first which will get >>> a review. >> >> We definitely need a calibration variant entry. >> I've pinged the development team to get the driver patch. > > > The patches were on the lists but were not accepted. Therefore DTS > property cannot get into the kernel. I am sorry, but this is not somehow > fluid or flexible that internal team can squeeze something into the kernel. I see bindings and DTS patches but no driver patch, even in my internal queue. > > Also post factum reasoning is not correct, because this would open the > gate to bypass any sort of review. Just squeeze your stuff into the DTS > and then you can bypass all DT maintainers :/ > > All properties must be documented and bindings must be accepted *before* > DTS patch is applied. There is no intention to bypass DT maintainers. We are just trying to upstream a large amount of downstream code, and in the process some pieces are coming out of order. And there is also confusion if binding, driver, and DTS changes should be in one series or three separate series. We are moving towards an upstream-first model, but we still have to address the existing technical debt. /jeff
On 25/02/2025 18:36, Jeff Johnson wrote: > >> >> Also post factum reasoning is not correct, because this would open the >> gate to bypass any sort of review. Just squeeze your stuff into the DTS >> and then you can bypass all DT maintainers :/ >> >> All properties must be documented and bindings must be accepted *before* >> DTS patch is applied. > > There is no intention to bypass DT maintainers. We are just trying to upstream > a large amount of downstream code, and in the process some pieces are coming I don't see how this is related here - patch was not sent by anyone from Qualcomm. > out of order. And there is also confusion if binding, driver, and DTS changes > should be in one series or three separate series. How is it related to incorrect property here? It feels like this topic is being hijacked for some other point. I am not happy with this because then Bjorn will see that discussion is going so he will ignore the patch. BTW, I gave my statement multiple times, writing bindings also mention this, so is anything going to change if I say it 100th time here? In one month there will be the same question :/ DTS must be applied via ARM SoC, thus you cannot combine DTS into patchsets being entirely applied by driver subsystem maintainers (Greg, netdev, sometimes maybe watchdog). For other maintainers, you can combine it, because they know to skip DTS. Bindings always go via subsystem, so they must be part of driver patchset, unless of course there is no driver (but then bindings are "the driver" patchset). Best regards, Krzysztof
On 25/02/2025 18:59, Krzysztof Kozlowski wrote: > On 25/02/2025 18:36, Jeff Johnson wrote: >> >>> >>> Also post factum reasoning is not correct, because this would open the >>> gate to bypass any sort of review. Just squeeze your stuff into the DTS >>> and then you can bypass all DT maintainers :/ >>> >>> All properties must be documented and bindings must be accepted *before* >>> DTS patch is applied. >> >> There is no intention to bypass DT maintainers. We are just trying to upstream >> a large amount of downstream code, and in the process some pieces are coming > > I don't see how this is related here - patch was not sent by anyone from > Qualcomm. > >> out of order. And there is also confusion if binding, driver, and DTS changes >> should be in one series or three separate series. > > How is it related to incorrect property here? It feels like this topic > is being hijacked for some other point. I am not happy with this because > then Bjorn will see that discussion is going so he will ignore the patch. > > BTW, I gave my statement multiple times, writing bindings also mention > this, so is anything going to change if I say it 100th time here? In one > month there will be the same question :/ Another BTW, not helpful to community, but if you asked above for Qualcomm, that Qualcomm does not know where to post DTS, then you are lucky, because your extensive internal guideline has it already very clearly documented (detailed in "Driver upstreaming process" and a bit in "feedback/review on lists"). The guide is quite comprehensive and covers all typical cases, like that one. Best regards, Krzysztof
diff --git a/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts b/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts index a3d53f2ba2c3..9aff5a1f044d 100644 --- a/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts +++ b/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts @@ -674,8 +674,6 @@ &pcie4_port0 { wifi@0 { compatible = "pci17cb,1107"; reg = <0x10000 0x0 0x0 0x0 0x0>; - - qcom,ath12k-calibration-variant = "LES790"; }; };
There is no such property as qcom,ath12k-calibration-variant: neither in the bindings nor in the driver. See dtbs_check: x1e80100-lenovo-yoga-slim7x.dtb: wifi@0: 'qcom,ath12k-calibration-variant' does not match any of the regexes: 'pinctrl-[0-9]+' Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts | 2 -- 1 file changed, 2 deletions(-)