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

[Christophe Leroy]

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