Re: [PATCH v5] ASoC: tas2552: Support TI TAS2552 Amplifier
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(>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(>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
Re: [PATCH v5] ASoC: tas2552: Support TI TAS2552 Amplifier
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(>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(>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?
> +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.
> + default:
> + dev_vdbg(codec->dev, "Substream sample rate is not found\n");
> + return -EINVAL;
> + }
Better to print the rate.
> + 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.
> +
> +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()?
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.
> +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.
> + /* 0dB gain */
> + snd_soc_write(codec, TAS2552_PGA_GAIN, 0x10);
Use the hardware default, your default might not be sensible for some
other user.
> +
Re: [PATCH v5] ASoC: tas2552: Support TI TAS2552 Amplifier
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?
+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.
+ default:
+ dev_vdbg(codec-dev, Substream sample rate is not found\n);
+ return -EINVAL;
+ }
Better to print the rate.
+ 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.
+
+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()?
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.
+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.
+ /* 0dB gain */
+ snd_soc_write(codec, TAS2552_PGA_GAIN, 0x10);
Use the hardware default, your default might not be sensible for some
other user.
+ /**
+ * Data sheet indicates to write 0x0c to 0x0d during init but no
+ * additional information is given to what
Re: [PATCH v5] ASoC: tas2552: Support TI TAS2552 Amplifier
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;
+
