Re: [PATCH 1/2] ASoC: sma1307: Add driver for Iron Device SMA1307

On 13/08/2024 04:54, Kiseok Jo wrote:

Signed-off-by: Kiseok Jo kiseok.jo@irondevice.com

sound/soc/codecs/Kconfig | 8 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/sma1307.c | 2630 ++++++++++++++++++++++++++++++++++++ sound/soc/codecs/sma1307.h | 456 +++++++ 4 files changed, 3096 insertions(+) create mode 100644 sound/soc/codecs/sma1307.c create mode 100644 sound/soc/codecs/sma1307.h

...

+static int sma1307_sw_ot1_prot_put(struct snd_kcontrol *kcontrol,

• 		   struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *component =

•    snd_soc_kcontrol_component(kcontrol);
  

• struct sma1307_priv *sma1307 = snd_soc_component_get_drvdata(component);
• bool change = false, val = (bool)ucontrol->value.integer.value[0];
• if (sma1307->sw_ot1_prot == val)

• change = false;
  

• else {

• change = true;
  

• sma1307->sw_ot1_prot = val;
  

• }
• dev_dbg(sma1307->dev,

•  "%s: Over Temperature Level 1 Software Protection %s\n",
  

•  __func__, sma1307->sw_ot1_prot ? "ON" : "OFF");
  

• return change;

+}

+static int sma1307_reset_get(struct snd_kcontrol *kcontrol,

• 	     struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *component =

•    snd_soc_kcontrol_component(kcontrol);
  

• struct sma1307_priv *sma1307 = snd_soc_component_get_drvdata(component);
• ucontrol->value.integer.value[0] = (int)sma1307->reset;
• dev_dbg(sma1307->dev, "%s: ready\n", __func__);

Drop all such simple function success messages.

• return 0;

+}

+static int sma1307_reset_put(struct snd_kcontrol *kcontrol,

• 	     struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *component =

•    snd_soc_kcontrol_component(kcontrol);
  

• struct sma1307_priv *sma1307 = snd_soc_component_get_drvdata(component);
• bool val = (bool)ucontrol->value.integer.value[0];
• if (sma1307->reset == val)

• return false;
  

• sma1307->reset = val;
• if (ucontrol->value.integer.value[0] != 0

•    && ucontrol->value.integer.value[0] != 1) {
  

• dev_err(sma1307->dev, "%s: Invalid value\n", __func__);
  

• return false;
  

• }
• sma1307_regmap_update_bits(sma1307, SMA1307_00_SYSTEM_CTRL,

• 		   SMA1307_RESET_MASK, SMA1307_RESET_ON);
  

• sma1307_reset(component);
• dev_dbg(sma1307->dev, "%s: reset complete\n", __func__);

Drop

• return true;

+}

+static int sma1307_binary_mode_get(struct snd_kcontrol *kcontrol,

• 		   struct snd_ctl_elem_value *ucontrol)
  

+{

• struct sma1307_priv *sma1307 = snd_kcontrol_chip(kcontrol);
• ucontrol->value.enumerated.item[0] = (unsigned int)sma1307->binary_mode;
• dev_dbg(sma1307->dev,

•  "%s: binary mode is %s\n",
  

•  __func__, sma1307_binary_mode_text[sma1307->binary_mode]);
  

Why do you debug every sound-API call?

• if (!sma1307->set.status) {

• dev_dbg(sma1307->dev,
  

• 	 "%s: no information has been read, try reset control again\n",
  

• 	 __func__);
  

• }
• return 0;

+}

...

+static void sma1307_setting_loaded(struct sma1307_priv *sma1307, char *file)

This was never properly build and tested. That's a string, but you pass here 'struct sma1307_setting_file'.

Do not send code which does not pass W=1 builds, smatch and sparse checks.

Also, thagt's a const char *.

