Re: [PATCH 1/2] ASoC: sma1303: Add driver for Iron Device SMA1303 Amp

diff --git a/sound/soc/codecs/sma1303.c b/sound/soc/codecs/sma1303.c new file mode 100644 index 000000000000..03c969a4891e --- /dev/null +++ b/sound/soc/codecs/sma1303.c @@ -0,0 +1,2119 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* sma1303.c -- sma1303 ALSA SoC Audio driver

• • Copyright 2022 Iron Device Corporation

Missing Copyright (c) ?

• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 as
• • published by the Free Software Foundation.

this is redundant with the SPDX line above.

• */

+#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/delay.h>

it's recommanded to list headers by alphabetical order.

+#include <linux/pm.h> +#include <linux/i2c.h> +#include <linux/regmap.h> +#include <linux/version.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/initval.h> +#include <sound/tlv.h> +#include <linux/of_device.h> +#include <linux/slab.h> +#include <asm/div64.h>

+#include "sma1303.h"

+enum sma1303_type {

• SMA1303,

+};

is this necessary?

+struct sma1303_pll_match {

• char *input_clk_name;
• char *output_clk_name;
• unsigned int input_clk;
• unsigned int post_n;
• unsigned int n;
• unsigned int vco;
• unsigned int p_cp;

+};

+struct sma1303_priv {

• enum sma1303_type devtype;
• struct device *dev;
• struct attribute_group *attr_grp;
• struct kobject *kobj;

Usually it's not recommended to muck with kobj in drivers. If this is for sysfs support there are better and safer ways.

• struct regmap *regmap;
• struct sma1303_pll_match *pll_matches;
• struct mutex lock;
• struct delayed_work check_fault_work;
• bool amp_power_status;
• bool usage_status;
• int num_of_pll_matches;
• unsigned int sys_clk_id;
• unsigned int init_vol;
• unsigned int cur_vol;
• unsigned int tdm_slot_rx;
• unsigned int tdm_slot_tx;
• unsigned int tsdw_cnt;
• unsigned int format;
• unsigned int rev_num;
• unsigned int last_over_temp;
• unsigned int last_ocp_val;
• unsigned int last_bclk;
• unsigned int frame_size;
• unsigned int amp_mode;
• long check_fault_period;
• long check_fault_status;

+};

+static struct sma1303_pll_match sma1303_pll_matches[] = { +PLL_MATCH("1.411MHz", "24.595MHz", 1411200, 0x07, 0xF4, 0x8B, 0x03), +PLL_MATCH("1.536MHz", "24.576MHz", 1536000, 0x07, 0xE0, 0x8B, 0x03), +PLL_MATCH("3.072MHz", "24.576MHz", 3072000, 0x07, 0x70, 0x8B, 0x03), +PLL_MATCH("6.144MHz", "24.576MHz", 6144000, 0x07, 0x70, 0x8B, 0x07), +PLL_MATCH("12.288MHz", "24.576MHz", 12288000, 0x07, 0x70, 0x8B, 0x0B), +PLL_MATCH("19.2MHz", "24.343MHz", 19200000, 0x07, 0x47, 0x8B, 0x0A), +PLL_MATCH("24.576MHz", "24.576MHz", 24576000, 0x07, 0x70, 0x8B, 0x0F), +};

Any reason to use strings instead of actual integer values for frequencies?

+static int sma1303_regmap_write(struct regmap *map, struct device *dev,

• 		unsigned int reg, unsigned int val)
  

+{

• int ret = 0;

unnecessary init, same comment for all the code.

• ret = regmap_write(map, reg, val);
• if (ret < 0) {

• dev_err(dev, "Failed to write, register: 0x%02X, ret: %d\n",
  

• 	       reg, ret);
  

• }
• return ret;

+}

+static int sma1303_regmap_update_bits(struct regmap *map, struct device *dev,

• unsigned int reg, unsigned int mask, unsigned int val)
  

+{

• int ret = 0;
• ret = regmap_update_bits(map, reg, mask, val);
• if (ret < 0) {

• dev_err(dev, "Failed to write, register: 0x%02X, ret: %d\n",
  

Failed to update?

• 	       reg, ret);
  

• }
• return ret;

+}

