| Message ID | Y5B3S6KZTrYlIH8g@shell.armlinux.org.uk |
|---|---|
| Headers | show |
| Series | Add I2C fwnode lookup/get interfaces | expand |
Hi, On Wed, Dec 07, 2022 at 11:22:24AM +0000, Russell King (Oracle) wrote: > Add fwnode APIs for finding and getting I2C adapters, which will be > used by the SFP code. These are passed the fwnode corresponding to > the adapter, and return the I2C adapter. It is the responsibility of > the caller to find the appropriate fwnode. > > We keep the DT and ACPI interfaces, but where appropriate, recode them > to use the fwnode interfaces internally. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Looks good, just few minor comments below. :) > --- > drivers/i2c/i2c-core-acpi.c | 13 +------ > drivers/i2c/i2c-core-base.c | 72 +++++++++++++++++++++++++++++++++++++ > drivers/i2c/i2c-core-of.c | 51 ++------------------------ > include/linux/i2c.h | 9 +++++ > 4 files changed, 85 insertions(+), 60 deletions(-) > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > index 4dd777cc0c89..d6037a328669 100644 > --- a/drivers/i2c/i2c-core-acpi.c > +++ b/drivers/i2c/i2c-core-acpi.c > @@ -442,18 +442,7 @@ EXPORT_SYMBOL_GPL(i2c_acpi_find_adapter_by_handle); > > static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev) > { > - struct device *dev; > - struct i2c_client *client; > - > - dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev); > - if (!dev) > - return NULL; > - > - client = i2c_verify_client(dev); > - if (!client) > - put_device(dev); > - > - return client; > + return i2c_find_device_by_fwnode(acpi_fwnode_handle(adev)); > } > > static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value, > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 9aa7b9d9a485..254ec043ce90 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -1011,6 +1011,27 @@ void i2c_unregister_device(struct i2c_client *client) > } > EXPORT_SYMBOL_GPL(i2c_unregister_device); > > +/* must call put_device() when done with returned i2c_client device */ I think proper kernel-doc would be better here and all the exported functions. > +struct i2c_client *i2c_find_device_by_fwnode(struct fwnode_handle *fwnode) > +{ > + struct i2c_client *client; > + struct device *dev; > + > + if (!fwnode) > + return NULL; > + > + dev = bus_find_device_by_fwnode(&i2c_bus_type, fwnode); > + if (!dev) > + return NULL; > + > + client = i2c_verify_client(dev); > + if (!client) > + put_device(dev); > + > + return client; > +} > +EXPORT_SYMBOL(i2c_find_device_by_fwnode); > + Drop this empty line. > > static const struct i2c_device_id dummy_id[] = { > { "dummy", 0 }, > @@ -1761,6 +1782,57 @@ int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter) > } > EXPORT_SYMBOL_GPL(devm_i2c_add_adapter); > > +static int i2c_dev_or_parent_fwnode_match(struct device *dev, const void *data) > +{ > + if (dev_fwnode(dev) == data) > + return 1; > + > + if (dev->parent && dev_fwnode(dev->parent) == data) > + return 1; > + > + return 0; > +} > + > +/* must call put_device() when done with returned i2c_adapter device */ > +struct i2c_adapter *i2c_find_adapter_by_fwnode(struct fwnode_handle *fwnode) > +{ > + struct i2c_adapter *adapter; > + struct device *dev; > + > + if (!fwnode) > + return NULL; > + > + dev = bus_find_device(&i2c_bus_type, NULL, fwnode, > + i2c_dev_or_parent_fwnode_match); > + if (!dev) > + return NULL; > + > + adapter = i2c_verify_adapter(dev); > + if (!adapter) > + put_device(dev); > + > + return adapter; > +} > +EXPORT_SYMBOL(i2c_find_adapter_by_fwnode); > + > +/* must call i2c_put_adapter() when done with returned i2c_adapter device */ > +struct i2c_adapter *i2c_get_adapter_by_fwnode(struct fwnode_handle *fwnode) > +{ > + struct i2c_adapter *adapter; > + > + adapter = i2c_find_adapter_by_fwnode(fwnode); > + if (!adapter) > + return NULL; > + > + if (!try_module_get(adapter->owner)) { > + put_device(&adapter->dev); > + adapter = NULL; > + } > + > + return adapter; > +} > +EXPORT_SYMBOL(i2c_get_adapter_by_fwnode); > + > static void i2c_parse_timing(struct device *dev, char *prop_name, u32 *cur_val_p, > u32 def_val, bool use_def) > { > diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c > index 3ed74aa4b44b..c3e565e4bddf 100644 > --- a/drivers/i2c/i2c-core-of.c > +++ b/drivers/i2c/i2c-core-of.c > @@ -113,69 +113,24 @@ void of_i2c_register_devices(struct i2c_adapter *adap) > of_node_put(bus); > } > > -static int of_dev_or_parent_node_match(struct device *dev, const void *data) > -{ > - if (dev->of_node == data) > - return 1; > - > - if (dev->parent) > - return dev->parent->of_node == data; > - > - return 0; > -} > - > /* must call put_device() when done with returned i2c_client device */ > struct i2c_client *of_find_i2c_device_by_node(struct device_node *node) > { > - struct device *dev; > - struct i2c_client *client; > - > - dev = bus_find_device_by_of_node(&i2c_bus_type, node); > - if (!dev) > - return NULL; > - > - client = i2c_verify_client(dev); > - if (!client) > - put_device(dev); > - > - return client; > + return i2c_find_device_by_fwnode(of_fwnode_handle(node)); > } > EXPORT_SYMBOL(of_find_i2c_device_by_node); > > /* must call put_device() when done with returned i2c_adapter device */ > struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node) > { > - struct device *dev; > - struct i2c_adapter *adapter; > - > - dev = bus_find_device(&i2c_bus_type, NULL, node, > - of_dev_or_parent_node_match); > - if (!dev) > - return NULL; > - > - adapter = i2c_verify_adapter(dev); > - if (!adapter) > - put_device(dev); > - > - return adapter; > + return i2c_find_adapter_by_fwnode(of_fwnode_handle(node)); > } > EXPORT_SYMBOL(of_find_i2c_adapter_by_node); > > /* must call i2c_put_adapter() when done with returned i2c_adapter device */ > struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node) > { > - struct i2c_adapter *adapter; > - > - adapter = of_find_i2c_adapter_by_node(node); > - if (!adapter) > - return NULL; > - > - if (!try_module_get(adapter->owner)) { > - put_device(&adapter->dev); > - adapter = NULL; > - } > - > - return adapter; > + return i2c_get_adapter_by_fwnode(of_fwnode_handle(node)); > } > EXPORT_SYMBOL(of_get_i2c_adapter_by_node); > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index d84e0e99f084..bcee9faaf2e6 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -965,6 +965,15 @@ int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr); > > #endif /* I2C */ > > +/* must call put_device() when done with returned i2c_client device */ > +struct i2c_client *i2c_find_device_by_fwnode(struct fwnode_handle *fwnode); With the kernel-docs in place you probably can drop these comments. > + > +/* must call put_device() when done with returned i2c_adapter device */ > +struct i2c_adapter *i2c_find_adapter_by_fwnode(struct fwnode_handle *fwnode); > + > +/* must call i2c_put_adapter() when done with returned i2c_adapter device */ > +struct i2c_adapter *i2c_get_adapter_by_fwnode(struct fwnode_handle *fwnode); > + > #if IS_ENABLED(CONFIG_OF) > /* must call put_device() when done with returned i2c_client device */ > struct i2c_client *of_find_i2c_device_by_node(struct device_node *node); > -- > 2.30.2
Hi Mika, On Thu, Dec 08, 2022 at 12:04:02PM +0200, Mika Westerberg wrote: > Hi, > > On Wed, Dec 07, 2022 at 11:22:24AM +0000, Russell King (Oracle) wrote: > > +struct i2c_client *i2c_find_device_by_fwnode(struct fwnode_handle *fwnode) > > +{ > > + struct i2c_client *client; > > + struct device *dev; > > + > > + if (!fwnode) > > + return NULL; > > + > > + dev = bus_find_device_by_fwnode(&i2c_bus_type, fwnode); > > + if (!dev) > > + return NULL; > > + > > + client = i2c_verify_client(dev); > > + if (!client) > > + put_device(dev); > > + > > + return client; > > +} > > +EXPORT_SYMBOL(i2c_find_device_by_fwnode); > > + > > Drop this empty line. The additional empty line was there before, and I guess is something the I2C maintainer wants to logically separate the i2c device stuff from the rest of the file. > > +/* must call put_device() when done with returned i2c_client device */ > > +struct i2c_client *i2c_find_device_by_fwnode(struct fwnode_handle *fwnode); > > With the kernel-docs in place you probably can drop these comments. It's what is there against the other prototypes - and is very easy to get wrong, as I've recently noticed in the sfp.c code as a result of creating this series. I find the whole _find_ vs _get_ thing a tad confusing, and there probably should be just one interface with one way of putting afterwards to avoid subtle long-standing bugs like this. Thanks.