| Message ID | 20221124110718.3925934-3-sbinding@opensource.cirrus.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use ACPI_COMPANION macro to obtain acpi_device in cs35l41_hda | expand |
On Thu, Nov 24, 2022 at 1:07 PM Stefan Binding <sbinding@opensource.cirrus.com> 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 ACPI companion device > we obtain the reset gpio. GPIO ... The idea seems to be an improvement to me. Thanks. But I have side question, are you going to address the https://bugzilla.kernel.org/show_bug.cgi?id=215993 P.S. It would be nice if you have an account there, so I can reassign that to you.
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; > }
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; }
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. Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> --- sound/pci/hda/cs35l41_hda.c | 50 ++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 29 deletions(-)