+static int bytes_ext_get(struct snd_kcontrol *kcontrol,

• 	struct snd_ctl_elem_value *ucontrol, int reg)
  

+{

• struct snd_soc_component *component =

• snd_soc_kcontrol_component(kcontrol);
  

• struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
• struct soc_bytes_ext *params = (void *)kcontrol->private_value;
• unsigned int i, reg_val;
• u8 *val;
• int ret = -1;
• if (component == NULL) {

if (!component)

• pr_err("%s:component is NULL\n", __func__);
  

can pr_err be avoided?

• return ret;
  

• }
• if (sma1303 == NULL) {

if (!sma1303)

• pr_err("%s:sma1303 is NULL\n", __func__);
  

can pr_err be avoided?

Same comments for all functions using this pattern.

• return ret;
  

• }
• val = (u8 *)ucontrol->value.bytes.data;
• for (i = 0; i < params->max; i++) {

• ret = regmap_read(sma1303->regmap, reg + i, &reg_val);
  

• if (ret < 0) {
  

• 	dev_err(component->dev,
  

• 		"Failed to read, register: %x ret: %d\n",
  

• 		reg + i, ret);
  

• 	return ret;
  

• }
  

• if (sizeof(reg_val) > 2)
  

• 	reg_val = cpu_to_le32(reg_val);
  

• else
  

• 	reg_val = cpu_to_le16(reg_val);
  

• memcpy(val + i, &reg_val, sizeof(u8));
  

I wasn't able to figure out what this code does. sizeof(reg_val) is a constant so the second branch is never taken, and you end-up using memcpy to copy one byte, so what is the issue with endianness?

• }
• return 0;

+}

+static int bytes_ext_put(struct snd_kcontrol *kcontrol,

• 	struct snd_ctl_elem_value *ucontrol, int reg)
  

