[PATCH 0/9] i2c: qcom-cci: fixes and updates

[PATCH 0/9] i2c: qcom-cci: fixes and updates

[Vladimir Zapolskiy]

The main intention of the patch series is to add support of vbus
regulators, which are commonly connected to CCI I2C busses.

The new bus adapter specific bus_regulator from commit 5a7b95fb993e
("i2c: core: support bus regulator controlling in adapter") is reused,
however its control is connected to runtime pm of the I2C master
controller rather than runtime pm of slaves.

In addition the series adds new compatible value for CCI found on QCOM
SM8450 SoC.

Vladimir Zapolskiy (9):
  dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible
  dt-bindings: i2c: qcom-cci: add description of a vbus-supply property
  i2c: qcom-cci: don't delete an unregistered adapter
  i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()
  i2c: qcom-cci: initialize CCI controller after registration of adapters
  i2c: qcom-cci: simplify probe by removing one loop over busses
  i2c: qcom-cci: simplify access to bus data structure
  i2c: qcom-cci: add support of optional vbus-supply regulators
  i2c: qcom-cci: add sm8450 compatible

 .../devicetree/bindings/i2c/i2c-qcom-cci.txt  |   9 +-
 drivers/i2c/busses/i2c-qcom-cci.c             | 159 ++++++++++++------
 2 files changed, 114 insertions(+), 54 deletions(-)

-- 
2.33.0

[PATCH 1/9] dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible

[Vladimir Zapolskiy]

The change adds QCOM SM8450 compatible value to the list of QCOM CCI
controller compatibles, the controller found on the SoC is equal to
the ones found on previous SoC generations.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
index 7b9fc0c22eaf..924ad8c03464 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
@@ -10,6 +10,7 @@ PROPERTIES:
 		"qcom,msm8996-cci"
 		"qcom,sdm845-cci"
 		"qcom,sm8250-cci"
+		"qcom,sm8450-cci"
 
 - reg
 	Usage: required
@@ -43,7 +44,8 @@ PROPERTIES:
 SUBNODES:
 
 The CCI provides I2C masters for one (msm8916) or two i2c busses (msm8996,
-sdm845 and sm8250), described as subdevices named "i2c-bus@0" and "i2c-bus@1".
+sdm845, sm8250 and sm8450), described as subdevices named "i2c-bus@0" and
+"i2c-bus@1".
 
 PROPERTIES:
 
-- 
2.33.0

[PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Vladimir Zapolskiy]

Quite regularly I2C bus lines on QCOM CCI controller require an external
pull-up to a regulator powered line, to be able to define all such
cases an additional vbus-supply property of a bus subnode is wanted.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
index 924ad8c03464..9f5b321748f1 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
@@ -60,6 +60,11 @@ PROPERTIES:
 	Definition: Desired I2C bus clock frequency in Hz, defaults to 100
 		    kHz if omitted.
 
+- vbus-supply:
+	Usage: optional
+	Value type: phandle
+	Definition: Regulator that provides power to SCL/SDA lines
+
 Example:
 
 	cci@a0c000 {
-- 
2.33.0

Re: [PATCH 1/9] dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible

[Robert Foss]

On Thu, 3 Feb 2022 at 17:46, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> The change adds QCOM SM8450 compatible value to the list of QCOM CCI
> controller compatibles, the controller found on the SoC is equal to
> the ones found on previous SoC generations.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> index 7b9fc0c22eaf..924ad8c03464 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -10,6 +10,7 @@ PROPERTIES:
>                 "qcom,msm8996-cci"
>                 "qcom,sdm845-cci"
>                 "qcom,sm8250-cci"
> +               "qcom,sm8450-cci"
>
>  - reg
>         Usage: required
> @@ -43,7 +44,8 @@ PROPERTIES:
>  SUBNODES:
>
>  The CCI provides I2C masters for one (msm8916) or two i2c busses (msm8996,
> -sdm845 and sm8250), described as subdevices named "i2c-bus@0" and "i2c-bus@1".
> +sdm845, sm8250 and sm8450), described as subdevices named "i2c-bus@0" and
> +"i2c-bus@1".
>
>  PROPERTIES:
>
> --
> 2.33.0
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Robert Foss]

