| Message ID | 20240506150830.23709-1-johan+linaro@kernel.org |
|---|---|
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic | expand |
Mon, May 06, 2024 at 05:08:20PM +0200, Johan Hovold kirjoitti: > Request and deassert any (optional) reset gpio during probe in case it > has been left asserted by the boot firmware. > > Note the reset line is not asserted to avoid reverting to the default > I2C address in case the firmware has configured an alternate address. ... > + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > + if (IS_ERR(reset)) > + return PTR_ERR(reset); Shouldn't you wait a bit to make chip settle down?
On Mon, 06 May 2024 17:08:17 +0200, Johan Hovold wrote: > The Qualcomm PM8008 PMIC is a so called QPNP PMIC with seven LDO > regulators, a temperature alarm block and two GPIO pins (which are also > used for interrupt signalling and reset). > > Unlike previous QPNP PMICs it uses an I2C rather than SPMI interface, > which has implications for how interrupts are handled. > > A previous attempt by Qualcomm to upstream support for PM8008 stalled > two years ago at version 15 after a lot of back and forth discussion on > how best to describe this device in the devicetree. [1] > > After reviewing the backstory on this and surveying the current SPMI > PMIC bindings and implementation, I opted for a new approach that does > not describe internal details like register offsets and interrupts in > the devicetree. > > The original decision to include registers offsets and internal > interrupts for SPMI PMICs has led to a number of PMIC dtsi being created > to avoid copying lots of boiler plate declarations. This in turn causes > trouble when the PMIC USID address is configurable as the address is > included in every interrupt specifier. > > The current SPMI bindings still do not describe the devices fully and > additional data is therefore already provided by drivers (e.g. > additional register blocks, supplies, additional interrupt specifiers). > > The fact that PMICs which use two USIDs (addresses) are modelled as two > separate devices causes trouble, for example, when there are > dependencies between subfunctions. [2] > > Subfunctions also do not necessarily map neatly onto the 256-register > block partitioning of the SPMI register space, something which has lead > to unresolved inconsistencies in how functions like PWM are described. > [3] > > In short, it's a bit of a mess. > > With the new style of bindings, by contrast, only essential information > that actually differs between machines would be included in the > devicetree. The bindings would also be mostly decoupled from the > implementation, which has started to leak out into the binding (e.g. how > the QPNP interrupts are handled). This also allows for extending the > implementation without having to update the binding, which is especially > important as Qualcomm does not publish any documentation (e.g. to later > enable regulator over-current protection). > > Some PMICs support both I2C and SPMI interfaces (e.g. PM8010) and we > want to be able to reuse the same bindings regardless of the interface. > > As a proof concept I have written a new pmc8280 driver for one of the > SPMI PMICs in the Lenovo ThinkPad X13s that uses the new style of > bindings and I've been using that one to control backlight and > peripheral regulators for a while now. Specifically, the gpio and > temperature-alarm blocks can be used with some minor updates to the > current drivers. > > That work still needs a bit of polish before posting, but my working PoC > means that I'm confident enough that the new model will work and that we > can go ahead and merge regulator support for the PM8008. > > This series is specifically needed for the camera sensors in the X13s, > for which camera subsystem (camss) support has now been merged for 6.10. > > The first seven patches are preparatory and can possibly be merged > separately from the rest of the series. The next two patches drops the > broken GPIO support for PM8008 which had already been upstreamed. The > last four patches rework the binding and MFD driver, add support for the > regulators and enable the camera PMIC on the X13s. > > Johan > > [1] https://lore.kernel.org/all/1655200111-18357-1-git-send-email-quic_c_skakit@quicinc.com > [2] https://lore.kernel.org/lkml/20231003152927.15000-3-johan+linaro@kernel.org > [3] https://lore.kernel.org/r/20220828132648.3624126-3-bryan.odonoghue@linaro.org > > > Johan Hovold (12): > dt-bindings: mfd: pm8008: add reset gpio > mfd: pm8008: fix regmap irq chip initialisation > mfd: pm8008: deassert reset on probe > mfd: pm8008: mark regmap structures as const > mfd: pm8008: use lower case hex notation > mfd: pm8008: rename irq chip > mfd: pm8008: drop unused driver data > dt-bindings: pinctrl: qcom,pmic-gpio: drop pm8008 > pinctrl: qcom: spmi-gpio: drop broken pm8008 support > dt-bindings: mfd: pm8008: rework binding > mfd: pm8008: rework driver > arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic > > Satya Priya (1): > regulator: add pm8008 pmic regulator driver > > .../devicetree/bindings/mfd/qcom,pm8008.yaml | 158 ++++++++----- > .../bindings/pinctrl/qcom,pmic-gpio.yaml | 3 - > .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 123 ++++++++++ > drivers/mfd/Kconfig | 1 + > drivers/mfd/qcom-pm8008.c | 163 ++++++++----- > drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 1 - > drivers/regulator/Kconfig | 7 + > drivers/regulator/Makefile | 1 + > drivers/regulator/qcom-pm8008-regulator.c | 215 ++++++++++++++++++ > include/dt-bindings/mfd/qcom-pm8008.h | 19 -- > 10 files changed, 554 insertions(+), 137 deletions(-) > create mode 100644 drivers/regulator/qcom-pm8008-regulator.c > delete mode 100644 include/dt-bindings/mfd/qcom-pm8008.h > > -- > 2.43.2 > > > My bot found new DTB warnings on the .dts files added or changed in this series. Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. No need to reply unless the platform maintainer has comments. If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date: pip3 install dtschema --upgrade New warnings running 'make CHECK_DTBS=y qcom/sc8280xp-lenovo-thinkpad-x13s.dtb' for 20240506150830.23709-1-johan+linaro@kernel.org: arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dtb: usb@a4f8800: interrupts-extended: [[1, 0, 130, 4], [1, 0, 135, 4], [1, 0, 857, 4], [1, 0, 856, 4], [1, 0, 131, 4], [1, 0, 136, 4], [1, 0, 860, 4], [1, 0, 859, 4], [136, 127, 3], [136, 126, 3], [136, 129, 3], [136, 128, 3], [136, 131, 3], [136, 130, 3], [136, 133, 3], [136, 132, 3], [136, 16, 4], [136, 17, 4]] is too long from schema $id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#
On 06/05/2024 17:08, Johan Hovold wrote: > The binding for PM8008 is being reworked so that internal details like > interrupts and register offsets are no longer described. This > specifically also involves dropping the gpio child node and its > compatible string which is no longer needed. > > Note that there are currently no users of the upstream binding and > driver. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml | 3 --- > 1 file changed, 3 deletions(-) Dropping compatibles from bindings must happen after they are dropped from kernel, so this should go after spmi-gpio patch. Best regards, Krzysztof
On Mon, May 06, 2024 at 09:57:07PM +0300, Andy Shevchenko wrote: > Mon, May 06, 2024 at 05:08:20PM +0200, Johan Hovold kirjoitti: > > Request and deassert any (optional) reset gpio during probe in case it > > has been left asserted by the boot firmware. > > > > Note the reset line is not asserted to avoid reverting to the default > > I2C address in case the firmware has configured an alternate address. > > ... > > > + reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > > + if (IS_ERR(reset)) > > + return PTR_ERR(reset); > > Shouldn't you wait a bit to make chip settle down? Yeah, probably. I actually asserted reset here for a while (e.g. to reset the address), but didn't have to use a post-reset delay then. I'll see if I can find someone with access to a datasheet or maybe add some small delay here anyway. Johan
On Tue, May 07, 2024 at 01:48:30PM +0200, Konrad Dybcio wrote: > On 5/6/24 17:08, Johan Hovold wrote: > > From: Satya Priya <quic_c_skakit@quicinc.com> > > > > Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing > > seven LDO regulators. Add a PM8008 regulator driver to support PMIC > > regulator management via the regulator framework. > > > > Note that this driver, originally submitted by Satya Priya [1], has been > > reworked to match the new devicetree binding which no longer describes > > each regulator as a separate device. > > > > This avoids describing internal details like register offsets in the > > devicetree and allows for extending the implementation with features > > like over-current protection without having to update the binding. > > > > Specifically note that the regulator interrupts are shared between all > > regulators. > > > > Note that the secondary regmap is looked up by name and that if the > > driver ever needs to be generalised to support regulators provided by > > the primary regmap (I2C address) such information could be added to a > > driver lookup table matching on the parent compatible. > > > > This also fixes the original implementation, which looked up regulators > > by 'regulator-name' property rather than devicetree node name and which > > prevented the regulators from being named to match board schematics. > > > > [1] https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@quicinc.com > > > > Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> > > Cc: Stephen Boyd <swboyd@chromium.org> > > [ johan: rework probe to match new binding, amend commit message and > > Kconfig entry] > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > I'm a bit lukewarm on calling this qcom-pm8008-regulator.. But then > qcom-i2c-regulator or qpnp-i2c-regulator may bite due to being overly > generic.. Would you know whether this code will also be used for e.g. > PM8010? Yes, for any sufficiently similar PMICs, including SPMI ones. So 'qpnp-regulator' would be a generic name, but only Qualcomm knows what PMICs they have and how they are related -- the rest of us is left doing tedious code forensics to try to make some sense of this. So just like for compatible strings, letting the first supported PMIC name the driver makes sense as we don't know when we'll want to add a second one for another set of devices (and we don't want to call that one 'qpnp-regulator-2'). On the other hand, these names are now mostly internal and can more easily be renamed later. Johan
On 07/05/2024 19:22, Andy Shevchenko wrote: > On Tue, May 7, 2024 at 6:44 PM Johan Hovold <johan@kernel.org> wrote: >> On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote: >>> Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti: > > ... > >>>> [ johan: rework probe to match new binding, amend commit message and >>>> Kconfig entry] >>> >>> Wouldn't be better on one line? >> >> Now you're really nit picking. ;) I think I prefer to stay within 72 >> columns. > > Not really. The tag block is special and the format is rather one > entry per line. This might break some scriptings. > > ... I think [] can be wrapped, I saw it at least many times and I use as well... ... > ... > >>>> +MODULE_ALIAS("platform:qcom-pm8008-regulator"); >>> >>> Use ID table instead. >> >> No, the driver is not using an id-table for matching so the alias is >> needed for module auto-loading. > > Then create one. Added Krzysztof for that. (He is working on dropping > MODULE_ALIAS() in cases like this one) Yeah, please use ID table, since this is a driver (unless I missed something). Module alias does not scale, leads to stale and duplicated entries, so should not be used as substitute of ID table. Alias is suitable for different cases. Best regards, Krzysztof
[ +To: Krishna ] On Mon, May 06, 2024 at 03:40:32PM -0500, Rob Herring wrote: > On Mon, 06 May 2024 17:08:17 +0200, Johan Hovold wrote: > > The Qualcomm PM8008 PMIC is a so called QPNP PMIC with seven LDO > > regulators, a temperature alarm block and two GPIO pins (which are also > > used for interrupt signalling and reset). > My bot found new DTB warnings on the .dts files added or changed in this > series. > > Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings > are fixed by another series. Ultimately, it is up to the platform > maintainer whether these warnings are acceptable or not. No need to reply > unless the platform maintainer has comments. > > If you already ran DT checks and didn't see these error(s), then > make sure dt-schema is up to date: > > pip3 install dtschema --upgrade > > > New warnings running 'make CHECK_DTBS=y qcom/sc8280xp-lenovo-thinkpad-x13s.dtb' for 20240506150830.23709-1-johan+linaro@kernel.org: > > arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dtb: usb@a4f8800: interrupts-extended: [[1, 0, 130, 4], [1, 0, 135, 4], [1, 0, 857, 4], [1, 0, 856, 4], [1, 0, 131, 4], [1, 0, 136, 4], [1, 0, 860, 4], [1, 0, 859, 4], [136, 127, 3], [136, 126, 3], [136, 129, 3], [136, 128, 3], [136, 131, 3], [136, 130, 3], [136, 133, 3], [136, 132, 3], [136, 16, 4], [136, 17, 4]] is too long > from schema $id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml# This one is unrelated to this series and you should only see it with linux-next which has: 80adfb54044e ("dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport") 3170a2c906c6 ("arm64: dts: qcom: sc8280xp: Add USB DWC3 Multiport controller") eb24bd3c593f ("arm64: dts: qcom: sc8280xp-x13s: enable USB MP and fingerprint reader") Apparently you already reported this two weeks ago without anyone following up: https://lore.kernel.org/lkml/171449016553.3484108.5214033788092698309.robh@kernel.org/ I've just sent a fix here: https://lore.kernel.org/lkml/20240509083822.397-1-johan+linaro@kernel.org/ Johan
On 09/05/2024 10:57, Johan Hovold wrote: > On Tue, May 07, 2024 at 08:14:43PM +0200, Krzysztof Kozlowski wrote: >> On 07/05/2024 19:22, Andy Shevchenko wrote: >>> On Tue, May 7, 2024 at 6:44 PM Johan Hovold <johan@kernel.org> wrote: >>>> On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote: >>>>> Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti: > >>>>>> +MODULE_ALIAS("platform:qcom-pm8008-regulator"); >>>>> >>>>> Use ID table instead. >>>> >>>> No, the driver is not using an id-table for matching so the alias is >>>> needed for module auto-loading. >>> >>> Then create one. Added Krzysztof for that. (He is working on dropping >>> MODULE_ALIAS() in cases like this one) >> >> Yeah, please use ID table, since this is a driver (unless I missed >> something). Module alias does not scale, leads to stale and duplicated >> entries, so should not be used as substitute of ID table. Alias is >> suitable for different cases. > > There's no scalability issue here. If the driver uses driver name > matching then there will always be exactly one alias needed. And then we add one more ID with driver data and how does it scale? There is a way to make drivers uniform, standard and easy to read. Why doing some other way? What is the benefit of the alias comparing to regular module ID table? Best regards, Krzysztof
On Thu, May 09, 2024 at 12:48:18PM +0200, Krzysztof Kozlowski wrote: > On 09/05/2024 10:57, Johan Hovold wrote: > > On Tue, May 07, 2024 at 08:14:43PM +0200, Krzysztof Kozlowski wrote: > >> On 07/05/2024 19:22, Andy Shevchenko wrote: > >> Yeah, please use ID table, since this is a driver (unless I missed > >> something). Module alias does not scale, leads to stale and duplicated > >> entries, so should not be used as substitute of ID table. Alias is > >> suitable for different cases. > > > > There's no scalability issue here. If the driver uses driver name > > matching then there will always be exactly one alias needed. > > And then we add one more ID with driver data and how does it scale? That's what I wrote in the part of my reply that you left out. If a driver is going to be used for multiple devices, then a module id table makes sense, but there is no need to go around adding redundant tables just for the sake of it when a simple alias will do. Johan
On 09/05/2024 14:26, Johan Hovold wrote: > On Thu, May 09, 2024 at 12:48:18PM +0200, Krzysztof Kozlowski wrote: >> On 09/05/2024 10:57, Johan Hovold wrote: >>> On Tue, May 07, 2024 at 08:14:43PM +0200, Krzysztof Kozlowski wrote: >>>> On 07/05/2024 19:22, Andy Shevchenko wrote: > >>>> Yeah, please use ID table, since this is a driver (unless I missed >>>> something). Module alias does not scale, leads to stale and duplicated >>>> entries, so should not be used as substitute of ID table. Alias is >>>> suitable for different cases. >>> >>> There's no scalability issue here. If the driver uses driver name >>> matching then there will always be exactly one alias needed. >> >> And then we add one more ID with driver data and how does it scale? > > That's what I wrote in the part of my reply that you left out. If a > driver is going to be used for multiple devices, then a module id table > makes sense, but there is no need to go around adding redundant tables > just for the sake of it when a simple alias will do. > I still in general prefer ID tables, because I saw many times people copy existing code while not understanding above subtleties thus they just keep multiplying MODULE_ALIAS, but I understand your explanation and it is reasonable. FWIW: Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof