On 13/08/2024 04:54, Kiseok Jo wrote:
> Signed-off-by: Kiseok Jo <kiseok.jo@xxxxxxxxxxxxxx>
> ---
> 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
