Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Steven Eckhoff]

On Wed, Dec 13, 2017 at 10:47:32AM +, Mark Brown wrote:
> The things the driver is doing automatically to mute the DACs and ADCs
> should be moved to user control if they're not suitable for doing
> automatically.
> 
The mutes can work normally without the PLL stuff. I have removed the PLL
stuff from the next version and have added a DAPM widget to handle the
PLL power up/down.

> Please delete unneeded context from mails when replying.  Doing this
> makes it much easier to find your reply in the message, helping ensure
> it won't be missed by people scrolling through the irrelevant quoted
> material.
I apologize again. I realized that I had done that after it was too
late. 

Thanks for the reviews and all the help. I believe this driver has improved
through this process.

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Steven Eckhoff]

On Wed, Dec 13, 2017 at 10:47:32AM +, Mark Brown wrote:
> The things the driver is doing automatically to mute the DACs and ADCs
> should be moved to user control if they're not suitable for doing
> automatically.
> 
The mutes can work normally without the PLL stuff. I have removed the PLL
stuff from the next version and have added a DAPM widget to handle the
PLL power up/down.

> Please delete unneeded context from mails when replying.  Doing this
> makes it much easier to find your reply in the message, helping ensure
> it won't be missed by people scrolling through the irrelevant quoted
> material.
I apologize again. I realized that I had done that after it was too
late. 

Thanks for the reviews and all the help. I believe this driver has improved
through this process.

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Mark Brown]

On Tue, Dec 12, 2017 at 04:51:35PM -0600, Steven Eckhoff wrote:

> Also I am not sure what you mean with the mute controls. Could you
> elaborate more on this?

The things the driver is doing automatically to mute the DACs and ADCs
should be moved to user control if they're not suitable for doing
automatically.

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.


signature.asc
Description: PGP signature

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Mark Brown]

On Tue, Dec 12, 2017 at 04:51:35PM -0600, Steven Eckhoff wrote:

> Also I am not sure what you mean with the mute controls. Could you
> elaborate more on this?

The things the driver is doing automatically to mute the DACs and ADCs
should be moved to user control if they're not suitable for doing
automatically.

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.


signature.asc
Description: PGP signature

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Steven Eckhoff]

On Tue, Dec 12, 2017 at 04:51:35PM -0600, Steven Eckhoff wrote:

> > > > +static int tscs42xx_mute_stream(struct snd_soc_dai *dai, int mute, int 
> > > > stream)
> > > > +{
> > > > +   struct snd_soc_codec *codec = dai->codec;
> > > > +   int ret;
> > > > +
> > > > +   if (mute)
> > > > +   if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > > > +   ret = dac_mute(codec);
> > > > +   else
> > > > +   ret = adc_mute(codec);
> > > > +   else
> > > > +   if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > > > +   ret = dac_unmute(codec);
> > > > +   else
> > > > +   ret = adc_unmute(codec);
> > > > +
> > > > +   return ret;
> > > > +}
> > > 
> > > All these mute functions also shut down the PLLs which since...
> > > 
> > This is a bit funky since it looks like if you unmuted the dac and then
> > muted the adc that the PLLs would be powered off on the playback stream,
> > but the logic in the chip is a bit funky in that it wont allow this unless
> > the playback interface is also powered off.
> > 
> > This should definitely be fixed since it is confusing. The PLL
> > powered up/down stuff will be removed from the mute functions in the
> > next version.
> > 
> > What are your thoughts on turning the PLL into a DAPM widget and using
> > the event callback to power up/down the PLLs? I have tested this and it
> > seems to work fine.
> > 
> > > > +   switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > > > +   case SND_SOC_DAIFMT_CBM_CFM:
> > > > +   ret = snd_soc_update_bits(codec, R_AIC1, RM_AIC1_MS,
> > > > +   RV_AIC1_MS_MASTER);
> > > > +   if (ret < 0)
> > > > +   dev_err(codec->dev,
> > > > +   "Failed to set codec DAI master 
> > > > (%d)\n", ret);
> > > > +   else
> > > > +   ret = 0;
> > > > +   break;
> > > > +   default:
> > > 
> > > ...we only support the CODEC being the clock master seems like it might
> > > mean we stop clocking the DAI?  If that's the case it's better to just
> > > not have the mute control and allow the user to just control these as
> > > normal mutes.
> > > 
> > I am going to put slave mode support in, but I may need some time to
> > test how it works.
> > 
> Okay I had to refresh my memory on why slave was not being supported.
> Our slave mode needs the audio clocks to stay active to avoid pops. This
> has something to do with how the charge pump is setup. In master mode
> this is not an issue. I will keep the slave mode support out of the next
> version.
> 
> Also I am not sure what you mean with the mute controls. Could you
> elaborate more on this?
> 
Okay I belive I know what this was all about. 

Were you asking why the mute functions are powering up/down the PLLs?

The answer is that is was a vestige of the very first version of the
driver where I was told I needed to power PLLs before unmuting and
muting before powering them down, which is correct I just didn't have
them in the correct callback. In the next version I have moved the
powering of the PLLs into the event call back of a DAPM supply widget.
If there is a better way I can implement that.

The previous comment on slave mode is still valid, so the next version
will only suppor the codec in master mode.

Sorry for any confusion that I may have caused.

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Steven Eckhoff]

On Tue, Dec 12, 2017 at 04:51:35PM -0600, Steven Eckhoff wrote:

> > > > +static int tscs42xx_mute_stream(struct snd_soc_dai *dai, int mute, int 
> > > > stream)
> > > > +{
> > > > +   struct snd_soc_codec *codec = dai->codec;
> > > > +   int ret;
> > > > +
> > > > +   if (mute)
> > > > +   if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > > > +   ret = dac_mute(codec);
> > > > +   else
> > > > +   ret = adc_mute(codec);
> > > > +   else
> > > > +   if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > > > +   ret = dac_unmute(codec);
> > > > +   else
> > > > +   ret = adc_unmute(codec);
> > > > +
> > > > +   return ret;
> > > > +}
> > > 
> > > All these mute functions also shut down the PLLs which since...
> > > 
> > This is a bit funky since it looks like if you unmuted the dac and then
> > muted the adc that the PLLs would be powered off on the playback stream,
> > but the logic in the chip is a bit funky in that it wont allow this unless
> > the playback interface is also powered off.
> > 
> > This should definitely be fixed since it is confusing. The PLL
> > powered up/down stuff will be removed from the mute functions in the
> > next version.
> > 
> > What are your thoughts on turning the PLL into a DAPM widget and using
> > the event callback to power up/down the PLLs? I have tested this and it
> > seems to work fine.
> > 
> > > > +   switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > > > +   case SND_SOC_DAIFMT_CBM_CFM:
> > > > +   ret = snd_soc_update_bits(codec, R_AIC1, RM_AIC1_MS,
> > > > +   RV_AIC1_MS_MASTER);
> > > > +   if (ret < 0)
> > > > +   dev_err(codec->dev,
> > > > +   "Failed to set codec DAI master 
> > > > (%d)\n", ret);
> > > > +   else
> > > > +   ret = 0;
> > > > +   break;
> > > > +   default:
> > > 
> > > ...we only support the CODEC being the clock master seems like it might
> > > mean we stop clocking the DAI?  If that's the case it's better to just
> > > not have the mute control and allow the user to just control these as
> > > normal mutes.
> > > 
> > I am going to put slave mode support in, but I may need some time to
> > test how it works.
> > 
> Okay I had to refresh my memory on why slave was not being supported.
> Our slave mode needs the audio clocks to stay active to avoid pops. This
> has something to do with how the charge pump is setup. In master mode
> this is not an issue. I will keep the slave mode support out of the next
> version.
> 
> Also I am not sure what you mean with the mute controls. Could you
> elaborate more on this?
> 
Okay I belive I know what this was all about. 

Were you asking why the mute functions are powering up/down the PLLs?

The answer is that is was a vestige of the very first version of the
driver where I was told I needed to power PLLs before unmuting and
muting before powering them down, which is correct I just didn't have
them in the correct callback. In the next version I have moved the
powering of the PLLs into the event call back of a DAPM supply widget.
If there is a better way I can implement that.

The previous comment on slave mode is still valid, so the next version
will only suppor the codec in master mode.

Sorry for any confusion that I may have caused.

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Steven Eckhoff]

On Tue, Dec 12, 2017 at 03:31:16PM -0600, Steven Eckhoff wrote:
> On Tue, Dec 12, 2017 at 04:32:54PM +, Mark Brown wrote:
> > On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> > > Currently there is no support for the TSCS42xx audio CODEC.
> > > 
> > > Add support for it.
> > > 
> > > v5 attempts to address all issues raised in the previous reviews.
> > > 
> > > Thank you to everyone who has invested their time reviewing these
> > > patches.
> > 
> > Please add inter-version changelogs and commentary like this after --- 
> > as covered in SubmittingPatches - this lets tools know they don't need
> > to end up in git.
> > 
> Again I apologize. I will fix this. I will also go over the document to
> make sure nothing else gets messed up. Also thank you for the time you
> have spent reviewing the numerous versions of this patch.
> 
> > > +  - compatible : "tscs:tscs42xx"
> > 
> > Compatible strings should be for specific devices, they may all be
> > treated identically by software now but may not be in future.
> > 
> Okay, this will be fixed in the next version.
> 
> > > + do {
> > > + ret = snd_soc_read(codec, R_DACCRSTAT);
> > > + if (ret < 0) {
> > > + dev_err(codec->dev,
> > > + "Failed to read stat (%d)\n", ret);
> > > + return ret;
> > > + }
> > > + } while (ret);
> > 
> > There should be some sort of upper bound on how many times we'll try to
> > wait for this in case the hardware fails somehow.
> > 
> I totally agree. Most of the time the DACCRSTAT should be cleared before
> the next iteration starts, so I will sleep it 20ms if it isn't
> cleared by then and error if it hasn't cleared after 20ms.
> 
> > > +static int tscs42xx_mute_stream(struct snd_soc_dai *dai, int mute, int 
> > > stream)
> > > +{
> > > + struct snd_soc_codec *codec = dai->codec;
> > > + int ret;
> > > +
> > > + if (mute)
> > > + if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > > + ret = dac_mute(codec);
> > > + else
> > > + ret = adc_mute(codec);
> > > + else
> > > + if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > > + ret = dac_unmute(codec);
> > > + else
> > > + ret = adc_unmute(codec);
> > > +
> > > + return ret;
> > > +}
> > 
> > All these mute functions also shut down the PLLs which since...
> > 
> This is a bit funky since it looks like if you unmuted the dac and then
> muted the adc that the PLLs would be powered off on the playback stream,
> but the logic in the chip is a bit funky in that it wont allow this unless
> the playback interface is also powered off.
> 
> This should definitely be fixed since it is confusing. The PLL
> powered up/down stuff will be removed from the mute functions in the
> next version.
> 
> What are your thoughts on turning the PLL into a DAPM widget and using
> the event callback to power up/down the PLLs? I have tested this and it
> seems to work fine.
> 
> > > + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > > + case SND_SOC_DAIFMT_CBM_CFM:
> > > + ret = snd_soc_update_bits(codec, R_AIC1, RM_AIC1_MS,
> > > + RV_AIC1_MS_MASTER);
> > > + if (ret < 0)
> > > + dev_err(codec->dev,
> > > + "Failed to set codec DAI master (%d)\n", ret);
> > > + else
> > > + ret = 0;
> > > + break;
> > > + default:
> > 
> > ...we only support the CODEC being the clock master seems like it might
> > mean we stop clocking the DAI?  If that's the case it's better to just
> > not have the mute control and allow the user to just control these as
> > normal mutes.
> > 
> I am going to put slave mode support in, but I may need some time to
> test how it works.
> 
Okay I had to refresh my memory on why slave was not being supported.
Our slave mode needs the audio clocks to stay active to avoid pops. This
has something to do with how the charge pump is setup. In master mode
this is not an issue. I will keep the slave mode support out of the next
version.

Also I am not sure what you mean with the mute controls. Could you
elaborate more on this?

> > > +static int tscs42xx_probe(struct snd_soc_codec *codec)
> > > +{
> > > + int i;
> > > + int ret;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(r_inits); ++i) {
> > > + ret = snd_soc_write(codec, r_inits[i].reg, r_inits[i].def);
> > > + if (ret < 0) {
> > > + dev_err(codec->dev,
> > > + "Failed to write codec defaults (%d)\n", ret);
> > > + return ret;
> > > + }
> > > + }
> > 
> > I'd expect the driver to just reset the CODEC (it appears to have that
> > feature) and the regmap.
> > 
> These init values were meant to be driver defaults that differed from
> the device defaults. In hindsight this was a bad idea. I am removing
> them in the next 

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Steven Eckhoff]

On Tue, Dec 12, 2017 at 03:31:16PM -0600, Steven Eckhoff wrote:
> On Tue, Dec 12, 2017 at 04:32:54PM +, Mark Brown wrote:
> > On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> > > Currently there is no support for the TSCS42xx audio CODEC.
> > > 
> > > Add support for it.
> > > 
> > > v5 attempts to address all issues raised in the previous reviews.
> > > 
> > > Thank you to everyone who has invested their time reviewing these
> > > patches.
> > 
> > Please add inter-version changelogs and commentary like this after --- 
> > as covered in SubmittingPatches - this lets tools know they don't need
> > to end up in git.
> > 
> Again I apologize. I will fix this. I will also go over the document to
> make sure nothing else gets messed up. Also thank you for the time you
> have spent reviewing the numerous versions of this patch.
> 
> > > +  - compatible : "tscs:tscs42xx"
> > 
> > Compatible strings should be for specific devices, they may all be
> > treated identically by software now but may not be in future.
> > 
> Okay, this will be fixed in the next version.
> 
> > > + do {
> > > + ret = snd_soc_read(codec, R_DACCRSTAT);
> > > + if (ret < 0) {
> > > + dev_err(codec->dev,
> > > + "Failed to read stat (%d)\n", ret);
> > > + return ret;
> > > + }
> > > + } while (ret);
> > 
> > There should be some sort of upper bound on how many times we'll try to
> > wait for this in case the hardware fails somehow.
> > 
> I totally agree. Most of the time the DACCRSTAT should be cleared before
> the next iteration starts, so I will sleep it 20ms if it isn't
> cleared by then and error if it hasn't cleared after 20ms.
> 
> > > +static int tscs42xx_mute_stream(struct snd_soc_dai *dai, int mute, int 
> > > stream)
> > > +{
> > > + struct snd_soc_codec *codec = dai->codec;
> > > + int ret;
> > > +
> > > + if (mute)
> > > + if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > > + ret = dac_mute(codec);
> > > + else
> > > + ret = adc_mute(codec);
> > > + else
> > > + if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > > + ret = dac_unmute(codec);
> > > + else
> > > + ret = adc_unmute(codec);
> > > +
> > > + return ret;
> > > +}
> > 
> > All these mute functions also shut down the PLLs which since...
> > 
> This is a bit funky since it looks like if you unmuted the dac and then
> muted the adc that the PLLs would be powered off on the playback stream,
> but the logic in the chip is a bit funky in that it wont allow this unless
> the playback interface is also powered off.
> 
> This should definitely be fixed since it is confusing. The PLL
> powered up/down stuff will be removed from the mute functions in the
> next version.
> 
> What are your thoughts on turning the PLL into a DAPM widget and using
> the event callback to power up/down the PLLs? I have tested this and it
> seems to work fine.
> 
> > > + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > > + case SND_SOC_DAIFMT_CBM_CFM:
> > > + ret = snd_soc_update_bits(codec, R_AIC1, RM_AIC1_MS,
> > > + RV_AIC1_MS_MASTER);
> > > + if (ret < 0)
> > > + dev_err(codec->dev,
> > > + "Failed to set codec DAI master (%d)\n", ret);
> > > + else
> > > + ret = 0;
> > > + break;
> > > + default:
> > 
> > ...we only support the CODEC being the clock master seems like it might
> > mean we stop clocking the DAI?  If that's the case it's better to just
> > not have the mute control and allow the user to just control these as
> > normal mutes.
> > 
> I am going to put slave mode support in, but I may need some time to
> test how it works.
> 
Okay I had to refresh my memory on why slave was not being supported.
Our slave mode needs the audio clocks to stay active to avoid pops. This
has something to do with how the charge pump is setup. In master mode
this is not an issue. I will keep the slave mode support out of the next
version.

