Message ID | 1383855737-8499-2-git-send-email-brian.austin@cirrus.com |
---|---|
State | Superseded, archived |
Headers | show |
On Thu, Nov 07, 2013 at 08:22:17PM +0000, Brian Austin wrote: > Add Device Tree Binding for the CS42L52 Codec > > Signed-off-by: Brian Austin <brian.austin@cirrus.com> > --- > .../devicetree/bindings/sound/cs42l52.txt | 34 ++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/cs42l52.txt > > diff --git a/Documentation/devicetree/bindings/sound/cs42l52.txt b/Documentation/devicetree/bindings/sound/cs42l52.txt > new file mode 100644 > index 0000000..7540198 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/cs42l52.txt > @@ -0,0 +1,34 @@ > +CS42L52 audio CODEC > + > +Required properties: > + > + - compatible : "cirrus,cs42l52" > + > + - reg : the I2C address of the device for I2C > + > +Optional properties: > + > + - reset_gpio : a GPIO spec for the reset pin. 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. s/GPIO spec/phandle + gpio-specifier/ > + - 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". When you say "values 0x00-0x0F" you mean the value is in that range? Inclusive? 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? > + - 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? Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 11, 2013 at 10:16:22AM +0000, Mark Rutland wrote: > On Thu, Nov 07, 2013 at 08:22:17PM +0000, Brian Austin wrote: > > + - 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". I rather suspect that this is the name of the register that gets the raw value specified written to it - using the register name seems reasonable for that. > > + - 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 something that would come from the electrical design of the system rather than something that users would tune at runtime.
> > 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/sound/cs42l52.txt b/Documentation/devicetree/bindings/sound/cs42l52.txt new file mode 100644 index 0000000..7540198 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/cs42l52.txt @@ -0,0 +1,34 @@ +CS42L52 audio CODEC + +Required properties: + + - compatible : "cirrus,cs42l52" + + - reg : the I2C address of the device for I2C + +Optional properties: + + - reset_gpio : a GPIO spec for the reset pin. + - chgfreq : Charge Pump Frequency values 0x00-0x0F + - mica_sel : MIC A single ended input select MIC1/MIC2 + - micb_sel : MIC B single ended input select MIC1/MIC2 + - mica_cfg : MIC A single-ended or differential select + - micb_cfg : MIC A single-ended or differential select + - 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 + +Example: + +codec: cs42l52@4a { + compatible = "cirrus,cs42l52"; + reg = <0x4a>; + reset_gpio = <&gpio 10 0>; + chgfreq = <0x05>; + mica_sel = <0x01>; + micbias_lvl = <0x05>; +};
Add Device Tree Binding for the CS42L52 Codec Signed-off-by: Brian Austin <brian.austin@cirrus.com> --- .../devicetree/bindings/sound/cs42l52.txt | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/cs42l52.txt