+{

• struct snd_soc_component *component =

• snd_soc_kcontrol_component(kcontrol);
  

• struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
• struct soc_bytes_ext *params = (void *)kcontrol->private_value;
• void *data;
• u8 *val;
• int i, ret = -1;
• if (component == NULL) {

• pr_err("%s:component is NULL\n", __func__);
  

• return ret;
  

• }
• if (sma1303 == NULL) {

• pr_err("%s:sma1303 is NULL\n", __func__);
  

• return ret;
  

• }
• data = kmemdup(ucontrol->value.bytes.data,

• 	params->max, GFP_KERNEL | GFP_DMA);
  

• if (!data)

• return -ENOMEM;
  

• val = (u8 *)data;
• for (i = 0; i < params->max; i++) {

• ret = sma1303_regmap_write(
  

• 		sma1303->regmap, component->dev,
  

• 		reg + i, *(val + i));
  

• if (ret < 0) {
  

• 	kfree(data);
  

• 	return ret;
  

• }
  

• }
• kfree(data);
• return 0;

ret = 0; for (i = 0; i < params->max; i++) { ret = sma1303_regmap_write( sma1303->regmap, component->dev, reg + i, *(val + i)); if (ret < 0) break; }

kfree(data); return ret;

+static int amp_usage_status_get(struct snd_kcontrol *kcontrol,

• struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *component =

• snd_soc_kcontrol_component(kcontrol);
  

• struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
• int ret = -1;
• if (component == NULL) {

• pr_err("%s:component is NULL\n", __func__);
  

• return ret;
  

• }
• if (sma1303 == NULL) {

• pr_err("%s:sma1303 is NULL\n", __func__);
  

• return ret;
  

• }
• ucontrol->value.integer.value[0] = sma1303->usage_status;
• if (sma1303->usage_status)

• dev_info(component->dev, "Amplifier Power Control Enabled\n");
  

• else

• dev_info(component->dev, "Amplifier Power Control Disabled\n");
  

• return 0;

+}

+static int amp_usage_status_put(struct snd_kcontrol *kcontrol,

• struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *component =

• snd_soc_kcontrol_component(kcontrol);
  

• struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
• int ret = -1;
• if (component == NULL) {

• pr_err("%s:component is NULL\n", __func__);
  

• return ret;
  

• }
• if (sma1303 == NULL) {

• pr_err("%s:sma1303 is NULL\n", __func__);
  

• return ret;
  

• }
• if ((sma1303->usage_status

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

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

• dev_info(component->dev, "%s\n", "Force AMP Power Down");
  

• ret = sma1303_shutdown(component);
  

• if (ret < 0) {
  

• 	ucontrol->value.integer.value[0]
  

• 	       = sma1303->usage_status;
  

• 	return ret;
  

• }
  

• }
• sma1303->usage_status = ucontrol->value.integer.value[0];
• return 0;

+}

+static const char * const sma1303_amp_mode_text[] = {

• "1 Chip", "Mono on 2 chips", "Left in 2 chips", "Right in 2chips"};

+static const struct soc_enum sma1303_amp_mode_enum =

• SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(sma1303_amp_mode_text),

• 	sma1303_amp_mode_text);
  

+static int sma1303_amp_mode_get(struct snd_kcontrol *kcontrol,

• 		struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *component =

• snd_soc_kcontrol_component(kcontrol);
  

• struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
• int ret = -1;
• if (component == NULL) {

• pr_err("%s:component is NULL\n", __func__);
  

• return ret;
  

• }
• if (sma1303 == NULL) {

• pr_err("%s:sma1303 is NULL\n", __func__);
  

• return ret;
  

• }
• ucontrol->value.integer.value[0] = sma1303->amp_mode;
• switch (sma1303->amp_mode) {
• case ONE_CHIP_SOLUTION:

• dev_info(component->dev, "Amplifier 1 Chip Solution\n");
  

• break;
  

• case MONO_TWO_CHIP_SOLUTION:

• dev_info(component->dev, "Amplifier Mono 2 Chips Solution\n");
  

• break;
  

• case LEFT_TWO_CHIP_SOLUTION:

• dev_info(component->dev, "Amplifier Stereo(Left) 2 Chips Solution\n");
  

• break;
  

• case RIGHT_TWO_CHIP_SOLUTION:

• dev_info(component->dev, "Amplifier Stereo(Right) 2 Chips Solution\n");
  

• break;
  

• default:

• dev_err(component->dev, "Invalid Value");
  

• return ret;
  

• }
• return 0;

+}

+static int sma1303_amp_mode_put(struct snd_kcontrol *kcontrol,

• struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *component =

• snd_soc_kcontrol_component(kcontrol);
  

• struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
• int ret = 0;
• if (component == NULL) {

• pr_err("%s:component is NULL\n", __func__);
  

• return -1;
  

• }
• if (sma1303 == NULL) {

• pr_err("%s:sma1303 is NULL\n", __func__);
  

• return -1;
  

• }
• sma1303->amp_mode = ucontrol->value.integer.value[0];
• switch (sma1303->amp_mode) {
• case ONE_CHIP_SOLUTION:
• case MONO_TWO_CHIP_SOLUTION:

• ret += sma1303_regmap_update_bits(
  

• 		sma1303->regmap, component->dev,
  

• 		SMA1303_11_SYSTEM_CTRL2,
  

• 		MONOMIX_MASK, MONOMIX_ON);
  

• ret += sma1303_regmap_update_bits(
  

• 		sma1303->regmap, component->dev,
  

• 		SMA1303_11_SYSTEM_CTRL2,
  

• 		LR_DATA_SW_MASK, LR_DATA_SW_NORMAL);
  

• break;
  

• case LEFT_TWO_CHIP_SOLUTION:

• ret += sma1303_regmap_update_bits(
  

• 		sma1303->regmap, component->dev,
  

• 		SMA1303_11_SYSTEM_CTRL2,
  

• 		MONOMIX_MASK, MONOMIX_OFF);
  

• ret += sma1303_regmap_update_bits(
  

• 		sma1303->regmap, component->dev,
  

• 		SMA1303_11_SYSTEM_CTRL2,
  

• 		LR_DATA_SW_MASK, LR_DATA_SW_NORMAL);
  

• break;
  

• case RIGHT_TWO_CHIP_SOLUTION:

• ret += sma1303_regmap_update_bits(
  

• 		sma1303->regmap, component->dev,
  

• 		SMA1303_11_SYSTEM_CTRL2,
  

• 		MONOMIX_MASK, MONOMIX_OFF);
  

• ret += sma1303_regmap_update_bits(
  

• 		sma1303->regmap, component->dev,
  

• 		SMA1303_11_SYSTEM_CTRL2,
  

• 		LR_DATA_SW_MASK, LR_DATA_SW_SWAP);
  

• break;
  

• default:

• dev_err(component->dev, "Invalid Value");
  

• ret += -1;
  

• }
• return ret;

Not sure I understand your arithmetic on combining error codes.

If one transaction fails, is there any point in trying another regmap_update_bits()?

{skipping all the way to the probe which has a lot of issues}

+static int sma1303_i2c_probe(struct i2c_client *client,

• 		const struct i2c_device_id *id)
  

+{

• struct sma1303_priv *sma1303;
• struct device_node *np = client->dev.of_node;
• int ret;
• u32 value;
• unsigned int device_info;
• dev_info(&client->dev, "%s is here. Driver version REV018\n", __func__);
• sma1303 = devm_kzalloc(&client->dev, sizeof(struct sma1303_priv),

• 					GFP_KERNEL);
  

• if (!sma1303)

• return -ENOMEM;
  

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

• ret = PTR_ERR(sma1303->regmap);
  

• dev_err(&client->dev,
  

• 	"Failed to allocate register map: %d\n", ret);
  

• devm_kfree(&client->dev, sma1303);
  

why use devm_ then?

• return ret;
  

• }
• if (np) {

• if (!of_property_read_u32(np, "tdm-slot-rx", &value)) {
  

• 	dev_info(&client->dev,
  

• 		"tdm slot rx is '%d' from DT\n", value);
  

• 	sma1303->tdm_slot_rx = value;
  

• } else {
  

• 	dev_info(&client->dev,
  

• 		"Default setting of tdm slot rx is '0'\n");
  

• 	sma1303->tdm_slot_rx = 0;
  

• }
  

• if (!of_property_read_u32(np, "tdm-slot-tx", &value)) {
  

• 	dev_info(&client->dev,
  

• 		"tdm slot tx is '%d' from DT\n", value);
  

• 	sma1303->tdm_slot_tx = value;
  

• } else {
  

• 	dev_info(&client->dev,
  

• 		"Default setting of tdm slot tx is '0'\n");
  

• 	sma1303->tdm_slot_tx = 0;
  

• }
  

• if (!of_property_read_u32(np, "sys-clk-id", &value)) {
  

• 	switch (value) {
  

• 	case SMA1303_EXTERNAL_CLOCK_19_2:
  

• 	case SMA1303_EXTERNAL_CLOCK_24_576:
  

• 	case SMA1303_PLL_CLKIN_MCLK:
  

• 		dev_info(&client->dev, "MCLK is not supported\n");
  

• 		break;
  

• 	case SMA1303_PLL_CLKIN_BCLK:
  

• 		dev_info(&client->dev,
  

• 		"Take an BCLK(SCK) and covert it to an internal PLL for use\n");
  

• 		break;
  

• 	default:
  

• 		dev_err(&client->dev,
  

• 			"Invalid sys-clk-id: %d\n", value);
  

• 		return -EINVAL;
  

• 	}
  

• 	sma1303->sys_clk_id = value;
  

• } else {
  

• 	dev_info(&client->dev, "Use the internal PLL clock by default\n");
  

• 	sma1303->sys_clk_id = SMA1303_PLL_CLKIN_BCLK;
  

• }
  

• } else {

• dev_err(&client->dev,
  

• 	"device node initialization error\n");
  

• devm_kfree(&client->dev, sma1303);
  

• return -ENODEV;
  

• }
• ret = regmap_read(sma1303->regmap,

• SMA1303_FF_DEVICE_INDEX, &device_info);
  

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

• dev_err(&client->dev, "device initialization error (%d 0x%02X)",
  

• 		ret, device_info);
  

• }
• dev_info(&client->dev, "chip version 0x%02X\n", device_info);
• sma1303->last_over_temp = 0xC0;
• sma1303->last_ocp_val = 0x0A;
• sma1303->tsdw_cnt = 0;
• sma1303->init_vol = 0x31;
• sma1303->cur_vol = sma1303->init_vol;
• sma1303->last_bclk = 0;
• INIT_DELAYED_WORK(&sma1303->check_fault_work,

• sma1303_check_fault_worker);
  

• mutex_init(&sma1303->lock);
• sma1303->check_fault_period = CHECK_PERIOD_TIME;
• sma1303->devtype = id->driver_data;
• sma1303->dev = &client->dev;
• sma1303->kobj = &client->dev.kobj;
• i2c_set_clientdata(client, sma1303);
• sma1303->amp_mode = ONE_CHIP_SOLUTION;
• sma1303->usage_status = true;
• sma1303->amp_power_status = false;
• sma1303->check_fault_status = true;
• sma1303->pll_matches = sma1303_pll_matches;
• sma1303->num_of_pll_matches =

• ARRAY_SIZE(sma1303_pll_matches);
  

• ret = devm_snd_soc_register_component(&client->dev,

• 	&sma1303_component, sma1303_dai, 1);
  

• if (ret) {

• dev_err(&client->dev, "Failed to register component");
  

• snd_soc_unregister_component(&client->dev);
  

no, this is not needed if you use devm_

• if (sma1303)
  

• 	devm_kfree(&client->dev, sma1303);
  

• return ret;
  

• }
• sma1303->attr_grp = &sma1303_attr_group;
• ret = sysfs_create_group(sma1303->kobj, sma1303->attr_grp);
• if (ret) {

• dev_err(&client->dev,
  

• 	"failed to create attribute group [%d]\n", ret);
  

• sma1303->attr_grp = NULL;
  

• }

not clear what you are trying to do with sysfs?

• return ret;

+}

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

