Hi Stefan,

On 11/24/22 12:07, Stefan Binding wrote:

This allows the i2c driver to obtain the ACPI_COMPANION.

Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com

drivers/platform/x86/serial-multi-instantiate.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/serial-multi-instantiate.c b/drivers/platform/x86/serial-multi-instantiate.c index 5362f1a7b77c..15ef2f3c442e 100644 --- a/drivers/platform/x86/serial-multi-instantiate.c +++ b/drivers/platform/x86/serial-multi-instantiate.c @@ -194,6 +194,7 @@ static int smi_i2c_probe(struct platform_device *pdev, struct smi *smi, strscpy(board_info.type, inst_array[i].type, I2C_NAME_SIZE); snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev), inst_array[i].type, i); board_info.dev_name = name;

• board_info.fwnode = acpi_fwnode_handle(adev);
  
  

  ret = smi_get_irq(pdev, adev, &inst_array[i]); if (ret < 0)

I'm afraid that making this change is not as straight forward as it looks.

I know that I have tried to do this in the past and it failed.

IIRC there were 3 problems:

1. I was expecting this to also allow the driver for the instantiated i2c-client to be able to bind using an acpi_match_table but that unfortunately does not work. acpi_match_table matches only work for the first physical_node linked under /sys/bus/acpi/devices/xxxx:xx/physical_node and that is the platform device to which serial-multi-instantiate.c binds. The i2c_client becomes the second physical node. Note this is not really an issue, just something to be aware of.

2. This causes the i2c-core to use the first IRQ resource in the ACPI fwnode as client->irq for any clients for which we do not set an IRQ when instantiating. Which may very well be wrong. Sometimes that IRQ is only valid for the first i2c-client which we instantiate; and not for the others! And sometimes it is a problem because it may point to an irqchip for which we never wrote a driver leading to all probes of the i2c-client failing with -EPROBE_DEFER, see:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...

Note that patch has been reverted since that specific -EPROBE_DEFER issue has been solved by making the ACPI core instantiate a platform_device instead of an i2c_client (in this case we did not need the actual i2c_client at all).

The current i2c-core code has a (!client-irq) test guarding its code of trying to use the first ACPI fwnode IRQ resource.

So we could disable this by setting client->irq = -ENOENT in serial-multi-instantiate.c when (inst->flags & IRQ_RESOURCE_TYPE) == IRQ_RESOURCE_NONE). But that will introduce a new problem. Many i2c-drivers check if there is an IRQ for them to use by doing: "if (client->irq) request_irq(client->irq, ...)" but then with error checking/so setting client->irq to -ENOENT will cause the request_irq to fail, leading the probe to fail.

So before you can write a patch setting client->irq = -ENOENT when (inst->flags & IRQ_RESOURCE_TYPE) == IRQ_RESOURCE_NONE), you would first need to patch all i2c-drivers for clients instantiated through serial-multi-instantiate.c changing:

if (client->irq) { ... }

to:

if (client->irq > 0) { ... }

Note this is not as bad as it sounds, since there are only a few drivers for clients instantiated by serial-multi-instantiate.c .

3. Some drivers may check for an ACPI companion device and then change their behavior. So all drivers for clients instantiated through serial-multi-instantiate.c will need to be audited for this (and a summary of this audit needs to be added to the commit msg).

Regards,

Hans

Hi,

On 11/24/22 12:07, Stefan Binding wrote:

Currently the driver finds the acpi_device used to read certain properties using the HID, however, this is not necessary, as the acpi_device can be obtained from the device itself.

With the ACPI_COMPANION correctly set, we can also simplify how we obtain the reset gpio.

Typically when you write "also do ..." in a commit message that is a hint to yourself that it might be better to split the commit into 2 commits which each do only 1 thing, for easier review. But e.g. also to easier see what is going on if a bisect points out the commit as being the first bad one.

So once the issues with patch 1/2 are resolved, please consider splitting this patch into 2 smaller patches.

Regards,

Hans

Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com

sound/pci/hda/cs35l41_hda.c | 50 ++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 29 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index e5f0549bf06d..50cbbcce4946 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -1214,16 +1214,15 @@ static int cs35l41_get_speaker_id(struct device *dev, int amp_index,

• And devm functions expect that the device requesting the resource has the correct
• fwnode.

*/ -static int cs35l41_no_acpi_dsd(struct cs35l41_hda *cs35l41, struct device *physdev, int id,

• 	       const char *hid)
  

+static int cs35l41_no_acpi_dsd(struct cs35l41_hda *cs35l41, int id, const char *hid) { struct cs35l41_hw_cfg *hw_cfg = &cs35l41->hw_cfg;

/* check I2C address to assign the index */ cs35l41->index = id == 0x40 ? 0 : 1; cs35l41->channel_index = 0;

• cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 0, GPIOD_OUT_HIGH);
• cs35l41->speaker_id = cs35l41_get_speaker_id(physdev, 0, 0, 2);

• cs35l41->reset_gpio = gpiod_get_index(cs35l41->dev, NULL, 0, GPIOD_OUT_HIGH);
• cs35l41->speaker_id = cs35l41_get_speaker_id(cs35l41->dev, 0, 0, 2); hw_cfg->spk_pos = cs35l41->index; hw_cfg->gpio2.func = CS35L41_INTERRUPT; hw_cfg->gpio2.valid = true;

@@ -1255,39 +1254,36 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i struct cs35l41_hw_cfg *hw_cfg = &cs35l41->hw_cfg; u32 values[HDA_MAX_COMPONENTS]; struct acpi_device *adev;

• struct device *physdev;

• const char *sub; char *property; size_t nval; int i, ret;

• adev = acpi_dev_get_first_match_dev(hid, NULL, -1);

• adev = ACPI_COMPANION(cs35l41->dev); if (!adev) {

• dev_err(cs35l41->dev, "Failed to find an ACPI device for %s\n", hid);
  

• dev_err(cs35l41->dev, "Failed to find an ACPI device for %s\n",
  

• 	dev_name(cs35l41->dev));
  

  return -ENODEV; }

• physdev = get_device(acpi_get_first_physical_node(adev));
• acpi_dev_put(adev);
• sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev));

