| Message ID | 1445293740-28537-2-git-send-email-dustin@cumulusnetworks.com |
|---|---|
| State | Superseded |
| Headers | show |
+Cc: Ismo Puustinen On Mon, 2015-10-19 at 15:29 -0700, Dustin Byford wrote: > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID > or > device property compatible string match) enumerating I2C client > devices > connected through a I2C mux device requires a little extra work. > > This change implements a method for describing an I2C device > hierarchy that > includes mux devices by using an ACPI Device() for each mux channel > along > with an _ADR to set the channel number for the device. See > Documentation/acpi/i2c-muxes.txt for a simple example. Ismo, can you test this patch on top of what you have to see if it goes smoothly (no break of Galileo Gen2 support) ? > > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com> > --- > Documentation/acpi/i2c-muxes.txt | 58 > ++++++++++++++++++++++++++++++++++++++++ > drivers/i2c/i2c-core.c | 15 +++++++++-- > drivers/i2c/i2c-mux.c | 8 ++++++ > include/linux/acpi.h | 6 +++++ > 4 files changed, 85 insertions(+), 2 deletions(-) > create mode 100644 Documentation/acpi/i2c-muxes.txt > > diff --git a/Documentation/acpi/i2c-muxes.txt > b/Documentation/acpi/i2c-muxes.txt > new file mode 100644 > index 0000000..9fcc4f0 > --- /dev/null > +++ b/Documentation/acpi/i2c-muxes.txt > @@ -0,0 +1,58 @@ > +ACPI I2C Muxes > +-------------- > + > +Describing an I2C device hierarchy that includes I2C muxes requires > an ACPI > +Device () scope per mux channel. > + > +Consider this topology: > + > ++------+ +------+ > +| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50) > +| | | 0x70 |--CH01--> i2c client B (0x50) > ++------+ +------+ > + > +which corresponds to the following ASL: > + > +Device (SMB1) > +{ > + Name (_HID, ...) > + Device (MUX0) > + { > + Name (_HID, ...) > + Name (_CRS, ResourceTemplate () { > + I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED, > + AddressingMode7Bit, "^SMB1", 0x00, > + ResourceConsumer,,) > + } > + > + Device (CH00) > + { > + Name (_ADR, 0) > + > + Device (CLIA) > + { > + Name (_HID, ...) > + Name (_CRS, ResourceTemplate () { > + I2cSerialBus (0x50, ControllerInitiated, > I2C_SPEED, > + AddressingMode7Bit, "^CH00", 0x00, > + ResourceConsumer,,) > + } > + } > + } > + > + Device (CH01) > + { > + Name (_ADR, 1) > + > + Device (CLIB) > + { > + Name (_HID, ...) > + Name (_CRS, ResourceTemplate () { > + I2cSerialBus (0x50, ControllerInitiated, > I2C_SPEED, > + AddressingMode7Bit, "^CH01", 0x00, > + ResourceConsumer,,) > + } > + } > + } > + } > +} > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index 579b99d..af0811c 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -156,7 +156,7 @@ static acpi_status > acpi_i2c_add_device(acpi_handle handle, u32 level, > info.fwnode = acpi_fwnode_handle(adev); > > memset(&lookup, 0, sizeof(lookup)); > - lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent); > + lookup.adapter_handle = ACPI_HANDLE(&adapter->dev); > lookup.device_handle = handle; > lookup.info = &info; > > @@ -212,7 +212,7 @@ static void acpi_i2c_register_devices(struct > i2c_adapter *adap) > { > acpi_status status; > > - if (!adap->dev.parent || !has_acpi_companion(adap- > >dev.parent)) > + if (!has_acpi_companion(&adap->dev)) > return; > > status = acpi_walk_namespace(ACPI_TYPE_DEVICE, > ACPI_ROOT_OBJECT, > @@ -1667,6 +1667,17 @@ int i2c_add_adapter(struct i2c_adapter > *adapter) > struct device *dev = &adapter->dev; > int id; > > + /* > + * By default, associate I2C adapters with their parent > device's ACPI > + * node. > + */ > + if (!has_acpi_companion(dev)) { > + struct acpi_device *adev = ACPI_COMPANION(dev- > >parent); > + > + if (adev) > + ACPI_COMPANION_SET(dev, adev); > + } > + > if (dev->of_node) { > id = of_alias_get_id(dev->of_node, "i2c"); > if (id >= 0) { > diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c > index 2ba7c0f..00fc5b1 100644 > --- a/drivers/i2c/i2c-mux.c > +++ b/drivers/i2c/i2c-mux.c > @@ -25,6 +25,7 @@ > #include <linux/i2c.h> > #include <linux/i2c-mux.h> > #include <linux/of.h> > +#include <linux/acpi.h> > > /* multiplexer per channel data */ > struct i2c_mux_priv { > @@ -173,6 +174,13 @@ struct i2c_adapter *i2c_add_mux_adapter(struct > i2c_adapter *parent, > } > } > > + /* > + * Associate the mux channel with an ACPI node. > + */ > + if (has_acpi_companion(mux_dev)) > + acpi_preset_companion(&priv->adap.dev, > ACPI_COMPANION(mux_dev), > + chan_id); > + > if (force_nr) { > priv->adap.nr = force_nr; > ret = i2c_add_numbered_adapter(&priv->adap); > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 51a96a8..66564f8 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -505,6 +505,12 @@ static inline bool has_acpi_companion(struct > device *dev) > return false; > } > > +static inline void acpi_preset_companion(struct device *dev, > + struct acpi_device *parent, > u64 addr) > +{ > + return; > +} > + > static inline const char *acpi_dev_name(struct acpi_device *adev) > { > return NULL; For me seems this one can go separately. Rafael, what do you think?
On Mon, Oct 19, 2015 at 03:29:00PM -0700, Dustin Byford wrote: > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or > device property compatible string match) enumerating I2C client devices > connected through a I2C mux device requires a little extra work. > > This change implements a method for describing an I2C device hierarchy that > includes mux devices by using an ACPI Device() for each mux channel along > with an _ADR to set the channel number for the device. See > Documentation/acpi/i2c-muxes.txt for a simple example. > > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com> In general this looks good to me. > --- > Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++ > drivers/i2c/i2c-core.c | 15 +++++++++-- > drivers/i2c/i2c-mux.c | 8 ++++++ > include/linux/acpi.h | 6 +++++ > 4 files changed, 85 insertions(+), 2 deletions(-) > create mode 100644 Documentation/acpi/i2c-muxes.txt > [...] > + /* > + * By default, associate I2C adapters with their parent device's ACPI > + * node. > + */ > + if (!has_acpi_companion(dev)) { > + struct acpi_device *adev = ACPI_COMPANION(dev->parent); > + > + if (adev) > + ACPI_COMPANION_SET(dev, adev); Instead of always doing this in the I2C core, maybe we can make it dependent on the host controller driver. For example the I2C designware driver already did this for both DT and ACPI: adap->dev.parent = &pdev->dev; adap->dev.of_node = pdev->dev.of_node; ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); Also I would like to ask what Rafael thinks about this since he authored b34bb1ee71158d5b ("ACPI / I2C: Use parent's ACPI_HANDLE() in acpi_i2c_register_devices()"). I don't see a problem multiple Linux devices sharing a single ACPI companion device like in this patch but I may be forgetting something ;-) -- 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
Hi Mika, On Tue Oct 20 15:51, Mika Westerberg wrote: > On Mon, Oct 19, 2015 at 03:29:00PM -0700, Dustin Byford wrote: > > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or > > device property compatible string match) enumerating I2C client devices > > connected through a I2C mux device requires a little extra work. > > > > This change implements a method for describing an I2C device hierarchy that > > includes mux devices by using an ACPI Device() for each mux channel along > > with an _ADR to set the channel number for the device. See > > Documentation/acpi/i2c-muxes.txt for a simple example. > > > > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com> > > In general this looks good to me. > > > --- > > Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++ > > drivers/i2c/i2c-core.c | 15 +++++++++-- > > drivers/i2c/i2c-mux.c | 8 ++++++ > > include/linux/acpi.h | 6 +++++ > > 4 files changed, 85 insertions(+), 2 deletions(-) > > create mode 100644 Documentation/acpi/i2c-muxes.txt > > > > [...] > > > + /* > > + * By default, associate I2C adapters with their parent device's ACPI > > + * node. > > + */ > > + if (!has_acpi_companion(dev)) { > > + struct acpi_device *adev = ACPI_COMPANION(dev->parent); > > + > > + if (adev) > > + ACPI_COMPANION_SET(dev, adev); > > Instead of always doing this in the I2C core, maybe we can make it > dependent on the host controller driver. For example the I2C designware > driver already did this for both DT and ACPI: I considered it, but I thought a default that fairly closely matches the old behavior was more convenient. On the other hand, leaving it up to the controllers makes it all very explicit and perhaps simpler to reason about. I could be convinced either way. But, if we move it to the controller drivers, which ones need the change? grep -i acpi drivers/i2c/busses/i2c* shows 18 drivers that might care. > adap->dev.parent = &pdev->dev; > adap->dev.of_node = pdev->dev.of_node; > ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); Interesting, this code isn't in my tree. I wonder why it was added, what code looks at the acpi companion on the i2c dev? Before my change it was supposed to be NULL, and it is NULL on every other controller. > Also I would like to ask what Rafael thinks about this since he authored > b34bb1ee71158d5b ("ACPI / I2C: Use parent's ACPI_HANDLE() in > acpi_i2c_register_devices()"). > > I don't see a problem multiple Linux devices sharing a single ACPI > companion device like in this patch but I may be forgetting something ;-) OK. Thanks. --Dustin -- 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
On Tuesday, October 20, 2015 03:51:11 PM Mika Westerberg wrote: > On Mon, Oct 19, 2015 at 03:29:00PM -0700, Dustin Byford wrote: > > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or > > device property compatible string match) enumerating I2C client devices > > connected through a I2C mux device requires a little extra work. > > > > This change implements a method for describing an I2C device hierarchy that > > includes mux devices by using an ACPI Device() for each mux channel along > > with an _ADR to set the channel number for the device. See > > Documentation/acpi/i2c-muxes.txt for a simple example. > > > > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com> > > In general this looks good to me. > > > --- > > Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++ > > drivers/i2c/i2c-core.c | 15 +++++++++-- > > drivers/i2c/i2c-mux.c | 8 ++++++ > > include/linux/acpi.h | 6 +++++ > > 4 files changed, 85 insertions(+), 2 deletions(-) > > create mode 100644 Documentation/acpi/i2c-muxes.txt > > > > [...] > > > + /* > > + * By default, associate I2C adapters with their parent device's ACPI > > + * node. > > + */ > > + if (!has_acpi_companion(dev)) { > > + struct acpi_device *adev = ACPI_COMPANION(dev->parent); > > + > > + if (adev) > > + ACPI_COMPANION_SET(dev, adev); > > Instead of always doing this in the I2C core, maybe we can make it > dependent on the host controller driver. For example the I2C designware > driver already did this for both DT and ACPI: > > adap->dev.parent = &pdev->dev; > adap->dev.of_node = pdev->dev.of_node; > ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); > > Also I would like to ask what Rafael thinks about this since he authored > b34bb1ee71158d5b ("ACPI / I2C: Use parent's ACPI_HANDLE() in > acpi_i2c_register_devices()"). > > I don't see a problem multiple Linux devices sharing a single ACPI > companion device like in this patch but I may be forgetting something ;-) Well, we already have that in the MFD case, but in principle it may be problematic for things like power management (say you want to put a child device into D3, so you use _PS3 on its ACPI companion and then the parent is powere down instead). At least, devices in that setup should not be attached to the ACPI PM domain. Thanks, Rafael -- 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
On Tuesday, October 20, 2015 10:49:59 AM Dustin Byford wrote: > Hi Mika, > > On Tue Oct 20 15:51, Mika Westerberg wrote: > > On Mon, Oct 19, 2015 at 03:29:00PM -0700, Dustin Byford wrote: > > > Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or > > > device property compatible string match) enumerating I2C client devices > > > connected through a I2C mux device requires a little extra work. > > > > > > This change implements a method for describing an I2C device hierarchy that > > > includes mux devices by using an ACPI Device() for each mux channel along > > > with an _ADR to set the channel number for the device. See > > > Documentation/acpi/i2c-muxes.txt for a simple example. > > > > > > Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com> > > > > In general this looks good to me. > > > > > --- > > > Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++ > > > drivers/i2c/i2c-core.c | 15 +++++++++-- > > > drivers/i2c/i2c-mux.c | 8 ++++++ > > > include/linux/acpi.h | 6 +++++ > > > 4 files changed, 85 insertions(+), 2 deletions(-) > > > create mode 100644 Documentation/acpi/i2c-muxes.txt > > > > > > > [...] > > > > > + /* > > > + * By default, associate I2C adapters with their parent device's ACPI > > > + * node. > > > + */ > > > + if (!has_acpi_companion(dev)) { > > > + struct acpi_device *adev = ACPI_COMPANION(dev->parent); > > > + > > > + if (adev) > > > + ACPI_COMPANION_SET(dev, adev); > > > > Instead of always doing this in the I2C core, maybe we can make it > > dependent on the host controller driver. For example the I2C designware > > driver already did this for both DT and ACPI: > > I considered it, but I thought a default that fairly closely matches the > old behavior was more convenient. > > On the other hand, leaving it up to the controllers makes it all very > explicit and perhaps simpler to reason about. > > > I could be convinced either way. But, if we move it to the controller > drivers, which ones need the change? > > grep -i acpi drivers/i2c/busses/i2c* > > shows 18 drivers that might care. > > > adap->dev.parent = &pdev->dev; > > adap->dev.of_node = pdev->dev.of_node; > > ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); > > Interesting, this code isn't in my tree. I wonder why it was added, > what code looks at the acpi companion on the i2c dev? Before my change > it was supposed to be NULL, and it is NULL on every other controller. As I said, IMO it should be NULL for i2c devices (power management is the main reason here). Thanks, Rafael -- 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
On Wed, Oct 21, 2015 at 01:12:01AM +0200, Rafael J. Wysocki wrote: > Well, we already have that in the MFD case, but in principle it may be > problematic for things like power management (say you want to put a > child device into D3, so you use _PS3 on its ACPI companion and then > the parent is powere down instead). That case I understand and we should not allow that. However, here we have an I2C adapter which is purely Linux abstraction that does not have any representation either in DT nor ACPI. And we don't do any power management for that either. If I understand correctly, DT shares the same of_node for both the host controller device and the adapter which then makes it possible to enumerate devices behind by just looking adap->dev.of_node. In case of ACPI we need to know that it's the parent device that we should look for child devices which may not be true always (like what this patch is trying to resolve). > At least, devices in that setup should not be attached to the ACPI PM > domain. Agreed. Currently acpi_dev_pm_attach() only attaches the first physical device to the ACPI power domain so this should be taken care already. -- 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
On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote: > I considered it, but I thought a default that fairly closely matches the > old behavior was more convenient. > > On the other hand, leaving it up to the controllers makes it all very > explicit and perhaps simpler to reason about. > > > I could be convinced either way. But, if we move it to the controller > drivers, which ones need the change? > > grep -i acpi drivers/i2c/busses/i2c* > > shows 18 drivers that might care. I'm quite confident the designware I2C is enough for now. Intel uses it for all SoCs with LPSS and I think AMD has the same block for their I2C solution. > > adap->dev.parent = &pdev->dev; > > adap->dev.of_node = pdev->dev.of_node; > > ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); > > Interesting, this code isn't in my tree. I wonder why it was added, > what code looks at the acpi companion on the i2c dev? Before my change > it was supposed to be NULL, and it is NULL on every other controller. It is not in any tree. I meant that before b34bb1ee71158d5b it looked something like that :-) -- 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
On Wed Oct 21 11:12, Mika Westerberg wrote: > On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote: > > I considered it, but I thought a default that fairly closely matches the > > old behavior was more convenient. > > > > On the other hand, leaving it up to the controllers makes it all very > > explicit and perhaps simpler to reason about. > > > > > > I could be convinced either way. But, if we move it to the controller > > drivers, which ones need the change? > > > > grep -i acpi drivers/i2c/busses/i2c* > > > > shows 18 drivers that might care. > > I'm quite confident the designware I2C is enough for now. Intel uses it > for all SoCs with LPSS and I think AMD has the same block for their I2C > solution. I certainly care about i801, ismt, and isch. Doesn't this affect any i2c controller with clients that may be enumerated through ACPI? --Dustin -- 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
On Wed, Oct 21, 2015 at 01:21:16AM -0700, Dustin Byford wrote: > On Wed Oct 21 11:12, Mika Westerberg wrote: > > On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote: > > > I considered it, but I thought a default that fairly closely matches the > > > old behavior was more convenient. > > > > > > On the other hand, leaving it up to the controllers makes it all very > > > explicit and perhaps simpler to reason about. > > > > > > > > > I could be convinced either way. But, if we move it to the controller > > > drivers, which ones need the change? > > > > > > grep -i acpi drivers/i2c/busses/i2c* > > > > > > shows 18 drivers that might care. > > > > I'm quite confident the designware I2C is enough for now. Intel uses it > > for all SoCs with LPSS and I think AMD has the same block for their I2C > > solution. > > I certainly care about i801, ismt, and isch. Doesn't this affect any > i2c controller with clients that may be enumerated through ACPI? Yes, but so far I haven't seen any other devices being used for this than the I2C designware. Which hardware you are testing this patch on? -- 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
On Wed Oct 21 11:34, Mika Westerberg wrote: > On Wed, Oct 21, 2015 at 01:21:16AM -0700, Dustin Byford wrote: > > On Wed Oct 21 11:12, Mika Westerberg wrote: > > > On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote: > > > > I considered it, but I thought a default that fairly closely matches the > > > > old behavior was more convenient. > > > > > > > > On the other hand, leaving it up to the controllers makes it all very > > > > explicit and perhaps simpler to reason about. > > > > > > > > > > > > I could be convinced either way. But, if we move it to the controller > > > > drivers, which ones need the change? > > > > > > > > grep -i acpi drivers/i2c/busses/i2c* > > > > > > > > shows 18 drivers that might care. > > > > > > I'm quite confident the designware I2C is enough for now. Intel uses it > > > for all SoCs with LPSS and I think AMD has the same block for their I2C > > > solution. > > > > I certainly care about i801, ismt, and isch. Doesn't this affect any > > i2c controller with clients that may be enumerated through ACPI? > > Yes, but so far I haven't seen any other devices being used for this > than the I2C designware. > > Which hardware you are testing this patch on? I'm working with a number of x86-based network switch platforms. Mostly rangeley at the moment, but I'm sure others are in the works. Have a look at: http://www.opencompute.org/wiki/Networking/SpecsAndDesigns for examples. My goal, hence the recent patches, is to help the network switch industry move a lot of platform description into ACPI. That means lots of complicated I2C trees; switches are full of I2C devices. --Dustin -- 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
On Wed, Oct 21, 2015 at 01:52:41AM -0700, Dustin Byford wrote: > On Wed Oct 21 11:34, Mika Westerberg wrote: > > On Wed, Oct 21, 2015 at 01:21:16AM -0700, Dustin Byford wrote: > > > On Wed Oct 21 11:12, Mika Westerberg wrote: > > > > On Tue, Oct 20, 2015 at 10:49:59AM -0700, Dustin Byford wrote: > > > > > I considered it, but I thought a default that fairly closely matches the > > > > > old behavior was more convenient. > > > > > > > > > > On the other hand, leaving it up to the controllers makes it all very > > > > > explicit and perhaps simpler to reason about. > > > > > > > > > > > > > > > I could be convinced either way. But, if we move it to the controller > > > > > drivers, which ones need the change? > > > > > > > > > > grep -i acpi drivers/i2c/busses/i2c* > > > > > > > > > > shows 18 drivers that might care. > > > > > > > > I'm quite confident the designware I2C is enough for now. Intel uses it > > > > for all SoCs with LPSS and I think AMD has the same block for their I2C > > > > solution. > > > > > > I certainly care about i801, ismt, and isch. Doesn't this affect any > > > i2c controller with clients that may be enumerated through ACPI? > > > > Yes, but so far I haven't seen any other devices being used for this > > than the I2C designware. > > > > Which hardware you are testing this patch on? > > I'm working with a number of x86-based network switch platforms. Mostly > rangeley at the moment, but I'm sure others are in the works. Have a > look at: > > http://www.opencompute.org/wiki/Networking/SpecsAndDesigns > > for examples. Cool :) > My goal, hence the recent patches, is to help the network switch > industry move a lot of platform description into ACPI. That means lots > of complicated I2C trees; switches are full of I2C devices. I see. I don't really have strong feelings whether it should be the I2C core or individual drivers setting the ACPI companion. However, it would be nice to match DT here and they assign their of_node per driver. -- 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
On Wed Oct 21 12:08, Mika Westerberg wrote: > I don't really have strong feelings whether it should be the I2C core or > individual drivers setting the ACPI companion. However, it would be nice > to match DT here and they assign their of_node per driver. OK with me, if we can convince Rafael this is a good idea, I'll send a new revision with drivers setting the companion. --Dustin -- 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
Hi, On Wed, Oct 21, 2015 at 11:25 AM, Dustin Byford <dustin@cumulusnetworks.com> wrote: > On Wed Oct 21 12:08, Mika Westerberg wrote: >> I don't really have strong feelings whether it should be the I2C core or >> individual drivers setting the ACPI companion. However, it would be nice >> to match DT here and they assign their of_node per driver. > > OK with me, if we can convince Rafael this is a good idea, I'll send a > new revision with drivers setting the companion. If you can guarantee that ACPI PM or anything like _DS or _SRS will never be invoked for the device objects that "inherit" the ACPI companion from their parent, it at least is not outright dangerous. That said I'm thinking that may need some more sophisticated approach here, so we really can guarantee certain things, but that's for the future. Thanks, Rafael -- 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
On Thu Oct 22 00:39, Rafael J. Wysocki wrote: > Hi, > > On Wed, Oct 21, 2015 at 11:25 AM, Dustin Byford > <dustin@cumulusnetworks.com> wrote: > > On Wed Oct 21 12:08, Mika Westerberg wrote: > >> I don't really have strong feelings whether it should be the I2C core or > >> individual drivers setting the ACPI companion. However, it would be nice > >> to match DT here and they assign their of_node per driver. > > > > OK with me, if we can convince Rafael this is a good idea, I'll send a > > new revision with drivers setting the companion. > > If you can guarantee that ACPI PM or anything like _DS or _SRS will > never be invoked for the device objects that "inherit" the ACPI > companion from their parent, it at least is not outright dangerous. I'm hoping an ack from Mika will suffice, but please let me know if there's something I can do to verify or help ensure this. --Dustin -- 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
diff --git a/Documentation/acpi/i2c-muxes.txt b/Documentation/acpi/i2c-muxes.txt new file mode 100644 index 0000000..9fcc4f0 --- /dev/null +++ b/Documentation/acpi/i2c-muxes.txt @@ -0,0 +1,58 @@ +ACPI I2C Muxes +-------------- + +Describing an I2C device hierarchy that includes I2C muxes requires an ACPI +Device () scope per mux channel. + +Consider this topology: + ++------+ +------+ +| SMB1 |-->| MUX0 |--CH00--> i2c client A (0x50) +| | | 0x70 |--CH01--> i2c client B (0x50) ++------+ +------+ + +which corresponds to the following ASL: + +Device (SMB1) +{ + Name (_HID, ...) + Device (MUX0) + { + Name (_HID, ...) + Name (_CRS, ResourceTemplate () { + I2cSerialBus (0x70, ControllerInitiated, I2C_SPEED, + AddressingMode7Bit, "^SMB1", 0x00, + ResourceConsumer,,) + } + + Device (CH00) + { + Name (_ADR, 0) + + Device (CLIA) + { + Name (_HID, ...) + Name (_CRS, ResourceTemplate () { + I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED, + AddressingMode7Bit, "^CH00", 0x00, + ResourceConsumer,,) + } + } + } + + Device (CH01) + { + Name (_ADR, 1) + + Device (CLIB) + { + Name (_HID, ...) + Name (_CRS, ResourceTemplate () { + I2cSerialBus (0x50, ControllerInitiated, I2C_SPEED, + AddressingMode7Bit, "^CH01", 0x00, + ResourceConsumer,,) + } + } + } + } +} diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 579b99d..af0811c 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -156,7 +156,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level, info.fwnode = acpi_fwnode_handle(adev); memset(&lookup, 0, sizeof(lookup)); - lookup.adapter_handle = ACPI_HANDLE(adapter->dev.parent); + lookup.adapter_handle = ACPI_HANDLE(&adapter->dev); lookup.device_handle = handle; lookup.info = &info; @@ -212,7 +212,7 @@ static void acpi_i2c_register_devices(struct i2c_adapter *adap) { acpi_status status; - if (!adap->dev.parent || !has_acpi_companion(adap->dev.parent)) + if (!has_acpi_companion(&adap->dev)) return; status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, @@ -1667,6 +1667,17 @@ int i2c_add_adapter(struct i2c_adapter *adapter) struct device *dev = &adapter->dev; int id; + /* + * By default, associate I2C adapters with their parent device's ACPI + * node. + */ + if (!has_acpi_companion(dev)) { + struct acpi_device *adev = ACPI_COMPANION(dev->parent); + + if (adev) + ACPI_COMPANION_SET(dev, adev); + } + if (dev->of_node) { id = of_alias_get_id(dev->of_node, "i2c"); if (id >= 0) { diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c index 2ba7c0f..00fc5b1 100644 --- a/drivers/i2c/i2c-mux.c +++ b/drivers/i2c/i2c-mux.c @@ -25,6 +25,7 @@ #include <linux/i2c.h> #include <linux/i2c-mux.h> #include <linux/of.h> +#include <linux/acpi.h> /* multiplexer per channel data */ struct i2c_mux_priv { @@ -173,6 +174,13 @@ struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent, } } + /* + * Associate the mux channel with an ACPI node. + */ + if (has_acpi_companion(mux_dev)) + acpi_preset_companion(&priv->adap.dev, ACPI_COMPANION(mux_dev), + chan_id); + if (force_nr) { priv->adap.nr = force_nr; ret = i2c_add_numbered_adapter(&priv->adap); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 51a96a8..66564f8 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -505,6 +505,12 @@ static inline bool has_acpi_companion(struct device *dev) return false; } +static inline void acpi_preset_companion(struct device *dev, + struct acpi_device *parent, u64 addr) +{ + return; +} + static inline const char *acpi_dev_name(struct acpi_device *adev) { return NULL;
Although I2C mux devices are easily enumerated using ACPI (_HID/_CID or device property compatible string match) enumerating I2C client devices connected through a I2C mux device requires a little extra work. This change implements a method for describing an I2C device hierarchy that includes mux devices by using an ACPI Device() for each mux channel along with an _ADR to set the channel number for the device. See Documentation/acpi/i2c-muxes.txt for a simple example. Signed-off-by: Dustin Byford <dustin@cumulusnetworks.com> --- Documentation/acpi/i2c-muxes.txt | 58 ++++++++++++++++++++++++++++++++++++++++ drivers/i2c/i2c-core.c | 15 +++++++++-- drivers/i2c/i2c-mux.c | 8 ++++++ include/linux/acpi.h | 6 +++++ 4 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 Documentation/acpi/i2c-muxes.txt