• struct sma1303_priv *sma1303 =

• (struct sma1303_priv *) i2c_get_clientdata(client);
  

• cancel_delayed_work_sync(&sma1303->check_fault_work);
• snd_soc_unregister_component(&client->dev);

not needed when you use devm_snd_soc_register_component ?

• if (sma1303)

• devm_kfree(&client->dev, sma1303);
  

is this needed? if you use devm_ the memory will be released when the remove() completes.

• return 0;

+}

+static const struct i2c_device_id sma1303_i2c_id[] = {

• {"sma1303", 0},
• {}

+}; +MODULE_DEVICE_TABLE(i2c, sma1303_i2c_id);

+static const struct of_device_id sma1303_of_match[] = {

• { .compatible = "irondevice,sma1303", },
• { }

+}; +MODULE_DEVICE_TABLE(of, sma1303_of_match);

+static struct i2c_driver sma1303_i2c_driver = {

• .driver = {

• .name = "sma1303",
  

• .of_match_table = sma1303_of_match,
  

• },
• .probe = sma1303_i2c_probe,
• .remove = sma1303_i2c_remove,
• .id_table = sma1303_i2c_id,

+};

+static int __init sma1303_init(void) +{

• int ret;
• ret = i2c_add_driver(&sma1303_i2c_driver);
• if (ret)

• pr_err("Failed to register sma1303 I2C driver: %d\n", ret);
  

• return ret;

+}

