Add a debugfs device node which initiates the turn-on diagnostic routine
feature of the TDA7802 amplifier. The four status registers (one per
channel) are returned.
Signed-off-by: Thomas Preston <[email protected]>
---
Changes since v1:
- Rename speaker-test to (turn-on) diagnostics
- Move turn-on diagnostic to debugfs as there is no standard ALSA
interface for this kind of routine.
sound/soc/codecs/tda7802.c | 186 ++++++++++++++++++++++++++++++++++++-
1 file changed, 185 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/tda7802.c b/sound/soc/codecs/tda7802.c
index 0f82a88bc1a4..74436212241d 100644
--- a/sound/soc/codecs/tda7802.c
+++ b/sound/soc/codecs/tda7802.c
@@ -5,6 +5,7 @@
* Copyright (C) 2016-2019 Tesla Motors, Inc.
*/
+#include <linux/debugfs.h>
#include <linux/delay.h>
#include <linux/i2c.h>
#include <linux/module.h>
@@ -26,6 +27,7 @@
#define TDA7802_IB5 0x05
#define TDA7802_DB0 0x10
+#define TDA7802_DB1 0x11
#define TDA7802_DB5 0x15
#define IB2_DIGITAL_MUTE_DISABLED (1 << 2)
@@ -47,6 +49,17 @@
#define IB3_FORMAT_TDM16_DEV3 (6 << IB3_FORMAT_SHIFT)
#define IB3_FORMAT_TDM16_DEV4 (7 << IB3_FORMAT_SHIFT)
+#define IB4_DIAG_MODE_ENABLE (1 << 0)
+
+#define IB5_AMPLIFIER_ON (1 << 0)
+
+#define DB0_STARTUP_DIAG_STATUS (1 << 6)
+
+#define DIAGNOSTIC_POLL_PERIOD_US 5000
+#define DIAGNOSTIC_TIMEOUT_US 5000000
+#define DIAGNOSTIC_SETTLE_MS 20
+#define NUM_IB 6
+
enum tda7802_type {
tda7802_base,
};
@@ -55,6 +68,12 @@ struct tda7802_priv {
struct i2c_client *i2c;
struct regmap *regmap;
struct regulator *enable_reg;
+ struct dentry *debugfs;
+ struct mutex diagnostic_mutex;
+};
+
+struct reg_update {
+ unsigned int reg, mask, val;
};
static const struct reg_default tda7802_reg[] = {
@@ -113,6 +132,19 @@ static const struct regmap_config tda7802_regmap_config = {
.cache_type = REGCACHE_RBTREE,
};
+/* The following bits need to be updated before diagnostics mode is set. */
+static const struct reg_update diagnostic_state[NUM_IB] = {
+ { TDA7802_IB0, 0, 0 },
+ { TDA7802_IB1, 0, 0 },
+ { TDA7802_IB2,
+ IB2_CH13_UNMUTED | IB2_CH24_UNMUTED | IB2_DIGITAL_MUTE_DISABLED,
+ IB2_CH13_UNMUTED | IB2_CH24_UNMUTED | IB2_DIGITAL_MUTE_DISABLED,
+ },
+ { TDA7802_IB3, 0, 0 },
+ { TDA7802_IB4, IB4_DIAG_MODE_ENABLE, 0 },
+ { TDA7802_IB5, IB5_AMPLIFIER_ON, 0 },
+};
+
static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute)
{
const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED;
@@ -414,7 +446,6 @@ static const struct snd_kcontrol_new tda7802_snd_controls[] = {
SOC_SINGLE("AC diag enable", TDA7802_IB4, 3, 1, 0),
SOC_ENUM("Diag mode channels 1 & 3", diag_mode_ch13),
SOC_ENUM("Diag mode channels 2 & 4", diag_mode_ch24),
- SOC_SINGLE("Diag mode enable", TDA7802_IB4, 0, 1, 0),
SOC_ENUM("Clipping detect channels 1 & 3", clip_detect_ch13),
SOC_ENUM("Clipping detect channels 2 & 4", clip_detect_ch24),
@@ -432,7 +463,160 @@ static const struct snd_soc_dapm_route tda7802_dapm_routes[] = {
{ "SPK", NULL, "DAC" },
};
+static int tda7802_bulk_update(struct regmap *map, struct reg_update *update,
+ size_t update_count)
+{
+ int i, err;
+
+ for (i = 0; i < update_count; i++) {
+ err = regmap_update_bits(map, update[i].reg, update[i].mask,
+ update[i].val);
+ if (err < 0)
+ return err;
+ }
+
+ return i;
+}
+
+static int run_turn_on_diagnostic(struct tda7802_priv *tda7802, u8 *status)
+{
+ struct device *dev = &tda7802->i2c->dev;
+ int err_status, err;
+ unsigned int val;
+ u8 state[NUM_IB];
+
+ dev_dbg(dev, "%s\n", __func__);
+
+ /* Save state and prepare for diagnostic */
+ err = regmap_bulk_read(tda7802->regmap, TDA7802_IB0, state,
+ ARRAY_SIZE(state));
+ if (err < 0) {
+ dev_err(dev, "Could not save device state, %d\n", err);
+ return err;
+ }
+
+ err = tda7802_bulk_update(tda7802->regmap, diagnostic_state,
+ ARRAY_SIZE(diagnostic_state));
+ if (err < 0) {
+ dev_err(dev, "Could not prepare for diagnostics, %d\n", err);
+ goto diagnostic_restore;
+ }
+
+ /* We must wait 20ms for device to settle, otherwise diagnostics will
+ * not start and regmap poll will timeout.
+ */
+ msleep(DIAGNOSTIC_SETTLE_MS);
+
+ /* Turn on diagnostic */
+ err = regmap_update_bits(tda7802->regmap, TDA7802_IB4,
+ IB4_DIAG_MODE_ENABLE, IB4_DIAG_MODE_ENABLE);
+ if (err < 0) {
+ dev_err(dev, "Could not enable diagnostic mode, %d\n", err);
+ goto diagnostic_restore;
+ }
+
+ /* Wait until DB0_STARTUP_DIAG_STATUS is set, then read status */
+ err_status = regmap_read_poll_timeout(tda7802->regmap, TDA7802_DB0, val,
+ val & DB0_STARTUP_DIAG_STATUS,
+ DIAGNOSTIC_POLL_PERIOD_US,
+ DIAGNOSTIC_TIMEOUT_US);
+ if (err_status < 0) {
+ dev_err(dev, "Diagnostic did not complete, err %d, reg %x",
+ err_status, val);
+ goto diagnostic_restore;
+ }
+
+ err = regmap_bulk_read(tda7802->regmap, TDA7802_DB1, status, 4);
+ if (err < 0) {
+ dev_err(dev, "Could not read channel status, %d\n", err);
+ goto diagnostic_restore;
+ }
+
+diagnostic_restore:
+ err = regmap_bulk_write(tda7802->regmap, TDA7802_IB0, state,
+ ARRAY_SIZE(state));
+ if (err < 0)
+ dev_err(dev, "Could not restore state, %d\n", err);
+
+ if (err_status < 0)
+ return err_status;
+ else
+ return err;
+}
+
+static int tda7802_diagnostic_show(struct seq_file *f, void *p)
+{
+ char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+ struct tda7802_priv *tda7802 = f->private;
+ u8 status[4] = { 0 };
+ int i, err;
+
+ mutex_lock(&tda7802->diagnostic_mutex);
+ err = run_turn_on_diagnostic(tda7802, status);
+ mutex_unlock(&tda7802->diagnostic_mutex);
+ if (err < 0)
+ return err;
+
+ for (i = 0; i < ARRAY_SIZE(status); i++)
+ seq_printf(f, "%02x: %02x\n", TDA7802_DB1+i, status[i]);
+
+ return 0;
+}
+
+static int tda7802_diagnostic_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, tda7802_diagnostic_show, inode->i_private);
+}
+
+static const struct file_operations tda7802_diagnostic_fops = {
+ .open = tda7802_diagnostic_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static int tda7802_probe(struct snd_soc_component *component)
+{
+ struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
+ struct device *dev = &tda7802->i2c->dev;
+ int err;
+
+ tda7802->debugfs = debugfs_create_dir(dev_name(dev), NULL);
+ if (IS_ERR_OR_NULL(tda7802->debugfs)) {
+ dev_info(dev,
+ "Failed to create debugfs node, err %ld\n",
+ PTR_ERR(tda7802->debugfs));
+ return 0;
+ }
+
+ mutex_init(&tda7802->diagnostic_mutex);
+ err = debugfs_create_file("diagnostic", 0444, tda7802->debugfs, tda7802,
+ &tda7802_diagnostic_fops);
+ if (err < 0) {
+ dev_err(dev,
+ "debugfs: Failed to create diagnostic node, err %d\n",
+ err);
+ goto cleanup_diagnostic;
+ }
+
+ return 0;
+
+cleanup_diagnostic:
+ mutex_destroy(&tda7802->diagnostic_mutex);
+ return err;
+}
+
+static void tda7802_remove(struct snd_soc_component *component)
+{
+ struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
+
+ debugfs_remove_recursive(tda7802->debugfs);
+ mutex_destroy(&tda7802->diagnostic_mutex);
+}
+
static const struct snd_soc_component_driver tda7802_component_driver = {
+ .probe = tda7802_probe,
+ .remove = tda7802_remove,
.set_bias_level = tda7802_set_bias_level,
.idle_bias_on = 1,
.suspend_bias_off = 1,
--
2.21.0
On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:
> Add a debugfs device node which initiates the turn-on diagnostic routine
> feature of the TDA7802 amplifier. The four status registers (one per
> channel) are returned.
>
> Signed-off-by: Thomas Preston <[email protected]>
> ---
> Changes since v1:
> - Rename speaker-test to (turn-on) diagnostics
> - Move turn-on diagnostic to debugfs as there is no standard ALSA
> interface for this kind of routine.
>
> sound/soc/codecs/tda7802.c | 186 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 185 insertions(+), 1 deletion(-)
>
> +static int tda7802_bulk_update(struct regmap *map, struct reg_update *update,
> + size_t update_count)
> +{
> + int i, err;
> +
> + for (i = 0; i < update_count; i++) {
> + err = regmap_update_bits(map, update[i].reg, update[i].mask,
> + update[i].val);
> + if (err < 0)
> + return err;
> + }
> +
> + return i;
> +}
This could probably be removed using regmap_multi_reg_write.
> +static int tda7802_probe(struct snd_soc_component *component)
> +{
> + struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
> + struct device *dev = &tda7802->i2c->dev;
> + int err;
> +
> + tda7802->debugfs = debugfs_create_dir(dev_name(dev), NULL);
> + if (IS_ERR_OR_NULL(tda7802->debugfs)) {
> + dev_info(dev,
> + "Failed to create debugfs node, err %ld\n",
> + PTR_ERR(tda7802->debugfs));
> + return 0;
> + }
> +
> + mutex_init(&tda7802->diagnostic_mutex);
> + err = debugfs_create_file("diagnostic", 0444, tda7802->debugfs, tda7802,
> + &tda7802_diagnostic_fops);
> + if (err < 0) {
> + dev_err(dev,
> + "debugfs: Failed to create diagnostic node, err %d\n",
> + err);
> + goto cleanup_diagnostic;
> + }
You shouldn't be failing the driver probe if debugfs fails, it
should be purely optional.
Thanks,
Charles
Hi,
Thanks for getting back to me so quickly.
On 30/07/2019 13:41, Charles Keepax wrote:
> On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:
>> Add a debugfs device node which initiates the turn-on diagnostic routine
>> feature of the TDA7802 amplifier. The four status registers (one per
>> channel) are returned.
>>
>> Signed-off-by: Thomas Preston <[email protected]>
>> ---
>> Changes since v1:
>> - Rename speaker-test to (turn-on) diagnostics
>> - Move turn-on diagnostic to debugfs as there is no standard ALSA
>> interface for this kind of routine.
>>
>> sound/soc/codecs/tda7802.c | 186 ++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 185 insertions(+), 1 deletion(-)
>>
>> +static int tda7802_bulk_update(struct regmap *map, struct reg_update *update,
>> + size_t update_count)
>> +{
>> + int i, err;
>> +
>> + for (i = 0; i < update_count; i++) {
>> + err = regmap_update_bits(map, update[i].reg, update[i].mask,
>> + update[i].val);
>> + if (err < 0)
>> + return err;
>> + }
>> +
>> + return i;
>> +}
>
> This could probably be removed using regmap_multi_reg_write.
>
The problem is that I want to retain the state of the other bits in those
registers. Maybe I should make a copy of the backed up state, set the bits
I want to off-device, then either:
1. Write the changes with regmap_multi_reg_write
2. Write all six regs again (if my device doesn't support the multi_reg)
>> +static int tda7802_probe(struct snd_soc_component *component)
>> +{
>> + struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
>> + struct device *dev = &tda7802->i2c->dev;
>> + int err;
>> +
>> + tda7802->debugfs = debugfs_create_dir(dev_name(dev), NULL);
>> + if (IS_ERR_OR_NULL(tda7802->debugfs)) {
>> + dev_info(dev,
>> + "Failed to create debugfs node, err %ld\n",
>> + PTR_ERR(tda7802->debugfs));
>> + return 0;
>> + }
>> +
>> + mutex_init(&tda7802->diagnostic_mutex);
>> + err = debugfs_create_file("diagnostic", 0444, tda7802->debugfs, tda7802,
>> + &tda7802_diagnostic_fops);
>> + if (err < 0) {
>> + dev_err(dev,
>> + "debugfs: Failed to create diagnostic node, err %d\n",
>> + err);
>> + goto cleanup_diagnostic;
>> + }
>
> You shouldn't be failing the driver probe if debugfs fails, it
> should be purely optional.
>
Got it, thanks.
On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:
> + struct dentry *debugfs;
> + struct mutex diagnostic_mutex;
> +};
It is unclear what this mutex usefully protects, it only gets taken when
writing to the debugfs file to trigger this diagnostic mode but doesn't
do anything to control interactions with any other code path in the
driver.
> +static int run_turn_on_diagnostic(struct tda7802_priv *tda7802, u8 *status)
> +{
> + struct device *dev = &tda7802->i2c->dev;
> + int err_status, err;
> + unsigned int val;
> + u8 state[NUM_IB];
> + /* We must wait 20ms for device to settle, otherwise diagnostics will
> + * not start and regmap poll will timeout.
> + */
> + msleep(DIAGNOSTIC_SETTLE_MS);
The comment and define might go out of sync...
> + err = regmap_bulk_read(tda7802->regmap, TDA7802_DB1, status, 4);
> + if (err < 0) {
> + dev_err(dev, "Could not read channel status, %d\n", err);
> + goto diagnostic_restore;
> + }
...but here we use a magic number for the array size :(
> +static int tda7802_diagnostic_show(struct seq_file *f, void *p)
> +{
> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
We neither use nor free buf?
> +static int tda7802_probe(struct snd_soc_component *component)
> +{
> + struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
> + struct device *dev = &tda7802->i2c->dev;
> + int err;
Why is this done at the component level?
On Tue, Jul 30, 2019 at 03:04:19PM +0100, Thomas Preston wrote:
> On 30/07/2019 13:41, Charles Keepax wrote:
> > This could probably be removed using regmap_multi_reg_write.
> The problem is that I want to retain the state of the other bits in those
> registers. Maybe I should make a copy of the backed up state, set the bits
> I want to off-device, then either:
> 1. Write the changes with regmap_multi_reg_write
> 2. Write all six regs again (if my device doesn't support the multi_reg)
Or make this a regmap function, there's nothing device specific about
it.
On Tue, Jul 30, 2019 at 03:04:19PM +0100, Thomas Preston wrote:
> On 30/07/2019 13:41, Charles Keepax wrote:
> >> +static int tda7802_bulk_update(struct regmap *map, struct reg_update *update,
> >> + size_t update_count)
> >> +{
> >> + int i, err;
> >> +
> >> + for (i = 0; i < update_count; i++) {
> >> + err = regmap_update_bits(map, update[i].reg, update[i].mask,
> >> + update[i].val);
> >> + if (err < 0)
> >> + return err;
> >> + }
> >> +
> >> + return i;
> >> +}
> >
> > This could probably be removed using regmap_multi_reg_write.
> >
>
> The problem is that I want to retain the state of the other bits in those
> registers. Maybe I should make a copy of the backed up state, set the bits
> I want to off-device, then either:
>
> 1. Write the changes with regmap_multi_reg_write
> 2. Write all six regs again (if my device doesn't support the multi_reg)
>
Nah sorry my bad you are probably better off they way you are.
Thanks,
Charles
On 30/07/2019 15:19, Mark Brown wrote:
> On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:
>
>> + struct dentry *debugfs;
>> + struct mutex diagnostic_mutex;
>> +};
>
> It is unclear what this mutex usefully protects, it only gets taken when
> writing to the debugfs file to trigger this diagnostic mode but doesn't
> do anything to control interactions with any other code path in the
> driver.
>
If another process reads the debugfs node "diagnostic" while the turn-on
diagnostic mode is running, this mutex prevents the second process
restarting the diagnostics.
This is redundant if debugfs reads are atomic, but I don't think they are.
>> +static int run_turn_on_diagnostic(struct tda7802_priv *tda7802, u8 *status)
>> +{
>> + struct device *dev = &tda7802->i2c->dev;
>> + int err_status, err;
>> + unsigned int val;
>> + u8 state[NUM_IB];
>
>> + /* We must wait 20ms for device to settle, otherwise diagnostics will
>> + * not start and regmap poll will timeout.
>> + */
>> + msleep(DIAGNOSTIC_SETTLE_MS);
>
> The comment and define might go out of sync...
>
Thanks, I will remove the 20ms but keep the comment here.
>> + err = regmap_bulk_read(tda7802->regmap, TDA7802_DB1, status, 4);
>> + if (err < 0) {
>> + dev_err(dev, "Could not read channel status, %d\n", err);
>> + goto diagnostic_restore;
>> + }
>
> ...but here we use a magic number for the array size :(
>
Thanks, will update.
>> +static int tda7802_diagnostic_show(struct seq_file *f, void *p)
>> +{
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>
> We neither use nor free buf?
>
>> +static int tda7802_probe(struct snd_soc_component *component)
>> +{
>> + struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
>> + struct device *dev = &tda7802->i2c->dev;
>> + int err;
>
> Why is this done at the component level?
>
Argh my bad, a previous iteration required the buf and component. I missed
this, sorry for the noise.
Thanks for feedback, I'll go back and tend to all of this.
On 30/07/2019 15:20, Mark Brown wrote:
> On Tue, Jul 30, 2019 at 03:04:19PM +0100, Thomas Preston wrote:
>> On 30/07/2019 13:41, Charles Keepax wrote:
>
>>> This could probably be removed using regmap_multi_reg_write.
>
>> The problem is that I want to retain the state of the other bits in those
>> registers. Maybe I should make a copy of the backed up state, set the bits
>> I want to off-device, then either:
>
>> 1. Write the changes with regmap_multi_reg_write
>> 2. Write all six regs again (if my device doesn't support the multi_reg)
>
> Or make this a regmap function, there's nothing device specific about
> it.
>
I did wonder why regmap didn't have an multi-update function. If appropriate,
I will add this in.
On Tue, Jul 30, 2019 at 04:25:56PM +0100, Thomas Preston wrote:
> On 30/07/2019 15:19, Mark Brown wrote:
> > It is unclear what this mutex usefully protects, it only gets taken when
> > writing to the debugfs file to trigger this diagnostic mode but doesn't
> > do anything to control interactions with any other code path in the
> > driver.
> If another process reads the debugfs node "diagnostic" while the turn-on
> diagnostic mode is running, this mutex prevents the second process
> restarting the diagnostics.
> This is redundant if debugfs reads are atomic, but I don't think they are.
Like I say it's not just debugfs though, there's the standard driver
interface too.
On 30/07/2019 16:50, Mark Brown wrote:
> On Tue, Jul 30, 2019 at 04:25:56PM +0100, Thomas Preston wrote:
>> On 30/07/2019 15:19, Mark Brown wrote:
>
>>> It is unclear what this mutex usefully protects, it only gets taken when
>>> writing to the debugfs file to trigger this diagnostic mode but doesn't
>>> do anything to control interactions with any other code path in the
>>> driver.
>
>> If another process reads the debugfs node "diagnostic" while the turn-on
>> diagnostic mode is running, this mutex prevents the second process
>> restarting the diagnostics.
>
>> This is redundant if debugfs reads are atomic, but I don't think they are.
>
> Like I say it's not just debugfs though, there's the standard driver
> interface too.
>
Ah right, I understand. So if we run the turn-on diagnostics routine, there's
nothing stopping anyone from interacting with the device in other ways.
I guess there's no way to share that mutex with ALSA? In that case, it doesn't
matter if this mutex is there or not - this feature is incompatible. How
compatible do debugfs interfaces have to be? I was under the impression anything
goes. I would argue that the debugfs is better off for having the mutex so
that no one re-reads "diagnostic" within the 5s poll timeout.
Alternatively, this diagnostic feature could be handled with an external-handler
kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.
What would be acceptable?
On Tue, Jul 30, 2019 at 05:28:11PM +0100, Thomas Preston wrote:
> On 30/07/2019 16:50, Mark Brown wrote:
> > On Tue, Jul 30, 2019 at 04:25:56PM +0100, Thomas Preston wrote:
> >> On 30/07/2019 15:19, Mark Brown wrote:
> >
> >>> It is unclear what this mutex usefully protects, it only gets taken when
> >>> writing to the debugfs file to trigger this diagnostic mode but doesn't
> >>> do anything to control interactions with any other code path in the
> >>> driver.
> >
> >> If another process reads the debugfs node "diagnostic" while the turn-on
> >> diagnostic mode is running, this mutex prevents the second process
> >> restarting the diagnostics.
> >
> >> This is redundant if debugfs reads are atomic, but I don't think they are.
> >
> > Like I say it's not just debugfs though, there's the standard driver
> > interface too.
> >
>
> Ah right, I understand. So if we run the turn-on diagnostics routine, there's
> nothing stopping anyone from interacting with the device in other ways.
>
> I guess there's no way to share that mutex with ALSA? In that case, it doesn't
> matter if this mutex is there or not - this feature is incompatible. How
> compatible do debugfs interfaces have to be? I was under the impression anything
> goes. I would argue that the debugfs is better off for having the mutex so
> that no one re-reads "diagnostic" within the 5s poll timeout.
>
> Alternatively, this diagnostic feature could be handled with an external-handler
> kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.
>
> What would be acceptable?
You could take the DAPM mutex in your debugfs handler that would
prevent any changes to the cards power state whilst your debug
stuff is running.
Thanks,
Charles
On Tue, Jul 30, 2019 at 05:28:11PM +0100, Thomas Preston wrote:
> On 30/07/2019 16:50, Mark Brown wrote:
> > Like I say it's not just debugfs though, there's the standard driver
> > interface too.
> Ah right, I understand. So if we run the turn-on diagnostics routine, there's
> nothing stopping anyone from interacting with the device in other ways.
> I guess there's no way to share that mutex with ALSA? In that case, it doesn't
> matter if this mutex is there or not - this feature is incompatible. How
> compatible do debugfs interfaces have to be? I was under the impression anything
> goes. I would argue that the debugfs is better off for having the mutex so
> that no one re-reads "diagnostic" within the 5s poll timeout.
It's not really something that's supported; like Charles says the DAPM
mutex is exposed but if the regular controls would still be able to do
stuff. It is kind of a "you broke it, you fix it" thing but on the
other hand it's better to make things safer if we can since it might not
be obvious later on why things are broken.
> Alternatively, this diagnostic feature could be handled with an external-handler
> kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.
>
> What would be acceptable?
Yes, that's definitely doable - we've got some other drivers with
similar things like calibration triggers exposed that way.
On 02/08/2019 00:42, Mark Brown wrote:
> On Tue, Jul 30, 2019 at 05:28:11PM +0100, Thomas Preston wrote:
>> On 30/07/2019 16:50, Mark Brown wrote:
>
>>> Like I say it's not just debugfs though, there's the standard driver
>>> interface too.
>
>> Ah right, I understand. So if we run the turn-on diagnostics routine, there's
>> nothing stopping anyone from interacting with the device in other ways.
>
>> I guess there's no way to share that mutex with ALSA? In that case, it doesn't
>> matter if this mutex is there or not - this feature is incompatible. How
>> compatible do debugfs interfaces have to be? I was under the impression anything
>> goes. I would argue that the debugfs is better off for having the mutex so
>> that no one re-reads "diagnostic" within the 5s poll timeout.
>
> It's not really something that's supported; like Charles says the DAPM
> mutex is exposed but if the regular controls would still be able to do
> stuff. It is kind of a "you broke it, you fix it" thing but on the
> other hand it's better to make things safer if we can since it might not
> be obvious later on why things are broken.
>
>> Alternatively, this diagnostic feature could be handled with an external-handler
>> kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.
>>
>> What would be acceptable?
>
> Yes, that's definitely doable - we've got some other drivers with
> similar things like calibration triggers exposed that way.
>
One problem with using a kcontrol as a trigger for the turn-on diagnostic
is that the diagnostic routine has a "return value".
It goes like this:
- Bring device to low-quiescent state
- Initiate diagnostics
- Poll for diagnostics-complete bit
- Read the four channel status registers
The final read clears the status registers, so this isn't something I
can just do with regmap.
One idea I had was to initiate the turn-on diagnostics using a kcontrol,
whose handler saves the four channel status registers and an epoch in
tda7802_priv. Then this can be read from debugfs. But it seems strange
to have to turn on this control over here, then go over there and read
this value.
Hm, maybe a better idea is to have the turn on diagnostic only run on
device probe (as its name suggests!), and print something to dmesg:
modprobe tda7802 turn_on_diagnostic=1
tda7802-codec i2c-TDA7802:00: Turn on diagnostic 04 04 04 04
Kirill Marinushkin mentioned this in the first review [0], it just didn't
really sink in until now!
[0] https://lkml.org/lkml/2019/6/14/1344
On Fri, Aug 02, 2019 at 09:32:17AM +0100, Thomas Preston wrote:
> On 02/08/2019 00:42, Mark Brown wrote:
> > Yes, that's definitely doable - we've got some other drivers with
> > similar things like calibration triggers exposed that way.
> One problem with using a kcontrol as a trigger for the turn-on diagnostic
> is that the diagnostic routine has a "return value".
You can use a read only control for the readback, or just have it be
triggered by overwriting the readback value. You can cache the result.
> Hm, maybe a better idea is to have the turn on diagnostic only run on
> device probe (as its name suggests!), and print something to dmesg:
> modprobe tda7802 turn_on_diagnostic=1
> tda7802-codec i2c-TDA7802:00: Turn on diagnostic 04 04 04 04
> Kirill Marinushkin mentioned this in the first review [0], it just didn't
> really sink in until now!
You could do that too, yeah. Depends on what this is diagnosing and if
that'd be useful.
On 02/08/2019 12:10, Mark Brown wrote:
> On Fri, Aug 02, 2019 at 09:32:17AM +0100, Thomas Preston wrote:
>> On 02/08/2019 00:42, Mark Brown wrote:
>
>>> Yes, that's definitely doable - we've got some other drivers with
>>> similar things like calibration triggers exposed that way.
>
>> One problem with using a kcontrol as a trigger for the turn-on diagnostic
>> is that the diagnostic routine has a "return value".
>
> You can use a read only control for the readback, or just have it be
> triggered by overwriting the readback value. You can cache the result.
>
Keeping the trigger and result together like that would be better I think,
although the routine isn't supposed to run mid way through playback. If
we're mid playback the debugfs routine has to turn off AMP_ON, take the
device back to a known state, run diagnostics, then restore. Which causes
a gap in the audible sound.
>> Hm, maybe a better idea is to have the turn on diagnostic only run on
>> device probe (as its name suggests!), and print something to dmesg:
>
>> modprobe tda7802 turn_on_diagnostic=1
>
>> tda7802-codec i2c-TDA7802:00: Turn on diagnostic 04 04 04 04
>
>> Kirill Marinushkin mentioned this in the first review [0], it just didn't
>> really sink in until now!
>
> You could do that too, yeah. Depends on what this is diagnosing and if
> that'd be useful.
>
The diagnostic status bits describe situations such as:
- open load (no speaker connected)
- short to GND
- short to VCC
- etc
The intention is to test if all the speakers are connected. So, one might
have a self test which runs the diagnostic and verifies it outputs:
00 00 00 00
For example, on my test rig there is only one speaker connected. So it
reads:
04 04 00 04
Where the second bit is "open load". So this would fail the test.
So in the kcontrol case the test would be something like:
amixer sset "AMP1 turn on diagnostic" on
amixer sget "AMP1 diagnostic"
And the module parameter case:
rmmod tda7802
modprobe tda7802 turn_on_diagnostic=1
dmesg | grep "Turn on diagnostic 04 04 04 04"
rmmod tda7802
modprobe tda7802
I think the module parameter method is more appropriate for a
"Turn-on diagnostic", even though I don't really like grepping dmesg
for the result. I'll go ahead and implement that unless anyone has a
particular preference for the kcontrol-trigger.
Thanks
On Fri, Aug 02, 2019 at 03:51:09PM +0100, Thomas Preston wrote:
> On 02/08/2019 12:10, Mark Brown wrote:
> > You can use a read only control for the readback, or just have it be
> > triggered by overwriting the readback value. You can cache the result.
> Keeping the trigger and result together like that would be better I think,
> although the routine isn't supposed to run mid way through playback. If
> we're mid playback the debugfs routine has to turn off AMP_ON, take the
> device back to a known state, run diagnostics, then restore. Which causes
> a gap in the audible sound.
Whatever method is used to do the triggering can always return -EBUSY
when you someone tries to do so during playback.
> >> Kirill Marinushkin mentioned this in the first review [0], it just didn't
> >> really sink in until now!
> > You could do that too, yeah. Depends on what this is diagnosing and if
> > that'd be useful.
> The diagnostic status bits describe situations such as:
> - open load (no speaker connected)
> - short to GND
> - short to VCC
> - etc
> The intention is to test if all the speakers are connected. So, one might
> have a self test which runs the diagnostic and verifies it outputs:
...
> I think the module parameter method is more appropriate for a
> "Turn-on diagnostic", even though I don't really like grepping dmesg
> for the result. I'll go ahead and implement that unless anyone has a
> particular preference for the kcontrol-trigger.
Right. It's not ideal for use in production systems for example but
perhaps fine for support techs or whoever. Up to you anyway.