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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 16 Aug 2016 18:20:06 +0100
From:	Mark Brown <broonie@...nel.org>
To:	John Keeping <john@...anate.com>
Cc:	alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
	Jie Yang <yang.jie@...ux.intel.com>,
	Bard Liao <bardliao@...ltek.com>,
	Oder Chiou <oder_chiou@...ltek.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>, Ben Zhang <benzh@...omium.org>,
	Dylan Reid <dgreid@...omium.org>
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?

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)