On Thu, 3 Feb 2022 at 17:46, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> Quite regularly I2C bus lines on QCOM CCI controller require an external
> pull-up to a regulator powered line, to be able to define all such
> cases an additional vbus-supply property of a bus subnode is wanted.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> index 924ad8c03464..9f5b321748f1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -60,6 +60,11 @@ PROPERTIES:
>         Definition: Desired I2C bus clock frequency in Hz, defaults to 100
>                     kHz if omitted.
>
> +- vbus-supply:
> +       Usage: optional
> +       Value type: phandle
> +       Definition: Regulator that provides power to SCL/SDA lines
> +
>  Example:
>
>         cci@a0c000 {
> --
> 2.33.0
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Bjorn Andersson]

On Thu 03 Feb 08:46 PST 2022, Vladimir Zapolskiy wrote:

> Quite regularly I2C bus lines on QCOM CCI controller require an external
> pull-up to a regulator powered line, to be able to define all such
> cases an additional vbus-supply property of a bus subnode is wanted.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> index 924ad8c03464..9f5b321748f1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -60,6 +60,11 @@ PROPERTIES:
>  	Definition: Desired I2C bus clock frequency in Hz, defaults to 100
>  		    kHz if omitted.
>  
> +- vbus-supply:

I don't think "vbus" is an appropriate name for his. Perhaps "vddio" or
something like that would be better.

