| Message ID | 20221101161801.1058969-1-luca@z3ntu.xyz |
|---|---|
| State | Accepted |
| Commit | 901ae537b7977716dc6d2db34b3a75d64a72ab2b |
| Headers | show |
| Series | [v3,1/2] dt-bindings: backlight: qcom-wled: Add PMI8950 compatible | expand |
On 2022-11-01 17:18:00, Luca Weiss wrote: > From: AngeloGioacchino Del Regno <kholk11@gmail.com> > > The PMI8950 features integrated peripherals like ADC, GPIO controller, > MPPs and others. > > [luca@z3ntu.xyz: remove pm8950, style changes for 2022 standards, add wled] > Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz> > --- > Changes since v2: > * Pick up patch, and adjust as mentioned above sign-offs > > arch/arm64/boot/dts/qcom/pmi8950.dtsi | 97 +++++++++++++++++++++++++++ > 1 file changed, 97 insertions(+) > create mode 100644 arch/arm64/boot/dts/qcom/pmi8950.dtsi > > diff --git a/arch/arm64/boot/dts/qcom/pmi8950.dtsi b/arch/arm64/boot/dts/qcom/pmi8950.dtsi > new file mode 100644 > index 000000000000..32d27e2187e3 > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/pmi8950.dtsi > @@ -0,0 +1,97 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2019, AngeloGioacchino Del Regno <kholk11@gmail.com> > + > +#include <dt-bindings/iio/qcom,spmi-vadc.h> > +#include <dt-bindings/interrupt-controller/irq.h> > +#include <dt-bindings/spmi/spmi.h> > + > +&spmi_bus { > + pmic@2 { > + compatible = "qcom,pmi8950", "qcom,spmi-pmic"; > + reg = <0x2 SPMI_USID>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + pmi8950_vadc: adc@3100 { > + compatible = "qcom,spmi-vadc"; > + reg = <0x3100>; > + interrupts = <0x2 0x31 0x0 IRQ_TYPE_EDGE_RISING>; > + #address-cells = <1>; > + #size-cells = <0>; > + #io-channel-cells = <1>; > + > + adc-chan@0 { > + reg = <VADC_USBIN>; > + qcom,pre-scaling = <1 4>; > + label = "usbin"; I've previously sent a patch with labels in the node name instead [1], what's the preferred way nowadays? [1]: https://lore.kernel.org/linux-arm-msm/20220926190148.283805-4-marijn.suijten@somainline.org/ > + }; > + > + adc-chan@1 { > + reg = <VADC_DCIN>; > + qcom,pre-scaling = <1 4>; > + label = "dcin"; > + }; > + > + adc-chan@2 { > + reg = <VADC_VCHG_SNS>; > + qcom,pre-scaling = <1 1>; > + label = "vchg_sns"; Shall we use hyphens in labels (especially if using them as node names). > + }; > + > + adc-chan@9 { > + reg = <VADC_REF_625MV>; > + qcom,pre-scaling = <1 1>; > + label = "ref_625mv"; > + }; > + > + adc-chan@a { > + reg = <VADC_REF_1250MV>; > + qcom,pre-scaling = <1 1>; > + label = "ref_1250v"; mv* :) > + }; > + > + adc-chan@d { > + reg = <VADC_SPARE2>; > + qcom,pre-scaling = <1 1>; > + label = "chg_temp"; > + }; > + }; > + > + pmi8950_mpps: mpps@a000 { > + compatible = "qcom,pmi8950-mpp", "qcom,spmi-mpp"; > + reg = <0xa000>; > + gpio-controller; > + gpio-ranges = <&pmi8950_mpps 0 0 4>; > + #gpio-cells = <2>; > + interrupt-controller; > + #interrupt-cells = <2>; > + }; > + > + pmi8950_gpio: gpio@c000 { It seems more common to name this label plural pmi8950_gpioS? > + compatible = "qcom,pmi8950-gpio", "qcom,spmi-gpio"; > + reg = <0xc000>; > + gpio-controller; > + gpio-ranges = <&pmi8950_gpio 0 0 2>; > + #gpio-cells = <2>; > + interrupt-controller; > + #interrupt-cells = <2>; > + }; > + }; > + > + pmic@3 { > + compatible = "qcom,pmi8950", "qcom,spmi-pmic"; > + reg = <0x3 SPMI_USID>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + pmi8950_wled: leds@d800 { > + compatible = "qcom,pmi8950-wled"; > + reg = <0xd800>, <0xd900>; > + interrupts = <0x3 0xd8 0x02 IRQ_TYPE_EDGE_RISING>; > + interrupt-names = "short"; > + label = "backlight"; Same question here about using the node name instead, and dropping the label altogether. - Marijn > + > + status = "disabled"; > + }; > + }; > +}; > -- > 2.38.1 >
On 2022-11-05 00:44:37, Marijn Suijten wrote: > On 2022-11-01 17:18:00, Luca Weiss wrote: > > From: AngeloGioacchino Del Regno <kholk11@gmail.com> > > > > The PMI8950 features integrated peripherals like ADC, GPIO controller, > > MPPs and others. > > > > [luca@z3ntu.xyz: remove pm8950, style changes for 2022 standards, add wled] > > Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com> > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz> > > --- > > Changes since v2: > > * Pick up patch, and adjust as mentioned above sign-offs > > > > arch/arm64/boot/dts/qcom/pmi8950.dtsi | 97 +++++++++++++++++++++++++++ > > 1 file changed, 97 insertions(+) > > create mode 100644 arch/arm64/boot/dts/qcom/pmi8950.dtsi > > > > diff --git a/arch/arm64/boot/dts/qcom/pmi8950.dtsi b/arch/arm64/boot/dts/qcom/pmi8950.dtsi > > new file mode 100644 > > index 000000000000..32d27e2187e3 > > --- /dev/null > > +++ b/arch/arm64/boot/dts/qcom/pmi8950.dtsi > > @@ -0,0 +1,97 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// Copyright (c) 2019, AngeloGioacchino Del Regno <kholk11@gmail.com> > > + > > +#include <dt-bindings/iio/qcom,spmi-vadc.h> > > +#include <dt-bindings/interrupt-controller/irq.h> > > +#include <dt-bindings/spmi/spmi.h> > > + > > +&spmi_bus { > > + pmic@2 { > > + compatible = "qcom,pmi8950", "qcom,spmi-pmic"; > > + reg = <0x2 SPMI_USID>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + pmi8950_vadc: adc@3100 { > > + compatible = "qcom,spmi-vadc"; > > + reg = <0x3100>; > > + interrupts = <0x2 0x31 0x0 IRQ_TYPE_EDGE_RISING>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + #io-channel-cells = <1>; > > + > > + adc-chan@0 { > > + reg = <VADC_USBIN>; > > + qcom,pre-scaling = <1 4>; > > + label = "usbin"; > > I've previously sent a patch with labels in the node name instead [1], > what's the preferred way nowadays? > > [1]: https://lore.kernel.org/linux-arm-msm/20220926190148.283805-4-marijn.suijten@somainline.org/ As it turns out that patch relied on the ADC5 driver propagating the DT node name (and label name if set) to IIO, which doesn't happen for the legacy VADC driver used here. I sent an RFC to that effect, with a large discussion whether or not we should use node names, labels, or rely on hardcoded names in the drivers entirely. The recent migration to fwnode made the node name include the `@xx` suffix which makes for unpleasant reading in sysfs, so that's at least one reason to have generic node names *and skip node names in these drivers altogether*. https://lore.kernel.org/linux-arm-msm/20221106193018.270106-1-marijn.suijten@somainline.org/T/#u In short: we may want to hold off changing these patches until a clear-cut decision has been made (but I think your patch here is the right approach in the end: generic node name *with label*, when the label is more clear than the name hardcoded in the driver). - Marijn
On 2022-11-06 20:37:24, Marijn Suijten wrote: > On 2022-11-05 00:44:37, Marijn Suijten wrote: > > On 2022-11-01 17:18:00, Luca Weiss wrote: > > > From: AngeloGioacchino Del Regno <kholk11@gmail.com> > > > > > > The PMI8950 features integrated peripherals like ADC, GPIO controller, > > > MPPs and others. > > > > > > [luca@z3ntu.xyz: remove pm8950, style changes for 2022 standards, add wled] > > > Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com> > > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz> > > > --- > > > Changes since v2: > > > * Pick up patch, and adjust as mentioned above sign-offs > > > > > > arch/arm64/boot/dts/qcom/pmi8950.dtsi | 97 +++++++++++++++++++++++++++ > > > 1 file changed, 97 insertions(+) > > > create mode 100644 arch/arm64/boot/dts/qcom/pmi8950.dtsi > > > > > > diff --git a/arch/arm64/boot/dts/qcom/pmi8950.dtsi b/arch/arm64/boot/dts/qcom/pmi8950.dtsi > > > new file mode 100644 > > > index 000000000000..32d27e2187e3 > > > --- /dev/null > > > +++ b/arch/arm64/boot/dts/qcom/pmi8950.dtsi > > > @@ -0,0 +1,97 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +// Copyright (c) 2019, AngeloGioacchino Del Regno <kholk11@gmail.com> > > > + > > > +#include <dt-bindings/iio/qcom,spmi-vadc.h> > > > +#include <dt-bindings/interrupt-controller/irq.h> > > > +#include <dt-bindings/spmi/spmi.h> > > > + > > > +&spmi_bus { > > > + pmic@2 { > > > + compatible = "qcom,pmi8950", "qcom,spmi-pmic"; > > > + reg = <0x2 SPMI_USID>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + pmi8950_vadc: adc@3100 { > > > + compatible = "qcom,spmi-vadc"; > > > + reg = <0x3100>; > > > + interrupts = <0x2 0x31 0x0 IRQ_TYPE_EDGE_RISING>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + #io-channel-cells = <1>; > > > + > > > + adc-chan@0 { > > > + reg = <VADC_USBIN>; > > > + qcom,pre-scaling = <1 4>; > > > + label = "usbin"; > > > > I've previously sent a patch with labels in the node name instead [1], > > what's the preferred way nowadays? > > > > [1]: https://lore.kernel.org/linux-arm-msm/20220926190148.283805-4-marijn.suijten@somainline.org/ > > As it turns out that patch relied on the ADC5 driver propagating the DT > node name (and label name if set) to IIO, which doesn't happen for the > legacy VADC driver used here. I sent an RFC to that effect, with a > large discussion whether or not we should use node names, labels, or > rely on hardcoded names in the drivers entirely. The recent migration > to fwnode made the node name include the `@xx` suffix which makes for > unpleasant reading in sysfs, so that's at least one reason to have > generic node names *and skip node names in these drivers altogether*. > > https://lore.kernel.org/linux-arm-msm/20221106193018.270106-1-marijn.suijten@somainline.org/T/#u > > In short: we may want to hold off changing these patches until a > clear-cut decision has been made (but I think your patch here is the > right approach in the end: generic node name *with label*, when the > label is more clear than the name hardcoded in the driver). We came to the conclusion in [1] that using labels is the way to go so that the name doesn't get all mangled, then we can opt for generic node names here as well. This does mean I'll have to send a followup for [2] and have to revise [3] again to use generic node names and labels. - Marijn [1]: https://lore.kernel.org/linux-arm-msm/20221112162719.0ac87998@jic23-huawei/ [2]: https://lore.kernel.org/linux-arm-msm/20220926190148.283805-4-marijn.suijten@somainline.org/ [3]: https://lore.kernel.org/linux-arm-msm/20221111120156.48040-10-angelogioacchino.delregno@collabora.com/
On 2022-12-04 17:19:05, Luca Weiss wrote: > On Freitag, 2. Dezember 2022 10:36:58 CET Marijn Suijten wrote: > [..] > > So the way this patch does it is good or does it need changes? Except the typo(s?) pointed out in my first reply, this is good to go. If we stick with generic adc-chan node names that should be documented in the bindings IMO, as it is currently only captured implicitly in the examples. Krzysztof, what is your thought on this? - Marijn
On 08/12/2022 11:12, Marijn Suijten wrote: > On 2022-12-04 17:19:05, Luca Weiss wrote: >> On Freitag, 2. Dezember 2022 10:36:58 CET Marijn Suijten wrote: >> [..] >> >> So the way this patch does it is good or does it need changes? > > Except the typo(s?) pointed out in my first reply, this is good to go. > > If we stick with generic adc-chan node names that should be documented > in the bindings IMO, as it is currently only captured implicitly in the > examples. Krzysztof, what is your thought on this? If I understand correctly, the outcome of other discussion [1] was to use labels and generic node names. In such case the patch was correct (except other comments). [1] https://lore.kernel.org/linux-arm-msm/20221112162719.0ac87998@jic23-huawei/ Best regards, Krzysztof
On 2022-12-08 11:23:17, Krzysztof Kozlowski wrote: > On 08/12/2022 11:12, Marijn Suijten wrote: > > On 2022-12-04 17:19:05, Luca Weiss wrote: > >> On Freitag, 2. Dezember 2022 10:36:58 CET Marijn Suijten wrote: > >> [..] > >> > >> So the way this patch does it is good or does it need changes? > > > > Except the typo(s?) pointed out in my first reply, this is good to go. > > > > If we stick with generic adc-chan node names that should be documented > > in the bindings IMO, as it is currently only captured implicitly in the > > examples. Krzysztof, what is your thought on this? > > If I understand correctly, the outcome of other discussion [1] was to > use labels and generic node names. The outcome was to use labels in the driver and disregard node names as the new fwnode API clobbers those names by including the @xx register bit. (I'll follow up with Jonathan whether or not to remove the current fallback to node names, as [1] ended up discussing many different issues and nits) > In such case the patch was correct > (except other comments). As a consequence it _doesn't matter_ how nodes are named, and we _can_ use generic node names. My question for you is whether we should, and if we should lock that in via dt-bindings to guide everyone towards using labels (which i did _not_ do in the recently-landed PM8950 and PM6125, but will send followup for). > [1] > https://lore.kernel.org/linux-arm-msm/20221112162719.0ac87998@jic23-huawei/ - Marijn
On Donnerstag, 8. Dezember 2022 12:20:55 CET Marijn Suijten wrote: > On 2022-12-08 11:23:17, Krzysztof Kozlowski wrote: > > On 08/12/2022 11:12, Marijn Suijten wrote: > > > On 2022-12-04 17:19:05, Luca Weiss wrote: > > >> On Freitag, 2. Dezember 2022 10:36:58 CET Marijn Suijten wrote: > > >> [..] > > >> > > >> So the way this patch does it is good or does it need changes? > > > > > > Except the typo(s?) pointed out in my first reply, this is good to go. > > > > > > If we stick with generic adc-chan node names that should be documented > > > in the bindings IMO, as it is currently only captured implicitly in the > > > examples. Krzysztof, what is your thought on this? > > > > If I understand correctly, the outcome of other discussion [1] was to > > use labels and generic node names. > > The outcome was to use labels in the driver and disregard node names as > the new fwnode API clobbers those names by including the @xx register > bit. > > (I'll follow up with Jonathan whether or not to remove the current > fallback to node names, as [1] ended up discussing many different issues > and nits) > > > In such case the patch was correct > > (except other comments). > > As a consequence it _doesn't matter_ how nodes are named, and we _can_ > use generic node names. My question for you is whether we should, and > if we should lock that in via dt-bindings to guide everyone towards > using labels (which i did _not_ do in the recently-landed PM8950 and > PM6125, but will send followup for). FYI the patch has been merged already and is now in linux-next https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/arch/arm64/boot/dts/qcom/pmi8950.dtsi?id=0d97fdf380b478c358c94f50f1b942e87f407b9b If you have any changes that need to be done please send a follow-up patch. Regards Luca > > > [1] > > https://lore.kernel.org/linux-arm-msm/20221112162719.0ac87998@jic23-huawei > > / > > - Marijn
On 09/12/2022 21:38, Marijn Suijten wrote: > On 2022-12-09 17:54:50, Luca Weiss wrote: >> On Donnerstag, 8. Dezember 2022 12:20:55 CET Marijn Suijten wrote: >>> On 2022-12-08 11:23:17, Krzysztof Kozlowski wrote: >>>> On 08/12/2022 11:12, Marijn Suijten wrote: >>>>> On 2022-12-04 17:19:05, Luca Weiss wrote: >>>>>> On Freitag, 2. Dezember 2022 10:36:58 CET Marijn Suijten wrote: >>>>>> [..] >>>>>> >>>>>> So the way this patch does it is good or does it need changes? >>>>> >>>>> Except the typo(s?) pointed out in my first reply, this is good to go. >>>>> >>>>> If we stick with generic adc-chan node names that should be documented >>>>> in the bindings IMO, as it is currently only captured implicitly in the >>>>> examples. Krzysztof, what is your thought on this? >>>> >>>> If I understand correctly, the outcome of other discussion [1] was to >>>> use labels and generic node names. >>> >>> The outcome was to use labels in the driver and disregard node names as >>> the new fwnode API clobbers those names by including the @xx register >>> bit. >>> >>> (I'll follow up with Jonathan whether or not to remove the current >>> fallback to node names, as [1] ended up discussing many different issues >>> and nits) >>> >>>> In such case the patch was correct >>>> (except other comments). >>> >>> As a consequence it _doesn't matter_ how nodes are named, and we _can_ >>> use generic node names. My question for you is whether we should, and >>> if we should lock that in via dt-bindings to guide everyone towards >>> using labels (which i did _not_ do in the recently-landed PM8950 and >>> PM6125, but will send followup for). >> >> FYI the patch has been merged already and is now in linux-next >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/arch/arm64/boot/dts/qcom/pmi8950.dtsi?id=0d97fdf380b478c358c94f50f1b942e87f407b9b >> >> If you have any changes that need to be done please send a follow-up patch. > > Unfortunately saw that today as well, well after sending this reply. I > would've loved to correct the pmi8950_gpio label _gpios before someone I don't understand what is there to correct. The "pmi8950_gpio" is a correct label. There is no single rule saying label should have "s" at the end. The only rules are: using underscores and having similar naming (e.g. mdss_ for all display labels). Best regards, Krzysztof
On 2022-12-10 11:58:24, Krzysztof Kozlowski wrote: > On 09/12/2022 21:38, Marijn Suijten wrote: > > On 2022-12-09 17:54:50, Luca Weiss wrote: > >> On Donnerstag, 8. Dezember 2022 12:20:55 CET Marijn Suijten wrote: > >>> On 2022-12-08 11:23:17, Krzysztof Kozlowski wrote: > >>>> On 08/12/2022 11:12, Marijn Suijten wrote: > >>>>> On 2022-12-04 17:19:05, Luca Weiss wrote: > >>>>>> On Freitag, 2. Dezember 2022 10:36:58 CET Marijn Suijten wrote: > >>>>>> [..] > >>>>>> > >>>>>> So the way this patch does it is good or does it need changes? > >>>>> > >>>>> Except the typo(s?) pointed out in my first reply, this is good to go. > >>>>> > >>>>> If we stick with generic adc-chan node names that should be documented > >>>>> in the bindings IMO, as it is currently only captured implicitly in the > >>>>> examples. Krzysztof, what is your thought on this? > >>>> > >>>> If I understand correctly, the outcome of other discussion [1] was to > >>>> use labels and generic node names. > >>> > >>> The outcome was to use labels in the driver and disregard node names as > >>> the new fwnode API clobbers those names by including the @xx register > >>> bit. > >>> > >>> (I'll follow up with Jonathan whether or not to remove the current > >>> fallback to node names, as [1] ended up discussing many different issues > >>> and nits) > >>> > >>>> In such case the patch was correct > >>>> (except other comments). > >>> > >>> As a consequence it _doesn't matter_ how nodes are named, and we _can_ > >>> use generic node names. My question for you is whether we should, and > >>> if we should lock that in via dt-bindings to guide everyone towards > >>> using labels (which i did _not_ do in the recently-landed PM8950 and > >>> PM6125, but will send followup for). > >> > >> FYI the patch has been merged already and is now in linux-next > >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/arch/arm64/boot/dts/qcom/pmi8950.dtsi?id=0d97fdf380b478c358c94f50f1b942e87f407b9b > >> > >> If you have any changes that need to be done please send a follow-up patch. > > > > Unfortunately saw that today as well, well after sending this reply. I > > would've loved to correct the pmi8950_gpio label _gpios before someone > > I don't understand what is there to correct. The "pmi8950_gpio" is a > correct label. There is no single rule saying label should have "s" at > the end. The only rules are: using underscores and having similar naming > (e.g. mdss_ for all display labels). If we were able to have rules for labels, would I then be allowed to "correct" this? The inconsistency between DTs is /super/ annoying (and it looks wrong to have a singular _gpio named thing contain /multiple gpios/), but just because we can't express this in dt-bindings (or so I think) we shouldn't change it? - Marijn
diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml index 4c15693f7a01..5ac8605a53b1 100644 --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml @@ -19,6 +19,7 @@ properties: compatible: enum: - qcom,pm8941-wled + - qcom,pmi8950-wled - qcom,pmi8994-wled - qcom,pmi8998-wled - qcom,pm660l-wled
Document the compatible for the wled block found in PMI8950. Signed-off-by: Luca Weiss <luca@z3ntu.xyz> --- Changes since v2: * New patch Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml | 1 + 1 file changed, 1 insertion(+)