Also I am not sure what you mean with the mute controls. Could you
elaborate more on this?

> > > +static int tscs42xx_probe(struct snd_soc_codec *codec)
> > > +{
> > > + int i;
> > > + int ret;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(r_inits); ++i) {
> > > + ret = snd_soc_write(codec, r_inits[i].reg, r_inits[i].def);
> > > + if (ret < 0) {
> > > + dev_err(codec->dev,
> > > + "Failed to write codec defaults (%d)\n", ret);
> > > + return ret;
> > > + }
> > > + }
> > 
> > I'd expect the driver to just reset the CODEC (it appears to have that
> > feature) and the regmap.
> > 
> These init values were meant to be driver defaults that differed from
> the device defaults. In hindsight this was a bad idea. I am removing
> them in the next 

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Steven Eckhoff]

On Tue, Dec 12, 2017 at 04:32:54PM +, Mark Brown wrote:
> On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> > Currently there is no support for the TSCS42xx audio CODEC.
> > 
> > Add support for it.
> > 
> > v5 attempts to address all issues raised in the previous reviews.
> > 
> > Thank you to everyone who has invested their time reviewing these
> > patches.
> 
> Please add inter-version changelogs and commentary like this after --- 
> as covered in SubmittingPatches - this lets tools know they don't need
> to end up in git.
> 
Again I apologize. I will fix this. I will also go over the document to
make sure nothing else gets messed up. Also thank you for the time you
have spent reviewing the numerous versions of this patch.

> > +  - compatible : "tscs:tscs42xx"
> 
> Compatible strings should be for specific devices, they may all be
> treated identically by software now but may not be in future.
> 
Okay, this will be fixed in the next version.

> > +   do {
> > +   ret = snd_soc_read(codec, R_DACCRSTAT);
> > +   if (ret < 0) {
> > +   dev_err(codec->dev,
> > +   "Failed to read stat (%d)\n", ret);
> > +   return ret;
> > +   }
> > +   } while (ret);
> 
> There should be some sort of upper bound on how many times we'll try to
> wait for this in case the hardware fails somehow.
> 
I totally agree. Most of the time the DACCRSTAT should be cleared before
the next iteration starts, so I will sleep it 20ms if it isn't
cleared by then and error if it hasn't cleared after 20ms.

> > +static int tscs42xx_mute_stream(struct snd_soc_dai *dai, int mute, int 
> > stream)
> > +{
> > +   struct snd_soc_codec *codec = dai->codec;
> > +   int ret;
> > +
> > +   if (mute)
> > +   if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > +   ret = dac_mute(codec);
> > +   else
> > +   ret = adc_mute(codec);
> > +   else
> > +   if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > +   ret = dac_unmute(codec);
> > +   else
> > +   ret = adc_unmute(codec);
> > +
> > +   return ret;
> > +}
> 
> All these mute functions also shut down the PLLs which since...
> 
This is a bit funky since it looks like if you unmuted the dac and then
muted the adc that the PLLs would be powered off on the playback stream,
but the logic in the chip is a bit funky in that it wont allow this unless
the playback interface is also powered off.

This should definitely be fixed since it is confusing. The PLL
powered up/down stuff will be removed from the mute functions in the
next version.

What are your thoughts on turning the PLL into a DAPM widget and using
the event callback to power up/down the PLLs? I have tested this and it
seems to work fine.

> > +   switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > +   case SND_SOC_DAIFMT_CBM_CFM:
> > +   ret = snd_soc_update_bits(codec, R_AIC1, RM_AIC1_MS,
> > +   RV_AIC1_MS_MASTER);
> > +   if (ret < 0)
> > +   dev_err(codec->dev,
> > +   "Failed to set codec DAI master (%d)\n", ret);
> > +   else
> > +   ret = 0;
> > +   break;
> > +   default:
> 
> ...we only support the CODEC being the clock master seems like it might
> mean we stop clocking the DAI?  If that's the case it's better to just
> not have the mute control and allow the user to just control these as
> normal mutes.
> 
I am going to put slave mode support in, but I may need some time to
test how it works.

> > +static int tscs42xx_probe(struct snd_soc_codec *codec)
> > +{
> > +   int i;
> > +   int ret;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(r_inits); ++i) {
> > +   ret = snd_soc_write(codec, r_inits[i].reg, r_inits[i].def);
> > +   if (ret < 0) {
> > +   dev_err(codec->dev,
> > +   "Failed to write codec defaults (%d)\n", ret);
> > +   return ret;
> > +   }
> > +   }
> 
> I'd expect the driver to just reset the CODEC (it appears to have that
> feature) and the regmap.
> 
These init values were meant to be driver defaults that differed from
the device defaults. In hindsight this was a bad idea. I am removing
them in the next revision and will have the machine driver setup the
codec appropriately.

> > +static int tscs42xx_remove(struct snd_soc_codec *codec)
> > +{
> > +   return 0;
> > +}
> 
> Just remove empty functions.
> 
Next revision will have this removed.

> > +static const struct i2c_device_id tscs42xx_i2c_id[] = {
> > +   { "tscs42xx", 0 },
> > +   { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, tscs42xx_i2c_id);
> 
> I2C IDs are better as explicit part numbers too like compatible
> strings.
I will update this in the next version.

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Steven Eckhoff]

