[PATCH 0/2] i2c: Allow disabling auto detection via devicetree

[PATCH 0/2] i2c: Allow disabling auto detection via devicetree

[Vincent Whitchurch]

When a driver with a ->detect callback (such as lm75) is loaded, the i2c core
performs I2C transactions on the bus to all the addresses listed in that
driver's address_list.  This kind of probing wastes time and as
Documentation/i2c/instantiating-devices.rst says, this method is not
recommended and it is instead advised to list all devices in the devicetree.

However, even if all the devices are listed in the devicetree, there is
currently no way to prevent the core from attempting auto detection short of
patching controller drivers to not pass the I2C_CLASS* bits in adap->class.
The latter is not always possible since generic drivers like i2c-gpio set these
bits.

To avoid this unnecessary probing and reduce boot time, this series adds a
property to the devicetree and support in the I2C core to allow this feature to
be disabled.

Cc: linux-i2c@vger.kernel.org

Cc: devicetree@vger.kernel.org
Cc: krzk+dt@kernel.org
Cc: robh+dt@kernel.org

Cc: linux-kernel@vger.kernel.org

Vincent Whitchurch (2):
  i2c: add binding to prevent device detection
  i2c: core: support no-detect property

 Documentation/devicetree/bindings/i2c/i2c.txt | 4 ++++
 drivers/i2c/i2c-core-base.c                   | 8 +++++++-
 include/linux/i2c.h                           | 1 +
 3 files changed, 12 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 1/2] i2c: add binding to prevent device detection

[Vincent Whitchurch]

When drivers with ->detect callbacks are loaded, the I2C core does a
bunch of transactions to try to probe for these devices, regardless of
whether they are specified in the devicetree or not.  (This only happens
on I2C controllers whose drivers enable the I2C_CLASS* flags, but this
is the case for generic drivers like i2c-gpio.)

These kinds of transactions are unnecessary on systems where the
devicetree specifies all the devices on the I2C bus, so add a property
to allow them to be disabled.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 Documentation/devicetree/bindings/i2c/i2c.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index fc3dd7ec0445..5f7bdf4851e8 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -72,6 +72,10 @@ wants to support one of the below features, it should adapt these bindings.
 	this information to adapt power management to keep the arbitration awake
 	all the time, for example. Can not be combined with 'single-master'.
 
+- no-detect
+	states that the system should not attempt to automatically detect
+	devices which are not explicitly specified as child nodes.
+
 - pinctrl
 	add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
 	recovery, call it "gpio" or "recovery" (deprecated) state
-- 
2.34.1

Re: [PATCH 1/2] i2c: add binding to prevent device detection

[Krzysztof Kozlowski]

On 05/04/2022 14:16, Vincent Whitchurch wrote:
> When drivers with ->detect callbacks are loaded, the I2C core does a
> bunch of transactions to try to probe for these devices, regardless of
> whether they are specified in the devicetree or not.  (This only happens
> on I2C controllers whose drivers enable the I2C_CLASS* flags, but this
> is the case for generic drivers like i2c-gpio.)
> 

Thank you for your patch. There is something to discuss/improve.

> These kinds of transactions are unnecessary on systems where the
> devicetree specifies all the devices on the I2C bus, so add a property
> to allow them to be disabled.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c.txt | 4 ++++

Use the subject matching the subsystem (git log --oneline -- .......).

>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
> index fc3dd7ec0445..5f7bdf4851e8 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
> @@ -72,6 +72,10 @@ wants to support one of the below features, it should adapt these bindings.
>  	this information to adapt power management to keep the arbitration awake
>  	all the time, for example. Can not be combined with 'single-master'.
>  
> +- no-detect
> +	states that the system should not attempt to automatically detect
> +	devices which are not explicitly specified as child nodes.

Focus on the hardware, not on system and behavior. The hardware property
is that bus is completely described in Devicetree or detection does not
work correctly. I guess the property name could stay like this, but the
description could be rephrased.

