May 2019 - Alsa-devel - mailman.alsa-project.org

[alsa-devel] [PATCH 1/3] ALSA: jack: Remove hard coding of jack_switch_types array size
by bgoswami＠codeaurora.org 27 May '19

27 May '19

From: Banajit Goswami <bgoswami(a)codeaurora.org> The size for jack_switch_types array is currently controlled by a MACRO 'SND_JACK_SWITCH_TYPES', whose value needs to be updated everytime a new jack switch type is added. Remove this MACRO and use ARRAY_SIZE instead to get size of the array. Signed-off-by: Gopikrishnaiah Anandan <agopik(a)codeaurora.org> Signed-off-by: Banajit Goswami <bgoswami(a)codeaurora.org> --- include/sound/jack.h | 3 --- sound/core/jack.c | 4 ++-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/include/sound/jack.h b/include/sound/jack.h index 1e84bfb..b0791c5 100644 --- a/include/sound/jack.h +++ b/include/sound/jack.h @@ -68,9 +68,6 @@ enum snd_jack_types { SND_JACK_BTN_5 = 0x0200, }; -/* Keep in sync with definitions above */ -#define SND_JACK_SWITCH_TYPES 6 - struct snd_jack { struct list_head kctl_list; struct snd_card *card; diff --git a/sound/core/jack.c b/sound/core/jack.c index 84c2a17..36b047b 100644 --- a/sound/core/jack.c +++ b/sound/core/jack.c @@ -33,7 +33,7 @@ struct snd_jack_kctl { }; #ifdef CONFIG_SND_JACK_INPUT_DEV -static int jack_switch_types[SND_JACK_SWITCH_TYPES] = { +static int jack_switch_types[] = { SW_HEADPHONE_INSERT, SW_MICROPHONE_INSERT, SW_LINEOUT_INSERT, @@ -250,7 +250,7 @@ int snd_jack_new(struct snd_card *card, const char *id, int type, jack->type = type; - for (i = 0; i < SND_JACK_SWITCH_TYPES; i++) + for (i = 0; i < ARRAY_SIZE(jack_switch_types); i++) if (type & (1 << i)) input_set_capability(jack->input_dev, EV_SW, jack_switch_types[i]); -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

2 1

[alsa-devel] [PATCH RFC] ASoC: hdac_hdmi: refine hdmi jacks
by libin.yang＠intel.com 27 May '19

27 May '19

