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

[Christophe Leroy]

Le 23/01/2023 à 13:17, Herve Codina a écrit :
> Hi Christophe,
> 
> On Mon, 23 Jan 2023 11:13:23 +0000
> Christophe Leroy <christophe.leroy@xxxxxxxxxx> wrote:
> 
>> Hi Hervé,
>>
>> Le 23/01/2023 à 09:56, Herve Codina a écrit :
>>>
>>> gpiochip_get_data() is defined only when CONFIG_GPIOLIB is set.
>>> That's why the #if section is used.
>>
>> gpiochip_get_data() is still declared when CONFIG_GPIOLIB is not set, so
>> it is not a problem, the call to it will be eliminated at buildtime.
>>
>> By the way, at the time being I get the following warnings:
>>
>>     CC      sound/soc/codecs/idt821034.o
>> sound/soc/codecs/idt821034.c:310:12: warning: 'idt821034_read_slic_raw'
>> defined but not used [-Wunused-function]
>>     310 | static int idt821034_read_slic_raw(struct idt821034 *idt821034,
>> u8 ch, u8 *slic_raw)
>>         |            ^~~~~~~~~~~~~~~~~~~~~~~
>> sound/soc/codecs/idt821034.c:305:11: warning:
>> 'idt821034_get_written_slic_raw' defined but not used [-Wunused-function]
>>     305 | static u8 idt821034_get_written_slic_raw(struct idt821034
>> *idt821034, u8 ch)
>>         |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> sound/soc/codecs/idt821034.c:276:12: warning: 'idt821034_write_slic_raw'
>> defined but not used [-Wunused-function]
>>     276 | static int idt821034_write_slic_raw(struct idt821034
>> *idt821034, u8 ch, u8 slic_raw)
>>         |            ^~~~~~~~~~~~~~~~~~~~~~~~
>> sound/soc/codecs/idt821034.c:271:11: warning: 'idt821034_get_slic_conf'
>> defined but not used [-Wunused-function]
>>     271 | static u8 idt821034_get_slic_conf(struct idt821034 *idt821034,
>> u8 ch)
>>         |           ^~~~~~~~~~~~~~~~~~~~~~~
>> sound/soc/codecs/idt821034.c:250:12: warning: 'idt821034_set_slic_conf'
>> defined but not used [-Wunused-function]
>>     250 | static int idt821034_set_slic_conf(struct idt821034 *idt821034,
>> u8 ch, u8 slic_dir)
>>         |            ^~~~~~~~~~~~~~~~~~~~~~~
>>
>>
>> With the following changes I have no warning and an objdump -x on
>> idt821034.o shows no reference to gpiochip_get_data()
>>
>> diff --git a/sound/soc/codecs/idt821034.c b/sound/soc/codecs/idt821034.c
>> index 5eb93fec6042..8b75388e22ce 100644
>> --- a/sound/soc/codecs/idt821034.c
>> +++ b/sound/soc/codecs/idt821034.c
>> @@ -968,7 +968,6 @@ static const struct snd_soc_component_driver
>> idt821034_component_driver = {
>>    	.endianness		= 1,
>>    };
>>
>> -#if IS_ENABLED(CONFIG_GPIOLIB)
>>    #define IDT821034_GPIO_OFFSET_TO_SLIC_CHANNEL(_offset) (((_offset) /
>> 5) % 4)
>>    #define IDT821034_GPIO_OFFSET_TO_SLIC_MASK(_offset)    BIT((_offset) % 5)
>>
>> @@ -1133,12 +1132,6 @@ static int idt821034_gpio_init(struct idt821034
>> *idt821034)
>>    	return devm_gpiochip_add_data(&idt821034->spi->dev,
>> &idt821034->gpio_chip,
>>    				      idt821034);
>>    }
>> -#else /* IS_ENABLED(CONFIG_GPIOLIB) */
>> -static int idt821034_gpio_init(struct idt821034 *idt821034)
>> -{
>> -	return 0;
>> -}
>> -#endif
>>
>>    static int idt821034_spi_probe(struct spi_device *spi)
>>    {
>> @@ -1165,6 +1158,9 @@ static int idt821034_spi_probe(struct spi_device *spi)
>>    	if (ret)
>>    		return ret;
>>
>> +	if (!IS_ENABLED(CONFIG_GPIOLIB))
>> +		return 0;
>> +
>>    	ret = idt821034_gpio_init(idt821034);
>>    	if (ret)
>>    		return ret;
>>
>>
>> Christophe
> 
> Right, I did the test too and indeed, I can remove the #if section.
> 
> I will use (I think is clearer) at idt821034_spi_probe():
> 	if (!IS_ENABLED(CONFIG_GPIOLIB)) {
>     		ret = idt821034_gpio_init(idt821034);
> 		if (ret)
>     			return ret;
> 	}
> 


I guess you mean :

	if (IS_ENABLED(CONFIG_GPIOLIB))


> Is that ok for you ?



What about:

	if (IS_ENABLED(CONFIG_GPIOLIB))
		return idt821034_gpio_init(idt821034);
	else
		return 0;

Christophe