| Message ID | 1358189602-24180-2-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 14, 2013 at 10:53:21AM -0800, Doug Anderson wrote: > This allows you to get the equivalent functionality of > i2c_add_numbered_adapter() with all data in the device tree and no > special case code in your driver. This is a common device tree > technique. > > For quick reference, the FDT syntax for using an alias to provide an > ID looks like: > aliases { > i2c0 = &i2c_0; > i2c1 = &i2c_1; > }; > > Signed-off-by: Doug Anderson <dianders@chromium.org> > Acked-by: Haojian Zhuang <haojian.zhuang@gmail.com> The call path for i2c_add_numbered_adapter() with nr == -1 is a little awkward now, but I don't see much how it can be improved much. Acked-by: Olof Johansson <olof@lixom.net> -Olof
Hi Doug, On Mon, Jan 14, 2013 at 10:53:21AM -0800, Doug Anderson wrote: > This allows you to get the equivalent functionality of > i2c_add_numbered_adapter() with all data in the device tree and no > special case code in your driver. This is a common device tree > technique. > > For quick reference, the FDT syntax for using an alias to provide an > ID looks like: > aliases { > i2c0 = &i2c_0; > i2c1 = &i2c_1; > }; > > Signed-off-by: Doug Anderson <dianders@chromium.org> > Acked-by: Haojian Zhuang <haojian.zhuang@gmail.com> > --- > Changes in v2: None > > drivers/i2c/i2c-core.c | 105 +++++++++++++++++++++++++++++++++++------------- > 1 files changed, 77 insertions(+), 28 deletions(-) > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index e388590..a60ed6d 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -921,13 +921,81 @@ out_list: > } > > /** > + * i2c_get_number_from_dt - get the adapter number based on dt alias > + * @adap: the adapter to look at > + * > + * Check whether there's an alias in the FDT that gives an ID for this i2c > + * device. Use an alias like "i2c<nr>", like: > + * aliases { > + * i2c0 = &i2c_0; > + * i2c1 = &i2c_1; > + * }; > + * > + * Returns the ID if found. If no alias is found returns -1. > + */ > +static int i2c_get_number_from_dt(struct i2c_adapter *adap) i2c_get_id_from_dt()? > +{ > + struct device *dev = &adap->dev; > + int id; > + > + if (!dev->of_node) > + return -1; -ESOMETHING? > + > + id = of_alias_get_id(dev->of_node, "i2c"); > + if (id < 0) > + return -1; > + return id; Simply 'return of_alias_get_id(...)'? Even more, since this function boils down to calling of_alias_get_id only and is only used once, I'd think we can implement that directly and drop this function. That shouldn't hurt readability. > +} > + > +/** > + * _i2c_add_numbered_adapter - i2c_add_numbered_adapter where nr is never -1 > + * @adap: the adapter to register (with adap->nr initialized) > + * Context: can sleep > + * > + * See i2c_add_numbered_adapter() for details. > + */ > +static int _i2c_add_numbered_adapter(struct i2c_adapter *adap) All other internal functions are prefixed with '__'. > +{ > + int id; > + int status; > + > + /* Handled by wrappers */ > + BUG_ON(adap->nr == -1); Is that a reason to halt the kernel? WARN and bailing out would do IMO. > + > + if (adap->nr & ~MAX_IDR_MASK) > + return -EINVAL; Note the idr-cleanup series from Tejun Heo. Given that his series is scheduled for v3.9, I'd like to have your patches on top of his. Thanks, Wolfram
Wolfram, Thanks for the review. New patch was just sent. :) On Sun, Feb 10, 2013 at 4:19 AM, Wolfram Sang <w.sang@pengutronix.de> wrote: >> +static int i2c_get_number_from_dt(struct i2c_adapter *adap) > > i2c_get_id_from_dt()? Done. >> + if (!dev->of_node) >> + return -1; > > -ESOMETHING? Function has been removed, as per below. >> + >> + id = of_alias_get_id(dev->of_node, "i2c"); >> + if (id < 0) >> + return -1; >> + return id; > > Simply 'return of_alias_get_id(...)'? Even more, since this function > boils down to calling of_alias_get_id only and is only used once, I'd > think we can implement that directly and drop this function. That > shouldn't hurt readability. Good point. Done. >> +/** >> + * _i2c_add_numbered_adapter - i2c_add_numbered_adapter where nr is never -1 >> + * @adap: the adapter to register (with adap->nr initialized) >> + * Context: can sleep >> + * >> + * See i2c_add_numbered_adapter() for details. >> + */ >> +static int _i2c_add_numbered_adapter(struct i2c_adapter *adap) > > All other internal functions are prefixed with '__'. Done. >> +{ >> + int id; >> + int status; >> + >> + /* Handled by wrappers */ >> + BUG_ON(adap->nr == -1); > > Is that a reason to halt the kernel? WARN and bailing out would do IMO. Done. >> + >> + if (adap->nr & ~MAX_IDR_MASK) >> + return -EINVAL; > > Note the idr-cleanup series from Tejun Heo. Given that his series is > scheduled for v3.9, I'd like to have your patches on top of his. Done.
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index e388590..a60ed6d 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -921,13 +921,81 @@ out_list: } /** + * i2c_get_number_from_dt - get the adapter number based on dt alias + * @adap: the adapter to look at + * + * Check whether there's an alias in the FDT that gives an ID for this i2c + * device. Use an alias like "i2c<nr>", like: + * aliases { + * i2c0 = &i2c_0; + * i2c1 = &i2c_1; + * }; + * + * Returns the ID if found. If no alias is found returns -1. + */ +static int i2c_get_number_from_dt(struct i2c_adapter *adap) +{ + struct device *dev = &adap->dev; + int id; + + if (!dev->of_node) + return -1; + + id = of_alias_get_id(dev->of_node, "i2c"); + if (id < 0) + return -1; + return id; +} + +/** + * _i2c_add_numbered_adapter - i2c_add_numbered_adapter where nr is never -1 + * @adap: the adapter to register (with adap->nr initialized) + * Context: can sleep + * + * See i2c_add_numbered_adapter() for details. + */ +static int _i2c_add_numbered_adapter(struct i2c_adapter *adap) +{ + int id; + int status; + + /* Handled by wrappers */ + BUG_ON(adap->nr == -1); + + if (adap->nr & ~MAX_IDR_MASK) + return -EINVAL; + +retry: + if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0) + return -ENOMEM; + + mutex_lock(&core_lock); + /* "above" here means "above or equal to", sigh; + * we need the "equal to" result to force the result + */ + status = idr_get_new_above(&i2c_adapter_idr, adap, adap->nr, &id); + if (status == 0 && id != adap->nr) { + status = -EBUSY; + idr_remove(&i2c_adapter_idr, id); + } + mutex_unlock(&core_lock); + if (status == -EAGAIN) + goto retry; + + if (status == 0) + status = i2c_register_adapter(adap); + return status; +} + +/** * i2c_add_adapter - declare i2c adapter, use dynamic bus number * @adapter: the adapter to add * Context: can sleep * * This routine is used to declare an I2C adapter when its bus number - * doesn't matter. Examples: for I2C adapters dynamically added by - * USB links or PCI plugin cards. + * doesn't matter or when its bus number is specified by an dt alias. + * Examples of bases when the bus number doesn't matter: I2C adapters + * dynamically added by USB links or PCI plugin cards. * * When this returns zero, a new bus number was allocated and stored * in adap->nr, and the specified adapter became available for clients. @@ -937,6 +1005,12 @@ int i2c_add_adapter(struct i2c_adapter *adapter) { int id, res = 0; + id = i2c_get_number_from_dt(adapter); + if (id >= 0) { + adapter->nr = id; + return _i2c_add_numbered_adapter(adapter); + } + retry: if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0) return -ENOMEM; @@ -983,34 +1057,9 @@ EXPORT_SYMBOL(i2c_add_adapter); */ int i2c_add_numbered_adapter(struct i2c_adapter *adap) { - int id; - int status; - if (adap->nr == -1) /* -1 means dynamically assign bus id */ return i2c_add_adapter(adap); - if (adap->nr & ~MAX_IDR_MASK) - return -EINVAL; - -retry: - if (idr_pre_get(&i2c_adapter_idr, GFP_KERNEL) == 0) - return -ENOMEM; - - mutex_lock(&core_lock); - /* "above" here means "above or equal to", sigh; - * we need the "equal to" result to force the result - */ - status = idr_get_new_above(&i2c_adapter_idr, adap, adap->nr, &id); - if (status == 0 && id != adap->nr) { - status = -EBUSY; - idr_remove(&i2c_adapter_idr, id); - } - mutex_unlock(&core_lock); - if (status == -EAGAIN) - goto retry; - - if (status == 0) - status = i2c_register_adapter(adap); - return status; + return _i2c_add_numbered_adapter(adap); } EXPORT_SYMBOL_GPL(i2c_add_numbered_adapter);
This allows you to get the equivalent functionality of i2c_add_numbered_adapter() with all data in the device tree and no special case code in your driver. This is a common device tree technique. For quick reference, the FDT syntax for using an alias to provide an ID looks like: aliases { i2c0 = &i2c_0; i2c1 = &i2c_1; }; Signed-off-by: Doug Anderson <dianders@chromium.org> Acked-by: Haojian Zhuang <haojian.zhuang@gmail.com> --- Changes in v2: None drivers/i2c/i2c-core.c | 105 +++++++++++++++++++++++++++++++++++------------- 1 files changed, 77 insertions(+), 28 deletions(-)