From: Libin Yang <libin.yang(a)intel.com> Sorry for a big patch. I will split it if the patch is acceptable. This patch does the following actions: 1) jack is bound to pcm in origal code. The patch binds jack to pin-port. This mean each pin-port (3x3) has a jack. pin-port is used for DP-MST mode. The port means device entry for DP-MST in hdac_hdmi As hdac_hdmi allows user to map the pin-port to a pcm manually, the jack is bound to pcm is not accurate. However, PA assumes jack is bound to PCM for the legacy mode. For example, for ubuntu, in /usr/share/pulseaudio/alsa-mixer/paths/hdmi-output-x.conf uses "Jack HDMI/DP,pcm=n" to find which jack event is used. This main issue is: pcm number is fixed (3, 7, 8, 9, 10) for hdmi in legacy mode. But it is not always the same for ASoC hdmi audio. 2) move snd_soc_card_jack_new() from machine driver to codec driver. This is because the jack information is more lated to the codec Also this patch calls snd_jack_add_new_kctl() to export the jack kctl to userspace. 3) Don't call hdac_hdmi_jack_report() in hdac_hdmi_create_pin_port_muxs() Jack event means the jack pin hotplug event. It should not related to user setting. Signed-off-by: Libin Yang <libin.yang(a)intel.com> --- sound/soc/codecs/hdac_hdmi.c | 141 +++++++++++---------- sound/soc/intel/boards/bxt_da7219_max98357a.c | 10 -- sound/soc/intel/boards/bxt_rt298.c | 10 -- sound/soc/intel/boards/glk_rt5682_max98357a.c | 10 -- sound/soc/intel/boards/kbl_da7219_max98357a.c | 10 -- sound/soc/intel/boards/kbl_da7219_max98927.c | 10 -- sound/soc/intel/boards/kbl_rt5660.c | 10 -- sound/soc/intel/boards/kbl_rt5663_max98927.c | 10 -- .../soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 9 -- sound/soc/intel/boards/skl_hda_dsp_common.c | 9 -- sound/soc/intel/boards/skl_nau88l25_max98357a.c | 11 -- sound/soc/intel/boards/skl_nau88l25_ssm4567.c | 11 -- sound/soc/intel/boards/skl_rt286.c | 10 -- sound/soc/intel/boards/sof_rt5682.c | 11 -- 14 files changed, 72 insertions(+), 200 deletions(-) diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 660e058..49f02b1 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -4,6 +4,7 @@ * Copyright (C) 2014-2015 Intel Corp * Author: Samreen Nilofer <samreen.nilofer(a)intel.com> * Subhransu S. Prusty <subhransu.s.prusty(a)intel.com> + * Libin Yang <libin.yang(a)intel.com> * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ * * This program is free software; you can redistribute it and/or modify @@ -30,6 +31,7 @@ #include <sound/hda_i915.h> #include <sound/pcm_drm_eld.h> #include <sound/hda_chmap.h> +#include <sound/hda_codec.h> #include "../../hda/local.h" #include "hdac_hdmi.h" @@ -95,6 +97,8 @@ struct hdac_hdmi_port { int num_mux_nids; hda_nid_t mux_nids[HDA_MAX_CONNECTIONS]; struct hdac_hdmi_eld eld; + struct snd_soc_jack jack; + int jack_event; const char *jack_pin; struct snd_soc_dapm_context *dapm; const char *output_pin; @@ -105,14 +109,12 @@ struct hdac_hdmi_pcm { int pcm_id; struct list_head port_list; struct hdac_hdmi_cvt *cvt; - struct snd_soc_jack *jack; int stream_tag; int channels; int format; bool chmap_set; unsigned char chmap[8]; /* ALSA API channel-map */ struct mutex lock; - int jack_event; }; struct hdac_hdmi_dai_port_map { @@ -166,8 +168,7 @@ hdac_hdmi_get_pcm_from_cvt(struct hdac_hdmi_priv *hdmi, return pcm; } -static void hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm, - struct hdac_hdmi_port *port, bool is_connect) +static void hdac_hdmi_jack_report(struct hdac_hdmi_port *port, bool is_connect) { struct hdac_device *hdev = port->pin->hdev; @@ -176,33 +177,24 @@ static void hdac_hdmi_jack_report(struct hdac_hdmi_pcm *pcm, else snd_soc_dapm_disable_pin(port->dapm, port->jack_pin); + snd_soc_dapm_sync(port->dapm); + + if (!port->jack.jack) + return; if (is_connect) { - /* - * Report Jack connect event when a device is connected - * for the first time where same PCM is attached to multiple - * ports. - */ - if (pcm->jack_event == 0) { + if (port->jack_event == 0) { dev_dbg(&hdev->dev, - "jack report for pcm=%d\n", - pcm->pcm_id); - snd_soc_jack_report(pcm->jack, SND_JACK_AVOUT, - SND_JACK_AVOUT); + "jack report for pin:port = %d:%d\n", + port->id, port->pin->nid); + snd_soc_jack_report(&port->jack, SND_JACK_AVOUT, + SND_JACK_AVOUT); } - pcm->jack_event++; + port->jack_event = 1; } else { - /* - * Report Jack disconnect event when a device is disconnected - * is the only last connected device when same PCM is attached - * to multiple ports. - */ - if (pcm->jack_event == 1) - snd_soc_jack_report(pcm->jack, 0, SND_JACK_AVOUT); - if (pcm->jack_event > 0) - pcm->jack_event--; + if (port->jack_event == 1) + snd_soc_jack_report(&port->jack, 0, SND_JACK_AVOUT); + port->jack_event = 0; } - - snd_soc_dapm_sync(port->dapm); } /* MST supported verbs */ @@ -855,7 +847,6 @@ static int hdac_hdmi_pin_mux_widget_event(struct snd_soc_dapm_widget *w, static int hdac_hdmi_set_pin_port_mux(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { - int ret; struct hdac_hdmi_port *p, *p_next; struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; struct snd_soc_dapm_widget *w = snd_soc_dapm_kcontrol_widget(kcontrol); @@ -865,6 +856,7 @@ static int hdac_hdmi_set_pin_port_mux(struct snd_kcontrol *kcontrol, struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev); struct hdac_hdmi_pcm *pcm = NULL; const char *cvt_name = e->texts[ucontrol->value.enumerated.item[0]]; + int ret; ret = snd_soc_dapm_put_enum_double(kcontrol, ucontrol); if (ret < 0) @@ -880,13 +872,12 @@ static int hdac_hdmi_set_pin_port_mux(struct snd_kcontrol *kcontrol, list_for_each_entry_safe(p, p_next, &pcm->port_list, head) { if (p == port && p->id == port->id && - p->pin == port->pin) { - hdac_hdmi_jack_report(pcm, port, false); + p->pin == port->pin) list_del(&p->head); - } } } + /* * Jack status is not reported during device probe as the * PCMs are not registered by then. So report it here. @@ -895,7 +886,6 @@ static int hdac_hdmi_set_pin_port_mux(struct snd_kcontrol *kcontrol, if (!strcmp(cvt_name, pcm->cvt->name)) { list_add_tail(&port->head, &pcm->port_list); if (port->eld.monitor_present && port->eld.eld_valid) { - hdac_hdmi_jack_report(pcm, port, true); mutex_unlock(&hdmi->pin_mutex); return ret; } @@ -1247,27 +1237,18 @@ static void hdac_hdmi_present_sense(struct hdac_hdmi_pin *pin, } pcm = hdac_hdmi_get_pcm(hdev, port); - if (!port->eld.monitor_present || !port->eld.eld_valid) { - dev_err(&hdev->dev, "%s: disconnect for pin:port %d:%d\n", __func__, pin->nid, port->id); - /* - * PCMs are not registered during device probe, so don't - * report jack here. It will be done in usermode mux - * control select. - */ - if (pcm) - hdac_hdmi_jack_report(pcm, port, false); + hdac_hdmi_jack_report(port, false); mutex_unlock(&hdmi->pin_mutex); return; } if (port->eld.monitor_present && port->eld.eld_valid) { - if (pcm) - hdac_hdmi_jack_report(pcm, port, true); + hdac_hdmi_jack_report(port, true); print_hex_dump_debug("ELD: ", DUMP_PREFIX_OFFSET, 16, 1, port->eld.eld_buffer, port->eld.eld_size, false); @@ -1299,6 +1280,7 @@ static int hdac_hdmi_add_ports(struct hdac_device *hdev, } pin->ports = ports; pin->num_ports = max_ports; + return 0; } @@ -1664,6 +1646,30 @@ static int create_fill_jack_kcontrols(struct snd_soc_card *card, return snd_soc_add_card_controls(card, kc, i); } +static void hdac_hdmi_present_sense_all_pins(struct hdac_device *hdev, + struct hdac_hdmi_priv *hdmi, bool detect_pin_caps) +{ + int i; + struct hdac_hdmi_pin *pin; + + list_for_each_entry(pin, &hdmi->pin_list, head) { + if (detect_pin_caps) { + + if (hdac_hdmi_get_port_len(hdev, pin->nid) == 0) + pin->mst_capable = false; + else + pin->mst_capable = true; + } + + for (i = 0; i < pin->num_ports; i++) { + if (!pin->mst_capable && i > 0) + continue; + + hdac_hdmi_present_sense(pin, &pin->ports[i]); + } + } +} + int hdac_hdmi_jack_port_init(struct snd_soc_component *component, struct snd_soc_dapm_context *dapm) { @@ -1673,6 +1679,7 @@ int hdac_hdmi_jack_port_init(struct snd_soc_component *component, struct snd_soc_dapm_widget *widgets; struct snd_soc_dapm_route *route; char w_name[NAME_SIZE]; + char jack_name[NAME_SIZE]; int i = 0, j, ret; widgets = devm_kcalloc(dapm->dev, hdmi->num_ports, @@ -1701,6 +1708,23 @@ int hdac_hdmi_jack_port_init(struct snd_soc_component *component, pin->ports[j].jack_pin = widgets[i].name; pin->ports[j].dapm = dapm; + /* create pin-port jack */ + snprintf(jack_name, sizeof(jack_name), + "HDMI/DP, pin:port=%d:%d Jack", + pin->nid, pin->ports[j].id); + ret = snd_soc_card_jack_new(dapm->card, jack_name, + SND_JACK_AVOUT, + &(pin->ports[j].jack), + NULL, 0); + if (ret) + return ret; + + ret = snd_jack_add_new_kctl(pin->ports[j].jack.jack, + jack_name, SND_JACK_AVOUT); + if (ret) + dev_warn(&hdev->dev, "failed creating Jack kctl\n"); + + /* add to route from Jack widget to output */ hdac_hdmi_fill_route(&route[i], pin->ports[j].jack_pin, NULL, pin->ports[j].output_pin, NULL); @@ -1708,6 +1732,8 @@ int hdac_hdmi_jack_port_init(struct snd_soc_component *component, i++; } } + /* now jack is ready, let's update the status */ + hdac_hdmi_present_sense_all_pins(hdev, hdmi, true); /* Add Route from Jack widget to the output widget */ ret = snd_soc_dapm_new_controls(dapm, widgets, hdmi->num_ports); @@ -1739,6 +1765,10 @@ int hdac_hdmi_jack_port_init(struct snd_soc_component *component, } EXPORT_SYMBOL_GPL(hdac_hdmi_jack_port_init); +/* + * TODO: maybe we should rename this function name + * as it is not related to jack init any more. + */ int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int device, struct snd_soc_jack *jack) { @@ -1758,8 +1788,6 @@ int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int device, return -ENOMEM; pcm->pcm_id = device; pcm->cvt = hdmi->dai_map[dai->id].cvt; - pcm->jack_event = 0; - pcm->jack = jack; mutex_init(&pcm->lock); INIT_LIST_HEAD(&pcm->port_list); snd_pcm = hdac_hdmi_get_pcm_from_id(dai->component->card, device); @@ -1779,30 +1807,6 @@ int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int device, } EXPORT_SYMBOL_GPL(hdac_hdmi_jack_init); -static void hdac_hdmi_present_sense_all_pins(struct hdac_device *hdev, - struct hdac_hdmi_priv *hdmi, bool detect_pin_caps) -{ - int i; - struct hdac_hdmi_pin *pin; - - list_for_each_entry(pin, &hdmi->pin_list, head) { - if (detect_pin_caps) { - - if (hdac_hdmi_get_port_len(hdev, pin->nid) == 0) - pin->mst_capable = false; - else - pin->mst_capable = true; - } - - for (i = 0; i < pin->num_ports; i++) { - if (!pin->mst_capable && i > 0) - continue; - - hdac_hdmi_present_sense(pin, &pin->ports[i]); - } - } -} - static int hdmi_codec_probe(struct snd_soc_component *component) { struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component); @@ -1837,7 +1841,6 @@ static int hdmi_codec_probe(struct snd_soc_component *component) return ret; } - hdac_hdmi_present_sense_all_pins(hdev, hdmi, true); /* Imp: Store the card pointer in hda_codec */ hdmi->card = dapm->card->snd_card; diff --git a/sound/soc/intel/boards/bxt_da7219_max98357a.c b/sound/soc/intel/boards/bxt_da7219_max98357a.c index 5cadb7f..c69cfa9 100644 --- a/sound/soc/intel/boards/bxt_da7219_max98357a.c +++ b/sound/soc/intel/boards/bxt_da7219_max98357a.c @@ -573,14 +573,12 @@ static const struct x86_cpu_id glk_ids[] = { {} }; -#define NAME_SIZE 32 static int bxt_card_late_probe(struct snd_soc_card *card) { struct bxt_card_private *ctx = snd_soc_card_get_drvdata(card); struct bxt_hdmi_pcm *pcm; struct snd_soc_component *component = NULL; int err, i = 0; - char jack_name[NAME_SIZE]; if (x86_match_cpu(glk_ids)) snd_soc_dapm_add_routes(&card->dapm, gemini_map, @@ -591,14 +589,6 @@ static int bxt_card_late_probe(struct snd_soc_card *card) list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP, pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, &broxton_hdmi[i], - NULL, 0); - - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &broxton_hdmi[i]); diff --git a/sound/soc/intel/boards/bxt_rt298.c b/sound/soc/intel/boards/bxt_rt298.c index e91057f..d1372891 100644 --- a/sound/soc/intel/boards/bxt_rt298.c +++ b/sound/soc/intel/boards/bxt_rt298.c @@ -507,25 +507,15 @@ static struct snd_soc_dai_link broxton_rt298_dais[] = { }, }; -#define NAME_SIZE 32 static int bxt_card_late_probe(struct snd_soc_card *card) { struct bxt_rt286_private *ctx = snd_soc_card_get_drvdata(card); struct bxt_hdmi_pcm *pcm; struct snd_soc_component *component = NULL; int err, i = 0; - char jack_name[NAME_SIZE]; list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP, pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, &broxton_hdmi[i], - NULL, 0); - - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &broxton_hdmi[i]); diff --git a/sound/soc/intel/boards/glk_rt5682_max98357a.c b/sound/soc/intel/boards/glk_rt5682_max98357a.c index d17126f..2a35974 100644 --- a/sound/soc/intel/boards/glk_rt5682_max98357a.c +++ b/sound/soc/intel/boards/glk_rt5682_max98357a.c @@ -29,7 +29,6 @@ #define MAXIM_DEV0_NAME "MX98357A:00" #define DUAL_CHANNEL 2 #define QUAD_CHANNEL 4 -#define NAME_SIZE 32 static struct snd_soc_jack geminilake_hdmi[3]; @@ -523,21 +522,12 @@ static int glk_card_late_probe(struct snd_soc_card *card) { struct glk_card_private *ctx = snd_soc_card_get_drvdata(card); struct snd_soc_component *component = NULL; - char jack_name[NAME_SIZE]; struct glk_hdmi_pcm *pcm; int err = 0; int i = 0; list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP, pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, &geminilake_hdmi[i], - NULL, 0); - - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &geminilake_hdmi[i]); diff --git a/sound/soc/intel/boards/kbl_da7219_max98357a.c b/sound/soc/intel/boards/kbl_da7219_max98357a.c index 07491a0..f293307 100644 --- a/sound/soc/intel/boards/kbl_da7219_max98357a.c +++ b/sound/soc/intel/boards/kbl_da7219_max98357a.c @@ -519,25 +519,15 @@ static struct snd_soc_dai_link kabylake_dais[] = { }, }; -#define NAME_SIZE 32 static int kabylake_card_late_probe(struct snd_soc_card *card) { struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(card); struct kbl_hdmi_pcm *pcm; struct snd_soc_component *component = NULL; int err, i = 0; - char jack_name[NAME_SIZE]; list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP, pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, &skylake_hdmi[i], - NULL, 0); - - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &skylake_hdmi[i]); diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c index f72a7bf..84d3609 100644 --- a/sound/soc/intel/boards/kbl_da7219_max98927.c +++ b/sound/soc/intel/boards/kbl_da7219_max98927.c @@ -34,7 +34,6 @@ #define DUAL_CHANNEL 2 #define QUAD_CHANNEL 4 -#define NAME_SIZE 32 static struct snd_soc_card *kabylake_audio_card; static struct snd_soc_jack kabylake_hdmi[3]; @@ -952,18 +951,9 @@ static int kabylake_card_late_probe(struct snd_soc_card *card) struct kbl_hdmi_pcm *pcm; struct snd_soc_component *component = NULL; int err, i = 0; - char jack_name[NAME_SIZE]; list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP, pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, &kabylake_hdmi[i], - NULL, 0); - - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &kabylake_hdmi[i]); diff --git a/sound/soc/intel/boards/kbl_rt5660.c b/sound/soc/intel/boards/kbl_rt5660.c index 3255e00..06411bf 100644 --- a/sound/soc/intel/boards/kbl_rt5660.c +++ b/sound/soc/intel/boards/kbl_rt5660.c @@ -447,25 +447,15 @@ static struct snd_soc_dai_link kabylake_rt5660_dais[] = { }; -#define NAME_SIZE 32 static int kabylake_card_late_probe(struct snd_soc_card *card) { struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(card); struct kbl_hdmi_pcm *pcm; struct snd_soc_component *component = NULL; int err, i = 0; - char jack_name[NAME_SIZE]; list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP, pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, &skylake_hdmi[i], - NULL, 0); - - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &skylake_hdmi[i]); diff --git a/sound/soc/intel/boards/kbl_rt5663_max98927.c b/sound/soc/intel/boards/kbl_rt5663_max98927.c index d714752..9c43c6c 100644 --- a/sound/soc/intel/boards/kbl_rt5663_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_max98927.c @@ -898,25 +898,15 @@ static struct snd_soc_dai_link kabylake_5663_dais[] = { }, }; -#define NAME_SIZE 32 static int kabylake_card_late_probe(struct snd_soc_card *card) { struct kbl_rt5663_private *ctx = snd_soc_card_get_drvdata(card); struct kbl_hdmi_pcm *pcm; struct snd_soc_component *component = NULL; int err, i = 0; - char jack_name[NAME_SIZE]; list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP, pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, &skylake_hdmi[i], - NULL, 0); - - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &skylake_hdmi[i]); diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c index 879f142..8fe4007 100644 --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c @@ -40,7 +40,6 @@ #define RT5663_DEV_NAME "i2c-10EC5663:00" #define RT5514_AIF1_BCLK_FREQ (48000 * 8 * 16) #define RT5514_AIF1_SYSCLK_FREQ 12288000 -#define NAME_SIZE 32 #define DMIC_CH(p) p->list[p->count-1] @@ -600,18 +599,10 @@ static int kabylake_card_late_probe(struct snd_soc_card *card) struct kbl_hdmi_pcm *pcm; struct snd_soc_component *component = NULL; int err, i = 0; - char jack_name[NAME_SIZE]; list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP,pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, &ctx->kabylake_hdmi[i], - NULL, 0); - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &ctx->kabylake_hdmi[i]); if (err < 0) diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c index 8b68f41..0f57fc2 100644 --- a/sound/soc/intel/boards/skl_hda_dsp_common.c +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c @@ -118,19 +118,10 @@ int skl_hda_hdmi_jack_init(struct snd_soc_card *card) struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card); struct snd_soc_component *component = NULL; struct skl_hda_hdmi_pcm *pcm; - char jack_name[NAME_SIZE]; int err; list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP, pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, &pcm->hdmi_jack, - NULL, 0); - - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &pcm->hdmi_jack); diff --git a/sound/soc/intel/boards/skl_nau88l25_max98357a.c b/sound/soc/intel/boards/skl_nau88l25_max98357a.c index 0922106..aca426f 100644 --- a/sound/soc/intel/boards/skl_nau88l25_max98357a.c +++ b/sound/soc/intel/boards/skl_nau88l25_max98357a.c @@ -587,26 +587,15 @@ static struct snd_soc_dai_link skylake_dais[] = { }, }; -#define NAME_SIZE 32 static int skylake_card_late_probe(struct snd_soc_card *card) { struct skl_nau8825_private *ctx = snd_soc_card_get_drvdata(card); struct skl_hdmi_pcm *pcm; struct snd_soc_component *component = NULL; int err, i = 0; - char jack_name[NAME_SIZE]; list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP, pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, - &skylake_hdmi[i], - NULL, 0); - - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &skylake_hdmi[i]); diff --git a/sound/soc/intel/boards/skl_nau88l25_ssm4567.c b/sound/soc/intel/boards/skl_nau88l25_ssm4567.c index 8433c52..788a837 100644 --- a/sound/soc/intel/boards/skl_nau88l25_ssm4567.c +++ b/sound/soc/intel/boards/skl_nau88l25_ssm4567.c @@ -638,26 +638,15 @@ static struct snd_soc_dai_link skylake_dais[] = { }, }; -#define NAME_SIZE 32 static int skylake_card_late_probe(struct snd_soc_card *card) { struct skl_nau88125_private *ctx = snd_soc_card_get_drvdata(card); struct skl_hdmi_pcm *pcm; struct snd_soc_component *component = NULL; int err, i = 0; - char jack_name[NAME_SIZE]; list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP, pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, - &skylake_hdmi[i], - NULL, 0); - - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &skylake_hdmi[i]); diff --git a/sound/soc/intel/boards/skl_rt286.c b/sound/soc/intel/boards/skl_rt286.c index 0e1818d..5d245f5 100644 --- a/sound/soc/intel/boards/skl_rt286.c +++ b/sound/soc/intel/boards/skl_rt286.c @@ -473,25 +473,15 @@ static struct snd_soc_dai_link skylake_rt286_dais[] = { }, }; -#define NAME_SIZE 32 static int skylake_card_late_probe(struct snd_soc_card *card) { struct skl_rt286_private *ctx = snd_soc_card_get_drvdata(card); struct skl_hdmi_pcm *pcm; struct snd_soc_component *component = NULL; int err, i = 0; - char jack_name[NAME_SIZE]; list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP, pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, &skylake_hdmi[i], - NULL, 0); - - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &skylake_hdmi[i]); diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c index f28fb98..660e4a6 100644 --- a/sound/soc/intel/boards/sof_rt5682.c +++ b/sound/soc/intel/boards/sof_rt5682.c @@ -22,8 +22,6 @@ #include "../../codecs/rt5682.h" #include "../../codecs/hdac_hdmi.h" -#define NAME_SIZE 32 - #define SOF_RT5682_SSP_CODEC(quirk) ((quirk) & GENMASK(2, 0)) #define SOF_RT5682_SSP_CODEC_MASK (GENMASK(2, 0)) #define SOF_RT5682_MCLK_EN BIT(3) @@ -216,7 +214,6 @@ static int sof_card_late_probe(struct snd_soc_card *card) { struct sof_card_private *ctx = snd_soc_card_get_drvdata(card); struct snd_soc_component *component = NULL; - char jack_name[NAME_SIZE]; struct sof_hdmi_pcm *pcm; int err = 0; int i = 0; @@ -227,14 +224,6 @@ static int sof_card_late_probe(struct snd_soc_card *card) list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { component = pcm->codec_dai->component; - snprintf(jack_name, sizeof(jack_name), - "HDMI/DP, pcm=%d Jack", pcm->device); - err = snd_soc_card_jack_new(card, jack_name, - SND_JACK_AVOUT, &sof_hdmi[i], - NULL, 0); - - if (err) - return err; err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, &sof_hdmi[i]); -- 2.7.4

2 1

[alsa-devel] [PATCH v2 00/15] soundwire: corrections to ACPI/DisCo/Intel support
by Pierre-Louis Bossart 27 May '19

27 May '19

Now that we are done with cleanups, we can start fixing the code with actual semantic or functional changes. This patchset corrects issues with Intel BIOS and hardware properties that prevented a successful init, aligns the code with the MIPI DisCo spec, adds rate-limiting for frequent errors and adds checks on number of links and PDIs. With all these changes, the hardware can be initialized correctly and modules can be added/removed without issues on WhiskyLake and IceLake. Parts of this code was initially written by my Intel colleagues Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah, who are either no longer with Intel or no longer involved in SoundWire development. When relevant, I explictly added a note in commit messages to give them credit for their hard work, but I removed their signed-off-by tags to avoid email bounces and avoid spamming them forever with SoundWire patches. Changes since v2: Feedback from Vinod: 1. improve the SoundWire controller search without magic values 2. split patches as needed Other additions rate-limiting to avoid flooding dmesg logs provide better Slave status on errors more checks on links and PDIs Pierre-Louis Bossart (15): soundwire: intel: filter SoundWire controller device search soundwire: mipi_disco: fix master/link error soundwire: add port-related definitions soundwire: remove master data port properties soundwire: mipi-disco: remove master_count property for masters soundwire: rename 'freq' fields soundwire: mipi-disco: fix clock stop modes soundwire: clarify comment soundwire: rename/clarify MIPI DisCo properties soundwire: cadence_master: use rate_limited dynamic debug soundwire: cadence_master: log Slave status mask on errors soundwire: cadence_master: check the number of bidir PDIs soundwire: Intel: add log for number of PCM and PDM PDIs soundwire: fix typo in comments soundwire: intel_init: add checks on link numbers drivers/soundwire/bus.c | 6 +- drivers/soundwire/cadence_master.c | 29 +++++----- drivers/soundwire/intel.c | 17 ++++-- drivers/soundwire/intel.h | 2 +- drivers/soundwire/intel_init.c | 25 ++++++++- drivers/soundwire/mipi_disco.c | 35 ++++++------ drivers/soundwire/stream.c | 8 +-- include/linux/soundwire/sdw.h | 88 +++++++++++++++++++++++------- 8 files changed, 147 insertions(+), 63 deletions(-) -- 2.20.1

2 16

[alsa-devel] [PATCH] soundwire: cdns: Fix compilation error on arm64
by Jan Kotas 27 May '19

27 May '19

On arm64 the cadence_master.c file doesn't compile. readl and writel are undefined. This patch fixes that by including io.h. Signed-off-by: Jan Kotas <jank(a)cadence.com> --- drivers/soundwire/cadence_master.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index cb6a331f4..0b5bcc209 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -9,6 +9,7 @@ #include <linux/delay.h> #include <linux/device.h> #include <linux/interrupt.h> +#include <linux/io.h> #include <linux/module.h> #include <linux/mod_devicetable.h> #include <linux/soundwire/sdw_registers.h> -- 2.15.0

4 6

[alsa-devel] [PATCH] soundwire: stream: fix out of boundary access on port properties
by Srinivas Kandagatla 27 May '19

27 May '19

Assigning local iterator to array element and using it again for indexing would cross the array boundary. Fix this by directly referring array element without using the local variable. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla(a)linaro.org> --- drivers/soundwire/stream.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index d01060dbee96..544925ff0b40 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1406,9 +1406,7 @@ struct sdw_dpn_prop *sdw_get_slave_dpn_prop(struct sdw_slave *slave, } for (i = 0; i < num_ports; i++) { - dpn_prop = &dpn_prop[i]; - - if (dpn_prop->num == port_num) + if (dpn_prop[i].num == port_num) return &dpn_prop[i]; } -- 2.21.0

3 2

[alsa-devel] [PATCH][next] ALSA: firewire-lib: remove redundant assignment to cip_header
by Colin King 25 May '19

25 May '19

From: Colin Ian King <colin.king(a)canonical.com> The assignement to cip_header is redundant as the value never read and it is being re-assigned in the if and else paths of the following if statement. Clean up the code by removing it. Addresses-Coverity: ("Unused value") Signed-off-by: Colin Ian King <colin.king(a)canonical.com> --- sound/firewire/amdtp-stream.c | 1 - 1 file changed, 1 deletion(-) diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c index 2d9c764061d1..4236955bbf57 100644 --- a/sound/firewire/amdtp-stream.c +++ b/sound/firewire/amdtp-stream.c @@ -675,7 +675,6 @@ static int handle_in_packet(struct amdtp_stream *s, unsigned int cycle, return -EIO; } - cip_header = ctx_header + 2; if (!(s->flags & CIP_NO_HEADER)) { cip_header = &ctx_header[2]; err = check_cip_header(s, cip_header, payload_length, -- 2.20.1

2 1

[alsa-devel] arecord doesnt record into file
by Aditya Paluri 25 May '19

25 May '19

Hello all, I use the below command: arecord --device plughw:0,2 -r 48000 -f S16_LE -c 2 recorded_sound.wav And as I see the audio stream getting dumped onto the DRAM/memory, I donot see recorded_sound.wav contain any data on the file system. I have tried to use strace and I could only see "write" system call happen on the file only for writing the wav format header. ioctl(4, _IOC(_IOC_READ|_IOC_WRITE, 0x41, 0x23, 0x88), 0x1ec27190) = 0 newfstatat(AT_FDCWD, "/home/root/recorded_wave.wav", 0x7fe8bfeb50, AT_SYMLINK_NOFOLLOW) = -1 ENOENT (No such file or directory) openat(AT_FDCWD, "/home/root/recorded_wave.wav", O_WRONLY|O_CREAT, 0644) = 3 write(3, "RIFF$\0\0\200WAVE", 12) = 12 write(3, "fmt \20\0\0\0", 8) = 8 write(3, "\1\0\2\0\200\273\0\0\0\356\2\0\4\0\20\0", 16) = 16 write(3, "data\0\0\0\200", 8) = 8 ioctl(4, _IOC(_IOC_READ|_IOC_WRITE, 0x41, 0x23, 0x88), 0x1ec27190) = 0 ioctl(4, _IOC(_IOC_READ, 0x41, 0x51, 0x18)[ 24.145583] [ 24.587069] [0 I also see EPIPE error due to overrun. , 0x7fe8bfeaa8) = -1 EPIPE (Broken pipe) ioctl(4, _IOC(_IOC_READ|_IOC_WRITE, 0x41, 0x24, 0x98), 0x7fe8bfe9b0) = 0 write(2, "overrun!!! (at least 0.717 ms lo"..., 36overrun!!! (at least 0.717 ms long) ) = 36 Could you please help me in understanding why the recorded_sound.wav doesn't contain any data ? BR\Thanks. Aditya Paluri.

2 1

[alsa-devel] [PATCH] tegra_wm9712: Fix a memory leaking bug in tegra_wm9712_driver_probe()
by Gen Zhang 24 May '19

24 May '19

In tegra_wm9712_driver_probe(), 'machine->codec' is allocated by platform_device_alloc(). When it is NULL, function returns ENOMEM. However, 'machine' is allocated by devm_kzalloc() before this site. Thus we should free 'machine' before function ends to prevent memory leaking. Further, we should free 'machine->util_data', 'machine->codec' and 'machine' before this function normally ends to prevent memory leaking. Signed-off-by: Gen Zhang <blackgod016574(a)gmail.com> --- diff --git a/sound/soc/tegra/tegra_wm9712.c b/sound/soc/tegra/tegra_wm9712.c index 864a334..295c41d 100644 --- a/sound/soc/tegra/tegra_wm9712.c +++ b/sound/soc/tegra/tegra_wm9712.c @@ -86,7 +86,8 @@ static int tegra_wm9712_driver_probe(struct platform_device *pdev) machine->codec = platform_device_alloc("wm9712-codec", -1); if (!machine->codec) { dev_err(&pdev->dev, "Can't allocate wm9712 platform device\n"); - return -ENOMEM; + ret = -ENOMEM; + goto codec_free; } ret = platform_device_add(machine->codec); @@ -127,6 +128,10 @@ static int tegra_wm9712_driver_probe(struct platform_device *pdev) goto asoc_utils_fini; } + tegra_asoc_utils_fini(&machine->util_data); + platform_device_del(machine->codec); + platform_device_put(machine->codec); + devm_kfree(&pdev->dev, machine); return 0; asoc_utils_fini: @@ -135,6 +140,8 @@ static int tegra_wm9712_driver_probe(struct platform_device *pdev) platform_device_del(machine->codec); codec_put: platform_device_put(machine->codec); +codec_free: + devm_kfree(&pdev->dev, machine); return ret; } ---

2 6

Re: [alsa-devel] [PATCH 2/5] ASoC: madera: Add common support for Cirrus Logic Madera codecs
by Charles Keepax 24 May '19

24 May '19

On Fri, May 24, 2019 at 03:56:03PM +0100, Mark Brown wrote: > On Fri, May 24, 2019 at 11:41:55AM +0100, Charles Keepax wrote: > > > + /* > > + * Just read a register a few times to ensure the internal > > + * oscillator sends out a few clocks. > > + */ > > + for (i = 0; i < 4; i++) { > > + ret = regmap_read(madera->regmap, MADERA_SOFTWARE_RESET, &val); > > + if (ret) > > + dev_err(madera->dev, > > + "%s Failed to read register: %d (%d)\n", > > + __func__, ret, i); > > Why use %s to format the __func__ rather than just concatenate? > Really the error message is quite out of character in general, we don't normally print the func and it doesn't really say what failed. I will refactor it. > > + } > > + > > + udelay(300); > > So we read the register a few times then add a few hundred us of delay > after? Surely that delay is going to be negligable compared to the time > spent on I/O? > This is one of those technically situations. We have to do the reads and we have to have at least 300uS after those. Normally there will be enough slack in the system but better to guarantee it. Maybe a comment? > > +int madera_sysclk_ev(struct snd_soc_dapm_widget *w, > > + struct snd_kcontrol *kcontrol, int event) > > +{ > > + struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); > > + struct madera_priv *priv = snd_soc_component_get_drvdata(component); > > + > > + madera_spin_sysclk(priv); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(madera_sysclk_ev); > > This will delay both before and after every power up and power down. > Are you sure that makes sense? > Regrettably yes, or well at least I am sure that despite a lot of complaining this is what the hardware guys insisted needed done. > > + > > + ret = madera_check_speaker_overheat(madera, &warn, &shutdown); > > + if (ret) > > + shutdown = true; /* for safety attempt to shutdown on error */ > > + > > + if (shutdown) { > > + dev_crit(madera->dev, "Thermal shutdown\n"); > > + ret = regmap_update_bits(madera->regmap, > > + MADERA_OUTPUT_ENABLES_1, > > + MADERA_OUT4L_ENA | > > + MADERA_OUT4R_ENA, 0); > > + if (ret != 0) > > + dev_crit(madera->dev, > > + "Failed to disable speaker outputs: %d\n", > > + ret); > > + } else if (warn) { > > + dev_crit(madera->dev, "Thermal warning\n"); > > + } > > + > > + return IRQ_HANDLED; > > We will flag the interrupt as handled if there was neither a warning nor > a critical overheat? I'd expect some warning about a spurious interrupt > at least. > Yeah I will update that one. > > +static int madera_get_variable_u32_array(struct madera_priv *priv, > > + const char *propname, > > + u32 *dest, > > + int n_max, > > + int multiple) > > +{ > > + struct madera *madera = priv->madera; > > + int n, ret; > > + > > + n = device_property_read_u32_array(madera->dev, propname, NULL, 0); > > + if (n == -EINVAL) { > > + return 0; /* missing, ignore */ > > + } else if (n < 0) { > > + dev_warn(madera->dev, "%s malformed (%d)\n", > > + propname, n); > > + return n; > > + } else if ((n % multiple) != 0) { > > + dev_warn(madera->dev, "%s not a multiple of %d entries\n", > > + propname, multiple); > > + return -EINVAL; > > + } > > + > > + if (n > n_max) > > + n = n_max; > > + > > + ret = device_property_read_u32_array(madera->dev, propname, dest, n); > > + > > + if (ret < 0) > > + return ret; > > + else > > + return n; > > +} > > This feels like it should perhaps be a generic OF helper function - > there's nothing driver specific I'm seeing here and arrays that need to > be a multiple of N entries aren't that uncommon I think. > I will have a look and see if we might do a generic helper function for this one. > > + mutex_lock(&priv->rate_lock); > > + cached_rate = priv->adsp_rate_cache[adsp_num]; > > + mutex_unlock(&priv->rate_lock); > > What's this lock protecting? The value can we read can change as soon > as the lock is released and we're just reading a single word here rather > than traversing a data structure that might change under us or > something. > Overall the lock protects anything that changes a rate on the chip, since we have to do that weird spin_sysclk thing before/after anything that does that. It might normally be safe without the lock here I guess, since you are only really protecting against the reads of adsp_rate_cache. But suppose that might depend on the architecture if the read/writes arn't effectively atomic (although that is probably unlikely on most modern architectures). > > +void madera_destroy_bus_error_irq(struct madera_priv *priv, int dsp_num) > > +{ > > + struct madera *madera = priv->madera; > > + > > + madera_free_irq(madera, > > + madera_dsp_bus_error_irqs[dsp_num], > > + &priv->adsp[dsp_num]); > > +} > > +EXPORT_SYMBOL_GPL(madera_destroy_bus_error_irq); > > We use free rather than destroy normally? > Yup will fix that. > > +static const char * const madera_dfc_width_text[MADERA_DFC_WIDTH_ENUM_SIZE] = { > > + "8bit", "16bit", "20bit", "24bit", "32bit", > > +}; > > Spaces might make these more readable. > No problem to fix that too. > > +static void madera_sleep(unsigned int delay) > > +{ > > + if (delay < 20) { > > + delay *= 1000; > > + usleep_range(delay, delay + 500); > > + } else { > > + msleep(delay); > > + } > > +} > > This feels like it might make sense as a helper function as well - I > could've sworn there was one already but I can't immediately find it. I do vaguely remember some discussion that it was preferred that people had to think about which delay function to call, rather than having a generic helper. TBF I could probably just switch it all to use msleep, for the most part this is a quite small optimisation of path bring up (although it was asked for by one of our customers). Thanks, Charles

1 0

Re: [alsa-devel] [PATCH 2/5] ASoC: madera: Add common support for Cirrus Logic Madera codecs
by Richard Fitzgerald 24 May '19

24 May '19

On 24/05/19 15:56, Mark Brown wrote: > On Fri, May 24, 2019 at 11:41:55AM +0100, Charles Keepax wrote: > >> + /* >> + * Just read a register a few times to ensure the internal >> + * oscillator sends out a few clocks. >> + */ >> + for (i = 0; i < 4; i++) { >> + ret = regmap_read(madera->regmap, MADERA_SOFTWARE_RESET, &val); >> + if (ret) >> + dev_err(madera->dev, >> + "%s Failed to read register: %d (%d)\n", >> + __func__, ret, i); > > Why use %s to format the __func__ rather than just concatenate? GCC docs say that it's a magic variable so cannot be concatenated with string literals. Though I never tried concatenation to see if it works. > >> + } >> + >> + udelay(300); > > So we read the register a few times then add a few hundred us of delay > after? Surely that delay is going to be negligable compared to the time > spent on I/O? The register reads are to create clock cycles in the silicon, not to generate delay. > >> +int madera_sysclk_ev(struct snd_soc_dapm_widget *w, >> + struct snd_kcontrol *kcontrol, int event) >> +{ >> + struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); >> + struct madera_priv *priv = snd_soc_component_get_drvdata(component); >> + >> + madera_spin_sysclk(priv); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(madera_sysclk_ev); > > This will delay both before and after every power up and power down. > Are you sure that makes sense? > I think that's correct but we can re-check with hardware people. It's not just a delay, it needs to ensure there are always a certain number of SYSCLK cycles in the hardware to avoid leaving certain state machines in limbo. >> + >> + ret = madera_check_speaker_overheat(madera, &warn, &shutdown); >> + if (ret) >> + shutdown = true; /* for safety attempt to shutdown on error */ >> + >> + if (shutdown) { >> + dev_crit(madera->dev, "Thermal shutdown\n"); >> + ret = regmap_update_bits(madera->regmap, >> + MADERA_OUTPUT_ENABLES_1, >> + MADERA_OUT4L_ENA | >> + MADERA_OUT4R_ENA, 0); >> + if (ret != 0) >> + dev_crit(madera->dev, >> + "Failed to disable speaker outputs: %d\n", >> + ret); >> + } else if (warn) { >> + dev_crit(madera->dev, "Thermal warning\n"); >> + } >> + >> + return IRQ_HANDLED; > > We will flag the interrupt as handled if there was neither a warning nor > a critical overheat? I'd expect some warning about a spurious interrupt > at least. > >> +static int madera_get_variable_u32_array(struct madera_priv *priv, >> + const char *propname, >> + u32 *dest, >> + int n_max, >> + int multiple) >> +{ >> + struct madera *madera = priv->madera; >> + int n, ret; >> + >> + n = device_property_read_u32_array(madera->dev, propname, NULL, 0); >> + if (n == -EINVAL) { >> + return 0; /* missing, ignore */ >> + } else if (n < 0) { >> + dev_warn(madera->dev, "%s malformed (%d)\n", >> + propname, n); >> + return n; >> + } else if ((n % multiple) != 0) { >> + dev_warn(madera->dev, "%s not a multiple of %d entries\n", >> + propname, multiple); >> + return -EINVAL; >> + } >> + >> + if (n > n_max) >> + n = n_max; >> + >> + ret = device_property_read_u32_array(madera->dev, propname, dest, n); >> + >> + if (ret < 0) >> + return ret; >> + else >> + return n; >> +} > > This feels like it should perhaps be a generic OF helper function - > there's nothing driver specific I'm seeing here and arrays that need to > be a multiple of N entries aren't that uncommon I think. > >> + mutex_lock(&priv->rate_lock); >> + cached_rate = priv->adsp_rate_cache[adsp_num]; >> + mutex_unlock(&priv->rate_lock); > > What's this lock protecting? The value can we read can change as soon > as the lock is released and we're just reading a single word here rather > than traversing a data structure that might change under us or > something. > >> +void madera_destroy_bus_error_irq(struct madera_priv *priv, int dsp_num) >> +{ >> + struct madera *madera = priv->madera; >> + >> + madera_free_irq(madera, >> + madera_dsp_bus_error_irqs[dsp_num], >> + &priv->adsp[dsp_num]); >> +} >> +EXPORT_SYMBOL_GPL(madera_destroy_bus_error_irq); > > We use free rather than destroy normally? > >> +static const char * const madera_dfc_width_text[MADERA_DFC_WIDTH_ENUM_SIZE] = { >> + "8bit", "16bit", "20bit", "24bit", "32bit", >> +}; > > Spaces might make these more readable. > >> +static void madera_sleep(unsigned int delay) >> +{ >> + if (delay < 20) { >> + delay *= 1000; >> + usleep_range(delay, delay + 500); >> + } else { >> + msleep(delay); >> + } >> +} > > This feels like it might make sense as a helper function as well - I > could've sworn there was one already but I can't immediately find it. >

1 1