[RFC,3/7] i2c: allow DT nodes without 'compatible'
diff mbox series

Message ID 20200220172403.26062-4-wsa+renesas@sang-engineering.com
State RFC
Headers show
Series
  • i2c: of: reserve unknown and ancillary addresses
Related show

Commit Message

Wolfram Sang Feb. 20, 2020, 5:23 p.m. UTC
Sometimes, we have unknown devices in a system and still want to block
their address. For that, we allow DT nodes with only a 'reg' property.
These devices will be bound to the "dummy" driver but with the name
"reserved". That way, we can distinguish them and even hand them over to
the "dummy" driver later when they are really requested using
i2c_new_ancillary_device().

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 Documentation/devicetree/bindings/i2c/i2c-ocores.txt | 1 -
 Documentation/devicetree/bindings/i2c/i2c.txt        | 4 +++-
 drivers/i2c/i2c-core-base.c                          | 1 +
 drivers/i2c/i2c-core-of.c                            | 8 +++-----
 drivers/i2c/i2c-core.h                               | 1 +
 5 files changed, 8 insertions(+), 7 deletions(-)

Comments

Geert Uytterhoeven Feb. 21, 2020, 9:45 a.m. UTC | #1
Hi Wolfram,

On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Sometimes, we have unknown devices in a system and still want to block
> their address. For that, we allow DT nodes with only a 'reg' property.
> These devices will be bound to the "dummy" driver but with the name
> "reserved". That way, we can distinguish them and even hand them over to
> the "dummy" driver later when they are really requested using
> i2c_new_ancillary_device().
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
but one question below.

> --- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
> @@ -50,7 +50,6 @@ Examples:
>                 reg-io-width = <1>;     /* 8 bit read/write */
>
>                 dummy@60 {
> -                       compatible = "dummy";
>                         reg = <0x60>;
>                 };
>         };

There's a second instance to remove 18 lines below.

> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -27,17 +27,15 @@ int of_i2c_get_board_info(struct device *dev, struct device_node *node,
>
>         memset(info, 0, sizeof(*info));
>
> -       if (of_modalias_node(node, info->type, sizeof(info->type)) < 0) {
> -               dev_err(dev, "of_i2c: modalias failure on %pOF\n", node);
> -               return -EINVAL;
> -       }
> -
>         ret = of_property_read_u32(node, "reg", &addr);
>         if (ret) {
>                 dev_err(dev, "of_i2c: invalid reg on %pOF\n", node);
>                 return ret;
>         }
>
> +       if (of_modalias_node(node, info->type, sizeof(info->type)) < 0)
> +               strlcpy(info->type, I2C_RESERVED_DRV_NAME, sizeof(I2C_RESERVED_DRV_NAME));

Could this cause a regression, e.g. if people already have such dummy
nodes in their DTS, and use sysfs new_device from userspace to
instantiate the device later?

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Geert Uytterhoeven Feb. 21, 2020, 9:48 a.m. UTC | #2
On Fri, Feb 21, 2020 at 10:45 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > Sometimes, we have unknown devices in a system and still want to block
> > their address. For that, we allow DT nodes with only a 'reg' property.
> > These devices will be bound to the "dummy" driver but with the name
> > "reserved". That way, we can distinguish them and even hand them over to
> > the "dummy" driver later when they are really requested using
> > i2c_new_ancillary_device().
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

FTR, depending on the extra dummy removed.

> but one question below.
>
> > --- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
> > @@ -50,7 +50,6 @@ Examples:
> >                 reg-io-width = <1>;     /* 8 bit read/write */
> >
> >                 dummy@60 {
> > -                       compatible = "dummy";
> >                         reg = <0x60>;
> >                 };
> >         };
>
> There's a second instance to remove 18 lines below.

Gr{oetje,eeting}s,

                        Geert
Luca Ceresoli Feb. 23, 2020, 11:11 p.m. UTC | #3
Hi,

On 21/02/20 10:45, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Thu, Feb 20, 2020 at 6:26 PM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
>> Sometimes, we have unknown devices in a system and still want to block
>> their address. For that, we allow DT nodes with only a 'reg' property.
>> These devices will be bound to the "dummy" driver but with the name
>> "reserved". That way, we can distinguish them and even hand them over to
>> the "dummy" driver later when they are really requested using
>> i2c_new_ancillary_device().
>>
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Cc:ing Alexandre who raised the need for a described-but-disabled I2C node.

