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

[Ki-Seok Jo]

Thanks your kindly feedback.

I'm correcting the part I missed.
I'd like to take a few hints.
(I revised the other parts that I didn't mention as you told me.)


>> + * Copyright 2022 Iron Device Corporation

> Missing Copyright (c) ?

I don't know this part exactly, so when I looked it up.
Most of them use a mix of two cases. Which would be better? Using Symbol or not?

>> +
>> +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.

Could you tell me which method you usually use?
I used 'sysfs_create_group' with .


>> +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?

I added the string to put it simply for the log.


>> +		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?

I'm sorry I don't understand this meaning.
In 'regmap_read', the last of the parameters is 'unsigned int' format.
So, I've considered the two format 2bytes or 4bytes according to the complier.
And our chip has only 1 byte data of each register, so I copy the data and cast the size only one byte.
Is there anything I thought wrong?


>> +	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}

I wanted to continue even if it failed in one part. The reason is to check whether it is all a problem or only that part. If there is a problem with each part, it is set to leave a log for each part.
So the error code was combined without initializing. If I do this, it will be different from the kernel standard error code, so I would like to ask if I can do this. Or is there a better way?

>> +
>> +	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?

Using work queue, check the fault status on polling.

>> +
>> +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() ?

It's the better. I'll modify.
Is there a problem if I make a module and register it with the i2c driver like above?