[alsa-devel] [PATCH] ASoC: si476x: Remove custom register I/O implementation
From: Mark Brown broonie@linaro.org
The current si476x I/O implementation wraps the regmap for the core with functions that make the register map cache only when the device is powered down. This implementation appears to be incomplete since there is no code to synchronise the cache so writes done while the core is powered down will be ignored, the device will only be configured if it is powered.
A better and more idiomatic approach would be to have the MFD manage the cache, making the device cache only when it powers things down. This also allows ASoC to use the standard regmap helpers for the device which helps remove the ASoC custom ones so do convert to do that.
Signed-off-by: Mark Brown broonie@linaro.org --- sound/soc/codecs/si476x.c | 46 +--------------------------------------------- 1 file changed, 1 insertion(+), 45 deletions(-)
diff --git a/sound/soc/codecs/si476x.c b/sound/soc/codecs/si476x.c index 38f3b10..ec29d26 100644 --- a/sound/soc/codecs/si476x.c +++ b/sound/soc/codecs/si476x.c @@ -60,48 +60,6 @@ enum si476x_pcm_format { SI476X_PCM_FORMAT_S24_LE = 6, };
-static unsigned int si476x_codec_read(struct snd_soc_codec *codec, - unsigned int reg) -{ - int err; - unsigned int val; - struct si476x_core *core = codec->control_data; - - si476x_core_lock(core); - if (!si476x_core_is_powered_up(core)) - regcache_cache_only(core->regmap, true); - - err = regmap_read(core->regmap, reg, &val); - - if (!si476x_core_is_powered_up(core)) - regcache_cache_only(core->regmap, false); - si476x_core_unlock(core); - - if (err < 0) - return err; - - return val; -} - -static int si476x_codec_write(struct snd_soc_codec *codec, - unsigned int reg, unsigned int val) -{ - int err; - struct si476x_core *core = codec->control_data; - - si476x_core_lock(core); - if (!si476x_core_is_powered_up(core)) - regcache_cache_only(core->regmap, true); - - err = regmap_write(core->regmap, reg, val); - - if (!si476x_core_is_powered_up(core)) - regcache_cache_only(core->regmap, false); - si476x_core_unlock(core); - - return err; -} - static const struct snd_soc_dapm_widget si476x_dapm_widgets[] = { SND_SOC_DAPM_OUTPUT("LOUT"), SND_SOC_DAPM_OUTPUT("ROUT"), @@ -239,7 +197,7 @@ static int si476x_codec_hw_params(struct snd_pcm_substream *substream,
static int si476x_codec_probe(struct snd_soc_codec *codec) { - codec->control_data = i2c_mfd_cell_to_core(codec->dev); + codec->control_data = dev_get_regmap(codec->dev.parent); return 0; }
@@ -268,8 +226,6 @@ static struct snd_soc_dai_driver si476x_dai = {
static struct snd_soc_codec_driver soc_codec_dev_si476x = { .probe = si476x_codec_probe, - .read = si476x_codec_read, - .write = si476x_codec_write, .dapm_widgets = si476x_dapm_widgets, .num_dapm_widgets = ARRAY_SIZE(si476x_dapm_widgets), .dapm_routes = si476x_dapm_routes,
On Thu, Sep 26, 2013 at 11:34 AM, Mark Brown broonie@kernel.org wrote:
From: Mark Brown broonie@linaro.org
The current si476x I/O implementation wraps the regmap for the core with functions that make the register map cache only when the device is powered down. This implementation appears to be incomplete since there is no code to synchronise the cache so writes done while the core is powered down will be ignored, the device will only be configured if it is powered.
I wouldn't be surprised if I messed up that part of the code since the HW that I used to have to test it didn't actually use the codec driver(I initially developed the driver on a different HW that did use the driver but after some time I lost my access to that particular HW)
A better and more idiomatic approach would be to have the MFD manage the cache, making the device cache only when it powers things down. This also allows ASoC to use the standard regmap helpers for the device which helps
Correct me if my understanding is wrong but using standard helper function would mean using hw_write and hw_read from soc-io.c. Looking at the source code of those functions I don't believe the are performing any sort of locking(once again I may be wrong) and rely on regmap_* implementation to do it internally. With si476x driver it is not the case and it is important that codec write/read functions perform si476x_core_lock(core) before they call regmap functions. If that is not done it is possible to wreak havoc on I2C bus by trying to access the registers of the codec and doing some V4L2 operations on the corresponding radio device.
Although I completely agree that cache managing should be moved to MFD.
remove the ASoC custom ones so do convert to do that.
Signed-off-by: Mark Brown broonie@linaro.org
sound/soc/codecs/si476x.c | 46 +--------------------------------------------- 1 file changed, 1 insertion(+), 45 deletions(-)
diff --git a/sound/soc/codecs/si476x.c b/sound/soc/codecs/si476x.c index 38f3b10..ec29d26 100644 --- a/sound/soc/codecs/si476x.c +++ b/sound/soc/codecs/si476x.c @@ -60,48 +60,6 @@ enum si476x_pcm_format { SI476X_PCM_FORMAT_S24_LE = 6, };
-static unsigned int si476x_codec_read(struct snd_soc_codec *codec,
unsigned int reg)-{
int err;unsigned int val;struct si476x_core *core = codec->control_data;si476x_core_lock(core);if (!si476x_core_is_powered_up(core))regcache_cache_only(core->regmap, true);err = regmap_read(core->regmap, reg, &val);if (!si476x_core_is_powered_up(core))regcache_cache_only(core->regmap, false);si476x_core_unlock(core);if (err < 0)return err;return val;-}
-static int si476x_codec_write(struct snd_soc_codec *codec,
unsigned int reg, unsigned int val)-{
int err;struct si476x_core *core = codec->control_data;si476x_core_lock(core);if (!si476x_core_is_powered_up(core))regcache_cache_only(core->regmap, true);err = regmap_write(core->regmap, reg, val);if (!si476x_core_is_powered_up(core))regcache_cache_only(core->regmap, false);si476x_core_unlock(core);return err;-}
static const struct snd_soc_dapm_widget si476x_dapm_widgets[] = { SND_SOC_DAPM_OUTPUT("LOUT"), SND_SOC_DAPM_OUTPUT("ROUT"), @@ -239,7 +197,7 @@ static int si476x_codec_hw_params(struct snd_pcm_substream *substream,
static int si476x_codec_probe(struct snd_soc_codec *codec) {
codec->control_data = i2c_mfd_cell_to_core(codec->dev);
codec->control_data = dev_get_regmap(codec->dev.parent); return 0;}
@@ -268,8 +226,6 @@ static struct snd_soc_dai_driver si476x_dai = {
static struct snd_soc_codec_driver soc_codec_dev_si476x = { .probe = si476x_codec_probe,
.read = si476x_codec_read,.write = si476x_codec_write, .dapm_widgets = si476x_dapm_widgets, .num_dapm_widgets = ARRAY_SIZE(si476x_dapm_widgets), .dapm_routes = si476x_dapm_routes,-- 1.8.4.rc3
On Thu, Sep 26, 2013 at 12:33:33PM -0700, Andrey Smirnov wrote:
On Thu, Sep 26, 2013 at 11:34 AM, Mark Brown broonie@kernel.org wrote:
A better and more idiomatic approach would be to have the MFD manage the cache, making the device cache only when it powers things down. This also allows ASoC to use the standard regmap helpers for the device which helps
Correct me if my understanding is wrong but using standard helper function would mean using hw_write and hw_read from soc-io.c. Looking at the source code of those functions I don't believe the are performing any sort of locking(once again I may be wrong) and rely on regmap_* implementation to do it internally. With si476x driver it is not the case and it is important that codec write/read functions perform si476x_core_lock(core) before they call regmap functions. If that is not done it is possible to wreak havoc on I2C bus by trying to access the registers of the codec and doing some V4L2 operations on the corresponding radio device.
Although I completely agree that cache managing should be moved to MFD.
What exactly is the locking you're talking about here? Both regmap and I/O have locking at those levels, if you try to do a bus interaction using them it shouldn't conflict with any other register write. If it's something higher level then shouldn't the locking be higher level too?
The way the code is written it looks like the lock is being held to ensure there's no race due to a power state change.
On Thu, Sep 26, 2013 at 12:43 PM, Mark Brown broonie@kernel.org wrote:
On Thu, Sep 26, 2013 at 12:33:33PM -0700, Andrey Smirnov wrote:
On Thu, Sep 26, 2013 at 11:34 AM, Mark Brown broonie@kernel.org wrote:
A better and more idiomatic approach would be to have the MFD manage the cache, making the device cache only when it powers things down. This also allows ASoC to use the standard regmap helpers for the device which helps
Correct me if my understanding is wrong but using standard helper function would mean using hw_write and hw_read from soc-io.c. Looking at the source code of those functions I don't believe the are performing any sort of locking(once again I may be wrong) and rely on regmap_* implementation to do it internally. With si476x driver it is not the case and it is important that codec write/read functions perform si476x_core_lock(core) before they call regmap functions. If that is not done it is possible to wreak havoc on I2C bus by trying to access the registers of the codec and doing some V4L2 operations on the corresponding radio device.
Although I completely agree that cache managing should be moved to MFD.
What exactly is the locking you're talking about here?
I am talking about locking specific for si476x MFD device that is hadled by si476x_core_lock / si476x_core_unlock. The code before the patch has both externally exposed functions via V4L2 driver and SoC driver obtain said lock for the duration of their interaction with the device. The patch appears to remove lock acquisition for SoC functions.
Both regmap and I/O have locking at those levels, if you try to do a bus interaction using them it shouldn't conflict with any other register write. If it's something higher level then shouldn't the locking be higher level too?
The way the code is written it looks like the lock is being held to ensure there's no race due to a power state change.
That lock is being held to prevent device access contention between all of the code that can be triggered to be executed via V4L2 ioctl API and SoC API. As an example, I can open the radio device and start tuning it using corresponding ioctl, meanwhile, parallel to that process my other code is trying to change the sample rate of the codec. To prevent those two from clashes both of them try to acquire "core lock" with si476x_core_lock before the start doing anything with chip/MFD.
On Thu, Sep 26, 2013 at 12:58:27PM -0700, Andrey Smirnov wrote:
On Thu, Sep 26, 2013 at 12:43 PM, Mark Brown broonie@kernel.org wrote:
What exactly is the locking you're talking about here?
I am talking about locking specific for si476x MFD device that is hadled by si476x_core_lock / si476x_core_unlock. The code before the patch has both externally exposed functions via V4L2 driver and SoC driver obtain said lock for the duration of their interaction with the device. The patch appears to remove lock acquisition for SoC functions.
But what exactly are these locks supposed to be protecting?
The way the code is written it looks like the lock is being held to ensure there's no race due to a power state change.
That lock is being held to prevent device access contention between all of the code that can be triggered to be executed via V4L2 ioctl API and SoC API. As an example, I can open the radio device and start tuning it using corresponding ioctl, meanwhile, parallel to that process my other code is trying to change the sample rate of the codec. To prevent those two from clashes both of them try to acquire "core lock" with si476x_core_lock before the start doing anything with chip/MFD.
In what way might these things conflict? What are the clashes you are concerned about?
On Thu, Sep 26, 2013 at 1:58 PM, Mark Brown broonie@kernel.org wrote:
On Thu, Sep 26, 2013 at 12:58:27PM -0700, Andrey Smirnov wrote:
On Thu, Sep 26, 2013 at 12:43 PM, Mark Brown broonie@kernel.org wrote:
What exactly is the locking you're talking about here?
I am talking about locking specific for si476x MFD device that is hadled by si476x_core_lock / si476x_core_unlock. The code before the patch has both externally exposed functions via V4L2 driver and SoC driver obtain said lock for the duration of their interaction with the device. The patch appears to remove lock acquisition for SoC functions.
But what exactly are these locks supposed to be protecting?
The way the code is written it looks like the lock is being held to ensure there's no race due to a power state change.
That lock is being held to prevent device access contention between all of the code that can be triggered to be executed via V4L2 ioctl API and SoC API. As an example, I can open the radio device and start tuning it using corresponding ioctl, meanwhile, parallel to that process my other code is trying to change the sample rate of the codec. To prevent those two from clashes both of them try to acquire "core lock" with si476x_core_lock before the start doing anything with chip/MFD.
In what way might these things conflict? What are the clashes you are concerned about?
That lock serves the purpose of making sure that si476x_core_send_command, calls to which all the rest of the functions boil down to, is not executed in parallel to itself. So I am concerned that with the lock acquisition removal it would be possible to make that condition happen.
On Thu, Sep 26, 2013 at 03:48:09PM -0700, Andrey Smirnov wrote:
On Thu, Sep 26, 2013 at 1:58 PM, Mark Brown broonie@kernel.org wrote:
In what way might these things conflict? What are the clashes you are concerned about?
That lock serves the purpose of making sure that si476x_core_send_command, calls to which all the rest of the functions boil down to, is not executed in parallel to itself. So I am concerned that with the lock acquisition removal it would be possible to make that condition happen.
OK, so if it's purely about send_command() then why is the locking not being done in that function? Surely the most obvious and robust place to protect the function is within the function itself?
OK, so if it's purely about send_command() then why is the locking not being done in that function? Surely the most obvious and robust place to protect the function is within the function itself?
Not having send_command acquire any locks allows me to have variable levels of granularity of locking, for example in the worker thread that fetches RDS data from the chip it allows me to get exclusive access to the chip for the duration of the FIFO draining. Also any band switch for that chip, like AM to FM, etc. require chip to be power cycled which translates to multiple calls to send_command as well, I think having those performed atomically simplifies the driver as well.
Even if such fine granularity could be made to work, I think it is a big change that would need to be tested on the real HW and unfortunately I don't have any now and I don't believe there are or has ever been any users of that driver besides me and the company I used to work for(which no longer ships the device with that chip, so we can exclude them). I am trying to get some sort of evaluation board from SiLabs, but until and if they give it to me, any changes like that can not be verified to work on a real HW.
On Fri, Sep 27, 2013 at 08:53:58AM -0700, Andrey Smirnov wrote:
OK, so if it's purely about send_command() then why is the locking not being done in that function? Surely the most obvious and robust place to protect the function is within the function itself?
Not having send_command acquire any locks allows me to have variable levels of granularity of locking, for example in the worker thread that fetches RDS data from the chip it allows me to get exclusive access to the chip for the duration of the FIFO draining. Also any band switch for that chip, like AM to FM, etc. require chip to be power cycled which translates to multiple calls to send_command as well, I think having those performed atomically simplifies the driver as well.
Right, so it's not just protecting send_command() but is instead protecting higher level things. In that case shouldn't the locking be performed further up in this driver too rather than on each register write?
On Fri, Sep 27, 2013 at 10:02 AM, Mark Brown broonie@kernel.org wrote:
On Fri, Sep 27, 2013 at 08:53:58AM -0700, Andrey Smirnov wrote:
OK, so if it's purely about send_command() then why is the locking not being done in that function? Surely the most obvious and robust place to protect the function is within the function itself?
Not having send_command acquire any locks allows me to have variable levels of granularity of locking, for example in the worker thread that fetches RDS data from the chip it allows me to get exclusive access to the chip for the duration of the FIFO draining. Also any band switch for that chip, like AM to FM, etc. require chip to be power cycled which translates to multiple calls to send_command as well, I think having those performed atomically simplifies the driver as well.
Right, so it's not just protecting send_command() but is instead protecting higher level things. In that case shouldn't the locking be performed further up in this driver too rather than on each register write?
That is the case for the V4L driver, in that code the locking is acquired for logical operations, such "tune" or "power up" rather than for a single register write, but for the codec driver, to the best of my knowledge, read and write register are the highest level operations that it provides. Or am I just unaware of some codec driver functionality that driver can provide and SoC sound subsystem will use that would allow the lock to be obtained at higher levels?
On Fri, Sep 27, 2013 at 12:28:14PM -0700, Andrey Smirnov wrote:
That is the case for the V4L driver, in that code the locking is acquired for logical operations, such "tune" or "power up" rather than for a single register write, but for the codec driver, to the best of my knowledge, read and write register are the highest level operations that it provides. Or am I just unaware of some codec driver functionality that driver can provide and SoC sound subsystem will use that would allow the lock to be obtained at higher levels?
Since the driver doesn't expose any controls to userspace the only things that can cause register writes are the operations provided by the CODEC driver, they could take the lock for their runtime (which is about the level the v4l stuff is running at AIUI).
This still won't fix the issue with the device being powered off but I guess that normal usage patterns will have powered the device up to do tuning or something prior to audio being started.
Since the driver doesn't expose any controls to userspace the only things that can cause register writes are the operations provided by the CODEC driver, they could take the lock for their runtime (which is about the level the v4l stuff is running at AIUI).
Do you mean moving it to si476x_codec_set_dai_fmt and si476x_codec_hw_params? If those are the only two places where register read and write are called by SoC sound subsystem then I think that would work.
This still won't fix the issue with the device being powered off but I guess that normal usage patterns will have powered the device up to do tuning or something prior to audio being started
You mean the issue cause by my forgetting to sync cache on power up? That would be a subject for a separate patch.
On Fri, Sep 27, 2013 at 09:37:01PM -0700, Andrey Smirnov wrote:
Since the driver doesn't expose any controls to userspace the only things that can cause register writes are the operations provided by the CODEC driver, they could take the lock for their runtime (which is about the level the v4l stuff is running at AIUI).
Do you mean moving it to si476x_codec_set_dai_fmt and si476x_codec_hw_params? If those are the only two places where register read and write are called by SoC sound subsystem then I think that would work.
Yes, exactly. Unless you provide controls or DAPM nothing outside of the callbacks in your driver is going to interact with registers.
participants (2)
-
Andrey Smirnov -
Mark Brown