+static void __exit sma1303_exit(void) +{

• i2c_del_driver(&sma1303_i2c_driver);

+}

+module_init(sma1303_init); +module_exit(sma1303_exit);

use module_i2c_driver() ?

+MODULE_DESCRIPTION("ALSA SoC SMA1303 driver"); +MODULE_AUTHOR("Gyuhwa Park, gyuhwa.park@irondevice.com"); +MODULE_AUTHOR("Kiseok Jo, kiseok.jo@irondevice.com"); +MODULE_LICENSE("GPL v2");

"GPL" is equivalent.

diff --git a/sound/soc/codecs/sma1303.h b/sound/soc/codecs/sma1303.h new file mode 100644 index 000000000000..d1fa2acaba85 --- /dev/null +++ b/sound/soc/codecs/sma1303.h @@ -0,0 +1,609 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later

• • sma1303.h -- sma1303 ALSA SoC Audio driver
• • Copyright 2022 Iron Device Corporation
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 as
• • published by the Free Software Foundation.

same, this paragraph is not needed

+/* SMA1303 Registers Bit Fields */

use prefixes for all definitions below?

+/* SYSTEM_CTRL : 0x00 */ +#define RESETBYI2C_MASK (1<<1) +#define RESETBYI2C_NORMAL (0<<1) +#define RESETBYI2C_RESET (1<<1)

+#define POWER_MASK (1<<0) +#define POWER_OFF (0<<0) +#define POWER_ON (1<<0)