[00/13] arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic

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

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.