> Thanks for your patch!
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> but one question below.
> 
>> --- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
>> @@ -50,7 +50,6 @@ Examples:
>>                 reg-io-width = <1>;     /* 8 bit read/write */
>>
>>                 dummy@60 {
>> -                       compatible = "dummy";
>>                         reg = <0x60>;
>>                 };
>>         };
> 
> There's a second instance to remove 18 lines below.
> 
>> --- a/drivers/i2c/i2c-core-of.c
>> +++ b/drivers/i2c/i2c-core-of.c
>> @@ -27,17 +27,15 @@ int of_i2c_get_board_info(struct device *dev, struct device_node *node,
>>
>>         memset(info, 0, sizeof(*info));
>>
>> -       if (of_modalias_node(node, info->type, sizeof(info->type)) < 0) {
>> -               dev_err(dev, "of_i2c: modalias failure on %pOF\n", node);
>> -               return -EINVAL;
>> -       }
>> -
>>         ret = of_property_read_u32(node, "reg", &addr);
>>         if (ret) {
>>                 dev_err(dev, "of_i2c: invalid reg on %pOF\n", node);
>>                 return ret;
>>         }
>>
>> +       if (of_modalias_node(node, info->type, sizeof(info->type)) < 0)
>> +               strlcpy(info->type, I2C_RESERVED_DRV_NAME, sizeof(I2C_RESERVED_DRV_NAME));
> 
> Could this cause a regression, e.g. if people already have such dummy
> nodes in their DTS, and use sysfs new_device from userspace to
> instantiate the device later?

Such a DTS would be illegal because "compatible" has been a required
property so far. Thus one could leave such people out in the cold
because they went on an unsupported path. Not super nice anyway.

However I'd like to view the issue from the DT point of view. DT
describes the hardware, and it is possible (and even desirable) that the
firmware provides the DTB independently from the OS, and the kernel
consumes it. It this scenario, firmware could and should describe all
I2C slaves with proper "compatible" property, and there is no way to
remove it, in a clean way at least.

But the kernel currently ignores nodes that have no matching driver,
right? So in this case the kernel knows that that address is used, but
ignores this information and considers the address as available.
Seen in this perspective, we should have a "compatible" for all nodes:
it is just describing the hardware and could be out of the kernel
control. But instead of discarding all nodes without a matching driver,
the i2c-core-of code should mark them as "reserved".

Does it sound correct?

Clearly this does not fit the case reported by Alexandre: a device
having a driver which is known to be badly buggy, so we don't want to
instantiate it. But again, this should not affect DT as it is not
describing the HW, but only an implementation detail. Probably disabling
or blacklisting the driver would be a better option there?

My apologies to Wolfram, I appreciate a lot the effort you are doing,
but before reviewing this patch I have never realized what I tried to
explain above.
Rob Herring Feb. 26, 2020, 4:30 p.m. UTC | #4
On Thu, 20 Feb 2020 18:23:59 +0100, Wolfram Sang wrote:
> Sometimes, we have unknown devices in a system and still want to block
> their address. For that, we allow DT nodes with only a 'reg' property.
> These devices will be bound to the "dummy" driver but with the name
> "reserved". That way, we can distinguish them and even hand them over to
> the "dummy" driver later when they are really requested using
> i2c_new_ancillary_device().
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-ocores.txt | 1 -
>  Documentation/devicetree/bindings/i2c/i2c.txt        | 4 +++-
>  drivers/i2c/i2c-core-base.c                          | 1 +
>  drivers/i2c/i2c-core-of.c                            | 8 +++-----
>  drivers/i2c/i2c-core.h                               | 1 +
>  5 files changed, 8 insertions(+), 7 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>
Wolfram Sang March 12, 2020, 11:19 a.m. UTC | #5
Hi Luca,

> But the kernel currently ignores nodes that have no matching driver,
> right? So in this case the kernel knows that that address is used, but
> ignores this information and considers the address as available.

I'd rather call it "unbound" than available. See later.

> Seen in this perspective, we should have a "compatible" for all nodes:
> it is just describing the hardware and could be out of the kernel
> control. But instead of discarding all nodes without a matching driver,

And what compatible value would you use if you know there is something
sitting there and don't know what? This is what this series aims to
address because we thought a compatible name like "reserved" would not
be a good idea.

> the i2c-core-of code should mark them as "reserved".
> 
> Does it sound correct?

With this patch series, this is quite what happens for ancillary
addresses. They get their own dummy device automatically now, are marked
as reserved and can only be obtained by the driver which bound to the
main address (of_node of ancillary addr == of_node of main addr).

For the main address, I think things are a bit different. They already
have their struct device. The only thing we gain from reserving them (=
binding to the dummy driver) is that they are kinda blocked for
userspace access. The "protection" is kinda low, though. There are
already ways to communicate with bound addresses from userspace.

In kernel space we still need to probe this address until a driver is
bound to it, I don't see what a "reserved" state gains us here. If we
are talking about the pool of available addresses, we are all good
because we operate on existing struct device and don't care if they are
bound or not. Or?

What would be kinda nice, though, is when i2cdetect could show reserved
addresses (unbound but having a struct device) as "RR" or so. However, I
currently can't see a way to do it without breaking compability.

> Clearly this does not fit the case reported by Alexandre: a device
> having a driver which is known to be badly buggy, so we don't want to
> instantiate it. But again, this should not affect DT as it is not
> describing the HW, but only an implementation detail. Probably disabling
> or blacklisting the driver would be a better option there?

"Fixing the driver" is the first thing coming to my mind ;) But yeah,
blacklisting would be another good solution. With only the information
above, DT is not the right place to fix a broken driver.