• sub = acpi_get_subsystem_id(ACPI_HANDLE(cs35l41->dev)); if (IS_ERR(sub)) sub = NULL; cs35l41->acpi_subsystem_id = sub;

  property = "cirrus,dev-index";

• ret = device_property_count_u32(physdev, property);
• if (ret <= 0) {

• ret = cs35l41_no_acpi_dsd(cs35l41, physdev, id, hid);
  

• goto err_put_physdev;
  

• }

• ret = device_property_count_u32(cs35l41->dev, property);
• if (ret <= 0)

• return cs35l41_no_acpi_dsd(cs35l41, id, hid);
  

• if (ret > ARRAY_SIZE(values)) { ret = -EINVAL; goto err; } nval = ret;

• ret = device_property_read_u32_array(physdev, property, values, nval);

• ret = device_property_read_u32_array(cs35l41->dev, property, values, nval); if (ret) goto err;

@@ -1307,11 +1303,10 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i /* To use the same release code for all laptop variants we can't use devm_ version of * gpiod_get here, as CLSA010* don't have a fully functional bios with an _DSD node */

• cs35l41->reset_gpio = fwnode_gpiod_get_index(acpi_fwnode_handle(adev), "reset", cs35l41->index,

• 				     GPIOD_OUT_LOW, "cs35l41-reset");
  

• cs35l41->reset_gpio = gpiod_get_index(cs35l41->dev, "reset", cs35l41->index, GPIOD_OUT_LOW);

  property = "cirrus,speaker-position";

• ret = device_property_read_u32_array(physdev, property, values, nval);

• ret = device_property_read_u32_array(cs35l41->dev, property, values, nval); if (ret) goto err; hw_cfg->spk_pos = values[cs35l41->index];

@@ -1322,41 +1317,41 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i cs35l41->channel_index++;

property = "cirrus,gpio1-func";

• ret = device_property_read_u32_array(physdev, property, values, nval);

• ret = device_property_read_u32_array(cs35l41->dev, property, values, nval); if (ret) goto err; hw_cfg->gpio1.func = values[cs35l41->index]; hw_cfg->gpio1.valid = true;

  property = "cirrus,gpio2-func";

• ret = device_property_read_u32_array(physdev, property, values, nval);

• ret = device_property_read_u32_array(cs35l41->dev, property, values, nval); if (ret) goto err; hw_cfg->gpio2.func = values[cs35l41->index]; hw_cfg->gpio2.valid = true;

  property = "cirrus,boost-peak-milliamp";

• ret = device_property_read_u32_array(physdev, property, values, nval);

• ret = device_property_read_u32_array(cs35l41->dev, property, values, nval); if (ret == 0) hw_cfg->bst_ipk = values[cs35l41->index]; else hw_cfg->bst_ipk = -1;

  property = "cirrus,boost-ind-nanohenry";

• ret = device_property_read_u32_array(physdev, property, values, nval);

• ret = device_property_read_u32_array(cs35l41->dev, property, values, nval); if (ret == 0) hw_cfg->bst_ind = values[cs35l41->index]; else hw_cfg->bst_ind = -1;

  property = "cirrus,boost-cap-microfarad";

• ret = device_property_read_u32_array(physdev, property, values, nval);

• ret = device_property_read_u32_array(cs35l41->dev, property, values, nval); if (ret == 0) hw_cfg->bst_cap = values[cs35l41->index]; else hw_cfg->bst_cap = -1;

• cs35l41->speaker_id = cs35l41_get_speaker_id(physdev, cs35l41->index, nval, -1);

• cs35l41->speaker_id = cs35l41_get_speaker_id(cs35l41->dev, cs35l41->index, nval, -1);

  if (hw_cfg->bst_ind > 0 || hw_cfg->bst_cap > 0 || hw_cfg->bst_ipk > 0) hw_cfg->bst_type = CS35L41_INT_BOOST;

@@ -1364,14 +1359,11 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i hw_cfg->bst_type = CS35L41_EXT_BOOST;

hw_cfg->valid = true;

• put_device(physdev);

  return 0;

err: dev_err(cs35l41->dev, "Failed property %s: %d\n", property, ret); -err_put_physdev:

• put_device(physdev);

  return ret;

}