| Message ID | 20200318150059.21714-3-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i2c: of: reserve unknown and ancillary addresses | expand |
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.
> 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.
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
> 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.
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
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 >
> > 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.
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.
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