+{

• const struct firmware *fw;
• int *data, size, offset, num_mode, ret;
• ret = request_firmware(&fw, file, sma1307->dev);
• dev_dbg(sma1307->dev, "%s: %d\n", __func__, ret);

Drop

• if (!fw) {

• dev_err(sma1307->dev, "%s: failed to read \"%s\"\n",
  

• 	__func__, setting_file);
  

• release_firmware(fw);
  

• sma1307->set.status = false;
  

• return;
  

• }
• dev_dbg(sma1307->dev, "%s: load the "%s"\n", __func__, setting_file);
• data = kzalloc(fw->size, GFP_KERNEL);
• size = fw->size >> 2;
• memcpy(data, fw->data, fw->size);
• release_firmware(fw);
• /* HEADER */
• sma1307->set.header_size = SMA1307_SETTING_HEADER_SIZE;
• sma1307->set.checksum = data[sma1307->set.header_size - 2];
• sma1307->set.num_mode = data[sma1307->set.header_size - 1];
• num_mode = sma1307->set.num_mode;
• sma1307->set.header = devm_kzalloc(sma1307->dev,

• 			   sma1307->set.header_size,
  

• 			   GFP_KERNEL);
  

• memcpy(sma1307->set.header, data,

•       sma1307->set.header_size * sizeof(int));
  

• dev_dbg(sma1307->dev, "%s: ===== header =====\n", __func__);
• dev_dbg(sma1307->dev, "%s: header_size=%d\n",

•  __func__, (int)sma1307->set.header_size);
  

• dev_dbg(sma1307->dev, "%s: %s\n", __func__, sma1307->set.header);
• dev_dbg(sma1307->dev, "%s: checksum=%d\n",

•  __func__, sma1307->set.checksum);
  

• dev_dbg(sma1307->dev, "%s: num_mode=%d\n",

•  __func__, sma1307->set.num_mode);
  

• if ((sma1307->set.checksum >> 8) != SMA1307_SETTING_CHECKSUM) {

• dev_err(sma1307->dev, "%s: failed by dismatch \"%s\"\n",
  

• 	__func__, setting_file);
  

• sma1307->set.status = false;
  

• return;
  

• }
• dev_dbg(sma1307->dev, "%s: version = r%03d\n",

• 	 __func__, sma1307->set.checksum & 0xFF);
  

• /* DEFAULT */
• sma1307->set.def_size = SMA1307_SETTING_DEFAULT_SIZE;
• sma1307->set.def

•    = devm_kzalloc(sma1307->dev,
  

• 	   sma1307->set.def_size * sizeof(int), GFP_KERNEL);
  

• memcpy(sma1307->set.def,

•       &data[sma1307->set.header_size],
  

•       sma1307->set.def_size * sizeof(int));
  

• dev_dbg(sma1307->dev, "%s: ===== default =====\n", __func__);
• dev_dbg(sma1307->dev, "%s: default_size=%d\n",

•  __func__, (int)sma1307->set.def_size);
  

• for (int i = 0; i < sma1307->set.def_size; i++)

• dev_dbg(sma1307->dev, "%s: %08X:%08X\n", __func__,
  

• 	 i, sma1307->set.def[i]);
  

• /* MODE */
• offset = sma1307->set.header_size + sma1307->set.def_size;
• sma1307->set.mode_size = DIV_ROUND_CLOSEST(size - offset, num_mode + 1);
• for (int i = 0; i < num_mode; i++) {

• sma1307->set.mode_set[i]
  

•     = devm_kzalloc(sma1307->dev,
  

• 		   sma1307->set.mode_size * 2 * sizeof(int),
  

• 		   GFP_KERNEL);
  

• for (int j = 0; j < sma1307->set.mode_size; j++) {
  

• 	sma1307->set.mode_set[i][2 * j]
  

• 	    = data[offset + ((num_mode + 1) * j)];
  

• 	sma1307->set.mode_set[i][2 * j + 1]
  

• 	    = data[offset + ((num_mode + 1) * j + i + 1)];
  

• }
  

• }
• dev_dbg(sma1307->dev, "%s: ===== mode =====\n", __func__);
• dev_dbg(sma1307->dev, "%s: mode_size=%d\n",

•  __func__, (int)sma1307->set.mode_size);
  

• for (int i = 0; i < num_mode; i++) {

• for (int j = 0; j < sma1307->set.mode_size; j++) {
  

• 	dev_dbg(sma1307->dev, "%s: [Mode%d] %08X:%08X\n",
  

• 		 __func__, i,
  

• 		 sma1307->set.mode_set[i][2 * j],
  

• 		 sma1307->set.mode_set[i][2 * j + 1]);
  

• }
  

• }
• kfree(data);
• sma1307->set.status = true;

+}

...