Best regards,
Krzysztof

Re: [PATCH 1/2] i2c: add binding to prevent device detection

[Vincent Whitchurch]

On Tue, Apr 05, 2022 at 03:57:16PM +0200, Krzysztof Kozlowski wrote:
> On 05/04/2022 14:16, Vincent Whitchurch wrote:
> >  Documentation/devicetree/bindings/i2c/i2c.txt | 4 ++++
> 
> Use the subject matching the subsystem (git log --oneline -- .......).

I did look at the git log when writing the subject, but there's a bunch
of variation there so I went for the subject prefix which Wolfram
himself used since I assumed he would be the one applying these patches.

$ git log -n5 --oneline origin/master -- Documentation/devicetree/bindings/i2c/i2c.txt
6881e493b08f dt-bindings: net: New binding mctp-i2c-controller
168290fb2675 i2c: add binding to mark a bus as supporting SMBus-Alert
e6277308ac21 i2c: add binding to mark a bus as SMBus
db36e827d876 dt-bindings: i2c: add generic properties for GPIO bus recovery
fad5972a1eca i2c: add 'single-master' property to generic bindings

> 
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
> > index fc3dd7ec0445..5f7bdf4851e8 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
> > @@ -72,6 +72,10 @@ wants to support one of the below features, it should adapt these bindings.
> >  	this information to adapt power management to keep the arbitration awake
> >  	all the time, for example. Can not be combined with 'single-master'.
> >  
> > +- no-detect
> > +	states that the system should not attempt to automatically detect
> > +	devices which are not explicitly specified as child nodes.
> 
> Focus on the hardware, not on system and behavior. The hardware property
> is that bus is completely described in Devicetree or detection does not
> work correctly. I guess the property name could stay like this, but the
> description could be rephrased.

I'm having a hard time drawing the line between hardware and the
"system" here.  The bus being completely described in devicetree doesn't
sound like a hardware property to me, but, yes, I can change the
description to say that.

Re: [PATCH 1/2] i2c: add binding to prevent device detection

[Krzysztof Kozlowski]

On 05/04/2022 16:12, Vincent Whitchurch wrote:
> On Tue, Apr 05, 2022 at 03:57:16PM +0200, Krzysztof Kozlowski wrote:
>> On 05/04/2022 14:16, Vincent Whitchurch wrote:
>>>  Documentation/devicetree/bindings/i2c/i2c.txt | 4 ++++
>>
>> Use the subject matching the subsystem (git log --oneline -- .......).
> 
> I did look at the git log when writing the subject, but there's a bunch
> of variation there so I went for the subject prefix which Wolfram
> himself used since I assumed he would be the one applying these patches.
> 
> $ git log -n5 --oneline origin/master -- Documentation/devicetree/bindings/i2c/i2c.txt
> 6881e493b08f dt-bindings: net: New binding mctp-i2c-controller
> 168290fb2675 i2c: add binding to mark a bus as supporting SMBus-Alert
> e6277308ac21 i2c: add binding to mark a bus as SMBus
> db36e827d876 dt-bindings: i2c: add generic properties for GPIO bus recovery
> fad5972a1eca i2c: add 'single-master' property to generic bindings

The proper command for a subsystem is:
git log --oneline -- Documentation/devicetree/bindings/i2c/

and then you see mostly different pattern.

Other way, like for example Mark Brown prefers, is:
i2c: dt-bindings:
It would be fine as well, but the point is that you did not include the
dt-bindings prefix here at all and almost always I ignore all such emails.

>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
>>> index fc3dd7ec0445..5f7bdf4851e8 100644
>>> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
>>> @@ -72,6 +72,10 @@ wants to support one of the below features, it should adapt these bindings.
>>>  	this information to adapt power management to keep the arbitration awake
>>>  	all the time, for example. Can not be combined with 'single-master'.
>>>  
>>> +- no-detect
>>> +	states that the system should not attempt to automatically detect
>>> +	devices which are not explicitly specified as child nodes.
>>
>> Focus on the hardware, not on system and behavior. The hardware property
>> is that bus is completely described in Devicetree or detection does not
>> work correctly. I guess the property name could stay like this, but the
>> description could be rephrased.
> 
> I'm having a hard time drawing the line between hardware and the
> "system" here.  The bus being completely described in devicetree doesn't
> sound like a hardware property to me, but, yes, I can change the
> description to say that.

Then you need to come up with better description. :) By system I
understood here Operating System, therefore a Software piece or pieces.
Bindings should not rather focus on hardware instead of referencing
software. Software actually not matter for the bindings... There is
quite clear distinction between hardware and operating system...

Can you explain why do you think "bus being completely described" is not
a hardware description? Bindings and DTS describe hardware, so one of
parts of such description is whether nodes and other properties are
present. If all child nodes are present in DT - no-detect - and this is
a purely hardware point of view. No software here.

Best regards,
Krzysztof

Re: [PATCH 1/2] i2c: add binding to prevent device detection

[Krzysztof Kozlowski]

On 06/04/2022 09:26, Krzysztof Kozlowski wrote:
>>
>> I'm having a hard time drawing the line between hardware and the
>> "system" here.  The bus being completely described in devicetree doesn't
>> sound like a hardware property to me, but, yes, I can change the
>> description to say that.
> 
> Then you need to come up with better description. :) By system I
> understood here Operating System, therefore a Software piece or pieces.
> Bindings should not rather focus on hardware instead of referencing
> software.

Ah... I meant:
Bindings should rather focus on hardware instead of referencing software.

> Software actually not matter for the bindings... There is
> quite clear distinction between hardware and operating system...
> 
> Can you explain why do you think "bus being completely described" is not
> a hardware description? Bindings and DTS describe hardware, so one of
> parts of such description is whether nodes and other properties are
> present. If all child nodes are present in DT - no-detect - and this is
> a purely hardware point of view. No software here.



Best regards,
Krzysztof

[PATCH 2/2] i2c: core: support no-detect property

[Vincent Whitchurch]

If the devicetree specifies the no-detect property, we can avoid calling
drivers' detect callback and wasting time probing for devices which do
not exist.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 drivers/i2c/i2c-core-base.c | 8 +++++++-
 include/linux/i2c.h         | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index d43db2c3876e..d43025b84546 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1341,7 +1341,8 @@ static int i2c_do_add_adapter(struct i2c_driver *driver,
 			      struct i2c_adapter *adap)
 {
 	/* Detect supported devices on that bus, and instantiate them */
-	i2c_detect(adap, driver);
+	if (adap->detect)
+		i2c_detect(adap, driver);
 
 	return 0;
 }
@@ -1432,6 +1433,7 @@ EXPORT_SYMBOL_GPL(i2c_handle_smbus_host_notify);
 
 static int i2c_register_adapter(struct i2c_adapter *adap)
 {
+	struct device_node *np = adap->dev.of_node;
 	int res = -EINVAL;
 
 	/* Can't register until after driver model init */
@@ -1502,6 +1504,10 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 			 "Failed to create compatibility class link\n");
 #endif
 
+	adap->detect = true;
+	if (np && of_property_read_bool(np, "no-detect"))
+		adap->detect = false;
+
 	/* create pre-declared device nodes */
 	of_i2c_register_devices(adap);
 	i2c_acpi_install_space_handler(adap);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fbda5ada2afc..8fad5fe85685 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -728,6 +728,7 @@ struct i2c_adapter {
 	struct rt_mutex bus_lock;
 	struct rt_mutex mux_lock;
 
+	bool detect;
 	int timeout;			/* in jiffies */
 	int retries;
 	struct device dev;		/* the adapter device */
-- 
2.34.1