> My apologies to Wolfram, I appreciate a lot the effort you are doing,
> but before reviewing this patch I have never realized what I tried to
> explain above.

All good, Luca! Talking over code usually brings in viewpoints which
have been missed so far. This is expected. Actually, I am very happy to
have this discussion!

All the best,

   Wolfram
Alexandre Belloni March 12, 2020, 11:44 a.m. UTC | #6
On 12/03/2020 12:19:51+0100, Wolfram Sang wrote:
> > Clearly this does not fit the case reported by Alexandre: a device
> > having a driver which is known to be badly buggy, so we don't want to
> > instantiate it. But again, this should not affect DT as it is not
> > describing the HW, but only an implementation detail. Probably disabling
> > or blacklisting the driver would be a better option there?
> 
> "Fixing the driver" is the first thing coming to my mind ;) But yeah,
> blacklisting would be another good solution. With only the information
> above, DT is not the right place to fix a broken driver.
> 

To be clear, the driver is working properly but the HW isn't. It is a
PMIC and we need to avoid linux talking to it so the PMIC doesn't end up
killing the bus.

We end up with a node properly described in the device tree but with
status = "disabled". The relevance to the discussion was that you know
what is there and you want to avoid using its address.

See the pmic node on i2c1 in arch/arm/boot/dts/at91-sama5d3_xplained.dts
for what I'm referring to.

> > My apologies to Wolfram, I appreciate a lot the effort you are doing,
> > but before reviewing this patch I have never realized what I tried to
> > explain above.
> 
> All good, Luca! Talking over code usually brings in viewpoints which
> have been missed so far. This is expected. Actually, I am very happy to
> have this discussion!
> 
> All the best,
> 
>    Wolfram
>

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
index 6b25a80ae8d3..2762effdd270 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
@@ -50,7 +50,6 @@  Examples:
 		reg-io-width = <1>;	/* 8 bit read/write */
 
 		dummy@60 {
-			compatible = "dummy";
 			reg = <0x60>;
 		};
 	};
diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index 9a53df4243c6..989b315e09dc 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -21,7 +21,9 @@  flags can be attached to the address. I2C_TEN_BIT_ADDRESS is used to mark a 10
 bit address. It is needed to avoid the ambiguity between e.g. a 7 bit address
 of 0x50 and a 10 bit address of 0x050 which, in theory, can be on the same bus.
 Another flag is I2C_OWN_SLAVE_ADDRESS to mark addresses on which we listen to
-be devices ourselves.
+be devices ourselves. The 'reg' property of a child is required. The
+'compatible' property is not. Empty 'compatible' child entries can be used to
+describe unknown devices or addresses which shall be blocked for other reasons.
 
 Optional properties
 -------------------
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 8df2fa10c48a..4000a4384306 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -854,6 +854,7 @@  EXPORT_SYMBOL_GPL(i2c_unregister_device);
 
 static const struct i2c_device_id dummy_id[] = {
 	{ I2C_DUMMY_DRV_NAME, 0 },
+	{ I2C_RESERVED_DRV_NAME, 0 },
 	{ },
 };
 
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index 6787c1f71483..d8d111ad6c85 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -27,17 +27,15 @@  int of_i2c_get_board_info(struct device *dev, struct device_node *node,
 
 	memset(info, 0, sizeof(*info));
 
-	if (of_modalias_node(node, info->type, sizeof(info->type)) < 0) {
-		dev_err(dev, "of_i2c: modalias failure on %pOF\n", node);
-		return -EINVAL;
-	}
-
 	ret = of_property_read_u32(node, "reg", &addr);
 	if (ret) {
 		dev_err(dev, "of_i2c: invalid reg on %pOF\n", node);
 		return ret;
 	}
 
+	if (of_modalias_node(node, info->type, sizeof(info->type)) < 0)
+		strlcpy(info->type, I2C_RESERVED_DRV_NAME, sizeof(I2C_RESERVED_DRV_NAME));
+
 	if (addr & I2C_TEN_BIT_ADDRESS) {
 		addr &= ~I2C_TEN_BIT_ADDRESS;
 		info->flags |= I2C_CLIENT_TEN;
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index fb89fabf84d3..77b3a925ed95 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -23,6 +23,7 @@  int i2c_dev_irq_from_resources(const struct resource *resources,
 			       unsigned int num_resources);
 
 #define I2C_DUMMY_DRV_NAME "dummy"
+#define I2C_RESERVED_DRV_NAME "reserved"
 
 /*
  * We only allow atomic transfers for very late communication, e.g. to send