[alsa-devel] [PATCH 2/2] dt: binding: sound cs42l52 driver

[Brian Austin]

>
> Nit: device tree convention is to use '-' rather than '_'. So this should be reset-gpio rather than reset_gpio.
>
> s/_/-/ for the later property names too.
>

Will do, thanks

> s/GPIO spec/phandle + gpio-specifier/

OK, Same here


>
>> +  - chgfreq    : Charge Pump Frequency values 0x00-0x0F
>
> In general, longer but more easily understood names are preferred (e.g.
> "clock-frequency"). This could be "charge-pump-frequency".
>
As Mark Brown pointed out, I would prefer to leave this as a reference to 
a register in the device. In general I was hoping to keep the original 
platform data names as carried over to dt.

> When you say "values 0x00-0x0F" you mean the value is in that range?
> Inclusive?
>
I'll be more specific, Thanks

> Due to punctuation, upon first reading I read this as being an array of values.  It would be nice to have some clarification to prevent others maknig the smae mistake.
>
>> +  - mica_sel   : MIC A single ended input select MIC1/MIC2
>> +  - micb_sel   : MIC B single ended input select MIC1/MIC2
>
> Is this a boolean? What values are expected?
>
> Could you elaborate on what this describes?
>
>> +  - mica_cfg   : MIC A single-ended or differential select
>> +  - micb_cfg   : MIC A single-ended or differential select
>
> Could you elaborate on what these describe, what values are expected, etc?
>

I will explain values better, Thanks


>> +  - micbias_lvl: Set the output voltage level on the MICBIAS Pin
>> +		 0x00 = 0.5 x VA
>> +		 0x01 = 0.6 x VA
>> +		 0x02 = 0.7 x VA
>> +		 0x03 = 0.8 x VA
>> +		 0x04 = 0.83 x VA
>> +		 0x05 = 0.91 x VA
>
> Is this not something we'd want to change at runtime instead? Why does this need to be in the dt?
>

This is set based on the system design so it usually will be a one time 
setting at boot.

I'll send a v2 shortly,

Thank you for the review and tips for the next ones.

Regards,
Brian