On Tue, Dec 12, 2017 at 04:32:54PM +, Mark Brown wrote:
> On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> > Currently there is no support for the TSCS42xx audio CODEC.
> > 
> > Add support for it.
> > 
> > v5 attempts to address all issues raised in the previous reviews.
> > 
> > Thank you to everyone who has invested their time reviewing these
> > patches.
> 
> Please add inter-version changelogs and commentary like this after --- 
> as covered in SubmittingPatches - this lets tools know they don't need
> to end up in git.
> 
Again I apologize. I will fix this. I will also go over the document to
make sure nothing else gets messed up. Also thank you for the time you
have spent reviewing the numerous versions of this patch.

> > +  - compatible : "tscs:tscs42xx"
> 
> Compatible strings should be for specific devices, they may all be
> treated identically by software now but may not be in future.
> 
Okay, this will be fixed in the next version.

> > +   do {
> > +   ret = snd_soc_read(codec, R_DACCRSTAT);
> > +   if (ret < 0) {
> > +   dev_err(codec->dev,
> > +   "Failed to read stat (%d)\n", ret);
> > +   return ret;
> > +   }
> > +   } while (ret);
> 
> There should be some sort of upper bound on how many times we'll try to
> wait for this in case the hardware fails somehow.
> 
I totally agree. Most of the time the DACCRSTAT should be cleared before
the next iteration starts, so I will sleep it 20ms if it isn't
cleared by then and error if it hasn't cleared after 20ms.

> > +static int tscs42xx_mute_stream(struct snd_soc_dai *dai, int mute, int 
> > stream)
> > +{
> > +   struct snd_soc_codec *codec = dai->codec;
> > +   int ret;
> > +
> > +   if (mute)
> > +   if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > +   ret = dac_mute(codec);
> > +   else
> > +   ret = adc_mute(codec);
> > +   else
> > +   if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> > +   ret = dac_unmute(codec);
> > +   else
> > +   ret = adc_unmute(codec);
> > +
> > +   return ret;
> > +}
> 
> All these mute functions also shut down the PLLs which since...
> 
This is a bit funky since it looks like if you unmuted the dac and then
muted the adc that the PLLs would be powered off on the playback stream,
but the logic in the chip is a bit funky in that it wont allow this unless
the playback interface is also powered off.

This should definitely be fixed since it is confusing. The PLL
powered up/down stuff will be removed from the mute functions in the
next version.

What are your thoughts on turning the PLL into a DAPM widget and using
the event callback to power up/down the PLLs? I have tested this and it
seems to work fine.

> > +   switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > +   case SND_SOC_DAIFMT_CBM_CFM:
> > +   ret = snd_soc_update_bits(codec, R_AIC1, RM_AIC1_MS,
> > +   RV_AIC1_MS_MASTER);
> > +   if (ret < 0)
> > +   dev_err(codec->dev,
> > +   "Failed to set codec DAI master (%d)\n", ret);
> > +   else
> > +   ret = 0;
> > +   break;
> > +   default:
> 
> ...we only support the CODEC being the clock master seems like it might
> mean we stop clocking the DAI?  If that's the case it's better to just
> not have the mute control and allow the user to just control these as
> normal mutes.
> 
I am going to put slave mode support in, but I may need some time to
test how it works.

> > +static int tscs42xx_probe(struct snd_soc_codec *codec)
> > +{
> > +   int i;
> > +   int ret;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(r_inits); ++i) {
> > +   ret = snd_soc_write(codec, r_inits[i].reg, r_inits[i].def);
> > +   if (ret < 0) {
> > +   dev_err(codec->dev,
> > +   "Failed to write codec defaults (%d)\n", ret);
> > +   return ret;
> > +   }
> > +   }
> 
> I'd expect the driver to just reset the CODEC (it appears to have that
> feature) and the regmap.
> 
These init values were meant to be driver defaults that differed from
the device defaults. In hindsight this was a bad idea. I am removing
them in the next revision and will have the machine driver setup the
codec appropriately.

> > +static int tscs42xx_remove(struct snd_soc_codec *codec)
> > +{
> > +   return 0;
> > +}
> 
> Just remove empty functions.
> 
Next revision will have this removed.

> > +static const struct i2c_device_id tscs42xx_i2c_id[] = {
> > +   { "tscs42xx", 0 },
> > +   { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, tscs42xx_i2c_id);
> 
> I2C IDs are better as explicit part numbers too like compatible
> strings.
I will update this in the next version.

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Steven Eckhoff]

On Tue, Dec 12, 2017 at 02:10:09PM +0100, Takashi Iwai wrote:
> On Tue, 12 Dec 2017 14:04:01 +0100,
> Charles Keepax wrote:
> > 
> > On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> > > Currently there is no support for the TSCS42xx audio CODEC.
> > > 
> > > Add support for it.
> > > 
> > > v5 attempts to address all issues raised in the previous reviews.
> > > 
> > > Thank you to everyone who has invested their time reviewing these
> > > patches.
> > > 
> > > Acked-by: Philippe Ombredanne 
> > > Signed-off-by: Steven Eckhoff 
> > > ---
> > > +int bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag,
> > > + unsigned int size, unsigned int __user *tlv)
> > > +{
> > > + struct tscs_dsp_ctl *ctl =
> > > + (struct tscs_dsp_ctl *)kcontrol->private_value;
> > > + struct soc_bytes_ext *params = >bytes_ext;
> > > + unsigned int count = size < params->max ? size : params->max;
> > > + int ret = -ENXIO;
> > > +
> > > + switch (op_flag) {
> > > + case SNDRV_CTL_TLV_OP_READ:
> > > + if (params->get)
> > > + ret = params->get(kcontrol, tlv, count);
> > > + break;
> > > + case SNDRV_CTL_TLV_OP_WRITE:
> > > + if (params->put)
> > > + ret = params->put(kcontrol, tlv, count);
> > > + break;
> > > + }
> > > + return ret;
> > > +}
> > 
> > Should this function be removed, I am assuming it is a hang over
> > from the earlier TLV implementation?
> 
> Yes, and bytes_ext_info() is also missing static.
> 
> 
> Takashi

