linux-kernel - Re: [PATCH v2 5/5] ASoC: adau17x1: Support platform data via DT

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: <4053162.T8NAULHXMt@wuerfel>
Date:	Tue, 16 Feb 2016 14:42:02 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Andreas Irestål <andreas.irestal@...s.com>
Cc:	Lars-Peter Clausen <lars@...afoo.de>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, devicetree@...r.kernel.org,
	alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
	Andreas Irestål <andire@...s.com>
Subject: Re: [PATCH v2 5/5] ASoC: adau17x1: Support platform data via DT

On Tuesday 16 February 2016 13:56:45 Andreas Irestål wrote:
> Currently, it is only possible to configure HW-specific options to the
> adau17x1 codecs by providing a platform data struct. With this patch,
> it is possible to provide the same data via DT instead.
> 
> Signed-off-by: Andreas Irestål <andire@...s.com>
> ---
>  .../devicetree/bindings/sound/adi,adau17x1.txt     |  31 +++++
>  include/dt-bindings/sound/adau17x1.h               |  14 +++
>  sound/soc/codecs/adau1761.c                        | 127 +++++++++++++++++++++
>  sound/soc/codecs/adau1781.c                        |  48 ++++++++
>  4 files changed, 220 insertions(+)
>  create mode 100644 include/dt-bindings/sound/adau17x1.h

It would be nicer to avoid the need for the extra header file, those
tend to cause more problems than they solve.
 
> diff --git a/Documentation/devicetree/bindings/sound/adi,adau17x1.txt b/Documentation/devicetree/bindings/sound/adi,adau17x1.txt
> index 8dbce0e..6050602 100644
> --- a/Documentation/devicetree/bindings/sound/adi,adau17x1.txt
> +++ b/Documentation/devicetree/bindings/sound/adi,adau17x1.txt
> @@ -13,6 +13,32 @@ Required properties:
>   - reg:			The i2c address. Value depends on the state of ADDR0
>  			and ADDR1, as wired in hardware.
>  
> +Optional properties:
> +
> + - adi,input-differential	bool to set if the input is differential
> + - adi,digital-microphone	bool to set if there is a digital microphone
> +				connected to digmic/jackdet pin.
> + - adi,micbias-vg		Microphone bias voltage
> +	MICBIAS_0_90_AVDD - 0.9 * AVDD
> +	MICBIAS_0_65_AVDD - 0.65 * AVDD

This could be an integer property, or possibly two (mutually exclusive)
boolean properties.

> +Optional properties (ADAU1361/ADAU1461/ADAU1761/ADAU1961 only)
> +
> + - adi,jack-detection	If present, configures codec to use the digmic/jackdet
> +			pin for jack detection. must provide one of
> +			JACKDETECT_ACTIVE_LO or JACKDETECT_ACTIVE_HI followed
> +			by debounce time in ms, which must be 5, 10, 20, or 40.

I would use one integer property for debounce and one bool property for
polarity.

> +The output mode must be one of:
> +	OUTPUT_MODE_HEADPHONE           - Headphone output
> +	OUTPUT_MODE_HEADPHONE_CAPLESS   - Capless headphone output
> +	OUTPUT_MODE_LINE                - Line output

And something along the same lines here. Or just document the three modes
as numbers in the binding file.

> +#ifdef CONFIG_OF
> +static void adau1781_pdata_from_of(struct device *dev,
> +				   struct adau1781_platform_data *pdata)

You can remove the #ifdef here...

> +	if (!dev->platform_data && np) {

if you change this to 

	if (IS_ENABLED(CONFIG_OF) && np) {

> +		of_pdata = devm_kzalloc(dev, sizeof(*of_pdata), GFP_KERNEL);
> +		if (!of_pdata)
> +			return -ENOMEM;
> +		adau1781_pdata_from_of(dev, of_pdata);
> +		dev->platform_data = of_pdata;

and here I'd try to avoid the dynamic allocation and just add the fields to the
driver private structure. You can copy the information from the platform
data in the 'else' path.

	Arnd