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