But there's a bigger question here, this is not a supply for the
i2c master, it's simply a supply for pulling up the bus. So it's not
entirely correct to specify it as a supply for the CCI node (which is
also the reason why the name isn't obvious).

Typically we don't don't mention the bus-supply because it happens to be
pulled up either by io-supply for the block, or by some always-on
regulator in the system.

Looping in Linus and Mark in hope they have seen this need elsewhere.

Regards,
Bjorn

> +	Usage: optional
> +	Value type: phandle
> +	Definition: Regulator that provides power to SCL/SDA lines
> +
>  Example:
>  
>  	cci@a0c000 {
> -- 
> 2.33.0
> 

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

On Fri, Feb 04, 2022 at 10:05:47AM -0800, Bjorn Andersson wrote:
> On Thu 03 Feb 08:46 PST 2022, Vladimir Zapolskiy wrote:

> > +- vbus-supply:

> I don't think "vbus" is an appropriate name for his. Perhaps "vddio" or
> something like that would be better.

> But there's a bigger question here, this is not a supply for the
> i2c master, it's simply a supply for pulling up the bus. So it's not
> entirely correct to specify it as a supply for the CCI node (which is
> also the reason why the name isn't obvious).

Does the device (controller?) not have a supply that the I2C bus is
referenced to?  If so that supply should be named.

> Typically we don't don't mention the bus-supply because it happens to be
> pulled up either by io-supply for the block, or by some always-on
> regulator in the system.

If the bus is being pulled up to some supply other than the supply that
the bus is referenced to that doesn't sound like the greatest electrical
engineering ever...  without any context it's hard to comment about this
particular system.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Bjorn Andersson]

On Fri 04 Feb 10:42 PST 2022, Mark Brown wrote:

> On Fri, Feb 04, 2022 at 10:05:47AM -0800, Bjorn Andersson wrote:
> > On Thu 03 Feb 08:46 PST 2022, Vladimir Zapolskiy wrote:
> 
> > > +- vbus-supply:
> 
> > I don't think "vbus" is an appropriate name for his. Perhaps "vddio" or
> > something like that would be better.
> 
> > But there's a bigger question here, this is not a supply for the
> > i2c master, it's simply a supply for pulling up the bus. So it's not
> > entirely correct to specify it as a supply for the CCI node (which is
> > also the reason why the name isn't obvious).
> 
> Does the device (controller?) not have a supply that the I2C bus is
> referenced to?  If so that supply should be named.
> 

No, for some reason the regulator in question is not connected to either
the master or the client devices, it's only used for pull up of a few
of the i2c busses.

> > Typically we don't don't mention the bus-supply because it happens to be
> > pulled up either by io-supply for the block, or by some always-on
> > regulator in the system.
> 
> If the bus is being pulled up to some supply other than the supply that
> the bus is referenced to that doesn't sound like the greatest electrical
> engineering ever...  without any context it's hard to comment about this
> particular system.

That's what the schematics says...

Regards,
Bjorn

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

On Fri, Feb 04, 2022 at 11:02:04AM -0800, Bjorn Andersson wrote:
> On Fri 04 Feb 10:42 PST 2022, Mark Brown wrote:
> > On Fri, Feb 04, 2022 at 10:05:47AM -0800, Bjorn Andersson wrote:

> > > Typically we don't don't mention the bus-supply because it happens to be
> > > pulled up either by io-supply for the block, or by some always-on
> > > regulator in the system.

> > If the bus is being pulled up to some supply other than the supply that
> > the bus is referenced to that doesn't sound like the greatest electrical
> > engineering ever...  without any context it's hard to comment about this
> > particular system.

> That's what the schematics says...

Oh, good.  I forsee no problems here.  Probably this is something that
should be in the I2C core if it's going to be dynamically managed,
though just setting the supply as always on is probably more expedient.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Vladimir Zapolskiy]

Hi Bjorn, Mark,

On 2/4/22 9:32 PM, Mark Brown wrote:
> On Fri, Feb 04, 2022 at 11:02:04AM -0800, Bjorn Andersson wrote:
>> On Fri 04 Feb 10:42 PST 2022, Mark Brown wrote:
>>> On Fri, Feb 04, 2022 at 10:05:47AM -0800, Bjorn Andersson wrote:
> 
>>>> Typically we don't don't mention the bus-supply because it happens to be
>>>> pulled up either by io-supply for the block, or by some always-on
>>>> regulator in the system.
> 
>>> If the bus is being pulled up to some supply other than the supply that
>>> the bus is referenced to that doesn't sound like the greatest electrical
>>> engineering ever...  without any context it's hard to comment about this
>>> particular system.
> 
>> That's what the schematics says...
> 
> Oh, good.  I forsee no problems here.  Probably this is something that
> should be in the I2C core if it's going to be dynamically managed,
> though just setting the supply as always on is probably more expedient.
> 

vbus-supply property has been added recently to another I2C master controller,
see commit c021087c43c8 ("dt-binding: i2c: mt65xx: add vbus-supply property").
It serves right the same purpose, and its handling is going to be done in i2c
core, however since the latter is not yet completed, I would propose to add
the property to i2c-bus subnodes of QCOM CCI and its support in the driver,
later on both the property and its generic support would be better to see in
i2c core.

--
Best wishes,
Vladimir

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

On Mon, Feb 07, 2022 at 04:08:01PM +0200, Vladimir Zapolskiy wrote:
> On 2/4/22 9:32 PM, Mark Brown wrote:

> > Oh, good.  I forsee no problems here.  Probably this is something that
> > should be in the I2C core if it's going to be dynamically managed,
> > though just setting the supply as always on is probably more expedient.

> vbus-supply property has been added recently to another I2C master controller,
> see commit c021087c43c8 ("dt-binding: i2c: mt65xx: add vbus-supply property").

Note that some devices do have supplies that I/O is referenced against
and it's not clear that this isn't what's goin on here.

> It serves right the same purpose, and its handling is going to be done in i2c
> core, however since the latter is not yet completed, I would propose to add
> the property to i2c-bus subnodes of QCOM CCI and its support in the driver,
> later on both the property and its generic support would be better to see in
> i2c core.

The bindings are ABI, it doesn't seem like a good idea to add new ABI as
a temporary bodge.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Vladimir Zapolskiy]

On 2/7/22 4:39 PM, Mark Brown wrote:
> On Mon, Feb 07, 2022 at 04:08:01PM +0200, Vladimir Zapolskiy wrote:
>> On 2/4/22 9:32 PM, Mark Brown wrote:
> 
>>> Oh, good.  I forsee no problems here.  Probably this is something that
>>> should be in the I2C core if it's going to be dynamically managed,
>>> though just setting the supply as always on is probably more expedient.
> 
>> vbus-supply property has been added recently to another I2C master controller,
>> see commit c021087c43c8 ("dt-binding: i2c: mt65xx: add vbus-supply property").
> 
> Note that some devices do have supplies that I/O is referenced against
> and it's not clear that this isn't what's goin on here.
>
>> It serves right the same purpose, and its handling is going to be done in i2c
>> core, however since the latter is not yet completed, I would propose to add
>> the property to i2c-bus subnodes of QCOM CCI and its support in the driver,
>> later on both the property and its generic support would be better to see in
>> i2c core.
> 
> The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> a temporary bodge.

The bindings are supposed to describe hardware, thus it's natural to extend
them, I believe there is a trilemma in this particular case:
1) add optional vbus-supply property to all I2C master controllers or I2C
    busses in case of multiple I2C busses managed by a single controller,
