| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 12 Jun 2019 19:56:16 +0200
From: Cezary Rojewski <cezary.rojewski@...el.com>
To: Thomas Preston <thomas.preston@...ethink.co.uk>
Cc: Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Charles Keepax <ckeepax@...nsource.cirrus.com>,
Jerome Brunet <jbrunet@...libre.com>,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Marco Felsch <m.felsch@...gutronix.de>,
Paul Cercueil <paul@...pouillou.net>,
Kirill Marinushkin <kmarinushkin@...dec.tech>,
Cheng-Yi Chiang <cychiang@...omium.org>,
Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
Vinod Koul <vkoul@...nel.org>,
Annaliese McDermond <nh6z@...z.net>,
alsa-devel@...a-project.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 4/4] ASoC: tda7802: Add speaker-test sysfs
On 2019-06-11 19:49, Thomas Preston wrote:
> Add speaker_test device attribute. When the speaker-test node is read
> the hardware speaker test is started.
>
> $ cat /sys/devices/.../device:00/speaker_test
> 04 04 04 04
>
> Signed-off-by: Thomas Preston <thomas.preston@...ethink.co.uk>
> Cc: Patrick Glaser <pglaser@...la.com>
> Cc: Rob Duncan <rduncan@...la.com>
> Cc: Nate Case <ncase@...la.com>
> ---
> sound/soc/codecs/tda7802.c | 172 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 172 insertions(+)
>
> diff --git a/sound/soc/codecs/tda7802.c b/sound/soc/codecs/tda7802.c
> index 62aae011d9f1..edfa752c0c9f 100644
> --- a/sound/soc/codecs/tda7802.c
> +++ b/sound/soc/codecs/tda7802.c
> @@ -157,6 +157,7 @@ static const u8 IB3_FORMAT[4][4] = {
> #define DUMP_NUM_REGS (DUMP_NUM_BLOCK * 2)
>
> struct tda7802_priv {
> + struct mutex mutex;
> struct i2c_client *i2c;
> struct regmap *regmap;
> struct regulator *enable_reg;
> @@ -458,6 +459,159 @@ static struct snd_soc_dai_driver tda7802_dai_driver = {
> .ops = &tda7802_dai_ops,
> };
>
> +/* The speaker test is triggered by reading a sysfs attribute file attached to
> + * the I2C device. The user space thread that's doing the reading is blocked
> + * until the test completes (or times out). We only permit one test to be in
> + * progress at a time.
> + */
> +
> +static int speaker_test_start(struct regmap *regmap)
> +{
> + int err;
> +
> + err = regmap_update_bits(regmap, TDA7802_IB5, IB5_AMPLIFIER_ON, 0);
> + if (err < 0) {
> + dev_err(regmap_get_device(regmap),
> + "Cannot disable amp for speaker test (%d)\n", err);
> + return err;
> + }
> +
> + err = regmap_update_bits(regmap, TDA7802_IB4, IB4_DIAG_MODE_ENABLE, 0);
> + if (err < 0) {
> + dev_err(regmap_get_device(regmap),
> + "Cannot disable diag mode before speaker test (%d)\n",
> + err);
> + return err;
> + }
> +
> + /* Seem to need at least a 15 ms delay here before the rising
> + * edge. Otherwise the diagnostics never complete (perhaps
> + * they never start).
> + */
> + msleep(20);
> +
> + err = regmap_update_bits(regmap, TDA7802_IB4,
> + IB4_DIAG_MODE_ENABLE, IB4_DIAG_MODE_ENABLE);
> + if (err < 0) {
> + dev_err(regmap_get_device(regmap),
> + "Cannot enable diag mode for speaker test (%d)\n", err);
> + return err;
> + }
> +
> + return 0;
You may want to follow path set by speaker_test_stop: replace "return 0"
with "return err" and have only error msg found within preceding
if-statement.
> +}
> +
> +static int speaker_test_stop(struct regmap *regmap)
> +{
> + int err;
> +
> + err = regmap_update_bits(regmap, TDA7802_IB4, IB4_DIAG_MODE_ENABLE, 0);
> + if (err < 0)
> + dev_err(regmap_get_device(regmap),
> + "Cannot disable diag mode after speaker test (%d)\n",
> + err);
> + return err;
> +}
> +
> +/* We poll the test completion bit, letting the current thread sleep
> + * until we're done. These values are not critical.
> + */
> +#define SPEAKER_TEST_DONE_POLL_PERIOD_US 5000
> +#define SPEAKER_TEST_DONE_TIMEOUT_US 5000000
> +
> +static int speaker_test_check(struct tda7802_priv *tda7802,
> + unsigned int *status)
> +{
> + struct regmap *regmap = tda7802->regmap;
> + struct device *dev = &tda7802->i2c->dev;
> + int reg_err = 0, err = 0, i, amp_on;
Both reg_err and err are initialized unnecessarily.
> + unsigned int val;
> +
> + reg_err = regulator_enable(tda7802->enable_reg);
> + if (reg_err < 0) {
> + dev_err(dev, "Could not enable (speaker-test).\n");
> + return reg_err;
> + }
> + msleep(ENABLE_DELAY_MS);
> +
> + /* we should return amp on state when speaker-test is done */
> + err = regmap_read(regmap, TDA7802_IB5, &_on);
> + if (err < 0) {
> + dev_err(dev, "Could not read amp on state (speaker-test)\n");
> + goto speaker_test_disable;
> + }
> +
> + for (i = 0; i < CHANNELS_PER_AMP; ++i)
> + status[i] = 0xff;
> +
> + err = speaker_test_start(regmap);
> + if (err < 0)
> + goto speaker_test_restore;
> +
> + /* Wait until DB0_STARTUP_DIAG_STATUS is set */
> + err = regmap_read_poll_timeout(regmap, TDA7802_DB0, val,
> + val & DB0_STARTUP_DIAG_STATUS,
> + SPEAKER_TEST_DONE_POLL_PERIOD_US,
> + SPEAKER_TEST_DONE_TIMEOUT_US);
> + if (err < 0) {
> + if (err == -ENODATA)
> + dev_err(dev,
> + "Speaker diagnostic test did not complete\n");
> + speaker_test_stop(regmap);
> + goto speaker_test_restore;
> + }
> +
> + err = speaker_test_stop(regmap);
> + if (err < 0)
> + goto speaker_test_restore;
This sequence looks weird and as if it could be simplified but I might
be just plain wrong. speaker_test_stop gets invoked whether or not
regmap_read_poll_timeout fails, though only in "happy" path its return
is account for. If it's not too much to ask, could you clarify?
> +
> + for (i = 0; i < CHANNELS_PER_AMP; ++i) {
> + err = regmap_read(regmap, TDA7802_DB1 + i, status + i);
> + if (err < 0) {
> + dev_err(dev,
> + "Cannot get speaker %d status (%d)\n", i, err);
> + goto speaker_test_restore;
> + }
> + }
> +
> +speaker_test_restore:
> + err = regmap_update_bits(regmap, TDA7802_IB5, IB5_AMPLIFIER_ON,
> + amp_on);
> + if (err < 0)
> + dev_err(dev, "Could not restore amp on state (speaker-test)\n");
> +
> +speaker_test_disable:
> + reg_err = regulator_disable(tda7802->enable_reg);
> + if (reg_err < 0) {
> + dev_err(dev, "Could not disable (speaker-test).\n");
> + return reg_err;
> + }
> + return err;
> +}
> +
> +static ssize_t speaker_test_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct tda7802_priv *tda7802 = dev_get_drvdata(dev);
> + unsigned int channel_status[CHANNELS_PER_AMP];
> + char *str = buf;
> + int ret, i;
> +
> + mutex_lock(&tda7802->mutex);
> + ret = speaker_test_check(tda7802, channel_status);
> + mutex_unlock(&tda7802->mutex);
> + if (ret < 0)
> + return ret;
> +
> + for (i = 0; i < CHANNELS_PER_AMP; i++)
> + str += sprintf(str, "%02x ", channel_status[i]);
> + str += sprintf(str, "\n");
> +
> + return str - buf;
> +}
> +
> +static DEVICE_ATTR_RO(speaker_test);
> +
> static ssize_t enable_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> @@ -537,12 +691,28 @@ static int tda7802_probe(struct snd_soc_component *component)
>
> dev_dbg(dev, "%s\n", __func__);
>
> + /* Device is ready, now we should create sysfs attributes.
> + * Rememer to cleanup
> + */
> err = device_create_file(dev, &dev_attr_enable);
> if (err < 0) {
> dev_err(dev, "Could not create enable attr\n");
> return err;
> }
>
> + mutex_init(&tda7802->mutex);
> + err = device_create_file(dev, &dev_attr_speaker_test);
> + if (err < 0) {
> + dev_err(dev, "Could not create speaker_test attr\n");
> + goto cleanup_speaker_test;
> + }
> +
> + return 0;
> +
> +cleanup_speaker_test:
> + mutex_destroy(&tda7802->mutex);
> + device_remove_file(&tda7802->i2c->dev, &dev_attr_enable);
> +
> return err;
> }
>
> @@ -550,6 +720,8 @@ static void tda7802_remove(struct snd_soc_component *component)
> {
> struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
>
> + device_remove_file(&tda7802->i2c->dev, &dev_attr_speaker_test);
> + mutex_destroy(&tda7802->mutex);
> device_remove_file(&tda7802->i2c->dev, &dev_attr_enable);
> }
>
>
Powered by blists - more mailing lists