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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thank you for your kindly feedback.

I have some questions and answers.

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

It need for IC(Amp) issue.


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

This is not power down device. It's only make zero signal level(only mute in amp).
I removed checking power status.


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

This function is in workqueue. So, I think it can be done at the same time. 





[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux