On Fri, Dec 20, 2019 at 06:15:34PM +0800, Jeff Chang wrote: > +++ b/sound/soc/codecs/mt6660.c > @@ -0,0 +1,653 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2019 MediaTek Inc. > + */ Please make the entire comment a C++ one so things look more intentional. > + { MT6660_REG_DEVID, 2}, > + { MT6660_REG_TDM_CFG3, 2}, > + { MT6660_REG_HCLIP_CTRL, 2}, > + { MT6660_REG_DA_GAIN, 2}, Missing space before the } (the same thing happens in some of the other tables). > +static int mt6660_component_get_volsw(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *component = > + snd_soc_kcontrol_component(kcontrol); > + struct mt6660_chip *chip = (struct mt6660_chip *) > + snd_soc_component_get_drvdata(component); > + int ret = -EINVAL; > + > + if (!strcmp(kcontrol->id.name, "Chip_Rev")) { Why would this be used on a different control? > + SOC_SINGLE_EXT("BoostMode", MT6660_REG_BST_CTRL, 0, 3, 0, > + snd_soc_get_volsw, snd_soc_put_volsw), Boost Mode. You've also got a lot of these controls that are _EXT but you then just use standard operations so it's not clear why you're using _EXT. > + SOC_SINGLE_EXT("audio input selection", MT6660_REG_DATAO_SEL, 6, 3, 0, > + snd_soc_get_volsw, snd_soc_put_volsw), Audio Input Selection, but this looks like it should be a DAPM control if it's controlling audio routing. A simple numerical setting definitely doesn't seem like the right thing. > + SOC_SINGLE_EXT("AUD LOOP BACK Switch", MT6660_REG_PATH_BYPASS, 4, 1, 0, > + snd_soc_get_volsw, snd_soc_put_volsw), This sounds like it should be a DAPM thing too. > +static int mt6660_component_probe(struct snd_soc_component *component) > +{ > + struct mt6660_chip *chip = snd_soc_component_get_drvdata(component); > + int ret = 0; > + > + dev_info(component->dev, "%s\n", __func__); dev_dbg() at most but probably better to remove this and the other similar dev_info()s. > +static inline int _mt6660_chip_id_check(struct mt6660_chip *chip) > +{ > + u8 id[2] = {0}; > + int ret = 0; > + > + ret = i2c_smbus_read_i2c_block_data(chip->i2c, MT6660_REG_DEVID, 2, id); > + if (ret < 0) > + return ret; > + ret = (id[0] << 8) + id[1]; > + ret &= 0x0ff0; > + if (ret != 0x00e0 && ret != 0x01e0) > + return -ENODEV; It'd be better to print an error message saying we don't recognize the device to help people doing debugging. > + if (of_property_read_u32(np, "rt,init_setting_num", &val)) { > + dev_info(dev, "no init setting\n"); > + chip->plat_data.init_setting_num = 0; You should be adding a DT binding document for any new DT bindings.
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel