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

[Krzysztof Kozlowski <krzk@kernel.org>]

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