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

[Pierre-Louis Bossart]

> 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 at irondevice.com>");
> +MODULE_AUTHOR("Kiseok Jo, <kiseok.jo at 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)
> +
>