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?