Mark Thanks for the review
On 07/07/2014 03:08 AM, Mark Brown wrote:
On Thu, Jul 03, 2014 at 11:24:35AM -0500, Dan Murphy wrote:
+static int tas2552_power(struct tas2552_data *data, u8 power) +{
- int ret = 0;
- mutex_lock(&data->mutex);
- if (power) {
if (data->enable_gpio)gpiod_set_value(data->enable_gpio, 1);data->power_state = 1;- } else {
if (data->enable_gpio)gpiod_set_value(data->enable_gpio, 0);data->power_state = 0;- }
- mutex_unlock(&data->mutex);
- return ret;
+}
I don't understand this function. It appears to be the only place where either power_state or mutex is used so it's just adding some wrapping around setting the GPIO value which doesn't seem like it's doing much. Why are we tracking power_state?
This function and mutex are artifacts from the development and probably can be consolidated into the runtime PM calls.
+static void tas2552_sw_shutdown(struct tas2552_data *tas_data, int sw_shutdown) +{
- u8 cfg1_reg = 0x0;
- if (sw_shutdown)
cfg1_reg |= TAS2552_SWS_MASK;- else
cfg1_reg &= ~TAS2552_SWS_MASK;- snd_soc_update_bits(tas_data->codec, TAS2552_CFG_1,
TAS2552_SWS_MASK, cfg1_reg);Given that you're using _update_bits() clearing the bits in a register that was just initialised to zero doesn't make a huge amount of sense.
This was an artifact from RFC in which I was not using the snd_soc functions. I can remove the initialization of the variable.
- default:
dev_vdbg(codec->dev, "Substream sample rate is not found\n");return -EINVAL;- }
Better to print the rate.
OK
- pm_runtime_get_sync(codec->dev);
- snd_soc_update_bits(codec, TAS2552_CFG_3, TAS2552_WCLK_MASK, wclk_reg);
- pm_runtime_put(codec->dev);
This seems really strange - why is the device being powered up to just set a bit and then potentially powered down immediately? I'd expect to just update the cache if the device is not active. You're also not checking that the power up worked.
I wanted to make sure that the device was on. I totally forgot that the device was using regmap and the values are cached when the device is not on.
I can remove the get/put around the update calls.
+static int tas2552_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt) +{
- u8 serial_format;
- u8 serial_control_mask = 0x00;
- if (fmt & SND_SOC_DAIFMT_FORMAT_MASK)
serial_control_mask |= TAS2552_DATA_FORMAT_MASK;- if (serial_control_mask) {
pm_runtime_get_sync(codec->dev);snd_soc_update_bits(codec, TAS2552_SER_CTRL_1, serial_control_mask,serial_format);pm_runtime_put(codec->dev);- }
This seems broken - if the format mask ever gets set then it won't be cleared since we only do an update_bits() if the bit is being set. Why isn't the driver just doing an _update_bits()?
I do not understand what this statement means.
Are you saying snd_soc_update_bits will not clear the bit if the bit mask is set appropriately?
The comments about runtime PM also apply, they applies throughout the driver.
+static int tas2552_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
unsigned int freq, int dir)+{
- struct snd_soc_codec *codec = dai->codec;
- /* Fill in the PLL control registers for J & D
* PLL_CLK = (.5 * freq * J.D) / 2^p* Need to fill in J and D here based on incoming freq*/- pm_runtime_get_sync(codec->dev);
- snd_soc_update_bits(codec, TAS2552_CFG_2, TAS2552_PLL_ENABLE, 0);
- snd_soc_write(codec, TAS2552_PLL_CTRL_1, 0x10);
- snd_soc_write(codec, TAS2552_PLL_CTRL_2, 0x00);
- snd_soc_write(codec, TAS2552_PLL_CTRL_3, 0x00);
- snd_soc_update_bits(codec, TAS2552_CFG_2, TAS2552_PLL_ENABLE,
TAS2552_PLL_ENABLE);- pm_runtime_put(codec->dev);
This makes no sense at all - please look at what other drivers are doing with set_sysclk(). It should be used to get information about how the device is clocked.
+static int tas2552_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- struct snd_soc_codec *codec = dai->codec;
- pm_runtime_get_sync(codec->dev);
- /* Turn on Class D amplifier */
- snd_soc_update_bits(codec, TAS2552_CFG_2, TAS2552_CLASSD_EN_MASK,
TAS2552_CLASSD_EN);This should be done using DAPM.
You mentioned this before. Isn't the startup part of the DAPM calls?
+static int tas2552_codec_probe(struct snd_soc_codec *codec) +{
- struct tas2552_data *tas2552 = snd_soc_codec_get_drvdata(codec);
- int ret;
- tas2552_power(tas2552, 1);
- tas2552_sw_shutdown(tas2552, 0);
- pm_runtime_set_active(codec->dev);
- pm_runtime_set_autosuspend_delay(codec->dev, 1000);
- pm_runtime_use_autosuspend(codec->dev);
- pm_runtime_enable(codec->dev);
- pm_runtime_mark_last_busy(codec->dev);
- pm_runtime_put_sync_autosuspend(codec->dev);
This should all be done at the device level probe.
I went back and forth on this in my head whether this should be in the device probe of the codec probe. I can move them to device probe.
- /* 0dB gain */
- snd_soc_write(codec, TAS2552_PGA_GAIN, 0x10);
Use the hardware default, your default might not be sensible for some other user.
So would other users just patch their own code? I can leave it to the default value. This is just what the hardware was tested with.
- /**
* Data sheet indicates to write 0x0c to 0x0d during init but no* additional information is given to what it means.*/- snd_soc_write(codec, TAS2552_LIMIT_LVL_CTRL, 0x0c);
- /**
* Data sheet indicates to write 0x20 to 0x0e during init but no* additional information is given to what it means.*/- snd_soc_write(codec, TAS2552_LIMIT_RATE_HYS, 0x20);
Use a regmap patch for these.
OK. I see that can be used for undocumented registers.
+static int tas2552_suspend(struct snd_soc_codec *codec) +{
- struct tas2552_data *tas2552 = snd_soc_codec_get_drvdata(codec);
- int ret;
- pm_runtime_put(codec->dev);
This won't work. Let the frameworks worry about this, or check if the device is already runtime suspended and then call your runtime suspend operation directly.
OK will fix
+static const struct i2c_device_id tas2552_id[] = {
- { "tas2552-codec", 0 },
- { }
+}; +MODULE_DEVICE_TABLE(i2c, tas2552_id);
No -codec, look at what other drivers do.
OK will remove.