On Thu, 31 Jul 2014, Mark Brown wrote:
> 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.
Well.... It _might_ turn into one at a later time but since it really
just ships as an AMP I haven't had the time to do the LED part yet.
>
> > +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?
Sure. I guess precious doesn't have any effect on cache then?
>
> > +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?
>
Yeah, I'm gonna scrap this whole thing...
> > +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.
>
I was trying to avoid using numbers, but if it is OK I can do that.
> > +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()?
Nope
>
> > +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.
>
OK.
> > + 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).
>
I should bail out here if this fails.
> > + /* 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?
>
Yeah, that's a good idea.
Thanks Mark
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
