On Wed, Aug 09, 2017 at 05:11:01PM +0100, Mark Brown wrote:
> On Wed, Aug 09, 2017 at 10:00:21AM -0500, Li Xu wrote:
>
> > + if (cs43130->dev_id == CS43131_CHIP_ID ||
> > + cs43130->dev_id == CS43198_CHIP_ID) {
> > + if (val >= 2)
>
> Throughout the driver you're doing lots of ID checks like this with if
> statements, these are better written with switch statements so it's
> easier to add new cases in (eg, if a new chip variant gets made) and so
> that it's clear that the default case is being handled properly.
>
Agreed.
> > +static const char * const enum_texts[] = {
> > + "Off",
> > + "On",
> > +};
> > +
> > +static SOC_ENUM_SINGLE_DECL(pcm_phs_enum, CS43130_PCM_FILT_OPT, 6, enum_texts);
> > +
> > +static SOC_ENUM_SINGLE_DECL(pcm_nos_enum, CS43130_PCM_FILT_OPT, 5, enum_texts);
> > +
> > +static SOC_ENUM_SINGLE_DECL(pcm_high_enum, CS43130_PCM_FILT_OPT, 1, enum_texts);
> > +
> > +static SOC_ENUM_SINGLE_DECL(pcm_dmp_enum, CS43130_PCM_FILT_OPT, 0, enum_texts);
>
> These look like simple on/off switches, why are they enums?
>
You are right. Changing to SOC_SINGLE.
> > +static int cs43130_pcm_mute(struct snd_soc_dai *dai, int mute)
> > +{
> > + struct snd_soc_codec *codec = dai->codec;
> > + struct cs43130_private *cs43130 = snd_soc_codec_get_drvdata(codec);
> > +
> > + if (mute) {
> > + if (cs43130->dev_id == CS43130_CHIP_ID ||
> > + cs43130->dev_id == CS4399_CHIP_ID) {
> > + regmap_multi_reg_write(cs43130->regmap, mute_seq,
> > + ARRAY_SIZE(mute_seq));
> > + regmap_update_bits(cs43130->regmap,
> > + CS43130_PCM_PATH_CTL_1,
> > + CS43130_MUTE_MASK, CS43130_MUTE_EN);
> > + /*
> > + * PCM Power Down Sequence
> > + * According to Design, 130ms is preferred.
> > + */
> > + msleep(130);
>
> Powering down doesn't sound like muting, shouldn't this be DAPM stuff
> (especially with the very long 130ms delay)? What is this actually
> doing at the chip level, the register names do sound like muting might
> be part of it but it looks like there's more?
>
130ms delay is added to remove pop noise during stopping audio playback.
You are right that it is not just muting, but part of stopping audio playback.
I am moving this sequence as part of DAPM.
> > +static char *hpload_msgs[] = {
> > + "HPLOAD_DC_L:%u\n",
> > + "HPLOAD_DC_R:%u\n",
> > +};
> > +
> > +static char *hpload_ac_msgs[] = {
> > + "HPLOAD_AC_L:%u:%u\n",
> > + "HPLOAD_AC_R:%u:%u\n",
> > +};
>
> > + if (cs43130->ac_meas) {
> > + for (k = 0; k < CS43130_AC_FREQ; k++) {
> > + for (j = 0; j < ARRAY_SIZE(hpload_ac_msgs); j++) {
>
> The loops are wrapping their iterators in the order i, k, j not i, j, k.
>
Will fix.
> > + ret = scnprintf(buf + i, PAGE_SIZE - i,
> > + hpload_ac_msgs[j],
> > + cs43130->ac_freq[k],
> > + cs43130->hpload_ac[k][j]);
>
> We're not using ARRAY_SIZE() for the k iteration, this whole thing feels
> risky. In any case...
>
OK.
> > +static DEVICE_ATTR(hpload, S_IRUGO, cs43130_show_hpload, NULL);
>
> ...you're adding device attributes that aren't just single values (which
> is a requirement for all sysfs files). It'd be a lot simpler and
> compliant with the sysfs ABI to just make four appropriately named files
> with the individual values in them if they exist.
>
OK. Seperating into 4 individual files, with each containing impedance for DC
Left, Right, and AC Left, Right.
> > + cs43130->idle_bias_off = of_property_read_bool(np,
> > + "cirrus,idle-bias-off");
>
> This should not be in the DT - either the device can reasonably power
> down when idle or it can't but that's not machine specific configuration
> and is a Linux specific implementation detail anyway.
>
OK, removing from DT.
> > +static const struct i2c_device_id cs43130_i2c_id[] = {
> > + {"cs43130", 0},
> > + {}
> > +};
>
> Since the driver supports multiple parts I'd expect to see all the parts
> individually recognized as I2C IDs, and similarly for DT compatible
> strings. This makes it easier for people to do system integration and
> means that if it turns out we need the data it's there.
OK, adding additional IDs for I2C and DT.
--
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
