| Message ID | 1400872614-21666-1-git-send-email-Paul.Handrigan@cirrus.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, May 23, 2014 at 02:16:53PM -0500, Paul Handrigan wrote: > +Optional properties: > + > + - reset-gpio : a GPIO spec for the reset pin. If specified, it will be > + deasserted before communication to the codec starts. GPIO properties are supposed to be named -gpios by convention even if they only specify a single GPIO.
On Fri, May 23, 2014 at 02:16:54PM -0500, Paul Handrigan wrote: > This patch adds support for the Cirrus Logic Stereo I2C CODEC This all looks pretty clean and nice, I have got a few comments below but they're all pretty small things. > + SOC_SINGLE("De-emp 44.1kHz", CS4265_DAC_CTL, 1, > + 1, 0), > + SOC_SINGLE("DAC INV", CS4265_DAC_CTL2, 5, > + 1, 0), > + SOC_SINGLE("DAC Zero Cross", CS4265_DAC_CTL2, 6, > + 1, 0), > + SOC_SINGLE("DAC Soft Ramp", CS4265_DAC_CTL2, 7, > + 1, 0), All boolean controls should have Switch at the end of the name. > + SOC_SINGLE("ADC HPF Disable", CS4265_ADC_CTL, 1, > + 1, 0), Invert this one and call it ADC HPF Switch (similarly for other disable controls). > + > + for (i = 0; i < ARRAY_SIZE(clk_map_table); i++) { > + if (clk_map_table[i].rate == rate && > + clk_map_table[i].mclk == mclk) > + return i; Indent the second line of the if condition with the (. > +static int cs4265_set_sysclk(struct snd_soc_dai *codec_dai, int clk_id, > + unsigned int freq, int dir) > +{ > + struct snd_soc_codec *codec = codec_dai->codec; > + struct cs4265_private *cs4265 = snd_soc_codec_get_drvdata(codec); > + int i; > + > + for (i = 0; i < ARRAY_SIZE(clk_map_table); i++) { > + if (clk_map_table[i].mclk == freq) { It's better to check that clk_id is 0 here, just in case you think of another clock to control in future (perhaps with a new device that can share the same driver even if it's not possible for this one). > + { > + .name = "cs4265-spdif", > + .playback = { > + .stream_name = "SPDIF Playback", > + .channels_min = 1, > + .channels_max = 2, > + .rates = CS4265_SPDIF_RATES, > + .formats = CS4265_FORMATS, > + }, > + .ops = &cs4265_ops, > + }, You should have separate operations for the DAC and S/PDIF DAIs and only mute the relevant interfaces. If you can't mute the DAC DAIs independently just don't provide an operation and let the user control it as they like. This avoids problems if more than one stream is running at once. > +static int cs4265_probe(struct snd_soc_codec *codec) > +{ > + return 0; > +} Remove empty operations. > + } else { > + pdata = devm_kzalloc(&i2c_client->dev, > + sizeof(struct cs4265_platform_data), > + GFP_KERNEL); > + if (!pdata) { > + dev_err(&i2c_client->dev, "could not allocate pdata\n"); > + return -ENOMEM; > + } > + pdata->reset_gpio = of_get_named_gpio(i2c_client->dev.of_node, > + "reset-gpio", 0); > + cs4265->pdata = *pdata; > + } Can you convert this to use the new gpiod_ APIs and directly request by name? There's also the -gpios thing I mentioned for the binding. > + ret = snd_soc_register_codec(&i2c_client->dev, > + &soc_codec_dev_cs4265, cs4265_dai, > + ARRAY_SIZE(cs4265_dai)); > + return ret; devm_
> On May 23, 2014, at 3:40 PM, "Mark Brown" <broonie@kernel.org> wrote: > >> On Fri, May 23, 2014 at 02:16:54PM -0500, Paul Handrigan wrote: >> This patch adds support for the Cirrus Logic Stereo I2C CODEC > > This all looks pretty clean and nice, I have got a few comments below > but they're all pretty small things. > >> + SOC_SINGLE("De-emp 44.1kHz", CS4265_DAC_CTL, 1, >> + 1, 0), >> + SOC_SINGLE("DAC INV", CS4265_DAC_CTL2, 5, >> + 1, 0), >> + SOC_SINGLE("DAC Zero Cross", CS4265_DAC_CTL2, 6, >> + 1, 0), >> + SOC_SINGLE("DAC Soft Ramp", CS4265_DAC_CTL2, 7, >> + 1, 0), > > All boolean controls should have Switch at the end of the name. > >> + SOC_SINGLE("ADC HPF Disable", CS4265_ADC_CTL, 1, >> + 1, 0), > > Invert this one and call it ADC HPF Switch (similarly for other disable > controls). > >> + >> + for (i = 0; i < ARRAY_SIZE(clk_map_table); i++) { >> + if (clk_map_table[i].rate == rate && >> + clk_map_table[i].mclk == mclk) >> + return i; > > Indent the second line of the if condition with the (. > >> +static int cs4265_set_sysclk(struct snd_soc_dai *codec_dai, int clk_id, >> + unsigned int freq, int dir) >> +{ >> + struct snd_soc_codec *codec = codec_dai->codec; >> + struct cs4265_private *cs4265 = snd_soc_codec_get_drvdata(codec); >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(clk_map_table); i++) { >> + if (clk_map_table[i].mclk == freq) { > > It's better to check that clk_id is 0 here, just in case you think of > another clock to control in future (perhaps with a new device that can > share the same driver even if it's not possible for this one). > >> + { >> + .name = "cs4265-spdif", >> + .playback = { >> + .stream_name = "SPDIF Playback", >> + .channels_min = 1, >> + .channels_max = 2, >> + .rates = CS4265_SPDIF_RATES, >> + .formats = CS4265_FORMATS, >> + }, >> + .ops = &cs4265_ops, >> + }, > > You should have separate operations for the DAC and S/PDIF DAIs and only > mute the relevant interfaces. If you can't mute the DAC DAIs > independently just don't provide an operation and let the user control > it as they like. This avoids problems if more than one stream is > running at once. > >> +static int cs4265_probe(struct snd_soc_codec *codec) >> +{ >> + return 0; >> +} > > Remove empty operations. > >> + } else { >> + pdata = devm_kzalloc(&i2c_client->dev, >> + sizeof(struct cs4265_platform_data), >> + GFP_KERNEL); >> + if (!pdata) { >> + dev_err(&i2c_client->dev, "could not allocate pdata\n"); >> + return -ENOMEM; >> + } >> + pdata->reset_gpio = of_get_named_gpio(i2c_client->dev.of_node, >> + "reset-gpio", 0); >> + cs4265->pdata = *pdata; >> + } > > Can you convert this to use the new gpiod_ APIs and directly request by > name? There's also the -gpios thing I mentioned for the binding. > >> + ret = snd_soc_register_codec(&i2c_client->dev, >> + &soc_codec_dev_cs4265, cs4265_dai, >> + ARRAY_SIZE(cs4265_dai)); >> + return ret; > > devm_ Thanks. I will make the changes as requested. -- 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 05/23/2014 09:16 PM, Paul Handrigan wrote: [...] A couple of trivial bits: > @@ -300,6 +301,10 @@ config SND_SOC_CS42L73 > tristate "Cirrus Logic CS42L73 CODEC" > depends on I2C > > +config SND_SOC_CS4265 > + tristate "Cirrus Logic CS4265 CODEC" > + depends on I2C select REGMAP_I2C > + > # Cirrus Logic CS4270 Codec > config SND_SOC_CS4270 > tristate "Cirrus Logic CS4270 CODEC" > diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile > index 1ccdaf0..b3c6b5f 100644 [...] > diff --git a/sound/soc/codecs/cs4265.c b/sound/soc/codecs/cs4265.c > new file mode 100644 > index 0000000..f7a6be9 > --- /dev/null > +++ b/sound/soc/codecs/cs4265.c > @@ -0,0 +1,716 @@ [...] > +struct cs4265_private { > + struct cs4265_platform_data pdata; > + struct regmap *regmap; > + struct snd_soc_codec *codec; > + struct device *dev; Both the codec and the dev field don't seem to be used. > + u8 format; > + u32 sysclk; > +}; > + [...] > + > +static const struct soc_enum digital_input_mux_enum = > + SOC_ENUM_SINGLE(CS4265_SIG_SEL, 7, > + ARRAY_SIZE(digital_input_mux_text), > + digital_input_mux_text); SOC_ENUM_SINGLE_DECL(), same for the other enums below. > + [...] > +static struct snd_soc_dai_ops cs4265_ops = { const > + .hw_params = cs4265_pcm_hw_params, > + .digital_mute = cs4265_digital_mute, > + .set_fmt = cs4265_set_fmt, > + .set_sysclk = cs4265_set_sysclk, > +}; [...] > +static struct snd_soc_codec_driver soc_codec_dev_cs4265 = { const The soc_codec_dev_foobar naming scheme used in some older driver comes from a time where there were no CODEC drivers. A better naming scheme for new driver is foobar_codec_driver. > + .probe = cs4265_probe, > + .remove = cs4265_remove, > + .set_bias_level = cs4265_set_bias_level, > + > + .dapm_widgets = cs4265_dapm_widgets, > + .num_dapm_widgets = ARRAY_SIZE(cs4265_dapm_widgets), > + .dapm_routes = cs4265_audio_map, > + .num_dapm_routes = ARRAY_SIZE(cs4265_audio_map), > + > + .controls = cs4265_snd_controls, > + .num_controls = ARRAY_SIZE(cs4265_snd_controls), > +}; > + > +static struct regmap_config cs4265_regmap = { const > + .reg_bits = 8, > + .val_bits = 8, > + > + .max_register = CS4265_MAX_REGISTER, > + .reg_defaults = cs4265_reg_defaults, > + .num_reg_defaults = ARRAY_SIZE(cs4265_reg_defaults), > + .readable_reg = cs4265_readable_register, > + .volatile_reg = cs4265_volatile_register, > + .cache_type = REGCACHE_RBTREE, > +}; > + [...] -- 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 May 23, 2014, at 3:40 PM, "Mark Brown" <broonie@kernel.org> wrote: > >> On Fri, May 23, 2014 at 02:16:54PM -0500, Paul Handrigan wrote: >> This patch adds support for the Cirrus Logic Stereo I2C CODEC > > This all looks pretty clean and nice, I have got a few comments below > but they're all pretty small things. > >> + SOC_SINGLE("De-emp 44.1kHz", CS4265_DAC_CTL, 1, >> + 1, 0), >> + SOC_SINGLE("DAC INV", CS4265_DAC_CTL2, 5, >> + 1, 0), >> + SOC_SINGLE("DAC Zero Cross", CS4265_DAC_CTL2, 6, >> + 1, 0), >> + SOC_SINGLE("DAC Soft Ramp", CS4265_DAC_CTL2, 7, >> + 1, 0), > > All boolean controls should have Switch at the end of the name. > >> + SOC_SINGLE("ADC HPF Disable", CS4265_ADC_CTL, 1, >> + 1, 0), > > Invert this one and call it ADC HPF Switch (similarly for other disable > controls). > >> + >> + for (i = 0; i < ARRAY_SIZE(clk_map_table); i++) { >> + if (clk_map_table[i].rate == rate && >> + clk_map_table[i].mclk == mclk) >> + return i; > > Indent the second line of the if condition with the (. > >> +static int cs4265_set_sysclk(struct snd_soc_dai *codec_dai, int clk_id, >> + unsigned int freq, int dir) >> +{ >> + struct snd_soc_codec *codec = codec_dai->codec; >> + struct cs4265_private *cs4265 = snd_soc_codec_get_drvdata(codec); >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(clk_map_table); i++) { >> + if (clk_map_table[i].mclk == freq) { > > It's better to check that clk_id is 0 here, just in case you think of > another clock to control in future (perhaps with a new device that can > share the same driver even if it's not possible for this one). > >> + { >> + .name = "cs4265-spdif", >> + .playback = { >> + .stream_name = "SPDIF Playback", >> + .channels_min = 1, >> + .channels_max = 2, >> + .rates = CS4265_SPDIF_RATES, >> + .formats = CS4265_FORMATS, >> + }, >> + .ops = &cs4265_ops, >> + }, > > You should have separate operations for the DAC and S/PDIF DAIs and only > mute the relevant interfaces. If you can't mute the DAC DAIs > independently just don't provide an operation and let the user control > it as they like. This avoids problems if more than one stream is > running at once. I am going to remove the S/PDIF DAI. Since it is TX only. > >> +static int cs4265_probe(struct snd_soc_codec *codec) >> +{ >> + return 0; >> +} > > Remove empty operations. > >> + } else { >> + pdata = devm_kzalloc(&i2c_client->dev, >> + sizeof(struct cs4265_platform_data), >> + GFP_KERNEL); >> + if (!pdata) { >> + dev_err(&i2c_client->dev, "could not allocate pdata\n"); >> + return -ENOMEM; >> + } >> + pdata->reset_gpio = of_get_named_gpio(i2c_client->dev.of_node, >> + "reset-gpio", 0); >> + cs4265->pdata = *pdata; >> + } > > Can you convert this to use the new gpiod_ APIs and directly request by > name? There's also the -gpios thing I mentioned for the binding. > >> + ret = snd_soc_register_codec(&i2c_client->dev, >> + &soc_codec_dev_cs4265, cs4265_dai, >> + ARRAY_SIZE(cs4265_dai)); >> + return ret; > > devm_ -- 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/cs4265.txt b/Documentation/devicetree/bindings/sound/cs4265.txt new file mode 100644 index 0000000..03d0dde --- /dev/null +++ b/Documentation/devicetree/bindings/sound/cs4265.txt @@ -0,0 +1,29 @@ +CS4265 audio CODEC + +This device supports I2C only. + +Required properties: + + - compatible : "cirrus,cs4265" + + - reg : the I2C address of the device for I2C. The I2C address depends on + the state of the AD0 pin. If AD0 is high, the i2c address is 0x4f. + If it is low, the i2c address is 0x4e. + +Optional properties: + + - reset-gpio : a GPIO spec for the reset pin. If specified, it will be + deasserted before communication to the codec starts. + +Examples: + +codec_ad0_high: cs4265@4f { /* AD0 Pin is high */ + compatible = "cirrus,cs4265"; + reg = <0x4f>; +}; + + +codec_ad0_low: cs4265@4e { /* AD0 Pin is low */ + compatible = "cirrus,cs4265"; + reg = <0x4e>; +};
This patch adds binding documentation for the Cirrus Logic CS4265 CODEC. Signed-off-by: Paul Handrigan <paul.handrigan@cirrus.com> --- Documentation/devicetree/bindings/sound/cs4265.txt | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/cs4265.txt