On Tue, Jul 29, 2014 at 03:02:42PM -0500, Brian Austin wrote: > + case CS35L32_LED_STATUS: > + case CS35L32_FLASH_MODE: > + case CS35L32_MOVIE_MODE: > + case CS35L32_FLASH_TIMER: > + case CS35L32_FLASH_INHIBIT:
Should this be an MFD? Can always be refactored later if required
though.
> +static bool cs35l32_volatile_register(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case CS35L32_DEVID_AB:
> + case CS35L32_DEVID_CD:
> + case CS35L32_DEVID_E:
> + case CS35L32_FAB_ID:
> + case CS35L32_REV_ID:
> + return 1;
> + default:
> + return 0;
> + }
> +}
Should the interrupt and LED status registers not also be volatile?
> +static const struct snd_kcontrol_new cs35l32_snd_controls[] = {
> + SOC_SINGLE_TLV("SPK Amp Volume", CS35L32_CLASSD_CTL,
> + 3, 0x04, 1, classd_ctl_tlv),
Speaker Volume.
> + SOC_SINGLE("Gain Zero Cross", CS35L32_CLASSD_CTL, 2, 1, 0),
Zero Cross Switch perhaps (if it's an on/off control it should be called
Switch)?
> +static int int_clear(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol, int event)
> +{
> + struct snd_soc_codec *codec = w->codec;
> +
> + if (SND_SOC_DAPM_EVENT_ON(event)) {
> + snd_soc_read(codec, CS35L32_INT_STATUS_1);
> + snd_soc_read(codec, CS35L32_INT_STATUS_2);
> + } else {
> + return 0;
> + }
> + return 0;
> +}
This seems... icky. Shouldn't there be an interrupt handler doing
this?
> +static int cs35l32_codec_set_sysclk(struct snd_soc_codec *codec,
> + int clk_id, int source, unsigned int freq, int
> dir)
> +{
> +
> + switch (freq) {
> + case CS35L32_MCLK_6144MHZ:
Not sure these defines add anything over just using the numbers and it
avoids ickyness with the fact that I bet this isn't really 6.144GHz.
> +static int cs35l32_probe(struct snd_soc_codec *codec)
> +{
> + /* Power down the AMP */
> + snd_soc_update_bits(codec, CS35L32_PWRCTL1, CS35L32_PDN_AMP,
> + CS35L32_PDN_AMP);
> +
> + /* Clear MCLK Error Bit since we don't have the clock yet */
> + snd_soc_read(codec, CS35L32_INT_STATUS_1);
> +
> + return 0;
> +}
Any reason not to do these in the device level probe()?
> +static int cs35l32_remove(struct snd_soc_codec *codec)
> +{
> + struct cs35l32_private *cs35l32 = snd_soc_codec_get_drvdata(codec);
> +
> + regulator_bulk_free(ARRAY_SIZE(cs35l32->supplies), cs35l32->supplies);
> + return 0;
> +}
The regulators should be being acquired and released in the device level
probe(), though this could be dropped entirely with devm.
> + ret = regmap_register_patch(cs35l32->regmap, cs35l32_monitor_patch,
> + ARRAY_SIZE(cs35l32_monitor_patch));
Should either pay attention to the return value or not assign ret
(better to pay attention but it's not like it'd be the first CODEC to
ignore it).
> + /* initialize codec */
> + ret = regmap_read(cs35l32->regmap, CS35L32_DEVID_AB, ®);
> + devid = (reg & 0xFF) << 12;
> +
> + ret = regmap_read(cs35l32->regmap, CS35L32_DEVID_CD, ®);
> + devid |= (reg & 0xFF) << 4;
> +
> + ret = regmap_read(cs35l32->regmap, CS35L32_DEVID_E, ®);
> + devid |= (reg & 0xF0) >> 4;
> +
> + if (devid != CS35L32_CHIP_ID) {
> + ret = -ENODEV;
> + dev_err(&i2c_client->dev,
> + "CS35L32 Device ID (%X). Expected %X\n",
> + devid, CS35L32_CHIP_ID);
> + return ret;
> + }
Should the ID check not be done before we register the patch in case
it's the wrong device and we do something bad to it by writing to it?
signature.asc
Description: Digital signature
