linux-kernel - Re: [PATCH v2 2/3] ASoC: codecs: Add support for the Renesas IDT821034 codec

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: <20b4b695-1ea5-7b70-5791-1b611d685c44@csgroup.eu>
Date:   Mon, 23 Jan 2023 10:47:23 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Herve Codina <herve.codina@...tlin.com>
CC:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 2/3] ASoC: codecs: Add support for the Renesas
 IDT821034 codec



Le 23/01/2023 à 09:56, Herve Codina a écrit :
>>> +
>>> +static int idt821034_set_channel_power(struct idt821034 *idt821034, u8 ch, u8 power)
>>> +{
>>> +	u8 conf;
>>> +	int ret;
>>> +
>>> +	dev_dbg(&idt821034->spi->dev, "set_channel_power(%u, 0x%x)\n", ch, power);
>>> +
>>> +	conf = IDT821034_MODE_CODEC(ch) | idt821034->cache.codec_conf;
>>> +
>>> +	if (power & IDT821034_CONF_PWRUP_RX) {
>>> +		ret = idt821034_8bit_write(idt821034, conf | IDT821034_CONF_PWRUP_RX);
>>> +		if (ret)
>>> +			return ret;
>>> +		ret = idt821034_8bit_write(idt821034, idt821034->cache.ch[ch].rx_slot);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +	if (power & IDT821034_CONF_PWRUP_TX) {
>>> +		ret = idt821034_8bit_write(idt821034, conf | IDT821034_CONF_PWRUP_TX);
>>> +		if (ret)
>>> +			return ret;
>>> +		ret = idt821034_8bit_write(idt821034, idt821034->cache.ch[ch].tx_slot);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +	if (!(power & (IDT821034_CONF_PWRUP_TX | IDT821034_CONF_PWRUP_RX))) {
>>> +		ret = idt821034_8bit_write(idt821034, conf);
>>> +		if (ret)
>>> +			return ret;
>>> +		ret = idt821034_8bit_write(idt821034, 0x00);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>
>> Can we refactor the three actions with an helper, that could also be
>> reused for idt821034_set_codec_conf() and idt821034_set_channel_ts() and
>> idt821034_set_slic_conf() and idt821034_write_slic_raw() and
>> idt821034_set_gain_channel, something like for instance:
>>
>> static int idt821034_set_conf(struct idt821034 *idt821034, u8 conf, u8 val)
>> {
>> 	ret = idt821034_8bit_write(idt821034, conf);
>> 	if (ret)
>> 		return ret;
>> 	return idt821034_8bit_write(idt821034, val);
>> }
> 
> It can be changed.
> The function name will not be idt821034_set_conf() as it is not the same
> kind of funtion as the idt821031_set_*() already available in the code.
> What do you think about:
>    static int idt821034_2x8bit_write(struct idt821034 *idt821034, u8 val1, u8 val2)
> or
>    static int idt821034_conf_write(struct idt821034 *idt821034, u8 conf, u8 val)
> 
> I prefer the first one but it is only a personal preference.
> On your side, what do you prefer ?

idt821034_2x8bit_write() looks good to me.

Christophe