Re: [PATCH] ASoC: Add ssm2518 support

[Lars-Peter Clausen <lars@metafoo.de>]

On 05/22/2013 07:57 PM, Mark Brown wrote:
> On Wed, May 22, 2013 at 07:00:13PM +0200, Lars-Peter Clausen wrote:
>> This patch adds a ASoC CODEC driver for the SSM2516. The SSM2516 is a stereo
>> Class-D audio amplifier with an I2S interface for audio in and a built-in
>> dynamic range control processor.
> 
> I'll apply this but 

I'll send a v2 with your comments fixed.

> 
>> +	if (slots == 0) {
>> +		return regmap_update_bits(ssm2518->regmap,
>> +			SSM2518_REG_SAI_CTRL1, SSM2518_SAI_CTRL1_SAI_MASK,
>> +			SSM2518_SAI_CTRL1_SAI_I2S);
>> +	}
> 
> You've got quite a few single statement if () blocks with { } which
> shouldn't be there.
> 

I prefer to only do this for single single-line statements.


[...]
> 
>> +	switch (freq) {
>> +	case 2048000:
> 
> Looks like the user can't select 0 for the SYSCLK, I'd expect that to be
> possible for systems that can reprogram the clock so that they can avoid
> having constraints set when they don't need them.
> 

Makes sense.

>> +	} else if (i2c->dev.of_node) {
>> +		ssm2518->enable_gpio = of_get_gpio(i2c->dev.of_node, 0);
>> +		if (ssm2518->enable_gpio == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;
> 
> Why are other errors being ignored here?

To make the property optional. But I think it might be better to only allow
-ENOENT and treat everything else as an error in this case.

Thanks,
- Lars