Re: [RFC PATCH 03/16] ASoC: Intel: sof-pcm512x: use gpiod for LED

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

>> +static struct gpiod_lookup_table pcm512x_gpios_table = {
>> +	/* .dev_id set during probe */
>> +	.table = {
> 
>> +		GPIO_LOOKUP("pcm512x-gpio", 3, "PCM512x-GPIO4", GPIO_ACTIVE_HIGH),
> 
> It says GPIO 4 and here is number 3.
> Does this 4 come from hardware documentation?

Yes TI count from 1 to 6 in their documentation. The initial HifiBerry 
DAC+ also counts from 1 to 6. I can add a comment here.

> 
>> +		{ },
> 
> No comma for terminator entries.

ok

> 
>> +	},
>> +};
> 
> ...
> 
>> +	gpiod_add_lookup_table(&pcm512x_gpios_table);
> 
> Where is the counterpart gpiod_remove_lookup_table() call?

Ah that's a miss, thanks for flagging this. I remember looking but 
obviously needed a coffee at the time.

>> +	ctx->gpio_4 = devm_gpiod_get(&pdev->dev, "PCM512x-GPIO4",
>> +				     GPIOD_OUT_LOW);
> 
> Can driver work without this GPIO? If so, perhaps devm_gpiod_get_optional().

that part yes, it's only for the LED, but if this fails then probably 
the rest of the code will also fail.
> 
>> +	if (IS_ERR(ctx->gpio_4)) {
>> +		dev_err(&pdev->dev, "gpio4 not found\n");
>> +		ret = PTR_ERR(ctx->gpio_4);
>> +		return ret;
>> +	}
>