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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1332 bytes --]

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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]