Thanks for the review! it is marked static in the next version.

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Steven Eckhoff]

On Tue, Dec 12, 2017 at 02:10:09PM +0100, Takashi Iwai wrote:
> On Tue, 12 Dec 2017 14:04:01 +0100,
> Charles Keepax wrote:
> > 
> > On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> > > Currently there is no support for the TSCS42xx audio CODEC.
> > > 
> > > Add support for it.
> > > 
> > > v5 attempts to address all issues raised in the previous reviews.
> > > 
> > > Thank you to everyone who has invested their time reviewing these
> > > patches.
> > > 
> > > Acked-by: Philippe Ombredanne 
> > > Signed-off-by: Steven Eckhoff 
> > > ---
> > > +int bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag,
> > > + unsigned int size, unsigned int __user *tlv)
> > > +{
> > > + struct tscs_dsp_ctl *ctl =
> > > + (struct tscs_dsp_ctl *)kcontrol->private_value;
> > > + struct soc_bytes_ext *params = >bytes_ext;
> > > + unsigned int count = size < params->max ? size : params->max;
> > > + int ret = -ENXIO;
> > > +
> > > + switch (op_flag) {
> > > + case SNDRV_CTL_TLV_OP_READ:
> > > + if (params->get)
> > > + ret = params->get(kcontrol, tlv, count);
> > > + break;
> > > + case SNDRV_CTL_TLV_OP_WRITE:
> > > + if (params->put)
> > > + ret = params->put(kcontrol, tlv, count);
> > > + break;
> > > + }
> > > + return ret;
> > > +}
> > 
> > Should this function be removed, I am assuming it is a hang over
> > from the earlier TLV implementation?
> 
> Yes, and bytes_ext_info() is also missing static.
> 
> 
> Takashi

Thanks for the review! it is marked static in the next version.

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Steven Eckhoff]

On Tue, Dec 12, 2017 at 01:04:01PM +, Charles Keepax wrote:
> On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> > Currently there is no support for the TSCS42xx audio CODEC.
> > 
> > Add support for it.
> > 
> > v5 attempts to address all issues raised in the previous reviews.
> > 
> > Thank you to everyone who has invested their time reviewing these
> > patches.
> > 
> > Acked-by: Philippe Ombredanne 
> > Signed-off-by: Steven Eckhoff 
> > ---
> > +int bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag,
> > +   unsigned int size, unsigned int __user *tlv)
> > +{
> > +   struct tscs_dsp_ctl *ctl =
> > +   (struct tscs_dsp_ctl *)kcontrol->private_value;
> > +   struct soc_bytes_ext *params = >bytes_ext;
> > +   unsigned int count = size < params->max ? size : params->max;
> > +   int ret = -ENXIO;
> > +
> > +   switch (op_flag) {
> > +   case SNDRV_CTL_TLV_OP_READ:
> > +   if (params->get)
> > +   ret = params->get(kcontrol, tlv, count);
> > +   break;
> > +   case SNDRV_CTL_TLV_OP_WRITE:
> > +   if (params->put)
> > +   ret = params->put(kcontrol, tlv, count);
> > +   break;
> > +   }
> > +   return ret;
> > +}
> 
> Should this function be removed, I am assuming it is a hang over
> from the earlier TLV implementation?
> 
> Thanks,
> Charles

Thanks for the review! bytes_tlv_callback will be removed in the next
version.

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Steven Eckhoff]

On Tue, Dec 12, 2017 at 01:04:01PM +, Charles Keepax wrote:
> On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> > Currently there is no support for the TSCS42xx audio CODEC.
> > 
> > Add support for it.
> > 
> > v5 attempts to address all issues raised in the previous reviews.
> > 
> > Thank you to everyone who has invested their time reviewing these
> > patches.
> > 
> > Acked-by: Philippe Ombredanne 
> > Signed-off-by: Steven Eckhoff 
> > ---
> > +int bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag,
> > +   unsigned int size, unsigned int __user *tlv)
> > +{
> > +   struct tscs_dsp_ctl *ctl =
> > +   (struct tscs_dsp_ctl *)kcontrol->private_value;
> > +   struct soc_bytes_ext *params = >bytes_ext;
> > +   unsigned int count = size < params->max ? size : params->max;
> > +   int ret = -ENXIO;
> > +
> > +   switch (op_flag) {
> > +   case SNDRV_CTL_TLV_OP_READ:
> > +   if (params->get)
> > +   ret = params->get(kcontrol, tlv, count);
> > +   break;
> > +   case SNDRV_CTL_TLV_OP_WRITE:
> > +   if (params->put)
> > +   ret = params->put(kcontrol, tlv, count);
> > +   break;
> > +   }
> > +   return ret;
> > +}
> 
> Should this function be removed, I am assuming it is a hang over
> from the earlier TLV implementation?
> 
> Thanks,
> Charles

Thanks for the review! bytes_tlv_callback will be removed in the next
version.

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Mark Brown]

