Re: [PATCH 1/2] ASoC: rt5677: Add ACPI support

[Mark Brown]

On Sun, Aug 14, 2016 at 12:18:22PM +0100, John Keeping wrote:

> The Chromebook Pixel 2015 uses this codec with the ACPI ID RT5677CE, so
> add an ACPI match table and support for reading properties from ACPI.

This would be a lot easier to review with a concrete description of what
"support for reading properties from ACPI" means and probably also split
out a bit so that different things were being added separately.

> +/* GPIO indexes defined by ACPI */
> +enum {
> +	RT5677_GPIO_PLUG_DET,
> +	RT5677_GPIO_MIC_PRESENT_L,
> +	RT5677_GPIO_HOTWORD_DET_L,
> +	RT5677_GPIO_DSP_INT,
> +	RT5677_GPIO_HP_AMP_SHDN_L,
> +};

If these are an ABI you should explicitly assign the values so that they
can't get remapped by future edits.  If they're not an ABI I don't
understand the comment.

> +	if (ACPI_HANDLE(dev)) {
> +		u32 val;
> +
> +		if (!device_property_read_u32(dev, "DCLK", &val))
> +			rt5677->pdata.dmic2_clk_pin = val;
> +
> +		rt5677->pdata.in1_diff = device_property_read_bool(dev, "IN1");
> +		rt5677->pdata.in2_diff = device_property_read_bool(dev, "IN2");

What happens if someone makes a machine which uses the DT<->ACPI
mappings (especially given that this is currently undocumented)?  That
would not work which defeats the whole purpose of using the device
property APIs.  Shouldn't we be accepting either property?

Attachment: signature.asc
Description: PGP signature