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

[Mark Brown]

On Mon, Jan 09, 2023 at 06:14:01PM +0900, Kiseok Jo wrote:

> The Iron Device SMA1303 is a boosted Class-D audio amplifier.

This looks pretty good now, there's some things that need fixing below
but nothing too huge.

> +		result = true;
> +		break;
> +	default:
> +		result = false;
> +	}

Please put the break statements in for all cases, even the last one.

> +void sma1303_set_callback_func(struct callback_ops ops)
> +{
> +	if (ops.set_i2c_err)
> +		gCallback.set_i2c_err = ops.set_i2c_err;
> +}
> +EXPORT_SYMBOL(sma1303_set_callback_func);

ASoC symbols should be _GPL, and variables shouldn't use hungarian
notation, but in any case this callback looks very questionable - why is
it needed?  Looking at the uses...

> +static int sma1303_regmap_write(struct sma1303_priv *sma1303,
> +				unsigned int reg, unsigned int val)
> +{
> +	int ret = 0;
> +	int cnt = sma1303->retry_cnt;
> +
> +	while (cnt--) {
> +		ret = regmap_write(sma1303->regmap, reg, val);
> +		if (ret < 0) {
> +			dev_err(sma1303->dev,
> +					"Failed to write [0x%02X]\n", reg);
> +			if (gCallback.set_i2c_err)
> +				gCallback.set_i2c_err(sma1303->dev, ret);
> +		} else
> +			break;
> +	}
> +	return ret;

...this isn't something we do in other drivers, not just the callback
but the whole retry mechanism.  Is it really the device itself that's
this unstable, the callback suggests it might be the board?  It feels
like if this is needed it'd fit better in regmap rather than wrapping
things in the driver.

> +static int sma1303_force_mute_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);
> +
> +	ucontrol->value.integer.value[0] = (int)sma1303->force_mute_status;
> +	dev_info(sma1303->dev, "%s : Force Mute %s\n", __func__,
> +			sma1303->force_mute_status ? "ON" : "OFF");

If you must add logging use dev_dbg() to avoid spamming the console,
same for most of the other logging at dev_info().

> +static int sma1303_force_mute_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);
> +
> +	sma1303->force_mute_status = (bool)ucontrol->value.integer.value[0];
> +	dev_info(sma1303->dev, "%s : Force Mute %s\n", __func__,
> +			sma1303->force_mute_status ? "ON" : "OFF");
> +
> +	return 0;
> +}

This (and the other controls) should return 1 if the value changed so
events are generated, the mixer-test selftest will spot this and other
errors for you.

> +static int sma1303_postscaler_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 val, ret;
> +
> +	ret = sma1303_regmap_read(sma1303, SMA1303_90_POSTSCALER, &val);
> +	ucontrol->value.integer.value[0] = (val & 0x7E) >> 1;
> +
> +	return ret;
> +}

Here we get with a mask of 0x7e...

> +static int sma1303_postscaler_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, sel = (int)ucontrol->value.integer.value[0];
> +
> +	ret = sma1303_regmap_update_bits(sma1303,
> +			SMA1303_90_POSTSCALER, 0x70, (sel << 1));

...but put with a mask of 0x70.  What's going on with lower bits there?

> +	if (!(sma1303->amp_power_status)) {
> +		dev_info(component->dev, "%s : %s\n",
> +			__func__, "Already AMP Shutdown");
> +		return ret;
> +	}
> +
> +	cancel_delayed_work_sync(&sma1303->check_fault_work);
> +
> +	msleep(55);
> +

That sleep looks odd - what are we delaying after?  

> +static int sma1303_power_event(struct snd_soc_dapm_widget *w,
> +		struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component =
> +		snd_soc_dapm_to_component(w->dapm);
> +	struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
> +	int ret = 0;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		dev_info(sma1303->dev,
> +			"%s : SND_SOC_DAPM_POST_PMU\n", __func__);
> +		ret = sma1303_startup(component);
> +		break;
> +	case SND_SOC_DAPM_PRE_PMD:
> +		dev_info(sma1303->dev,
> +			"%s : SND_SOC_DAPM_PRE_PMD\n", __func__);
> +		ret = sma1303_shutdown(component);
> +		break;
> +	}
> +	return ret;
> +}