2) add optional vbus-supply property to all I2C slave devices,
3) ignore peculiarities of particular (multiple in fact) PCB designs and
    a necessity of adding a regulator finely described as a pull-up for I2C
    bus lines.

My assumption is that a decision should be generic for all similar cases,
Wolfram, could you share your point of view on the subject?

--
Best wishes,
Vladimir

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 969 bytes --]

On Mon, Feb 07, 2022 at 08:31:30PM +0200, Vladimir Zapolskiy wrote:
> On 2/7/22 4:39 PM, Mark Brown wrote:

> > The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> > a temporary bodge.

> The bindings are supposed to describe hardware, thus it's natural to extend
> them, I believe there is a trilemma in this particular case:
> 1) add optional vbus-supply property to all I2C master controllers or I2C
>    busses in case of multiple I2C busses managed by a single controller,
> 2) add optional vbus-supply property to all I2C slave devices,

If you add a named supply to all I2C controllers or devices then if any
of them have an actual vbus supply there will be a namespace collision.

> 3) ignore peculiarities of particular (multiple in fact) PCB designs and
>    a necessity of adding a regulator finely described as a pull-up for I2C
>    bus lines.

There's also the option of representing this as a separate thing on or
part of the bus.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Dmitry Baryshkov]

On Tue, 8 Feb 2022 at 16:16, Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Feb 07, 2022 at 08:31:30PM +0200, Vladimir Zapolskiy wrote:
> > On 2/7/22 4:39 PM, Mark Brown wrote:
>
> > > The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> > > a temporary bodge.

It's not a temporary bodge. The i2c-core piece was reverted, but not
the mediatek driver code/bindings.
Vladimir has provided a replacement for the i2c-core code handling the
vbus-regulator. When thee code will be back, the code from i2c-cci can
be removed. The bindings will be the same.

>
> > The bindings are supposed to describe hardware, thus it's natural to extend
> > them, I believe there is a trilemma in this particular case:
> > 1) add optional vbus-supply property to all I2C master controllers or I2C
> >    busses in case of multiple I2C busses managed by a single controller,
> > 2) add optional vbus-supply property to all I2C slave devices,
>
> If you add a named supply to all I2C controllers or devices then if any
> of them have an actual vbus supply there will be a namespace collision.
>
> > 3) ignore peculiarities of particular (multiple in fact) PCB designs and
> >    a necessity of adding a regulator finely described as a pull-up for I2C
> >    bus lines.
>
> There's also the option of representing this as a separate thing on or
> part of the bus.

4) (which you have implemented in your patch). Add support for  the
vbus-supplies property for the I2C CCI controllers.

This is the option I'd vote for.

-- 
With best wishes
Dmitry

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1053 bytes --]

On Thu, Feb 10, 2022 at 06:33:09PM +0300, Dmitry Baryshkov wrote:
> On Tue, 8 Feb 2022 at 16:16, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Feb 07, 2022 at 08:31:30PM +0200, Vladimir Zapolskiy wrote:
> > > On 2/7/22 4:39 PM, Mark Brown wrote:

> > > > The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> > > > a temporary bodge.

> It's not a temporary bodge. The i2c-core piece was reverted, but not
> the mediatek driver code/bindings.
> Vladimir has provided a replacement for the i2c-core code handling the
> vbus-regulator. When thee code will be back, the code from i2c-cci can
> be removed. The bindings will be the same.

I would hope it's a temporary thing given the namespace collision
issues...

> > There's also the option of representing this as a separate thing on or
> > part of the bus.

> 4) (which you have implemented in your patch). Add support for  the
> vbus-supplies property for the I2C CCI controllers.

> This is the option I'd vote for.

Do these controllers actually have a supply called vbus?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Dmitry Baryshkov]

On Thu, 10 Feb 2022 at 18:45, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Feb 10, 2022 at 06:33:09PM +0300, Dmitry Baryshkov wrote:
> > On Tue, 8 Feb 2022 at 16:16, Mark Brown <broonie@kernel.org> wrote:
> > > On Mon, Feb 07, 2022 at 08:31:30PM +0200, Vladimir Zapolskiy wrote:
> > > > On 2/7/22 4:39 PM, Mark Brown wrote:
>
> > > > > The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> > > > > a temporary bodge.
>
> > It's not a temporary bodge. The i2c-core piece was reverted, but not
> > the mediatek driver code/bindings.
> > Vladimir has provided a replacement for the i2c-core code handling the
> > vbus-regulator. When thee code will be back, the code from i2c-cci can
> > be removed. The bindings will be the same.
>
> I would hope it's a temporary thing given the namespace collision
> issues...

Which collision? CCI doesn't have a separate vbus power input (and
probably never will).

>
> > > There's also the option of representing this as a separate thing on or
> > > part of the bus.
>
> > 4) (which you have implemented in your patch). Add support for  the
> > vbus-supplies property for the I2C CCI controllers.
>
> > This is the option I'd vote for.
>
> Do these controllers actually have a supply called vbus?

No. It's a separate entity, a regulator-controller pull-up for the bus.
So far we'd like to hear better suggestions. Using regulator-always-on
doesn't sound like a good idea, it will increase unnecessary power
drain.

-- 
With best wishes
Dmitry

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

On Thu, Feb 10, 2022 at 08:32:09PM +0300, Dmitry Baryshkov wrote:
> On Thu, 10 Feb 2022 at 18:45, Mark Brown <broonie@kernel.org> wrote:

> > I would hope it's a temporary thing given the namespace collision
> > issues...

> Which collision? CCI doesn't have a separate vbus power input (and
> probably never will).

That "probably" there is doing some work, and if you're doing something
at the I2C core level (as it seems should be done) it needs to cope with
all possible controllers and devices.

> > Do these controllers actually have a supply called vbus?

> No. It's a separate entity, a regulator-controller pull-up for the bus.
> So far we'd like to hear better suggestions. Using regulator-always-on
> doesn't sound like a good idea, it will increase unnecessary power
> drain.

Please see my suggestions elsewhere in the thread.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Dmitry Baryshkov]

On Thu, 10 Feb 2022 at 20:36, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Feb 10, 2022 at 08:32:09PM +0300, Dmitry Baryshkov wrote:
> > On Thu, 10 Feb 2022 at 18:45, Mark Brown <broonie@kernel.org> wrote:
>
> > > I would hope it's a temporary thing given the namespace collision
> > > issues...
>
> > Which collision? CCI doesn't have a separate vbus power input (and
> > probably never will).
>
> That "probably" there is doing some work, and if you're doing something
> at the I2C core level (as it seems should be done) it needs to cope with
> all possible controllers and devices.
>
> > > Do these controllers actually have a supply called vbus?
>
> > No. It's a separate entity, a regulator-controller pull-up for the bus.
> > So far we'd like to hear better suggestions. Using regulator-always-on
> > doesn't sound like a good idea, it will increase unnecessary power
> > drain.
>
> Please see my suggestions elsewhere in the thread.

