[RFC,v2,2/6] i2c: allow DT nodes without 'compatible'
diff mbox series

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

Commit Message

Wolfram Sang March 18, 2020, 3 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>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/i2c/i2c-ocores.txt | 2 --
 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(+), 8 deletions(-)

Comments

Luca Ceresoli April 10, 2020, 1:49 p.m. UTC | #1
Hi,

On 18/03/20 16:00, 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>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Rob Herring <robh@kernel.org>

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

As I said in the reply to v1, I think we should reserve addresses also
when there is a compatible string but no matching driver, but this is
another story and can be handled separately.
Wolfram Sang April 15, 2020, 7:59 a.m. UTC | #2
> As I said in the reply to v1, I think we should reserve addresses also
> when there is a compatible string but no matching driver, but this is
> another story and can be handled separately.

Unless I misunderstand you, I think they do already. Note that
only 'i2cdetect' shows a device as busy *IFF* there is a driver bound to
it. The internal 'i2c_check_addr_busy' does not care about a driver
being bound. You can check this by trying to use
i2c_new_ancillary_device() with an address which is already described in
DT but which driver is disabled.
Kieran Bingham April 15, 2020, 8:07 a.m. UTC | #3
Hi Wolfram,

On 15/04/2020 08:59, Wolfram Sang wrote:
> 
>> As I said in the reply to v1, I think we should reserve addresses also
>> when there is a compatible string but no matching driver, but this is
>> another story and can be handled separately.
> 
> Unless I misunderstand you, I think they do already. Note that
> only 'i2cdetect' shows a device as busy *IFF* there is a driver bound to
> it. The internal 'i2c_check_addr_busy' does not care about a driver
> being bound. You can check this by trying to use
> i2c_new_ancillary_device() with an address which is already described in
> DT but which driver is disabled.

Aha, is it easy enough to distinguish that difference in user-space so
that we can present a specific character to indicate this in i2cdetect?
Or is that not so easy?

--
Kieran
Wolfram Sang April 15, 2020, 8:16 a.m. UTC | #4
> Aha, is it easy enough to distinguish that difference in user-space so
> that we can present a specific character to indicate this in i2cdetect?
> Or is that not so easy?

I thought about it shortly but have not come up with a way of doing
that. This is the code in i2cdetect:

	/* Set slave address */
	if (ioctl(file, I2C_SLAVE, i+j) < 0) {
		if (errno == EBUSY) {
			printf("UU ");
			continue;
		} else {
			fprintf(stderr, "Error: Could not set "
				"address to 0x%02x: %s\n", i+j,
				strerror(errno));
			return -1;
		}
	}

So, if we chose to use another errno to indicate 'reserved' and update
i2cdetect, all old versions of i2cdetect will have ugly error messages.
And adding another IOCTL just for printing reserved addresses neither
sounds great.
Kieran Bingham April 15, 2020, 8:38 a.m. UTC | #5
Hi Wolfram,

On 15/04/2020 09:16, Wolfram Sang wrote:
> 
>> Aha, is it easy enough to distinguish that difference in user-space so
>> that we can present a specific character to indicate this in i2cdetect?
>> Or is that not so easy?
> 
> I thought about it shortly but have not come up with a way of doing
> that. This is the code in i2cdetect:
> 
> 	/* Set slave address */
> 	if (ioctl(file, I2C_SLAVE, i+j) < 0) {
> 		if (errno == EBUSY) {
> 			printf("UU ");
> 			continue;
> 		} else {
> 			fprintf(stderr, "Error: Could not set "
> 				"address to 0x%02x: %s\n", i+j,
> 				strerror(errno));
> 			return -1;
> 		}
> 	}
> 
> So, if we chose to use another errno to indicate 'reserved' and update
> i2cdetect, all old versions of i2cdetect will have ugly error messages.
> And adding another IOCTL just for printing reserved addresses neither
> sounds great.

Indeed, a different errno would be about all we could do - and it would
seemingly leave old i2cdetects with complete failures, if it goes
through that non-EBUSY code path.

Ayeeeee :-S

--
Kieran
Kieran Bingham April 15, 2020, 8:48 a.m. UTC | #6
Hi Wolfram,

On 18/03/2020 15:00, 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().

Oh how I long to be able to give these 'identifiable names' within the
system, but that will probably mess up all the driver matching and
binding, so would be quite tricky perhaps.

But I like the ability to distinguish the two different types.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-ocores.txt | 2 --
>  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(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
> index 6b25a80ae8d3..fc8ea27934b3 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>;
>  		};
>  	};
> @@ -68,7 +67,6 @@ or
>  		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 3d7b8a00a7d9..84464e439df5 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
>
Wolfram Sang April 15, 2020, 9:46 a.m. UTC | #7
> > 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().
> 
> Oh how I long to be able to give these 'identifiable names' within the
> system, but that will probably mess up all the driver matching and
> binding, so would be quite tricky perhaps.

I haven't found a way yet to use 'name' to give more meaningful
descriptions to dummies. My best bet so far is to use additional links
in sysfs.
Luca Ceresoli April 16, 2020, 2:53 p.m. UTC | #8
Hi,

On 15/04/20 09:59, Wolfram Sang wrote:
> 
>> As I said in the reply to v1, I think we should reserve addresses also
>> when there is a compatible string but no matching driver, but this is
>> another story and can be handled separately.
> 
> Unless I misunderstand you, I think they do already. Note that
> only 'i2cdetect' shows a device as busy *IFF* there is a driver bound to
> it. The internal 'i2c_check_addr_busy' does not care about a driver
> being bound. You can check this by trying to use
> i2c_new_ancillary_device() with an address which is already described in
> DT but which driver is disabled.
> 

Ah, yes! I was assuming the opposite but I double checked and you're
right of course.

Sorry for the noise.

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
index 6b25a80ae8d3..fc8ea27934b3 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>;
 		};
 	};
@@ -68,7 +67,6 @@  or
 		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 3d7b8a00a7d9..84464e439df5 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