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!