Please excuse me. I missed the e-mail suggesting to move support for
that into the core level.
I'd second a request to handle the adapter->bus_regulator in the core code.
Would you be ok with the 'external-sda-scl-supply' property? Would you
demand that it's completely handled by the core layer (including DT
parsing) or should we let a driver parse the DT property?


-- 
With best wishes
Dmitry

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

On Thu, Feb 10, 2022 at 09:21:42PM +0300, Dmitry Baryshkov wrote:

> Please excuse me. I missed the e-mail suggesting to move support for
> that into the core level.

No problem.

> I'd second a request to handle the adapter->bus_regulator in the core code.
> Would you be ok with the 'external-sda-scl-supply' property? Would you
> demand that it's completely handled by the core layer (including DT
> parsing) or should we let a driver parse the DT property?

I'm not super worried about how it's implemented so long as the binding
is good for the long term - if doing it in a driver helps get things
done that's fixable later on without breaking ABI.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Dmitry Baryshkov]

On Thu, 10 Feb 2022 at 21:26, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Feb 10, 2022 at 09:21:42PM +0300, Dmitry Baryshkov wrote:
>
> > I'd second a request to handle the adapter->bus_regulator in the core code.
> > Would you be ok with the 'external-sda-scl-supply' property? Would you
> > demand that it's completely handled by the core layer (including DT
> > parsing) or should we let a driver parse the DT property?
>
> I'm not super worried about how it's implemented so long as the binding
> is good for the long term - if doing it in a driver helps get things
> done that's fixable later on without breaking ABI.

So, 'external-sda-scl-supply'?



-- 
With best wishes
Dmitry

Re: [PATCH 1/9] dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible

[Rob Herring]

On Thu, 03 Feb 2022 18:46:28 +0200, Vladimir Zapolskiy wrote:
> The change adds QCOM SM8450 compatible value to the list of QCOM CCI
> controller compatibles, the controller found on the SoC is equal to
> the ones found on previous SoC generations.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 0/9] i2c: qcom-cci: fixes and updates

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 470 bytes --]

Hi,

> The new bus adapter specific bus_regulator from commit 5a7b95fb993e
> ("i2c: core: support bus regulator controlling in adapter") is reused,

Reusing is nice, of course, but I hope you noticed that I needed to
revert this feature:

a19f75de73c2 ("Revert "i2c: core: support bus regulator controlling in adapter"")

The thread to get it re-applied is currently here:

https://lore.kernel.org/all/20220106122452.18719-1-wsa@kernel.org/

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/9] i2c: qcom-cci: fixes and updates

[Vladimir Zapolskiy]

Hi Wolfram,

On 2/11/22 7:49 PM, Wolfram Sang wrote:
> Hi,
> 
>> The new bus adapter specific bus_regulator from commit 5a7b95fb993e
>> ("i2c: core: support bus regulator controlling in adapter") is reused,
> 
> Reusing is nice, of course, but I hope you noticed that I needed to
> revert this feature:
> 
> a19f75de73c2 ("Revert "i2c: core: support bus regulator controlling in adapter"")

yes, I've seen it, and as far as I understand it's expected to get it
back after the regression fixes.

> The thread to get it re-applied is currently here:
> 
> https://lore.kernel.org/all/20220106122452.18719-1-wsa@kernel.org/
> 

A presented change in the series is I2C controller specific, so it works well
on top of the reverted feature, however it has a potential to be simplified
after the re-application.

Wolfram, can you please share your opinion on device tree binding name and
placement for an SDA/SDC pull-up controlled by a regulator?

See https://lore.kernel.org/all/682b7ffe-e162-bcf7-3c07-36b3a39c25ab@linaro.org

Thank you in advance.

--
Best wishes,
Vladimir

Re: [PATCH 0/9] i2c: qcom-cci: fixes and updates

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]


