Messages in this thread | | | Date | Tue, 16 Aug 2016 18:20:06 +0100 | From | Mark Brown <> | Subject | Re: [PATCH 1/2] ASoC: rt5677: Add ACPI support |
| |
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? [unhandled content-type:application/pgp-signature] | |