On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> Currently there is no support for the TSCS42xx audio CODEC.
> 
> Add support for it.
> 
> v5 attempts to address all issues raised in the previous reviews.
> 
> Thank you to everyone who has invested their time reviewing these
> patches.

Please add inter-version changelogs and commentary like this after --- 
as covered in SubmittingPatches - this lets tools know they don't need
to end up in git.

> +  - compatible : "tscs:tscs42xx"

Compatible strings should be for specific devices, they may all be
treated identically by software now but may not be in future.

> + do {
> + ret = snd_soc_read(codec, R_DACCRSTAT);
> + if (ret < 0) {
> + dev_err(codec->dev,
> + "Failed to read stat (%d)\n", ret);
> + return ret;
> + }
> + } while (ret);

There should be some sort of upper bound on how many times we'll try to
wait for this in case the hardware fails somehow.

> +static int tscs42xx_mute_stream(struct snd_soc_dai *dai, int mute, int 
> stream)
> +{
> + struct snd_soc_codec *codec = dai->codec;
> + int ret;
> +
> + if (mute)
> + if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> + ret = dac_mute(codec);
> + else
> + ret = adc_mute(codec);
> + else
> + if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> + ret = dac_unmute(codec);
> + else
> + ret = adc_unmute(codec);
> +
> + return ret;
> +}

All these mute functions also shut down the PLLs which since...

> + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> + case SND_SOC_DAIFMT_CBM_CFM:
> + ret = snd_soc_update_bits(codec, R_AIC1, RM_AIC1_MS,
> + RV_AIC1_MS_MASTER);
> + if (ret < 0)
> + dev_err(codec->dev,
> + "Failed to set codec DAI master (%d)\n", ret);
> + else
> + ret = 0;
> + break;
> + default:

...we only support the CODEC being the clock master seems like it might
mean we stop clocking the DAI?  If that's the case it's better to just
not have the mute control and allow the user to just control these as
normal mutes.

> +static int tscs42xx_probe(struct snd_soc_codec *codec)
> +{
> + int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(r_inits); ++i) {
> + ret = snd_soc_write(codec, r_inits[i].reg, r_inits[i].def);
> + if (ret < 0) {
> + dev_err(codec->dev,
> + "Failed to write codec defaults (%d)\n", ret);
> + return ret;
> + }
> + }

I'd expect the driver to just reset the CODEC (it appears to have that
feature) and the regmap.

> +static int tscs42xx_remove(struct snd_soc_codec *codec)
> +{
> + return 0;
> +}

Just remove empty functions.

> +static const struct i2c_device_id tscs42xx_i2c_id[] = {
> + { "tscs42xx", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, tscs42xx_i2c_id);

I2C IDs are better as explicit part numbers too like compatible
strings.


signature.asc
Description: PGP signature

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Mark Brown]

On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> Currently there is no support for the TSCS42xx audio CODEC.
> 
> Add support for it.
> 
> v5 attempts to address all issues raised in the previous reviews.
> 
> Thank you to everyone who has invested their time reviewing these
> patches.

Please add inter-version changelogs and commentary like this after --- 
as covered in SubmittingPatches - this lets tools know they don't need
to end up in git.

> +  - compatible : "tscs:tscs42xx"

Compatible strings should be for specific devices, they may all be
treated identically by software now but may not be in future.

> + do {
> + ret = snd_soc_read(codec, R_DACCRSTAT);
> + if (ret < 0) {
> + dev_err(codec->dev,
> + "Failed to read stat (%d)\n", ret);
> + return ret;
> + }
> + } while (ret);

There should be some sort of upper bound on how many times we'll try to
wait for this in case the hardware fails somehow.

> +static int tscs42xx_mute_stream(struct snd_soc_dai *dai, int mute, int 
> stream)
> +{
> + struct snd_soc_codec *codec = dai->codec;
> + int ret;
> +
> + if (mute)
> + if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> + ret = dac_mute(codec);
> + else
> + ret = adc_mute(codec);
> + else
> + if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> + ret = dac_unmute(codec);
> + else
> + ret = adc_unmute(codec);
> +
> + return ret;
> +}

All these mute functions also shut down the PLLs which since...

> + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> + case SND_SOC_DAIFMT_CBM_CFM:
> + ret = snd_soc_update_bits(codec, R_AIC1, RM_AIC1_MS,
> + RV_AIC1_MS_MASTER);
> + if (ret < 0)
> + dev_err(codec->dev,
> + "Failed to set codec DAI master (%d)\n", ret);
> + else
> + ret = 0;
> + break;
> + default:

...we only support the CODEC being the clock master seems like it might
mean we stop clocking the DAI?  If that's the case it's better to just
not have the mute control and allow the user to just control these as
normal mutes.

> +static int tscs42xx_probe(struct snd_soc_codec *codec)
> +{
> + int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(r_inits); ++i) {
> + ret = snd_soc_write(codec, r_inits[i].reg, r_inits[i].def);
> + if (ret < 0) {
> + dev_err(codec->dev,
> + "Failed to write codec defaults (%d)\n", ret);
> + return ret;
> + }
> + }

I'd expect the driver to just reset the CODEC (it appears to have that
feature) and the regmap.

> +static int tscs42xx_remove(struct snd_soc_codec *codec)
> +{
> + return 0;
> +}

Just remove empty functions.

> +static const struct i2c_device_id tscs42xx_i2c_id[] = {
> + { "tscs42xx", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, tscs42xx_i2c_id);

I2C IDs are better as explicit part numbers too like compatible
strings.


signature.asc
Description: PGP signature

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Takashi Iwai]