+static int sma1307_i2c_probe(struct i2c_client *client) +{

• struct sma1307_priv *sma1307;
• struct device_node *np = client->dev.of_node;
• int ret = 0;
• unsigned int device_info;
• dev_dbg(&client->dev, "%s: i2c client name: %s\n",

•  __func__, client->name);
  

Drop

• sma1307 = devm_kzalloc(&client->dev,

• 	       sizeof(struct sma1307_priv), GFP_KERNEL);
  

sizeof(*)

• if (!sma1307)

• return -ENOMEM;
  

• sma1307->regmap = devm_regmap_init_i2c(client, &sma_i2c_regmap);
• if (IS_ERR(sma1307->regmap)) {

• ret = PTR_ERR(sma1307->regmap);
  

• dev_err(&client->dev,
  

• 	"%s: failed to allocate register map: %d\n",
  

• 	__func__, ret);
  

return dev_err_probe() would be much smaller

• return ret;
  

• }
• /* set initial value as normal AMP IC status */
• sma1307->name = client->name;
• sma1307->force_mute_status = false;
• sma1307->amp_mode = SMA1307_MONO_MODE;
• sma1307->num_of_pll_matches = ARRAY_SIZE(sma1307_pll_matches);
• sma1307->retry_cnt = SMA1307_I2C_RETRY_COUNT;
• sma1307->check_fault_period = CHECK_PERIOD_TIME;
• sma1307->check_fault_status = true;
• sma1307->isr_manual_mode = true;
• sma1307->init_vol = 0x32;
• sma1307->cur_vol = sma1307->init_vol;
• sma1307->format = SND_SOC_DAIFMT_I2S;
• sma1307->frame_size = 0;
• sma1307->last_bclk = 0;
• sma1307->otp_trm2 = 0;

This is never read... Just like most of all other variables/members here - totally useless.

• sma1307->otp_trm3 = 0;
• sma1307->reset = 0;
• sma1307->rev_num = 0;
• sma1307->sys_clk_id = SMA1307_PLL_CLKIN_BCLK;
• sma1307->tdm_slot0_rx = 0;
• sma1307->tdm_slot1_rx = 0;
• sma1307->tdm_slot0_tx = 0;
• sma1307->tdm_slot1_tx = 0;
• sma1307->tsdw_cnt = 0;
• sma1307->dapm_aif_in = 0;
• sma1307->dapm_aif_out0 = 0;
• sma1307->dapm_aif_out1 = 0;
• sma1307->dapm_amp_en = 0;
• sma1307->dapm_sdo_en = 0;
• sma1307->dapm_sdo_setting = 0;
• sma1307->set.status = false;
• sma1307->sw_ot1_prot = true;
• sma1307->binary_mode = 0;
• mutex_init(&sma1307->default_lock);
• INIT_DELAYED_WORK(&sma1307->check_fault_work,

• 	  sma1307_check_fault_worker);
  

• sma1307->dev = &client->dev;
• sma1307->kobj = &client->dev.kobj;
• i2c_set_clientdata(client, sma1307);
• sma1307->pll_matches = sma1307_pll_matches;
• ret = sma1307_regmap_read(sma1307,

• 		  SMA1307_FF_DEVICE_INDEX, &device_info);
  

• if ((ret != 0) || ((device_info & 0xF8) != SMA1307_DEVICE_ID)) {

• dev_err(&client->dev,
  

• 	"%s: device initialization error (%d 0x%02X)", __func__,
  

• 	ret, device_info);
  

• return -ENODEV;
  

• }
• dev_dbg(&client->dev, "%s: chip version 0x%02X\n",

•  __func__, device_info);
  

• if (of_property_read_bool(np, "use-binary")) {

• dev_dbg(&client->dev,
  

• 	 "%s: Use binary file for setting\n", __func__);
  

• sma1307->amp_set = SMA1307_BINARY_FILE_SET;
  

• } else {

• dev_dbg(&client->dev, "%s: Use default setting\n", __func__);
  

• sma1307->amp_set = SMA1307_DEFAULT_SET;
  

• }
• if (!strcmp(sma1307->name, DEVICE_NAME_SMA1307)) {

• sma1307->irq = client->irq;
  

• dev_dbg(&client->dev,
  

• 	 "%s: sma1307->irq=%d\n", __func__, sma1307->irq);
  

Drop, pretty obvious thus useless debug.

• dev_dbg(&client->dev, "%s: interrupt Enable\n", __func__);
  

Drop

• /* Request system IRQ for SMA1307 */
  

• ret = devm_request_threaded_irq(&client->dev,
  

• 				sma1307->irq,
  

• 				NULL,
  

• 				sma1307_isr,
  

• 				IRQF_ONESHOT |
  

• 				IRQF_SHARED |
  

• 				IRQF_TRIGGER_FALLING,
  

• 				dev_name(&client->dev),
  

• 				sma1307);
  

Way too wrapped.

• if (ret < 0) {
  

• 	dev_err(&client->dev,
  

• 		"%s: failed to request IRQ(%u) [%d]\n",
  

• 		__func__, sma1307->irq, ret);
  

• 	i2c_set_clientdata(client, NULL);
  

• 	sma1307->irq = 0;
  

Why? Drop both and just return dev_err_probe

• 	return ret;
  

• }
  

• if (sma1307->irq)
  

• 	disable_irq(sma1307->irq);
  

• } else {

• sma1307->irq = 0;
  

• }
• i2c_set_clientdata(client, sma1307);
• ret = devm_snd_soc_register_component(&client->dev,

• 			      &sma1307_component, sma1307_dai,
  

• 			      1);
  

• if (ret) {

• dev_err(&client->dev, "%s: failed to register component\n",
  

• 	__func__);
  

• return ret;
  

• }
• sma1307->attr_grp = &sma1307_attr_group;
• ret = sysfs_create_group(sma1307->kobj, sma1307->attr_grp);

You need to document sysfs ABI.

Best regards, Krzysztof