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.