If this is done using DAPM then it's a bit concerning that you need the
amp_enabled checks in your startup() and shutdown() functions, DAPM
should refcount appropriately.  TBH I'd just inline those functions,
they are small and only called from here.  I'd also rename this to have
something about it being for the speaker/amplifier in the function name,
it looked like it was a whole CODEC thing.

> +	SOC_SINGLE_BOOL_EXT("Force Mute", 0,
> +		sma1303_force_mute_get, sma1303_force_mute_put),

Simple on/off controls should have Switch at the end of the name -
mixer-test will spot that one too.

> +	for (index = 0; index < ARRAY_SIZE(sma1303_snd_controls); index++) {
> +		sma1303_controls[index] = sma1303_snd_controls[index];
> +		name[index] = devm_kzalloc(sma1303->dev,
> +				MAX_CONTROL_NAME, GFP_KERNEL);
> +		size = strlen(sma1303_snd_controls[index].name)
> +			+ strlen(sma1303->dev->driver->name);

No need to add the driver name or anything here, the core has support
for allowing boards to add prefixes to all the control names if there's
a need to avoid naming clashes which allows things to be more user
friendly and supports more than one of a given device on a board.  See
name_prefix.

> +static int sma1303_dai_mute(struct snd_soc_dai *dai, int mute, int stream)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
> +	int ret = 0;
> +
> +	if (stream == SNDRV_PCM_STREAM_CAPTURE)
> +		return ret;
> +
> +	if (!(sma1303->amp_power_status)) {
> +		dev_info(component->dev, "%s : %s\n",
> +			__func__, "Already AMP Shutdown");
> +		return ret;
> +	}
> +
> +	if (mute) {
> +		dev_info(component->dev, "%s : %s\n", __func__, "MUTE");
> +
> +		ret += sma1303_regmap_update_bits(sma1303,
> +				SMA1303_0E_MUTE_VOL_CTRL,
> +				SMA1303_SPK_MUTE_MASK,
> +				SMA1303_SPK_MUTE);
> +	} else {
> +		if (!sma1303->force_mute_status) {
> +			dev_info(component->dev, "%s : %s\n",
> +					__func__, "UNMUTE");
> +			ret += sma1303_regmap_update_bits(sma1303,
> +					SMA1303_0E_MUTE_VOL_CTRL,
> +					SMA1303_SPK_MUTE_MASK,
> +					SMA1303_SPK_UNMUTE);
> +		} else {
> +			dev_info(sma1303->dev,
> +					"%s : FORCE MUTE!!!\n", __func__);
> +		}
> +	}

If you need to shut the device down to implement mute then it's better
to just not implement it, you shouldn't emulate features in the driver
but instead let the core worry about how to handle that case.  AFAICT
this is why there's the startup/shutdown thing for the speaker amp?

> +	case SND_SOC_DAIFMT_CBS_CFS:

Use the modern names _CBC_CFC 

> +	case SND_SOC_DAIFMT_CBM_CFM:

and _CBP_CFP instead, we're trying to phase out the old defines.

> +static void sma1303_check_fault_worker(struct work_struct *work)
> +{
> +	struct sma1303_priv *sma1303 =
> +		container_of(work, struct sma1303_priv, check_fault_work.work);
> +	int ret = 0;
> +	unsigned int over_temp, ocp_val, uvlo_val;
> +
> +	mutex_lock(&sma1303->lock);
> +

It looks like this mutex is only taken in this function, is it needed?

> +static int sma1303_probe(struct snd_soc_component *component)
> +{
> +	struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
> +	struct snd_soc_dapm_context *dapm =
> +		snd_soc_component_get_dapm(component);
> +	int ret = 0;
> +
> +	ret += sma1303_add_component_controls(component);
> +
> +	snd_soc_dapm_sync(dapm);
> +
> +	ret += sma1303_regmap_write(sma1303,
> +			SMA1303_0A_SPK_VOL, sma1303->init_vol);

Just use the hardware defaults for the registers, let userspace set what
it needs to.

> +static ssize_t check_fault_period_show(struct device *dev,
> +	struct device_attribute *devattr, char *buf)
> +{
> +	struct sma1303_priv *sma1303 = dev_get_drvdata(dev);
> +	int rc;
> +
> +	rc = (int)snprintf(buf, PAGE_SIZE,
> +			"%ld\n", sma1303->check_fault_period);

Use sysfs_emit().
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20230113/b12252c8/attachment.sig>