[U-Boot] [PATCH 1/2] dm: i2c: add DM_I2C_REQ_ALIAS to make sequence number optional
Simon Glass
sjg at chromium.org
Tue Feb 24 05:48:16 CET 2015
Hi Masahiro,
On Feb 23, 2015 8:16 PM, "Masahiro Yamada" <yamada.m at jp.panasonic.com>
wrote:
>
> Hi Simon,
>
>
>
> On Mon, 23 Feb 2015 11:01:02 -0700
> Simon Glass <sjg at chromium.org> wrote:
>
> > Hi Masahiro,
> >
> > On 20 February 2015 at 04:38, Masahiro Yamada <yamada.m at jp.panasonic.com>
wrote:
> > > For I2C (and some other uclasses), all the devices are required to
> > > have their own request sequence numbers to use them. Otherwise,
> > > uclass_get_device_by_seq() fails to get them. To specify those
> > > numbers, we have to add aliases to device trees. As a result, the
> > > "aliases" nodes grow bigger and bigger as we support more and more
> > > drivers in driver model.
> > > (See arch/arm/dts/uniphier-ph1-pro4-ref.dts for example.)
> > >
> > > It is true that the sequence number is useful for identifying a
> > > device with the same number independently of the binding order, but
> > > it is painful to add an alias to every device.
> >
> > That tends to me the standard. If you look at the .dts files in the
> > kernel they have aliases. I didn't see any Uniphier dts files in there
> > though.
>
> This is a problem for of my company.
> We are still using the locally hacked kernel
> that has not been mainlined yet.
>
> (BTW, before me, U-Boot was also locally hacked in an ugly way...)
>
>
> It would not be a problem to have some aliases,
> but I was just wondering if we must have an alias for every device.
>
> If this is the standardized way, that's OK.
>
>
> > >
> > > This commit intends to make this feature optional.
> > > If CONFIG_DM_I2C_SEQ_ALIAS is not defined, uclass_get_device() is
> > > called instead of uclass_get_device_by_seq(). So we do not have to
> > > add aliases of I2C devices.
> > >
> > > In most cases, this should work well enough because device trees are
> > > scanned from top to bottom and we usually describe device nodes in
> > > the order we expect for binding.
> >
> > As of recently the sequence numbers should be assigned sensibly (0, 1,
> > 2...) even without aliases. What do you see when you type 'dm uclass'
> > on your board?
>
>
> The following was displayed.
> No sequence number was displayed for I2C.
OK, this command displays ->req_seq. Can you check ->seq after you probe
the device?
>
>
>
> => dm uclass
> uclass 0: root
> - * root_driver @ 9fb52028
>
> Cannot find uclass for id 1: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
> Cannot find uclass for id 2: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
> Cannot find uclass for id 3: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
> Cannot find uclass for id 4: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
> Cannot find uclass for id 5: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
> Cannot find uclass for id 6: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
> uclass 7: simple_bus
> - * soc @ 9fb520a0
> - systembus at 00000000 @ 9fb520f8
>
> Cannot find uclass for id 8: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
> uclass 9: serial
> - * serial at 54006800 @ 9fb52170, 0
> - serial at 54006900 @ 9fb521d8, 1
>
> Cannot find uclass for id 10: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
> Cannot find uclass for id 11: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
> Cannot find uclass for id 12: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
> Cannot find uclass for id 13: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
> Cannot find uclass for id 14: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
> uclass 15: i2c
> - i2c at 58400000 @ 9fb52260
>
> uclass 16: i2c_generic
> uclass 17: i2c_eeprom
> - eeprom @ 9fb522d8
>
> Cannot find uclass for id 18: please add the UCLASS_DRIVER() declaration
for this UCLASS_... id
>
>
>
> > > Signed-off-by: Masahiro Yamada <yamada.m at jp.panasonic.com>
> > > ---
> > >
> > > common/cmd_i2c.c | 2 +-
> > > drivers/i2c/Kconfig | 10 ++++++++++
> > > drivers/i2c/i2c-uclass-compat.c | 2 +-
> > > drivers/i2c/i2c-uclass.c | 4 +++-
> > > include/i2c.h | 17 +++++++++++++++++
> > > 5 files changed, 32 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
> > > index fe8f77a..5fc927a 100644
> > > --- a/common/cmd_i2c.c
> > > +++ b/common/cmd_i2c.c
> > > @@ -138,7 +138,7 @@ static int cmd_i2c_set_bus_num(unsigned int
busnum)
> > > struct udevice *bus;
> > > int ret;
> > >
> > > - ret = uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus);
> > > + ret = i2c_get_bus(busnum, &bus);
> > > if (ret) {
> > > debug("%s: No bus %d\n", __func__, busnum);
> > > return ret;
> > > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> > > index 2cc776c..5c778ec 100644
> > > --- a/drivers/i2c/Kconfig
> > > +++ b/drivers/i2c/Kconfig
> > > @@ -13,6 +13,16 @@ config DM_I2C
> > > enabled together (it is not possible to use driver model
> > > for one and not the other).
> > >
> > > +config DM_I2C_SEQ_ALIAS
> > > + bool "Use aliases for numbering I2C buses"
> > > + depends on DM_I2C
> > > + default y
> > > + help
> > > + If this option is enabled, each I2C bus is numbered base on
its
> > > + alias in the device tree to identify a device with the same
number
> > > + all the time. Otherwise, I2C buses are numbered as they
are scanned
> > > + at binding.
> > > +
> > > config SYS_I2C_UNIPHIER
> > > bool "UniPhier I2C driver"
> > > depends on ARCH_UNIPHIER && DM_I2C
> > > diff --git a/drivers/i2c/i2c-uclass-compat.c
b/drivers/i2c/i2c-uclass-compat.c
> > > index 223f238..35736e8 100644
> > > --- a/drivers/i2c/i2c-uclass-compat.c
> > > +++ b/drivers/i2c/i2c-uclass-compat.c
> > > @@ -35,7 +35,7 @@ int i2c_probe(uint8_t chip_addr)
> > > struct udevice *bus, *dev;
> > > int ret;
> > >
> > > - ret = uclass_get_device_by_seq(UCLASS_I2C, cur_busnum, &bus);
> > > + ret = i2c_get_bus(UCLASS_I2C, cur_busnum, &bus);
> > > if (ret) {
> > > debug("Cannot find I2C bus %d: err=%d\n", cur_busnum,
ret);
> > > return ret;
> > > diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
> > > index a6991bf..0992737 100644
> > > --- a/drivers/i2c/i2c-uclass.c
> > > +++ b/drivers/i2c/i2c-uclass.c
> > > @@ -289,7 +289,7 @@ int i2c_get_chip_for_busnum(int busnum, int
chip_addr, uint offset_len,
> > > struct udevice *bus;
> > > int ret;
> > >
> > > - ret = uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus);
> > > + ret = i2c_get_bus(busnum, &bus);
> > > if (ret) {
> > > debug("Cannot find I2C bus %d\n", busnum);
> > > return ret;
> > > @@ -457,7 +457,9 @@ static int i2c_child_post_bind(struct udevice
*dev)
> > > UCLASS_DRIVER(i2c) = {
> > > .id = UCLASS_I2C,
> > > .name = "i2c",
> > > +#ifdef CONFIG_DM_I2C_SEQ_ALIAS
> > > .flags = DM_UC_FLAG_SEQ_ALIAS,
> > > +#endif
> > > .post_bind = i2c_post_bind,
> > > .post_probe = i2c_post_probe,
> > > .per_device_auto_alloc_size = sizeof(struct dm_i2c_bus),
> > > diff --git a/include/i2c.h b/include/i2c.h
> > > index 31b0389..6941df6 100644
> > > --- a/include/i2c.h
> > > +++ b/include/i2c.h
> > > @@ -410,6 +410,23 @@ int i2c_get_chip(struct udevice *bus, uint
chip_addr, uint offset_len,
> > > struct udevice **devp);
> > >
> > > /**
> > > + * i2c_get_bus() - get an I2C bus
> > > + *
> > > + * This returns the bus for the given bus number.
> > > + *
> > > + * @busnum: Bus number. If CONFIG_DM_I2C_SEQ_ALIAS is enabled,
this is a
> > > + * sequence number. Otherwise, it is a simple index.
> > > + * @devp: Stores pointer to a new bus if found
> > > + */
> > > +#ifdef CONFIG_DM_I2C_SEQ_ALIAS
> > > +#define i2c_get_bus(busnum, devp) \
> > > + uclass_get_device_by_seq(UCLASS_I2C, busnum,
devp)
> > > +#else
> > > +#define i2c_get_bus(busnum, devp) \
> > > + uclass_get_device(UCLASS_I2C, busnum, devp)
> > > +#endif
> >
> > I would prefer a solution that sits inside driver model itself. Once I
> > have a response to the above question we can see if this is possible.
> >
>
> I understand your thought.
> I take this series back and I marked it as RFC.
Sorry I can't test right now. But I wonder if we could/should build your
logic into a new function ? I need to think about it too.
Regards,
Simon
More information about the U-Boot
mailing list