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.
+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?
+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?
+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.
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...
+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.
- 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.
+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.