> > Reusing is nice, of course, but I hope you noticed that I needed to
> > revert this feature:
> > 
> > a19f75de73c2 ("Revert "i2c: core: support bus regulator controlling in adapter"")
> 
> yes, I've seen it, and as far as I understand it's expected to get it
> back after the regression fixes.

True, but work on this has stalled, sadly. I am gathering interested
parties for the topic here :)

> Wolfram, can you please share your opinion on device tree binding name and
> placement for an SDA/SDC pull-up controlled by a regulator?

For efficiency reasons, not before bus regulator has been applied again
because the above question depends on it IIUC. Until then, I'll work on
other items of my too long todo list.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/9] i2c: qcom-cci: fixes and updates

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]


> Vladimir Zapolskiy (9):
>   dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible
>   dt-bindings: i2c: qcom-cci: add description of a vbus-supply property
>   i2c: qcom-cci: don't delete an unregistered adapter
>   i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()
>   i2c: qcom-cci: initialize CCI controller after registration of adapters
>   i2c: qcom-cci: simplify probe by removing one loop over busses
>   i2c: qcom-cci: simplify access to bus data structure
>   i2c: qcom-cci: add support of optional vbus-supply regulators
>   i2c: qcom-cci: add sm8450 compatible

Patches 3+4 are already upstream. I wonder if patches 1+9 could be
applied to for-next also? Or is the vbus-supply a hard dependency here?
Patches 6+7 could probably also be resent individually after some
rebasing.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/9] i2c: qcom-cci: fixes and updates

[Vladimir Zapolskiy]

Hi Wolfram,

On 2/17/22 9:57 PM, Wolfram Sang wrote:
> 
>> Vladimir Zapolskiy (9):
>>    dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible
>>    dt-bindings: i2c: qcom-cci: add description of a vbus-supply property
>>    i2c: qcom-cci: don't delete an unregistered adapter
>>    i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()
>>    i2c: qcom-cci: initialize CCI controller after registration of adapters
>>    i2c: qcom-cci: simplify probe by removing one loop over busses
>>    i2c: qcom-cci: simplify access to bus data structure
>>    i2c: qcom-cci: add support of optional vbus-supply regulators
>>    i2c: qcom-cci: add sm8450 compatible
> 
> Patches 3+4 are already upstream. I wonder if patches 1+9 could be
> applied to for-next also? Or is the vbus-supply a hard dependency here?
> Patches 6+7 could probably also be resent individually after some
> rebasing.
> 

thank you for applying the fixes, 1/9 and 9/9 are also good to be applied
for-next, there is no dependency on vbus-supply, so I would appreciate, if
you take two more changes.

As you suggested I'd start working on a generic support of such an optional
bus supplier property, I believe at the moment everything is quite clear,
I'll start from testing the previous solution, however my preference is
to connect regulator on/off to master controller pm ops rather than slave
pm ops. Additionally I am not quite satisfied with 'vbus-supply' name,
some name without a confusing starting 'v' should be preferred IMO, like
'i2c-bus-supply' or 'sda-scl-supply', name suggestions are welcome.

--
Best wishes,
Vladimir

Re: [PATCH 1/9] dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

On Thu, Feb 03, 2022 at 06:46:28PM +0200, Vladimir Zapolskiy wrote:
> The change adds QCOM SM8450 compatible value to the list of QCOM CCI
> controller compatibles, the controller found on the SoC is equal to
> the ones found on previous SoC generations.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/9] i2c: qcom-cci: fixes and updates

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]


> thank you for applying the fixes, 1/9 and 9/9 are also good to be applied
> for-next, there is no dependency on vbus-supply, so I would appreciate, if
> you take two more changes.

Done now.

> As you suggested I'd start working on a generic support of such an optional
> bus supplier property, I believe at the moment everything is quite clear,
> I'll start from testing the previous solution, however my preference is
> to connect regulator on/off to master controller pm ops rather than slave
> pm ops. Additionally I am not quite satisfied with 'vbus-supply' name,

Did I get this right? You want to reimplement bus regulator handling in
the bus driver when we already have pending patches to add it to the I2C
core?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]