On Tue, 12 Dec 2017 14:04:01 +0100,
Charles Keepax wrote:
> 
> On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> > Currently there is no support for the TSCS42xx audio CODEC.
> > 
> > Add support for it.
> > 
> > v5 attempts to address all issues raised in the previous reviews.
> > 
> > Thank you to everyone who has invested their time reviewing these
> > patches.
> > 
> > Acked-by: Philippe Ombredanne 
> > Signed-off-by: Steven Eckhoff 
> > ---
> > +int bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag,
> > +   unsigned int size, unsigned int __user *tlv)
> > +{
> > +   struct tscs_dsp_ctl *ctl =
> > +   (struct tscs_dsp_ctl *)kcontrol->private_value;
> > +   struct soc_bytes_ext *params = >bytes_ext;
> > +   unsigned int count = size < params->max ? size : params->max;
> > +   int ret = -ENXIO;
> > +
> > +   switch (op_flag) {
> > +   case SNDRV_CTL_TLV_OP_READ:
> > +   if (params->get)
> > +   ret = params->get(kcontrol, tlv, count);
> > +   break;
> > +   case SNDRV_CTL_TLV_OP_WRITE:
> > +   if (params->put)
> > +   ret = params->put(kcontrol, tlv, count);
> > +   break;
> > +   }
> > +   return ret;
> > +}
> 
> Should this function be removed, I am assuming it is a hang over
> from the earlier TLV implementation?

Yes, and bytes_ext_info() is also missing static.


Takashi

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Takashi Iwai]

On Tue, 12 Dec 2017 14:04:01 +0100,
Charles Keepax wrote:
> 
> On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> > Currently there is no support for the TSCS42xx audio CODEC.
> > 
> > Add support for it.
> > 
> > v5 attempts to address all issues raised in the previous reviews.
> > 
> > Thank you to everyone who has invested their time reviewing these
> > patches.
> > 
> > Acked-by: Philippe Ombredanne 
> > Signed-off-by: Steven Eckhoff 
> > ---
> > +int bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag,
> > +   unsigned int size, unsigned int __user *tlv)
> > +{
> > +   struct tscs_dsp_ctl *ctl =
> > +   (struct tscs_dsp_ctl *)kcontrol->private_value;
> > +   struct soc_bytes_ext *params = >bytes_ext;
> > +   unsigned int count = size < params->max ? size : params->max;
> > +   int ret = -ENXIO;
> > +
> > +   switch (op_flag) {
> > +   case SNDRV_CTL_TLV_OP_READ:
> > +   if (params->get)
> > +   ret = params->get(kcontrol, tlv, count);
> > +   break;
> > +   case SNDRV_CTL_TLV_OP_WRITE:
> > +   if (params->put)
> > +   ret = params->put(kcontrol, tlv, count);
> > +   break;
> > +   }
> > +   return ret;
> > +}
> 
> Should this function be removed, I am assuming it is a hang over
> from the earlier TLV implementation?

Yes, and bytes_ext_info() is also missing static.


Takashi

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Charles Keepax]

On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> Currently there is no support for the TSCS42xx audio CODEC.
> 
> Add support for it.
> 
> v5 attempts to address all issues raised in the previous reviews.
> 
> Thank you to everyone who has invested their time reviewing these
> patches.
> 
> Acked-by: Philippe Ombredanne 
> Signed-off-by: Steven Eckhoff 
> ---
> +int bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag,
> + unsigned int size, unsigned int __user *tlv)
> +{
> + struct tscs_dsp_ctl *ctl =
> + (struct tscs_dsp_ctl *)kcontrol->private_value;
> + struct soc_bytes_ext *params = >bytes_ext;
> + unsigned int count = size < params->max ? size : params->max;
> + int ret = -ENXIO;
> +
> + switch (op_flag) {
> + case SNDRV_CTL_TLV_OP_READ:
> + if (params->get)
> + ret = params->get(kcontrol, tlv, count);
> + break;
> + case SNDRV_CTL_TLV_OP_WRITE:
> + if (params->put)
> + ret = params->put(kcontrol, tlv, count);
> + break;
> + }
> + return ret;
> +}

Should this function be removed, I am assuming it is a hang over
from the earlier TLV implementation?

Thanks,
Charles

Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

[Charles Keepax]

On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> Currently there is no support for the TSCS42xx audio CODEC.
> 
> Add support for it.
> 
> v5 attempts to address all issues raised in the previous reviews.
> 
> Thank you to everyone who has invested their time reviewing these
> patches.
> 
> Acked-by: Philippe Ombredanne 
> Signed-off-by: Steven Eckhoff 
> ---
> +int bytes_tlv_callback(struct snd_kcontrol *kcontrol, int op_flag,
> + unsigned int size, unsigned int __user *tlv)
> +{
> + struct tscs_dsp_ctl *ctl =
> + (struct tscs_dsp_ctl *)kcontrol->private_value;
> + struct soc_bytes_ext *params = >bytes_ext;
> + unsigned int count = size < params->max ? size : params->max;
> + int ret = -ENXIO;
> +
> + switch (op_flag) {
> + case SNDRV_CTL_TLV_OP_READ:
> + if (params->get)
> + ret = params->get(kcontrol, tlv, count);
> + break;
> + case SNDRV_CTL_TLV_OP_WRITE:
> + if (params->put)
> + ret = params->put(kcontrol, tlv, count);
> + break;
> + }
> + return ret;
> +}

Should this function be removed, I am assuming it is a hang over
from the earlier TLV implementation?

Thanks,
Charles