linux-kernel - Re: [PATCH 2/3] ASoC: da7219: Add ACPI parsing 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]

Message-ID: <20160506123909.GT6292@sirena.org.uk>
Date:	Fri, 6 May 2016 13:39:09 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Adam Thomson <Adam.Thomson.Opensource@...semi.com>
Cc:	Liam Girdwood <lgirdwood@...il.com>,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org,
	Support Opensource <support.opensource@...semi.com>,
	Sathyanarayana Nujella <sathyanarayana.nujella@...el.com>
Subject: Re: [PATCH 2/3] ASoC: da7219: Add ACPI parsing support

On Thu, May 05, 2016 at 11:53:05AM +0100, Adam Thomson wrote:

> @@ -27,7 +28,6 @@
>  #include "da7219.h"
>  #include "da7219-aad.h"
> 
> -
>  /*
>   * Detection control
>   */

Random whitespace change.

>  static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev,
>  							  const char *name)
>  {
> @@ -551,6 +571,9 @@ static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev,
>  			of_node = to_of_node(child);
>  			if (of_node_cmp(of_node->name, name) == 0)
>  				return child;
> +		} else if (is_acpi_data_node(child)) {
> +			if (da7219_aad_of_acpi_node_matched(child, name))
> +				return child;
>  		}
>  	}
> 

This seems messy.  It is a function with a DT specific name that's
matching ACPI stuff and the fwnode API isn't hiding anything for us
which suggests this isn't something that's expected to work
transparently.  At least the naming needs to be corrected, and if this
*is* supposed to be something we do in ACPI I'd expect the handling to
be pushed into the fwnode API rather than open coded in a driver - at
the minute I'm unsure if this is messy because it's a bad idea to do
this at all or if it's just the naming and so on.

> -	/* Handle any DT/platform data */
> -	if ((codec->dev->of_node) && (da7219->pdata))
> +	/* Handle any DT/ACPI/platform data */
> +	if (((codec->dev->of_node) || is_acpi_node(codec->dev->fwnode)) &&
> +	    (da7219->pdata))
>  		da7219->pdata->aad_pdata = da7219_aad_of_to_pdata(codec);
> 
>  	da7219_aad_handle_pdata(codec);

Surely we should be able to check if there's firmware data without
enumerating every possible firmware type?

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