On Fri, Aug 25, 2023 at 12:01:35AM +0300, Marian Postevca wrote:
> +static int acp_asoc_suspend_pre(struct snd_soc_card *card)
> +{
> + int ret;
> +
> + ret = acp_ops_suspend_pre(card);
> + if (ret == 1)
> + return 0;
> + else
> + return ret;
> +}
> +
> +static int acp_asoc_resume_post(struct snd_soc_card *card)
> +{
> + int ret;
> +
> + ret = acp_ops_resume_post(card);
> + if (ret == 1)
> + return 0;
> + else
> + return ret;
> +}
This feels like it should've been part of the prior commit adding
support for more complex cards?
> + card->drvdata = (struct acp_card_drvdata *)pdev->id_entry->driver_data;
> + acp_card_drvdata = card->drvdata;
> + acp_card_drvdata->acpi_mach = (struct snd_soc_acpi_mach *)pdev->dev.platform_data;
Similarly these changes in probe() -
> + {
> + .name = "acp3x-es83xx",
> + .driver_data = (kernel_ulong_t)&es83xx_rn_data,
> + },
The main thing I'd expect to see in the generic code in a patch adding a
specific driver is table entries like this and the ones you had earlier.
> + if (drv_data->hs_codec_id == ES83XX) {
> + rc = acp_ops_configure_link(card, &links[i]);
> + if (rc != 0) {
> + dev_err(dev, "Failed to configure link for ES83XX: %d\n", rc);
> + return rc;
> + }
> + }
This function should ideally have been using switch statemnts but that's
not an issue your patch introduced.
> +#define ES83XX_ENABLE_DMIC BIT(4)
> +#define ES83XX_48_MHZ_MCLK BIT(5)
> +static void acp3x_es83xx_set_gpios_values(struct acp3x_es83xx_private *priv,
> + bool speaker, bool headphone)
> +{
> + gpiod_set_value_cansleep(priv->gpio_speakers, speaker);
> + gpiod_set_value_cansleep(priv->gpio_headphone, headphone);
> +}
I'm not clear what this function buys us other than just calling gpiolib
directly in the DAPM events?
Otherewise the actual machine driver looks good.
Attachment:
signature.asc
Description: PGP signature
