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

[Tomasz Figa]

Hi Brian,

On Wednesday 13 of November 2013 12:58:28 Brian Austin wrote:
> >> +  - compatible : "cirrus,cs42l52"
> >> +
> >> +  - reg : the I2C address of the device for I2C
> >> +
> >> +Optional properties:
> >> +
> >> +  - reset-gpio : GPIO controller's phandle and the number
> >> +		 of the GPIO used to reset the codec.
> >
> > As Kumar has already mentioned, all device-specific properties should be
> > prefixed with vendor prefix, "cirrus," in this case.
> >
> 
> I can do that, Thanks
> 
> 
> >> +  - chgfreq    : Charge Pump Frequency values. Allowable values of
> >> +		 0x00 through 0x0F.
> >> +		 Frequency = (64xFs)/(N+2)
> >
> > I suppose N means the value of chgfreq property here, but this should be
> > clearly stated. Same for Fs - I suppose it is sampling frequency, but
> > is it default, minimum, maximum or maybe something else?
> >
> Frequency = (64xFs)/(N+2)
> N = chgfreq value
> Fs = sample rate

As I mentioned in second part of my comment, is it default, minimum,
maximum or what kind of sample rate? Or is the sample rate fixed?

> 
> 
> > Personally I don't like this kind of raw values being passed using Device
> > Tree, but this one can't be really represented reasonably in a generic
> > way (such as in Hz units) without too much effort to calculate original
> > value in the driver, then it's fine.
> >
> >> +  - mica-cfg   : MIC A single-ended or differential select.
> >> +		 0x00 = Single-Ended
> >> +		 0x01 = Differential
> >
> > I'd rather make it boolean and call it cirrus,mic-a-differential.
> >
> >> +  - micb-cfg   : MIC B single-ended or differential select.
> >> +		 0x00 = Single-Ended
> >> +		 0x01 = Differential
> >
> > Ditto.
> >
> 
> I can do that as well. Thanks
> 
> >> +  - mica-sel   : MIC A single ended input select.  For Single-Ended
> >> +		 configuration, select which MIC to use.
> >> +		 0x00 = MIC1
> >> +		 0x01 = MIC2
> >> +  - micb-sel   : MIC B single ended input select.  For Single-Ended
> >> +		 configuration, select which MIC to use.
> >> +		 0x00 = MIC1
> >> +		 0x01 = MIC2
> >
> > Could you explain what are MIC A, MIC B, MIC1 and MIC2?
> >
> I can add a little more but it is best explained in the datasheet. I can 
> add a little more explaination and reference the datasheet section. I feel 
> this file might be the wrong place to go into too much depth of the pieces 
> of the device when the datasheet could be referenced. Would a reference be 
> OK?
> 

I meant just explaining to me, for the purpose of this review. However it
seems like the datasheet is publicly available, so let me just check this
there.

> 
> >> +  - 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
> >
> > For such small range of values, decimal representation is preferred from
> > readability reasons.
> >
> OK, Thanks
> >> +
> >> +Example:
> >> +
> >> +codec: cs42l52 at 4a {
> >
> > coding style: Node name should be generic, i.e. codec at 4a.
> 
> I can change this as well.  While we are talking about coding style, is 
> there a new format document somewhere with the style that was agreed to at 
> the conference just recently?

I believe that the latest document is in the works, but as for style
guidelines, most of recommendations are to be taken from ePAPR[1].

[1] https://www.power.org/wp-content/uploads/2012/06/Power_ePAPR_APPROVED_v1.1.pdf

Best regards,
Tomasz