| 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 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:56:05PM +0300, Andy Shevchenko wrote: > Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti: > > The regmap irq array is potentially shared between multiple PMICs and > > IRQ I'm referring to an array of struct regmap_irq. Perhaps I can add an underscore. > > should only contain static data. > > > > Use a custom macro to initialise also the type fields and drop the > > unnecessary updates on each probe. > > ... > > > +#define _IRQ_TYPE_ALL (IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW) > > This is repetition of IRQ_TYPE_DEFAULT. Thanks, I guess I should use IRQ_TYPE_SENSE_MASK here even. > ... > > > - dev_err(dev, "Failed to probe irq periphs: %d\n", rc); > > + dev_err(dev, "failed to add IRQ chip: %d\n", rc); > > dev_err_probe(...); ? This function won't return -EPROBE_DEFER, and that would be a separate change in any case. Johan
On Tue, May 07, 2024 at 08:43:08AM +0200, Krzysztof Kozlowski wrote: > On 06/05/2024 17:08, Johan Hovold wrote: > > Rework the pm8008 binding by dropping internal details like register > > offsets and interrupts and by adding the missing regulator and > > temperature alarm properties. > > > > Note that child nodes are still used for pinctrl and regulator > > configuration. > > > > Also note that the pinctrl state definition will be extended later and > > could eventually also be shared with other PMICs (e.g. by breaking out > > bits of qcom,pmic-gpio.yaml). > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > reg: > > - description: > > - I2C slave address. > > Please split cleanups from actual functional/content rework. Sure. > > - > > maxItems: 1 > > > > interrupts: > > maxItems: 1 > > > > - description: Parent interrupt. > > - > > reset-gpios: > > maxItems: 1 > > > > - "#interrupt-cells": > > + vdd_l1_l2-supply: true > > No underscores in property names. Indeed. These names come from Qualcomm's v15, but I should have caught that. Thanks. Johan
On Tue, May 7, 2024 at 6:01 PM Johan Hovold <johan@kernel.org> wrote: > On Mon, May 06, 2024 at 09:56:05PM +0300, Andy Shevchenko wrote: > > Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti: > > > The regmap irq array is potentially shared between multiple PMICs and ... > > > - dev_err(dev, "Failed to probe irq periphs: %d\n", rc); > > > + dev_err(dev, "failed to add IRQ chip: %d\n", rc); > > > > dev_err_probe(...); ? > > This function won't return -EPROBE_DEFER, This is not an argument for a long time (since documentation of dev_err_probe() had been amended to encourage its use for any error cases in probe). > and that would be a separate > change in any case. Sure, but why to add a technical debt? Perhaps a precursor cleanup patch?
Quoting Johan Hovold (2024-05-07 08:23:04) > On Tue, May 07, 2024 at 08:43:08AM +0200, Krzysztof Kozlowski wrote: > > > - > > > maxItems: 1 > > > > > > interrupts: > > > maxItems: 1 > > > > > > - description: Parent interrupt. > > > - > > > reset-gpios: > > > maxItems: 1 > > > > > > - "#interrupt-cells": > > > + vdd_l1_l2-supply: true > > > > No underscores in property names. > > Indeed. These names come from Qualcomm's v15, but I should have caught > that. Thanks. Drive by comment: we have underscores to match the label on the datasheet. Not sure that will sway your opinion. Only trying to provide some background rationale.
On 09/05/2024 00:09, Stephen Boyd wrote: > Quoting Johan Hovold (2024-05-07 08:23:04) >> On Tue, May 07, 2024 at 08:43:08AM +0200, Krzysztof Kozlowski wrote: >>>> - >>>> maxItems: 1 >>>> >>>> interrupts: >>>> maxItems: 1 >>>> >>>> - description: Parent interrupt. >>>> - >>>> reset-gpios: >>>> maxItems: 1 >>>> >>>> - "#interrupt-cells": >>>> + vdd_l1_l2-supply: true >>> >>> No underscores in property names. >> >> Indeed. These names come from Qualcomm's v15, but I should have caught >> that. Thanks. > > Drive by comment: we have underscores to match the label on the > datasheet. Not sure that will sway your opinion. Only trying to provide > some background rationale. I know, but if datasheet calls this "yellow_pony_!!!#1l33t-supply" we still won't use datasheet names directly, so s/_/-/. That's also W=2 warning. 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 Tue, May 07, 2024 at 08:16:45PM +0300, Andy Shevchenko wrote: > On Tue, May 7, 2024 at 6:01 PM Johan Hovold <johan@kernel.org> wrote: > > On Mon, May 06, 2024 at 09:56:05PM +0300, Andy Shevchenko wrote: > > > Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti: > > > > The regmap irq array is potentially shared between multiple PMICs and > > ... > > > > > - dev_err(dev, "Failed to probe irq periphs: %d\n", rc); > > > > + dev_err(dev, "failed to add IRQ chip: %d\n", rc); > > > > > > dev_err_probe(...); ? > > > > This function won't return -EPROBE_DEFER, > > This is not an argument for a long time (since documentation of > dev_err_probe() had been amended to encourage its use for any error > cases in probe). There was apparently a kernel doc update made in December 2023: 532888a59505 ("driver core: Better advertise dev_err_probe()") to clarify that people are *allowed* to use it also for functions not returning -EPROBE_DEFER. That's hardly a long time ago and, importantly, this is of course still nothing that is *required*. > > and that would be a separate > > change in any case. > > Sure, but why to add a technical debt? Perhaps a precursor cleanup patch? This is not in any way technical debt. Johan
Thu, May 09, 2024 at 10:49:28AM +0200, Johan Hovold kirjoitti: > On Tue, May 07, 2024 at 08:16:45PM +0300, Andy Shevchenko wrote: > > On Tue, May 7, 2024 at 6:01 PM Johan Hovold <johan@kernel.org> wrote: > > > On Mon, May 06, 2024 at 09:56:05PM +0300, Andy Shevchenko wrote: > > > > Mon, May 06, 2024 at 05:08:19PM +0200, Johan Hovold kirjoitti: > > > > > The regmap irq array is potentially shared between multiple PMICs and ... > > > > > - dev_err(dev, "Failed to probe irq periphs: %d\n", rc); > > > > > + dev_err(dev, "failed to add IRQ chip: %d\n", rc); > > > > > > > > dev_err_probe(...); ? > > > > > > This function won't return -EPROBE_DEFER, > > > > This is not an argument for a long time (since documentation of > > dev_err_probe() had been amended to encourage its use for any error > > cases in probe). > > There was apparently a kernel doc update made in December 2023: > > 532888a59505 ("driver core: Better advertise dev_err_probe()") > > to clarify that people are *allowed* to use it also for functions not > returning -EPROBE_DEFER. That's hardly a long time ago and, importantly, > this is of course still nothing that is *required*. Fair enough. > > > and that would be a separate > > > change in any case. > > > > Sure, but why to add a technical debt? Perhaps a precursor cleanup patch? > > This is not in any way technical debt. OK.