Re: [PATCH v2 1/3] ASoC: sma1307: Add driver for Iron Device SMA1307

[Mark Brown <broonie@xxxxxxxxxx>]

On Tue, Sep 03, 2024 at 02:44:33PM +0900, Kiseok Jo wrote:

This looks basically fine, there's some mostly minor or stylistic things
below but nothing too major.

> +static int sma1307_regmap_write(struct sma1307_priv *sma1307,
> +				unsigned int reg, unsigned int val)
> +{
> +	int ret = 0;
> +	int cnt = sma1307->retry_cnt;
> +
> +	while (cnt--) {
> +		ret = regmap_write(sma1307->regmap, reg, val);
> +		if (ret == 0)
> +			break;
> +	}

If the hardware is actually stable we probably shouldn't bother with
these wrappers.  If they really are needed then we might want to
consider having regmap support doing retries.

> +	if (sma1307->force_mute_status == val)
> +		change = false;
> +	else {
> +		change = true;
> +		sma1307->force_mute_status = val;
> +	}

If one side of the if has {} both sides should.

> +	} else {
> +		dev_err(sma1307->dev, "%s: Invalid Control ID - %s\n",
> +			__func__, kcontrol->id.name);
> +		return -EINVAL;
> +	}

We shouldn't log errors that userspace can easily trigger like this, it
lets people DoS the logs - just return the error code (a bunch of the
other controls have this).

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

You probably don't need to store a value here, you can just always read
0 for the control.  It's how other similar one shot controls work IIRC.

> +	sma1307_regmap_update_bits(sma1307, SMA1307_00_SYSTEM_CTRL,
> +				   SMA1307_RESET_MASK, SMA1307_RESET_ON);
> +	sma1307_reset(component);

This should really generate a change notification for all the controls
with non-default values (or all the controls would be easier and
probably fine, it's not like this is going to be a particularly pretty
process for userspace whatever happens).  snd_ctl_notify() does this.

It'd also be good to have a comment about why we've got this.

> +static int sma1307_dapm_amp_enable_put(struct snd_kcontrol *kcontrol,
> +				       struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_dapm_context *dapm =
> +	    snd_soc_dapm_kcontrol_dapm(kcontrol);
> +	struct sma1307_priv *sma1307 =
> +	    snd_soc_component_get_drvdata(dapm->component);
> +	int val = (int)ucontrol->value.integer.value[0];
> +	bool change;
> +
> +	if ((val < 0) || (val > 1)) {
> +		dev_err(sma1307->dev, "%s: Out of range\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (sma1307->dapm_amp_en != val) {
> +		change = true;
> +		sma1307->dapm_amp_en = val;

I didn't manage to find what reads this value - do we need this control
at all, I'm not clear what it does?  If it's for stopping the amp from
coming on the normal approach is for the board to register a
_PIN_SWITCH() DAPM control attached to whatever the end output is for
the path, that will cause DAPM to not power anything in the output path
up.

A similar thing is true for at least the binary_mode control, I can't
see where the written value is read.

> +static void sma1307_check_fault_worker(struct work_struct *work)
> +{

> +	ret = sma1307_regmap_read(sma1307, SMA1307_FA_STATUS1, &status1_val);
> +	if (ret != 0) {
> +		dev_err(sma1307->dev,
> +			"%s: failed to read SMA1307_FA_STATUS1 : %d\n",
> +			__func__, ret);
> +		return;
> +	}
> +
> +	ret = sma1307_regmap_read(sma1307, SMA1307_FB_STATUS2, &status2_val);
> +	if (ret != 0) {
> +		dev_err(sma1307->dev,
> +			"%s: failed to read SMA1307_FB_STATUS2 : %d\n",
> +			__func__, ret);
> +		return;
> +	}

If we manage to read one of the registers should we perhaps soldier on
and try to report any errors it shows?  Probably a bit academic.

> +	if (~status1_val & SMA1307_OT1_OK_STATUS) {
> +		dev_crit(sma1307->dev,
> +			 "%s: OT1(Over Temperature Level 1)\n", __func__);
> +		if (sma1307->sw_ot1_prot) {
> +			/* Volume control (Current Volume -3dB) */
> +			if ((sma1307->cur_vol + 6) <= 0xFA)
> +				sma1307_regmap_write(sma1307,
> +						     SMA1307_0A_SPK_VOL,
> +						     sma1307->cur_vol + 6);
> +		}
> +		sma1307->tsdw_cnt++;

This is changing a user visible control so it should really generate an
event, although given that it should never happen it's not the end of
the world.  Given that a lot of other speaker drivers have a similar
setup with warning and critical temperature alerts I actually wonder if
we should consider factoring this out as a helper other things can use,
that's definitely a separate thing though and you don't need to do it
right now.

> +static DEVICE_ATTR_RW(check_fault_period);

Any reason the fault stuff isn't an ALSA control?

> +static const struct regmap_config sma_i2c_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = SMA1307_FF_DEVICE_INDEX,
> +	.readable_reg = sma1307_readable_register,
> +	.writeable_reg = sma1307_writeable_register,
> +	.volatile_reg = sma1307_volatile_register,
> +
> +	.cache_type = REGCACHE_NONE,

_NONE is the default, although given that you've described the volatile
registers I don't see why you wouldn't just enable _MAPLE.  There's
regcache_drop_region() which can be used to throw away cached values
during reset if you want to do that.  Most drivers use a cache to help
make suspend/resume easier to implement - if the device looses power you
can just write the cache contents back to it to restore most userspace
visible things.

Not doing a cache (or suspend/resume) is also OK though, it can always
be implemented when needed.

> +++ b/sound/soc/codecs/sma1307.h
> @@ -0,0 +1,454 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + * sma1307.h -- sma1307 ALSA SoC Audio driver
> + *
> + * r005,
> + *
> + * Copyright 2023 Iron Device Corporation

2024 now!

Attachment: signature.asc
Description: PGP signature