| Message ID | b57c66fc1b7e790bd8c3e525faf65ce46cd91f23.1428546834.git.sathyanarayanan.kuppuswamy@linux.intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Apr 09, 2015 at 12:15:42PM -0700, Kuppuswamy Sathyanarayanan wrote: > Added support to enumerate i2c mux clients via ACPI. > > Currently we don't have any fixed way for enumerating > mux clients via acpi. This patch adds support to scan > for all client i2c devices under the mux device in DSDT > table and enumerate them in order. > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> From ACPI standpoint I don't see problems with this code. Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Mika. On 04/10/2015 01:42 AM, Mika Westerberg wrote: > On Thu, Apr 09, 2015 at 12:15:42PM -0700, Kuppuswamy Sathyanarayanan wrote: >> Added support to enumerate i2c mux clients via ACPI. >> >> Currently we don't have any fixed way for enumerating >> mux clients via acpi. This patch adds support to scan >> for all client i2c devices under the mux device in DSDT >> table and enumerate them in order. >> >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > >From ACPI standpoint I don't see problems with this code. > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> >
ping. On 04/10/2015 01:42 AM, Mika Westerberg wrote: > On Thu, Apr 09, 2015 at 12:15:42PM -0700, Kuppuswamy Sathyanarayanan wrote: >> Added support to enumerate i2c mux clients via ACPI. >> >> Currently we don't have any fixed way for enumerating >> mux clients via acpi. This patch adds support to scan >> for all client i2c devices under the mux device in DSDT >> table and enumerate them in order. >> >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > >From ACPI standpoint I don't see problems with this code. > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> >
gentle ping. On 04/10/2015 01:42 AM, Mika Westerberg wrote: > On Thu, Apr 09, 2015 at 12:15:42PM -0700, Kuppuswamy Sathyanarayanan wrote: >> Added support to enumerate i2c mux clients via ACPI. >> >> Currently we don't have any fixed way for enumerating >> mux clients via acpi. This patch adds support to scan >> for all client i2c devices under the mux device in DSDT >> table and enumerate them in order. >> >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > >From ACPI standpoint I don't see problems with this code. > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> >
> +#ifdef CONFIG_ACPI > +static void acpi_i2c_mux_register_devices(struct i2c_adapter *adap, > + struct device *mux_dev) > +{ > + acpi_handle handle; > + acpi_status status; > + > + handle = ACPI_HANDLE(mux_dev); > + if (!handle) > + return; > + > + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > + acpi_i2c_add_device, NULL, > + adap, NULL); > + > + if (ACPI_FAILURE(status)) > + dev_warn(mux_dev, "mux adapter slave enumeration fails\n"); > +} > +#else /* !CONFIG_ACPI */ > +static inline void acpi_i2c_mux_register_devices(struct i2c_adapter *adap, > + struct device *mux_dev) { } > +#endif /* CONFIG_ACPI */ IMO, this shares too much code with acpi_i2c_register_devices(). And it pulls in ACPI into mux.c which is not really needed. What about naming the above function acpi_i2c_register_devices_from_dev() and let acpi_i2c_register_devices() then call it as a helper function, all this in i2c-core.c? Thanks for your patience BTW...
Hi Sang Thanks for your comments. Please find my reply inline. On 06/02/2015 09:19 AM, Wolfram Sang wrote: >> +#ifdef CONFIG_ACPI >> +static void acpi_i2c_mux_register_devices(struct i2c_adapter *adap, >> + struct device *mux_dev) >> +{ >> + acpi_handle handle; >> + acpi_status status; >> + >> + handle = ACPI_HANDLE(mux_dev); >> + if (!handle) >> + return; >> + >> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, >> + acpi_i2c_add_device, NULL, >> + adap, NULL); >> + >> + if (ACPI_FAILURE(status)) >> + dev_warn(mux_dev, "mux adapter slave enumeration fails\n"); >> +} >> +#else /* !CONFIG_ACPI */ >> +static inline void acpi_i2c_mux_register_devices(struct i2c_adapter *adap, >> + struct device *mux_dev) { } >> +#endif /* CONFIG_ACPI */ > IMO, this shares too much code with acpi_i2c_register_devices(). And it > pulls in ACPI into mux.c which is not really needed. Even though mux is a just a virtual adapter without any ACPI ID, Its slaves are a still actual devices and needs to be enumerated by ACPI. So think its accptable to include ACPI in mux code. Don't you agree ? Also ACPI handle code in this function is slightly different from acpi_i2c_register_devices() code. > > What about naming the above function > acpi_i2c_register_devices_from_dev() and let acpi_i2c_register_devices() If you call this function from i2c-core register device function, then how will you maintain the hierarchy ? We need some way to indicate that these devices are under mux adapter right ? > then call it as a helper function, all this in i2c-core.c? > > Thanks for your patience BTW... >
On Tue, Jun 02, 2015 at 05:38:20PM -0700, sathyanarayanan kuppuswamy wrote: > Hi Sang > > Thanks for your comments. Please find my reply inline. > > On 06/02/2015 09:19 AM, Wolfram Sang wrote: > >>+#ifdef CONFIG_ACPI > >>+static void acpi_i2c_mux_register_devices(struct i2c_adapter *adap, > >>+ struct device *mux_dev) > >>+{ > >>+ acpi_handle handle; > >>+ acpi_status status; > >>+ > >>+ handle = ACPI_HANDLE(mux_dev); > >>+ if (!handle) > >>+ return; > >>+ > >>+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > >>+ acpi_i2c_add_device, NULL, > >>+ adap, NULL); > >>+ > >>+ if (ACPI_FAILURE(status)) > >>+ dev_warn(mux_dev, "mux adapter slave enumeration fails\n"); > >>+} > >>+#else /* !CONFIG_ACPI */ > >>+static inline void acpi_i2c_mux_register_devices(struct i2c_adapter *adap, > >>+ struct device *mux_dev) { } > >>+#endif /* CONFIG_ACPI */ > >IMO, this shares too much code with acpi_i2c_register_devices(). And it > >pulls in ACPI into mux.c which is not really needed. > Even though mux is a just a virtual adapter without any ACPI ID, Its slaves > are a still actual devices and needs to be enumerated by ACPI. So think its > accptable to include ACPI in mux code. Don't you agree ? The resulting function should be called from mux code. But the function itself should be in the core. > Also ACPI handle code in this function is slightly different from > acpi_i2c_register_devices() code. The only difference I see is that one takes a device as parameter, the other an adapter. So, you can read out the device from the adapter in acpi_i2c_register_devices() and then call acpi_i2c_register_devices_from_dev().
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index edf274c..94f76e3 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -118,7 +118,7 @@ static int acpi_i2c_add_resource(struct acpi_resource *ares, void *data) return 1; } -static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level, +acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level, void *data, void **return_value) { struct i2c_adapter *adapter = data; @@ -155,6 +155,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level, return AE_OK; } +EXPORT_SYMBOL_GPL(acpi_i2c_add_device); /** * acpi_i2c_register_devices - enumerate I2C slave devices behind adapter diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c index 593f7ca..32cf7a2 100644 --- a/drivers/i2c/i2c-mux.c +++ b/drivers/i2c/i2c-mux.c @@ -100,6 +100,29 @@ static unsigned int i2c_mux_parent_classes(struct i2c_adapter *parent) return class; } +#ifdef CONFIG_ACPI +static void acpi_i2c_mux_register_devices(struct i2c_adapter *adap, + struct device *mux_dev) +{ + acpi_handle handle; + acpi_status status; + + handle = ACPI_HANDLE(mux_dev); + if (!handle) + return; + + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, + acpi_i2c_add_device, NULL, + adap, NULL); + + if (ACPI_FAILURE(status)) + dev_warn(mux_dev, "mux adapter slave enumeration fails\n"); +} +#else /* !CONFIG_ACPI */ +static inline void acpi_i2c_mux_register_devices(struct i2c_adapter *adap, + struct device *mux_dev) { } +#endif /* CONFIG_ACPI */ + struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent, struct device *mux_dev, void *mux_priv, u32 force_nr, u32 chan_id, @@ -193,6 +216,9 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent, dev_info(&parent->dev, "Added multiplexed i2c bus %d\n", i2c_adapter_id(&priv->adap)); + /* scan for mux clients */ + acpi_i2c_mux_register_devices(&priv->adap, mux_dev); + return &priv->adap; } EXPORT_SYMBOL_GPL(i2c_add_mux_adapter); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index f17da50..67b1412 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -33,6 +33,7 @@ #include <linux/of.h> /* for struct device_node */ #include <linux/swab.h> /* for swab16 */ #include <uapi/linux/i2c.h> +#include <linux/acpi.h> extern struct bus_type i2c_bus_type; extern struct device_type i2c_adapter_type; @@ -608,4 +609,8 @@ static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node } #endif /* CONFIG_OF */ +#ifdef CONFIG_ACPI +extern acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level, + void *data, void **return_value); +#endif /* CONFIG_ACPI */ #endif /* _LINUX_I2C_H */
Added support to enumerate i2c mux clients via ACPI. Currently we don't have any fixed way for enumerating mux clients via acpi. This patch adds support to scan for all client i2c devices under the mux device in DSDT table and enumerate them in order. Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> --- drivers/i2c/i2c-core.c | 3 ++- drivers/i2c/i2c-mux.c | 26 ++++++++++++++++++++++++++ include/linux/i2c.h | 5 +++++ 3 files changed, 33 insertions(+), 1 deletion(-)