Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 3/4] ASoC: wm8904: automatically choose clock source
From: Michał Mirosław @ 2019-08-25 12:17 UTC (permalink / raw)
  To: alsa-devel, patches
  Cc: Kate Stewart, Maxime Jourdan, Alexandre Belloni,
	Kuninori Morimoto, Kirill Marinushkin, Takashi Iwai,
	Paul Cercueil, Srinivas Kandagatla, Jerome Brunet, Anders Roxell,
	Ludovic Desroches, linux-arm-kernel, Codrin Ciubotariu,
	Charles Keepax, Liam Girdwood, Piotr Stankiewicz,
	Annaliese McDermond, Richard Fitzgerald, Mark Brown,
	Nariman Poushin, Thomas Gleixner, Jaroslav Kysela, zhong jiang,
	Allison Randal, Greg Kroah-Hartman, Randy Dunlap, Nikesh Oswal,
	linux-kernel, Enrico Weigelt
In-Reply-To: <cover.1566734630.git.mirq-linux@rere.qmqm.pl>

Choose clock source automatically if not provided. This will be the case
with eg. audio-graph-card.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 sound/soc/codecs/wm8904.c | 42 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c
index c9318fe34f91..946315d4cecf 100644
--- a/sound/soc/codecs/wm8904.c
+++ b/sound/soc/codecs/wm8904.c
@@ -367,15 +367,34 @@ static int wm8904_enable_sysclk(struct wm8904_priv *priv)
 	return err;
 }
 
+static int wm8904_bump_fll_sysclk(unsigned int *rate);
+
 static int wm8904_set_sysclk(struct snd_soc_dai *dai, int clk_id,
 			     unsigned int rate, int dir)
 {
 	struct snd_soc_component *component = dai->component;
 	struct wm8904_priv *wm8904 = snd_soc_component_get_drvdata(component);
 	unsigned int clock0, clock2;
-	int err;
+	int err, do_div = false;
 
 	switch (clk_id) {
+	case 0:
+		if (rate == clk_round_rate(wm8904->mclk, rate)) {
+			clk_id = WM8904_CLK_MCLK;
+		} else if (rate * 2 == clk_round_rate(wm8904->mclk, rate * 2)) {
+			rate *= 2;
+			clk_id = WM8904_CLK_MCLK;
+			do_div = true;
+		} else {
+			clk_id = WM8904_CLK_FLL;
+			err = wm8904_bump_fll_sysclk(&rate);
+			if (err) {
+				dev_dbg(component->dev, "Can't match %u over FLL 1406250 Hz minimum\n", rate);
+				return err;
+			}
+		}
+		break;
+
 	case WM8904_CLK_MCLK:
 	case WM8904_CLK_FLL:
 		break;
@@ -421,7 +440,9 @@ static int wm8904_set_sysclk(struct snd_soc_dai *dai, int clk_id,
 	}
 
 	/* SYSCLK shouldn't be over 13.5MHz */
-	if (rate > 13500000) {
+	if (rate > 13500000)
+		do_div = true;
+	if (do_div) {
 		clock0 = WM8904_MCLK_DIV;
 		wm8904->sysclk_rate = rate / 2;
 	} else {
@@ -1350,6 +1371,23 @@ static struct {
 	{ 480, 20 },
 };
 
+static int wm8904_bump_fll_sysclk(unsigned int *rate)
+{
+	int i;
+
+	/* bump SYSCLK rate if below minimal FLL output */
+
+	for (i = 0; i < ARRAY_SIZE(bclk_divs); i++) {
+		if (*rate * bclk_divs[i].div >= 1406250 * 10)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(bclk_divs))
+		return -ERANGE;
+
+	*rate = (*rate * bclk_divs[i].div) / 10;
+	return 0;
+}
 
 static int wm8904_hw_params(struct snd_pcm_substream *substream,
 			    struct snd_pcm_hw_params *params,
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH v2 4/4] [RFT] ASoC: wm8994: use common FLL code
From: Michał Mirosław @ 2019-08-25 12:17 UTC (permalink / raw)
  To: alsa-devel, patches
  Cc: Maxime Jourdan, Kate Stewart, Alexandre Belloni,
	Kuninori Morimoto, Kirill Marinushkin, Liam Girdwood,
	Paul Cercueil, Srinivas Kandagatla, Jerome Brunet, Anders Roxell,
	Ludovic Desroches, linux-arm-kernel, Codrin Ciubotariu,
	Charles Keepax, Piotr Stankiewicz, Nariman Poushin,
	Richard Fitzgerald, Mark Brown, Annaliese McDermond,
	Thomas Gleixner, Jaroslav Kysela, zhong jiang, Allison Randal,
	Greg Kroah-Hartman, Randy Dunlap, Nikesh Oswal, Takashi Iwai,
	linux-kernel, Enrico Weigelt
In-Reply-To: <cover.1566734630.git.mirq-linux@rere.qmqm.pl>

Rework FLL handling to use common code.
This uses polling for now to wait for FLL lock.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 sound/soc/codecs/Kconfig  |   2 +
 sound/soc/codecs/wm8994.c | 281 +++++++++++---------------------------
 sound/soc/codecs/wm8994.h |   4 +-
 3 files changed, 84 insertions(+), 203 deletions(-)

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 1a680023af7d..1ff6290ce18d 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -1382,6 +1382,8 @@ config SND_SOC_WM8993
 
 config SND_SOC_WM8994
 	tristate
+	select SND_SOC_WM_FLL
+	select SND_SOC_WM_FLL_EFS
 
 config SND_SOC_WM8995
 	tristate
diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c
index c3d06e8bc54f..d0dbc352303b 100644
--- a/sound/soc/codecs/wm8994.c
+++ b/sound/soc/codecs/wm8994.c
@@ -2030,101 +2030,57 @@ static const struct snd_soc_dapm_route wm8958_intercon[] = {
 	{ "AIF3ADC Mux", "Mono PCM", "Mono PCM Out Mux" },
 };
 
-/* The size in bits of the FLL divide multiplied by 10
- * to allow rounding later */
-#define FIXED_FLL_SIZE ((1 << 16) * 10)
-
-struct fll_div {
-	u16 outdiv;
-	u16 n;
-	u16 k;
-	u16 lambda;
-	u16 clk_ref_div;
-	u16 fll_fratio;
+static const struct wm_fll_desc wm8994_fll_desc[2] = {
+	/* FLL1 */
+	{
+		.ctl_offset = WM8994_FLL1_CONTROL_1,
+		.int_offset = WM8994_INTERRUPT_RAW_STATUS_2,
+		.int_mask = WM8994_IM_FLL1_LOCK_EINT_MASK,
+		.nco_reg0 = WM8994_FLL1_CONTROL_5,
+		.frc_nco_shift = 6,
+		.nco_reg1 = WM8994_FLL1_CONTROL_5,
+		.frc_nco_val_shift = 7,
+		.clk_ref_map = { FLL_REF_MCLK, FLL_REF_MCLK2, FLL_REF_FSCLK, FLL_REF_BCLK },
+	},
+	/* FLL2 */
+	{
+		.ctl_offset = WM8994_FLL2_CONTROL_1,
+		.int_offset = WM8994_INTERRUPT_RAW_STATUS_2,
+		.int_mask = WM8994_IM_FLL2_LOCK_EINT_MASK,
+		.nco_reg0 = WM8994_FLL2_CONTROL_5,
+		.frc_nco_shift = 6,
+		.nco_reg1 = WM8994_FLL2_CONTROL_5,
+		.frc_nco_val_shift = 7,
+		.clk_ref_map = { FLL_REF_MCLK, FLL_REF_MCLK2, FLL_REF_FSCLK, FLL_REF_BCLK },
+	},
 };
 
-static int wm8994_get_fll_config(struct wm8994 *control, struct fll_div *fll,
-				 int freq_in, int freq_out)
-{
-	u64 Kpart;
-	unsigned int K, Ndiv, Nmod, gcd_fll;
-
-	pr_debug("FLL input=%dHz, output=%dHz\n", freq_in, freq_out);
-
-	/* Scale the input frequency down to <= 13.5MHz */
-	fll->clk_ref_div = 0;
-	while (freq_in > 13500000) {
-		fll->clk_ref_div++;
-		freq_in /= 2;
-
-		if (fll->clk_ref_div > 3)
-			return -EINVAL;
-	}
-	pr_debug("CLK_REF_DIV=%d, Fref=%dHz\n", fll->clk_ref_div, freq_in);
-
-	/* Scale the output to give 90MHz<=Fvco<=100MHz */
-	fll->outdiv = 3;
-	while (freq_out * (fll->outdiv + 1) < 90000000) {
-		fll->outdiv++;
-		if (fll->outdiv > 63)
-			return -EINVAL;
-	}
-	freq_out *= fll->outdiv + 1;
-	pr_debug("OUTDIV=%d, Fvco=%dHz\n", fll->outdiv, freq_out);
-
-	if (freq_in > 1000000) {
-		fll->fll_fratio = 0;
-	} else if (freq_in > 256000) {
-		fll->fll_fratio = 1;
-		freq_in *= 2;
-	} else if (freq_in > 128000) {
-		fll->fll_fratio = 2;
-		freq_in *= 4;
-	} else if (freq_in > 64000) {
-		fll->fll_fratio = 3;
-		freq_in *= 8;
-	} else {
-		fll->fll_fratio = 4;
-		freq_in *= 16;
-	}
-	pr_debug("FLL_FRATIO=%d, Fref=%dHz\n", fll->fll_fratio, freq_in);
-
-	/* Now, calculate N.K */
-	Ndiv = freq_out / freq_in;
-
-	fll->n = Ndiv;
-	Nmod = freq_out % freq_in;
-	pr_debug("Nmod=%d\n", Nmod);
-
-	switch (control->type) {
-	case WM8994:
-		/* Calculate fractional part - scale up so we can round. */
-		Kpart = FIXED_FLL_SIZE * (long long)Nmod;
-
-		do_div(Kpart, freq_in);
-
-		K = Kpart & 0xFFFFFFFF;
-
-		if ((K % 10) >= 5)
-			K += 5;
-
-		/* Move down to proper range now rounding is done */
-		fll->k = K / 10;
-		fll->lambda = 0;
-
-		pr_debug("N=%x K=%x\n", fll->n, fll->k);
-		break;
-
-	default:
-		gcd_fll = gcd(freq_out, freq_in);
-
-		fll->k = (freq_out - (freq_in * fll->n)) / gcd_fll;
-		fll->lambda = freq_in / gcd_fll;
-		
-	}
-
-	return 0;
-}
+static const struct wm_fll_desc wm8958_fll_desc[2] = {
+	/* FLL1 */
+	{
+		.ctl_offset = WM8994_FLL1_CONTROL_1,
+		.int_offset = WM8994_INTERRUPT_RAW_STATUS_2,
+		.int_mask = WM8994_IM_FLL1_LOCK_EINT_MASK,
+		.nco_reg0 = WM8994_FLL1_CONTROL_5,
+		.frc_nco_shift = 6,
+		.nco_reg1 = WM8994_FLL1_CONTROL_5,
+		.frc_nco_val_shift = 7,
+		.efs_offset = WM8958_FLL1_EFS_1,
+		.clk_ref_map = { FLL_REF_MCLK, FLL_REF_MCLK2, FLL_REF_FSCLK, FLL_REF_BCLK },
+	},
+	/* FLL2 */
+	{
+		.ctl_offset = WM8994_FLL2_CONTROL_1,
+		.int_offset = WM8994_INTERRUPT_RAW_STATUS_2,
+		.int_mask = WM8994_IM_FLL2_LOCK_EINT_MASK,
+		.nco_reg0 = WM8994_FLL2_CONTROL_5,
+		.frc_nco_shift = 6,
+		.nco_reg1 = WM8994_FLL2_CONTROL_5,
+		.frc_nco_val_shift = 7,
+		.efs_offset = WM8958_FLL1_EFS_1,
+		.clk_ref_map = { FLL_REF_MCLK, FLL_REF_MCLK2, FLL_REF_FSCLK, FLL_REF_BCLK },
+	},
+};
 
 static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
 			  unsigned int freq_in, unsigned int freq_out)
@@ -2132,9 +2088,7 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
 	struct wm8994_priv *wm8994 = snd_soc_component_get_drvdata(component);
 	struct wm8994 *control = wm8994->wm8994;
 	int reg_offset, ret;
-	struct fll_div fll;
 	u16 reg, clk1, aif_reg, aif_src;
-	unsigned long timeout;
 	bool was_enabled;
 
 	switch (id) {
@@ -2152,9 +2106,6 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
 		return -EINVAL;
 	}
 
-	reg = snd_soc_component_read32(component, WM8994_FLL1_CONTROL_1 + reg_offset);
-	was_enabled = reg & WM8994_FLL1_ENA;
-
 	switch (src) {
 	case 0:
 		/* Allow no source specification when stopping */
@@ -2166,10 +2117,12 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
 	case WM8994_FLL_SRC_MCLK2:
 	case WM8994_FLL_SRC_LRCLK:
 	case WM8994_FLL_SRC_BCLK:
+		src = wm8994_fll_desc[0].clk_ref_map[src - WM8994_FLL_SRC_MCLK1];
 		break;
 	case WM8994_FLL_SRC_INTERNAL:
 		freq_in = 12000000;
 		freq_out = 12000000;
+		src = FLL_REF_OSC;
 		break;
 	default:
 		return -EINVAL;
@@ -2180,18 +2133,6 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
 	    wm8994->fll[id].in == freq_in && wm8994->fll[id].out == freq_out)
 		return 0;
 
-	/* If we're stopping the FLL redo the old config - no
-	 * registers will actually be written but we avoid GCC flow
-	 * analysis bugs spewing warnings.
-	 */
-	if (freq_out)
-		ret = wm8994_get_fll_config(control, &fll, freq_in, freq_out);
-	else
-		ret = wm8994_get_fll_config(control, &fll, wm8994->fll[id].in,
-					    wm8994->fll[id].out);
-	if (ret < 0)
-		return ret;
-
 	/* Make sure that we're not providing SYSCLK right now */
 	clk1 = snd_soc_component_read32(component, WM8994_CLOCKING_1);
 	if (clk1 & WM8994_SYSCLK_SRC)
@@ -2207,9 +2148,11 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
 		return -EBUSY;
 	}
 
-	/* We always need to disable the FLL while reconfiguring */
-	snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_1 + reg_offset,
-			    WM8994_FLL1_ENA, 0);
+	was_enabled = wm_fll_is_enabled(&wm8994->fll_hw[id]) > 0;
+
+	ret = wm_fll_disable(&wm8994->fll_hw[id]);
+	if (ret)
+		return ret;
 
 	if (wm8994->fll_byp && src == WM8994_FLL_SRC_BCLK &&
 	    freq_in == freq_out && freq_out) {
@@ -2217,46 +2160,21 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
 		snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_5 + reg_offset,
 				    WM8958_FLL1_BYP, WM8958_FLL1_BYP);
 		goto out;
+	} else if (wm8994->fll_byp) {
+		snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_5 + reg_offset,
+				    WM8958_FLL1_BYP, 0);
 	}
 
-	reg = (fll.outdiv << WM8994_FLL1_OUTDIV_SHIFT) |
-		(fll.fll_fratio << WM8994_FLL1_FRATIO_SHIFT);
-	snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_2 + reg_offset,
-			    WM8994_FLL1_OUTDIV_MASK |
-			    WM8994_FLL1_FRATIO_MASK, reg);
-
-	snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_3 + reg_offset,
-			    WM8994_FLL1_K_MASK, fll.k);
-
-	snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_4 + reg_offset,
-			    WM8994_FLL1_N_MASK,
-			    fll.n << WM8994_FLL1_N_SHIFT);
-
-	if (fll.lambda) {
-		snd_soc_component_update_bits(component, WM8958_FLL1_EFS_1 + reg_offset,
-				    WM8958_FLL1_LAMBDA_MASK,
-				    fll.lambda);
-		snd_soc_component_update_bits(component, WM8958_FLL1_EFS_2 + reg_offset,
-				    WM8958_FLL1_EFS_ENA, WM8958_FLL1_EFS_ENA);
-	} else {
-		snd_soc_component_update_bits(component, WM8958_FLL1_EFS_2 + reg_offset,
-				    WM8958_FLL1_EFS_ENA, 0);
-	}
-
-	snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_5 + reg_offset,
-			    WM8994_FLL1_FRC_NCO | WM8958_FLL1_BYP |
-			    WM8994_FLL1_REFCLK_DIV_MASK |
-			    WM8994_FLL1_REFCLK_SRC_MASK,
-			    ((src == WM8994_FLL_SRC_INTERNAL)
-			     << WM8994_FLL1_FRC_NCO_SHIFT) |
-			    (fll.clk_ref_div << WM8994_FLL1_REFCLK_DIV_SHIFT) |
-			    (src - 1));
-
-	/* Clear any pending completion from a previous failure */
-	try_wait_for_completion(&wm8994->fll_locked[id]);
-
-	/* Enable (with fractional mode if required) */
 	if (freq_out) {
+		wm8994->fll_hw[id].freq_in = freq_in;
+		ret = wm_fll_set_parent(&wm8994->fll_hw[id], src);
+		if (!ret)
+			ret = wm_fll_set_rate(&wm8994->fll_hw[id], freq_out);
+		if (!ret)
+			ret = wm_fll_enable(&wm8994->fll_hw[id]);
+		if (ret < 0)
+			return ret;
+
 		/* Enable VMID if we need it */
 		if (!was_enabled) {
 			active_reference(component);
@@ -2273,27 +2191,6 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
 				break;
 			}
 		}
-
-		reg = WM8994_FLL1_ENA;
-
-		if (fll.k)
-			reg |= WM8994_FLL1_FRAC;
-		if (src == WM8994_FLL_SRC_INTERNAL)
-			reg |= WM8994_FLL1_OSC_ENA;
-
-		snd_soc_component_update_bits(component, WM8994_FLL1_CONTROL_1 + reg_offset,
-				    WM8994_FLL1_ENA | WM8994_FLL1_OSC_ENA |
-				    WM8994_FLL1_FRAC, reg);
-
-		if (wm8994->fll_locked_irq) {
-			timeout = wait_for_completion_timeout(&wm8994->fll_locked[id],
-							      msecs_to_jiffies(10));
-			if (timeout == 0)
-				dev_warn(component->dev,
-					 "Timed out waiting for FLL lock\n");
-		} else {
-			msleep(5);
-		}
 	} else {
 		if (was_enabled) {
 			switch (control->type) {
@@ -2350,15 +2247,6 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
 	return 0;
 }
 
-static irqreturn_t wm8994_fll_locked_irq(int irq, void *data)
-{
-	struct completion *completion = data;
-
-	complete(completion);
-
-	return IRQ_HANDLED;
-}
-
 static int opclk_divs[] = { 10, 20, 30, 40, 55, 60, 80, 120, 160 };
 
 static int wm8994_set_fll(struct snd_soc_dai *dai, int id, int src,
@@ -3992,6 +3880,18 @@ static int wm8994_component_probe(struct snd_soc_component *component)
 
 	snd_soc_component_init_regmap(component, control->regmap);
 
+	for (i = 0; i < ARRAY_SIZE(wm8994->fll_hw); ++i) {
+		wm8994->fll_hw[i].regmap = control->regmap;
+		if (control->type == WM8994)
+			wm8994->fll_hw[i].desc = wm8994_fll_desc;
+		else
+			wm8994->fll_hw[i].desc = wm8958_fll_desc;
+
+		ret = wm_fll_init(&wm8994->fll_hw[i]);
+		if (ret)
+			return ret;
+	}
+
 	wm8994->hubs.component = component;
 
 	mutex_init(&wm8994->accdet_lock);
@@ -4013,9 +3913,6 @@ static int wm8994_component_probe(struct snd_soc_component *component)
 
 	INIT_DELAYED_WORK(&wm8994->mic_complete_work, wm8958_mic_work);
 
-	for (i = 0; i < ARRAY_SIZE(wm8994->fll_locked); i++)
-		init_completion(&wm8994->fll_locked[i]);
-
 	wm8994->micdet_irq = control->pdata.micdet_irq;
 
 	/* By default use idle_bias_off, will override for WM8994 */
@@ -4166,16 +4063,6 @@ static int wm8994_component_probe(struct snd_soc_component *component)
 		break;
 	}
 
-	wm8994->fll_locked_irq = true;
-	for (i = 0; i < ARRAY_SIZE(wm8994->fll_locked); i++) {
-		ret = wm8994_request_irq(wm8994->wm8994,
-					 WM8994_IRQ_FLL1_LOCK + i,
-					 wm8994_fll_locked_irq, "FLL lock",
-					 &wm8994->fll_locked[i]);
-		if (ret != 0)
-			wm8994->fll_locked_irq = false;
-	}
-
 	/* Make sure we can read from the GPIOs if they're inputs */
 	pm_runtime_get_sync(component->dev);
 
@@ -4377,9 +4264,6 @@ static int wm8994_component_probe(struct snd_soc_component *component)
 	wm8994_free_irq(wm8994->wm8994, WM8994_IRQ_MIC1_SHRT, wm8994);
 	if (wm8994->micdet_irq)
 		free_irq(wm8994->micdet_irq, wm8994);
-	for (i = 0; i < ARRAY_SIZE(wm8994->fll_locked); i++)
-		wm8994_free_irq(wm8994->wm8994, WM8994_IRQ_FLL1_LOCK + i,
-				&wm8994->fll_locked[i]);
 	wm8994_free_irq(wm8994->wm8994, WM8994_IRQ_DCS_DONE,
 			&wm8994->hubs);
 	wm8994_free_irq(wm8994->wm8994, WM8994_IRQ_FIFOS_ERR, component);
@@ -4393,11 +4277,6 @@ static void wm8994_component_remove(struct snd_soc_component *component)
 {
 	struct wm8994_priv *wm8994 = snd_soc_component_get_drvdata(component);
 	struct wm8994 *control = wm8994->wm8994;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(wm8994->fll_locked); i++)
-		wm8994_free_irq(wm8994->wm8994, WM8994_IRQ_FLL1_LOCK + i,
-				&wm8994->fll_locked[i]);
 
 	wm8994_free_irq(wm8994->wm8994, WM8994_IRQ_DCS_DONE,
 			&wm8994->hubs);
diff --git a/sound/soc/codecs/wm8994.h b/sound/soc/codecs/wm8994.h
index 1d6f2abe1c11..9c61d95c9053 100644
--- a/sound/soc/codecs/wm8994.h
+++ b/sound/soc/codecs/wm8994.h
@@ -13,6 +13,7 @@
 #include <linux/mutex.h>
 
 #include "wm_hubs.h"
+#include "wm_fll.h"
 
 /* Sources for AIF1/2 SYSCLK - use with set_dai_sysclk() */
 #define WM8994_SYSCLK_MCLK1 1
@@ -80,8 +81,7 @@ struct wm8994_priv {
 	int aifdiv[2];
 	int channels[2];
 	struct wm8994_fll_config fll[2], fll_suspend[2];
-	struct completion fll_locked[2];
-	bool fll_locked_irq;
+	struct wm_fll_data fll_hw[2];
 	bool fll_byp;
 	bool clk_has_run;
 
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH v2 0/4] wm8904: adapt driver for use with audio-graph-card
From: Michał Mirosław @ 2019-08-25 12:17 UTC (permalink / raw)
  To: alsa-devel, patches
  Cc: Kate Stewart, Maxime Jourdan, Alexandre Belloni,
	Kuninori Morimoto, Kirill Marinushkin, Liam Girdwood,
	Paul Cercueil, Srinivas Kandagatla, Jerome Brunet, Anders Roxell,
	Takashi Iwai, Ludovic Desroches, linux-arm-kernel,
	Codrin Ciubotariu, Charles Keepax, Piotr Stankiewicz,
	Annaliese McDermond, Richard Fitzgerald, Mark Brown,
	Nariman Poushin, Thomas Gleixner, Jaroslav Kysela, zhong jiang,
	Allison Randal, Greg Kroah-Hartman, Randy Dunlap, Nikesh Oswal,
	linux-kernel, Enrico Weigelt

This series allows to use WM8904 codec as audio-graph-card component.
It starts with rework of FLL handling in the codec's driver, and as an
example includes (untested) rework for codec with similar FLL: WM8994.

Series based on tiwai/sound/for-next tree. You can also pull from:
   https://rere.qmqm.pl/git/linux
branch:
   wm8904

(branch includes two fixes already sent to alsa-devel, but not merged yet).

Michał Mirosław (4):
  ASoC: wm_fll: extract common code for Wolfson FLLs
  ASoC: wm8904: use common FLL code
  ASoC: wm8904: automatically choose clock source
  [RFT] ASoC: wm8994: use common FLL code

 sound/soc/atmel/atmel_wm8904.c |  11 +-
 sound/soc/codecs/Kconfig       |   9 +
 sound/soc/codecs/Makefile      |   2 +
 sound/soc/codecs/wm8904.c      | 516 +++++++++++---------------------
 sound/soc/codecs/wm8904.h      |   5 -
 sound/soc/codecs/wm8994.c      | 281 +++++-------------
 sound/soc/codecs/wm8994.h      |   4 +-
 sound/soc/codecs/wm_fll.c      | 518 +++++++++++++++++++++++++++++++++
 sound/soc/codecs/wm_fll.h      |  60 ++++
 9 files changed, 849 insertions(+), 557 deletions(-)
 create mode 100644 sound/soc/codecs/wm_fll.c
 create mode 100644 sound/soc/codecs/wm_fll.h

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [GIT PULL] i.MX clock changes for 5.4
From: Shawn Guo @ 2019-08-25 11:55 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Stefan Agner, linux-imx, kernel, Fabio Estevam, linux-clk,
	linux-arm-kernel

Hi Stephen,

This is the i.MX clock changes I collected for 5.4.  Please help pull,
and keep commit 6ad7cb7122ce ("clk: imx8: Add DSP related clocks")
stable, as I pulled it into my DT branch as dependency.  Thanks!

Shawn


The following changes since commit 5f9e832c137075045d15cd6899ab0505cfb2ca4b:

  Linus 5.3-rc1 (2019-07-21 14:05:38 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git tags/clk-imx-5.4

for you to fetch changes up to 760e548e7f885d89bf2dfab4838df9379edd19fc:

  clk: imx: imx8mn: fix audio pll setting (2019-08-24 21:04:27 +0200)

----------------------------------------------------------------
i.MX clock changes for 5.4:
 - Add clock driver for i.MX8MN SoC.
 - Switch i.MX8MM clock driver to platform driver.
 - Add API for clk unregister when driver probe fail.
 - Add Hifi4 DSP related clocks for i.MX8QXP SoC.
 - Fix Audio PLL setting and parent clock for USB.
 - Misc i.MX8 clock driver improvements and corrections.

----------------------------------------------------------------
Abel Vesa (3):
      clk: imx: Remove unused clk based API
      clk: imx8mm: Switch to platform driver
      clk: imx8mq: Mark AHB clock as critical

Anson Huang (14):
      dt-bindings: imx: Add clock binding doc for i.MX8MN
      clk: imx8mm: Make 1416X/1443X PLL macro definitions common for usage
      clk: imx: Add API for clk unregister when driver probe fail
      clk: imx: Add support for i.MX8MN clock driver
      clk: imx8mq: Remove CLK_IS_CRITICAL flag for IMX8MQ_CLK_TMU_ROOT
      clk: imx8mm: Fix typo of pwm3 clock's mux option #4
      clk: imx8mm: GPT1 clock mux option #5 should be sys_pll1_80m
      clk: imx7ulp: Make sure earlycon's clock is enabled
      clk: imx: Remove unused function statement
      clk: imx8mn: Keep uart clocks on for early console
      clk: imx8mm: Unregister clks when of_clk_add_provider failed
      clk: imx8mq: Unregister clks when of_clk_add_provider failed
      clk: imx8mn: Add missing rate_count assignment for each PLL structure
      clk: imx8mn: Add necessary frequency support for ARM PLL table

Daniel Baluta (1):
      clk: imx8: Add DSP related clocks

Fancy Fang (1):
      clk: imx8mm: rename 'share_count_dcss' to 'share_count_disp'

Leonard Crestez (4):
      clk: imx8mq: Fix sys3 pll references
      clk: imx8mm: Fix incorrect parents
      clk: imx8mn: Fix incorrect parents
      clk: imx8mn: Add GIC clock

Li Jun (2):
      clk: imx8mm: correct the usb1_ctrl parent to be usb_bus
      clk: imx8mq: set correct parent for usb ctrl clocks

Peng Fan (3):
      clk: imx: imx8mm: fix audio pll setting
      clk: imx8mn: fix int pll clk gate
      clk: imx: imx8mn: fix audio pll setting

 .../devicetree/bindings/clock/imx8mn-clock.yaml    | 112 ++++
 drivers/clk/imx/Kconfig                            |   6 +
 drivers/clk/imx/Makefile                           |   1 +
 drivers/clk/imx/clk-imx7ulp.c                      |  31 +
 drivers/clk/imx/clk-imx8mm.c                       | 109 ++--
 drivers/clk/imx/clk-imx8mn.c                       | 660 +++++++++++++++++++++
 drivers/clk/imx/clk-imx8mq.c                       | 131 ++--
 drivers/clk/imx/clk-imx8qxp-lpcg.c                 |   5 +
 drivers/clk/imx/clk.c                              |   8 +
 drivers/clk/imx/clk.h                              |  43 +-
 include/dt-bindings/clock/imx8-clock.h             |   6 +-
 include/dt-bindings/clock/imx8mn-clock.h           | 216 +++++++
 12 files changed, 1188 insertions(+), 140 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/imx8mn-clock.yaml
 create mode 100644 drivers/clk/imx/clk-imx8mn.c
 create mode 100644 include/dt-bindings/clock/imx8mn-clock.h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH net-next v3 2/3] net: ethernet: mediatek: Re-add support SGMII
From: René van Dorst @ 2019-08-25 11:41 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Nelson Chang, Frank Wunderlich, netdev, Sean Wang, linux-mips,
	linux-mediatek, John Crispin, Matthias Brugger, Stefan Roese,
	David S . Miller, linux-arm-kernel
In-Reply-To: <20190824133225.GE13294@shell.armlinux.org.uk>

Hi Russell,

Quoting Russell King - ARM Linux admin <linux@armlinux.org.uk>:

> Hi René,
>
> On Sat, Aug 24, 2019 at 01:11:17PM +0000, René van Dorst wrote:
>> Hi Russell,
>>
>> Mediatek calls it Turbo RGMII. It is a overclock version of RGMII mode.
>> It is used between first GMAC and port 6 of the mt7530 switch. Can be used
>> with
>> an internal and an external mt7530 switch.
>>
>> TRGMII speed are:
>> * mt7621: 1200Mbit
>> * mt7623: 2000Mbit and 2600Mbit.
>>
>> I think that TRGMII is only used in a fixed-link situation in combination
>> with a
>> mt7530 switch and running and maximum speed/full duplex. So reporting
>> 1000baseT_Full seems to me the right option.
>
> I think we can ignore this one for the purposes of merging this patch
> set, since this seems to be specific to this setup.  Neither 1000BaseT
> nor 1000BaseX fit very well, but we have to choose something.
>
>> PHY_INTERFACE_MODE_GMII:
>> 	  10baseT_Half
>> 	  10baseT_Full
>> 	 100baseT_Half
>> 	 100baseT_Full
>> 	1000baseT_Half
>> 	1000baseT_Full
>
> I think GMII can be connected to a PHY that can convert to 1000BaseX, so
> should probably include that here too.
>

Thanks for reviewing.
I shall add that too.

I send v4 today.

Greats,

René


> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Jerry-ch Chen @ 2019-08-25  9:18 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: devicetree@vger.kernel.org, Sean Cheng (鄭昇弘),
	Frederic Chen (陳俊元),
	Rynn Wu (吳育恩), srv_heupstream, po-yang.huang,
	suleiman@chromium.org, shik@chromium.org, jerry-ch.chen,
	Jungo Lin (林明俊),
	Sj Huang (黃信璋), yuzhao@chromium.org,
	linux-mediatek@lists.infradead.org, zwisler@chromium.org,
	hans.verkuil@cisco.com, matthias.bgg@gmail.com,
	Christie Yu (游雅惠), mchehab@kernel.org,
	laurent.pinchart+renesas@ideasonboard.com,
	linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <20190802082815.GA203993@chromium.org>

Hi Tomasz,

On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote: 
> Hi Jerry,
> 
> On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote:
> > From: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
> > 
> > This patch adds the driver of Face Detection (FD) unit in
> > Mediatek camera system, providing face detection function.
> > 
> > The mtk-isp directory will contain drivers for multiple IP
> > blocks found in Mediatek ISP system. It will include ISP Pass 1
> > driver (CAM), sensor interface driver, DIP driver and face
> > detection driver.
> > 
> > Signed-off-by: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
> > ---
> >  drivers/media/platform/Makefile               |    2 +
> >  drivers/media/platform/mtk-isp/fd/Makefile    |    5 +
> >  drivers/media/platform/mtk-isp/fd/mtk_fd.h    |  157 +++
> >  drivers/media/platform/mtk-isp/fd/mtk_fd_40.c | 1259 +++++++++++++++++++++++++
> >  include/uapi/linux/v4l2-controls.h            |    4 +
> >  5 files changed, 1427 insertions(+)
> >  create mode 100644 drivers/media/platform/mtk-isp/fd/Makefile
> >  create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd.h
> >  create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> >
> 
> Thanks for the patch! I finally got a chance to fully review the code. Sorry
> for the delay. Please check my comments inline.
> 
I appreciate your comments.
I've fixed most of the comments and verifying them,
Sorry for the delay, here is the reply.

> 
> > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > index e6deb25..8b817cc 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -94,6 +94,8 @@ obj-$(CONFIG_VIDEO_MEDIATEK_MDP)	+= mtk-mdp/
> >  
> >  obj-$(CONFIG_VIDEO_MEDIATEK_JPEG)	+= mtk-jpeg/
> >  
> > +obj-$(CONFIG_VIDEO_MEDIATEK_FD)		+= mtk-isp/fd/
> > +
> >  obj-$(CONFIG_VIDEO_QCOM_CAMSS)		+= qcom/camss/
> >  
> >  obj-$(CONFIG_VIDEO_QCOM_VENUS)		+= qcom/venus/
> > diff --git a/drivers/media/platform/mtk-isp/fd/Makefile b/drivers/media/platform/mtk-isp/fd/Makefile
> > new file mode 100644
> > index 0000000..9b1c501
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +mtk-fd-objs += mtk_fd_40.o
> > +
> > +obj-$(CONFIG_VIDEO_MEDIATEK_FD) += mtk-fd.o
> > \ No newline at end of file
> > diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd.h b/drivers/media/platform/mtk-isp/fd/mtk_fd.h
> > new file mode 100644
> > index 0000000..289999b
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd.h
> > @@ -0,0 +1,157 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> > +
> > +#ifndef __MTK_FD_HW_H__
> > +#define __MTK_FD_HW_H__
> > +
> > +#include <linux/io.h>
> > +#include <linux/types.h>
> > +#include <linux/platform_device.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/videobuf2-v4l2.h>
> > +
> > +#define MTK_FD_OUTPUT_MIN_WIDTH			26U
> > +#define MTK_FD_OUTPUT_MIN_HEIGHT		26U
> > +#define MTK_FD_OUTPUT_MAX_WIDTH			640U
> > +#define MTK_FD_OUTPUT_MAX_HEIGHT		480U
> > +
> > +/* Control the user defined image widths and heights
> > + * to be scaled and performed face detection in FD HW.
> > + * MTK FD support up to 14 user defined image sizes to perform face detection.
> > + */
> > +#define V4L2_CID_MTK_FD_SCALE_IMG_WIDTH		(V4L2_CID_USER_MTK_FD_BASE + 1)
> > +#define V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT	(V4L2_CID_USER_MTK_FD_BASE + 2)
> > +
> > +/* Control the numbers of user defined image sizes.
> > + * The default value is 0 which means user is not going
> > + * to define the specific image sizes.
> > + */
> > +#define V4L2_CID_MTK_FD_SCALE_IMG_NUM		(V4L2_CID_USER_MTK_FD_BASE + 3)
> > +
> > +/* Control the Face Pose to be detected.
> > + * Here describe the value as following:
> > + * {0, detect the front face with rotation from 0 to 270 degrees},
> > + * {1, detect the front face with rotation from 0 to 240 and 300 degrees},
> > + * {2, detect the front face with rotation from 0 to 240 and 330 degrees},
> > + * {3, detect the front face with rotation from 0 to 240 and left side face}.
> > + */
> > +#define V4L2_CID_MTK_FD_DETECT_POSE		(V4L2_CID_USER_MTK_FD_BASE + 4)
> > +#define V4L2_CID_MTK_FD_DETECT_SPEEDUP		(V4L2_CID_USER_MTK_FD_BASE + 5)
> > +#define V4L2_CID_MTK_FD_EXTRA_MODEL		(V4L2_CID_USER_MTK_FD_BASE + 6)
> > +
> > +/* We reserve 16 controls for this driver. */
> > +#define V4L2_CID_MTK_FD_MAX			16
> > +
> > +#define ENABLE_FD				0x111
> > +#define FD_HW_ENABLE				0x4
> > +#define FD_INT_EN				0x15c
> > +#define FD_INT					0x168
> > +#define FD_RESULT				0x178
> > +#define FD_IRQ_MASK				0x001
> > +
> > +#define RS_MAX_BUF_SIZE				2288788
> > +#define FD_MAX_SPEEDUP				7
> > +#define FD_MAX_POSE_VAL				0xfffffffffffffff
> > +#define FD_DEF_POSE_VAL				0x3ff
> > +#define MAX_FD_SEL_NUM				1026
> > +
> > +/* The max. number of face sizes could be detected, for feature scaling */
> > +#define FACE_SIZE_NUM_MAX			14
> > +/* FACE_SIZE_NUM_MAX + 1, first scale for input image W/H */
> > +#define FD_SCALE_NUM				15
> > +
> > +enum fd_state {
> > +	FD_ENQ,
> > +	FD_CBD,
> > +};
> > +
> > +enum fd_img_format {
> > +	FMT_VYUY = 2,
> > +	FMT_UYVY,
> > +	FMT_YVYU,
> > +	FMT_YUYV,
> > +	FMT_UNKNOWN
> > +};
> 
> Please use #define macros for hardware/firmware interface definitions.
> 
Ok, I will refine as following
#define MTK_FD_HW_FMT_VYUY    2
#define MTK_FD_HW_FMT_UYVY    3
#define MTK_FD_HW_FMT_YVYU    4
#define MTK_FD_HW_FMT_YUYV    5
#define MTK_FD_HW_FMT_UNKNOWN    6

> > +
> > +struct fd_buffer {
> > +	__u32 scp_addr;	/* used by SCP */
> > +	__u32 dma_addr;	/* used by DMA HW */
> > +} __packed;
> > +
> > +enum fd_scp_cmd {
> > +	FD_CMD_INIT,
> > +	FD_CMD_ENQUEUE,
> > +};
> 
> Ditto.
Ok, I will refine as following 
#define MTK_FD_IPI_CMD_INIT 0
#define MTK_FD_IPI_CMD_ENQUEUE 1 
> 
> > +
> > +struct fd_user_output {
> > +	__u64 results[MAX_FD_SEL_NUM];
> > +	__u16 number;
> > +};
> > +
> > +struct user_param {
> > +	u8 fd_pose;
> > +	u8 fd_speedup;
> > +	u8 fd_extra_model;
> > +	u8 scale_img_num;
> > +	u8 src_img_fmt;
> > +	__u16 scale_img_width[FD_SCALE_NUM];
> > +	__u16 scale_img_height[FD_SCALE_NUM];
> > +} __packed;
> > +
> > +struct fd_hw_param {
> > +	struct fd_buffer src_img;
> > +	struct fd_buffer user_result;
> > +	struct user_param user_param;
> > +} __packed;
> > +
> > +struct cmd_init_info {
> > +	struct fd_buffer fd_manager;
> > +	__u32 rs_dma_addr;
> > +} __packed;
> > +
> > +struct ipi_message {
> > +	u8 cmd_id;
> > +	union {
> > +		struct cmd_init_info init_param;
> > +		struct fd_hw_param hw_param;
> > +	};
> > +} __packed;
> > +
> > +struct mtk_fd_hw {
> > +	struct clk *fd_clk;
> > +	struct rproc *rproc_handle;
> > +	struct platform_device *scp_pdev;
> > +	struct fd_buffer scp_mem;
> > +	wait_queue_head_t wq;
> > +	void __iomem *fd_base;
> > +	atomic_t fd_user_cnt;
> > +	enum fd_state state;
> > +	u32 fd_irq_result;
> > +	/* Ensure only one job in hw */
> > +	struct mutex fd_hw_lock;
> > +};
> > +
> > +struct mtk_fd_dev {
> > +	struct platform_device *pdev;
> > +	struct v4l2_device v4l2_dev;
> > +	struct v4l2_m2m_dev *m2m_dev;
> > +	struct media_device mdev;
> > +	struct video_device vfd;
> > +	struct mtk_fd_hw fd_hw;
> 
> Could we just put the contents of that struct directly here? That should
> simplify dereference chains in the code.
> 
Ok, I will fix it.

> > +	/* Lock for V4L2 operations */
> > +	struct mutex vfd_lock;
> > +};
> > +
> > +struct mtk_fd_ctx {
> > +	struct mtk_fd_dev *fd_dev;
> > +	struct device *dev;
> > +	struct v4l2_fh fh;
> > +	struct v4l2_ctrl_handler hdl;
> > +	struct v4l2_pix_format_mplane src_fmt;
> > +	struct v4l2_meta_format dst_fmt;
> > +	struct user_param user_param;
> > +};
> > +
> > +#endif/*__MTK_FD_HW_H__*/
> > diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c b/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> > new file mode 100644
> > index 0000000..246d3aa
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> > @@ -0,0 +1,1259 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> > +
> > +#include <linux/clk.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_data/mtk_scp.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/remoteproc.h>
> > +#include <media/videobuf2-dma-contig.h>
> > +#include <media/videobuf2-v4l2.h>
> > +#include <linux/wait.h>
> > +#include <media/v4l2-ioctl.h>
> > +#include <media/v4l2-event.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-mem2mem.h>
> > +#include <media/videobuf2-core.h>
> > +
> > +#include "mtk_fd.h"
> > +
> > +static struct v4l2_meta_format fw_param_fmts[] = {
> 
> const?
> 
> Also, isn't this the buffer with the detected faces rather than params?

This struct will be removed. Yes, it's the buffer with detected faces.

> 
> > +	{
> > +		.dataformat = V4L2_META_FMT_MTISP_PARAMS,
> 
> This should use its own format.
Ok, I will define  V4L2_META_FMT_MTFD_RESULT in videodev2.h  

> 
> > +		.buffersize = 1024 * 30,
> 
> Please define a C struct for this buffer and use sizeof() here.
> 
Ok, that will be sizeof(struct fd_user_output);

> > +	},
> > +};
> 
> Actually, is there a reason to have this array at all if there is only 1
> format supported? I think we could just put the right constants directly in
> the code.
> 
I agree, I'll remove this array and use the constants in the code.

> > +
> > +static const struct v4l2_pix_format_mplane in_img_fmts[] = {
> > +	{
> > +		.width = MTK_FD_OUTPUT_MAX_WIDTH,
> > +		.height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > +		.pixelformat = V4L2_PIX_FMT_VYUY,
> > +		.colorspace = V4L2_COLORSPACE_BT2020,
> > +		.field = V4L2_FIELD_NONE,
> > +		.num_planes = 1,
> > +	},
> > +	{
> > +		.width = MTK_FD_OUTPUT_MAX_WIDTH,
> > +		.height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > +		.pixelformat = V4L2_PIX_FMT_YUYV,
> > +		.colorspace = V4L2_COLORSPACE_BT2020,
> > +		.field = V4L2_FIELD_NONE,
> > +		.num_planes = 1,
> > +	},
> > +	{
> > +		.width = MTK_FD_OUTPUT_MAX_WIDTH,
> > +		.height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > +		.pixelformat = V4L2_PIX_FMT_YVYU,
> > +		.colorspace = V4L2_COLORSPACE_BT2020,
> > +		.field = V4L2_FIELD_NONE,
> > +		.num_planes = 1,
> > +	},
> > +	{
> > +		.width = MTK_FD_OUTPUT_MAX_WIDTH,
> > +		.height = MTK_FD_OUTPUT_MAX_HEIGHT,
> > +		.pixelformat = V4L2_PIX_FMT_UYVY,
> > +		.colorspace = V4L2_COLORSPACE_BT2020,
> > +		.field = V4L2_FIELD_NONE,
> > +		.num_planes = 1,
> 
> If all the formats have the same values for a field, there is no reason to
> have the field initialized here. Just make initialize them to the constant
> values inside TRY_FMT.
> 
Ok, I will do so.

> Hmm, are the interleaved YUV formats the only formats supported by this
> hardware? That would be quite unfortunate given the formats supported by the
> other IP blocks on this SoC use the more typical planar formats.
> 
The supported YUV 1 plane formats are listed above.
The rest supported format is YUV 2 plane format, which should be
V4L2_PIX_FMT_NV16M.
For the in_img_fmts[], I will refine as following:

static const struct v4l2_pix_format_mplane mtk_fd_img_fmts[] = {
{
.pixelformat = V4L2_PIX_FMT_VYUY,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_YUYV,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_YVYU,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_UYVY,
.num_planes = 1,
},
{
.pixelformat = V4L2_PIX_FMT_NV16M,
.num_planes = 2,
},
{
.pixelformat = V4L2_PIX_FMT_NV61M,
.num_planes = 2,
},
};

> > +	},
> > +};
> > +
> > +#define NUM_FORMATS ARRAY_SIZE(in_img_fmts)
> > +
> > +static inline struct mtk_fd_dev *mtk_fd_hw_to_dev(struct mtk_fd_hw *fd_hw)
> > +{
> > +	return container_of(fd_hw, struct mtk_fd_dev, fd_hw);
> > +}
> > +
> > +static inline struct mtk_fd_ctx *fh_to_ctx(struct v4l2_fh *fh)
> > +{
> > +	return container_of(fh, struct mtk_fd_ctx, fh);
> > +}
> > +
> > +static int mtk_fd_load_scp(struct mtk_fd_hw *fd_hw)
> > +{
> > +	struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > +	struct device *dev = &fd_dev->pdev->dev;
> > +	phandle rproc_phandle;
> > +	int ret;
> > +
> > +	/* init scp */
> > +	fd_hw->scp_pdev = scp_get_pdev(fd_dev->pdev);
> > +	if (!fd_hw->scp_pdev) {
> > +		dev_err(dev, "Failed to get scp device\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	if (of_property_read_u32(fd_dev->pdev->dev.of_node, "mediatek,scp",
> > +				 &rproc_phandle)) {
> > +		dev_err(dev, "Could not get scp device\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	fd_hw->rproc_handle = rproc_get_by_phandle(rproc_phandle);
> > +	if (!fd_hw->rproc_handle) {
> > +		dev_err(dev, "Could not get FD's rproc_handle\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = rproc_boot(fd_hw->rproc_handle);
> > +	if (ret < 0) {
> > +		/**
> > +		 * Return 0 if downloading firmware successfully,
> > +		 * otherwise it is failed
> > +		 */
> > +		dev_err(dev, "rproc_boot failed\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static dma_addr_t mtk_fd_hw_alloc_rs_dma_addr(struct mtk_fd_hw *fd_hw)
> > +{
> > +	struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > +	struct device *dev = &fd_dev->pdev->dev;
> > +	void *va;
> > +	dma_addr_t dma_handle;
> > +
> > +	va = dma_alloc_coherent(dev, RS_MAX_BUF_SIZE, &dma_handle, GFP_KERNEL);
> 
> I see this allocated here, but I don't see this freed anywhere.
> 
I will fix it in next version.

> > +	if (!va) {
> > +		dev_err(dev, "dma_alloc null va\n");
> > +		return -ENOMEM;
> > +	}
> > +	memset(va, 0, RS_MAX_BUF_SIZE);
> > +
> > +	return dma_handle;
> > +}
> > +
> > +static int mtk_fd_send_ipi_init(struct mtk_fd_hw *fd_hw)
> > +{
> > +	struct ipi_message fd_init_msg;
> > +	dma_addr_t rs_dma_addr;
> > +
> > +	fd_init_msg.cmd_id = FD_CMD_INIT;
> > +
> > +	fd_init_msg.init_param.fd_manager.scp_addr = fd_hw->scp_mem.scp_addr;
> > +	fd_init_msg.init_param.fd_manager.dma_addr = fd_hw->scp_mem.dma_addr;
> > +
> > +	rs_dma_addr = mtk_fd_hw_alloc_rs_dma_addr(fd_hw);
> > +	if (!rs_dma_addr)
> > +		return -ENOMEM;
> > +
> > +	fd_init_msg.init_param.rs_dma_addr = rs_dma_addr;
> > +
> > +	return scp_ipi_send(fd_hw->scp_pdev, SCP_IPI_FD_CMD, &fd_init_msg,
> > +			    sizeof(fd_init_msg), 0);
> > +}
> > +
> > +static int mtk_fd_hw_enable(struct mtk_fd_hw *fd_hw)
> > +{
> > +	int ret;
> > +
> > +	ret = mtk_fd_load_scp(fd_hw);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = mtk_fd_send_ipi_init(fd_hw);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_hw_connect(struct mtk_fd_hw *fd_hw)
> > +{
> > +	struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > +	s32 usercount;
> > +
> > +	mutex_lock(&fd_hw->fd_hw_lock);
> 
> First of all, we don't need a mutex here, because all the V4L2 ioctls are
> already running with the video device mutex.
Ok, I will remove the using of the mutex here.

> 
> > +	usercount = atomic_inc_return(&fd_hw->fd_user_cnt);
> 
> If we already use a mutex, there is no point in using atomic types.
> 
Ok, I will replace the atomic types with normal types

> > +	if (usercount == 1) {
> > +		pm_runtime_get_sync(&fd_dev->pdev->dev);
> > +		if (mtk_fd_hw_enable(fd_hw)) {
> > +			pm_runtime_put_sync(&fd_dev->pdev->dev);
> > +			atomic_dec_return(&fd_hw->fd_user_cnt);
> > +			mutex_unlock(&fd_hw->fd_hw_lock);
> > +			return -EINVAL;
> > +		}
> > +	}
> 
> This is a simple mem-to-mem device, so there is no reason to keep it active
> all the time it's streaming. Please just get the runtime PM counter when
> queuing a job to the hardware and release it when the job finishes.
> 
> I guess we might still want to do the costly operations like rproc_boot()
> when we start streaming, though.
> 
Do you mean by moving the pm_runtime_get/put stuff to the job execution
and job finish place?
the rproc_boot() operation will be done here.

> > +	mutex_unlock(&fd_hw->fd_hw_lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_wait_irq(struct mtk_fd_hw *fd_hw)
> > +{
> > +	int timeout;
> > +	struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > +	struct device *dev = &fd_dev->pdev->dev;
> > +
> > +	timeout = wait_event_interruptible_timeout
> > +		(fd_hw->wq, (fd_hw->fd_irq_result & FD_IRQ_MASK),
> > +		 usecs_to_jiffies(1000000));
> > +	if (!timeout) {
> > +		dev_err(dev, "%s timeout, %d\n", __func__,
> > +			fd_hw->fd_irq_result);
> > +		return -EAGAIN;
> > +	} else if (!(fd_hw->fd_irq_result & FD_IRQ_MASK)) {
> > +		dev_err(dev, "%s No IRQ mask:0x%8x\n",
> > +			__func__, fd_hw->fd_irq_result);
> > +		return -EINVAL;
> > +	}
> > +	fd_hw->fd_irq_result = 0;
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtk_fd_hw_disconnect(struct mtk_fd_hw *fd_hw)
> > +{
> > +	struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > +	s32 usercount;
> > +
> > +	mutex_lock(&fd_hw->fd_hw_lock);
> > +	atomic_dec_return(&fd_hw->fd_user_cnt);
> > +	usercount = atomic_read(&fd_hw->fd_user_cnt);
> > +	if (usercount == 0) {
> > +		if (fd_hw->state == FD_ENQ)
> > +			mtk_fd_wait_irq(fd_hw);
> 
> This shouldn't be possible to happen as the queues should be already stopped
> at this point.
> 
Ok, I will remove it. 
> > +
> > +		pm_runtime_put_sync(&fd_dev->pdev->dev);
> 
> Any reason to use pm_runtime_put_sync() over pm_runtime_put()?
> 
No special reason to do so, the pm_runtime_put_sync here will be moved
to the place each job finished.

> > +		rproc_shutdown(fd_hw->rproc_handle);
> > +		rproc_put(fd_hw->rproc_handle);
> > +	}
> > +	mutex_unlock(&fd_hw->fd_hw_lock);
> > +}
> > +
> > +static void mtk_fd_hw_job_finish(struct mtk_fd_hw *fd_hw,
> > +				 struct fd_hw_param *fd_param,
> > +				 enum vb2_buffer_state vb_state)
> > +{
> > +	struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > +	struct mtk_fd_ctx *ctx;
> > +	struct device *dev = &fd_dev->pdev->dev;
> > +	struct vb2_buffer *src_vb, *dst_vb;
> > +	struct vb2_v4l2_buffer *src_vbuf = NULL, *dst_vbuf = NULL;
> > +
> > +	ctx = v4l2_m2m_get_curr_priv(fd_dev->m2m_dev);
> > +	if (!ctx) {
> > +		dev_err(dev, "Instance released before end of transaction\n");
> > +		return;
> > +	}
> 
> This shouldn't be possible. I suspect you're seeing this because
> .stop_streaming is not implemented correctly. See my comment there.
> 
Ok, I will remove this. 
> > +
> > +	src_vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > +	if (WARN_ON(!src_vb))
> > +		return;
> 
> This shouldn't be possible.
> 
Ditto. 
> > +	src_vbuf = to_vb2_v4l2_buffer(src_vb);
> > +
> > +	dst_vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > +	if (WARN_ON(!dst_vb))
> > +		return;
> 
> Ditto.
> 
Ditto. 
> > +	dst_vbuf = to_vb2_v4l2_buffer(dst_vb);
> > +
> > +	dst_vbuf->vb2_buf.timestamp = src_vbuf->vb2_buf.timestamp;
> > +	dst_vbuf->timecode = src_vbuf->timecode;
> > +	dst_vbuf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> > +	dst_vbuf->flags |= src_vbuf->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> 
> We should be able to just use v4l2_m2m_buf_copy_metadata().
> 
Ok, I will refine as:
v4l2_m2m_buf_copy_metadata(src_vbuf, dst_vbuf,
   V4L2_BUF_FLAG_TSTAMP_SRC_MASK); 
> > +
> > +	v4l2_m2m_buf_done(src_vbuf, vb_state);
> > +	v4l2_m2m_buf_done(dst_vbuf, vb_state);
> > +	v4l2_m2m_job_finish(fd_dev->m2m_dev, ctx->fh.m2m_ctx);
> > +}
> > +
> > +static int mtk_fd_hw_job_exec(struct mtk_fd_hw *fd_hw,
> > +			      struct fd_hw_param *fd_param,
> > +			      void *output_vaddr)
> > +{
> > +	struct fd_user_output *fd_output;
> > +	struct ipi_message fd_ipi_msg;
> > +	int ret;
> > +	u32 num;
> > +
> > +	if (fd_param->user_param.src_img_fmt == FMT_UNKNOWN)
> > +		goto param_err;
> 
> Is this possible?
> 
Only if user set wrong format, I will remove this.

> > +
> > +	mutex_lock(&fd_hw->fd_hw_lock);
> > +	fd_hw->state = FD_ENQ;
> 
> What is this state for?
> 
It was for checking status, if a job is processing, the state is
FD_ENQ, 
then we should wait for the last job to be done when pm_suspend().

> > +	fd_ipi_msg.cmd_id = FD_CMD_ENQUEUE;
> > +	memcpy(&fd_ipi_msg.hw_param, fd_param, sizeof(fd_ipi_msg.hw_param));
> > +	ret = scp_ipi_send(fd_hw->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg,
> > +			   sizeof(fd_ipi_msg), 0);
> > +	if (ret)
> > +		goto buf_err;
> > +
> > +	ret = mtk_fd_wait_irq(fd_hw);
> > +	if (ret)
> > +		goto buf_err;
> 
> This function is called from device_run and it shouldn't be synchronous. It
> should only queue the job to the hardware to be handled asynchronously when
> it finishes.
> 
Ok, I will fix it.

> > +
> > +	num = readl(fd_hw->fd_base + FD_RESULT);
> > +	/* Disable FD ISR */
> > +	writel(0x0, fd_hw->fd_base + FD_INT_EN);
> > +
> > +	fd_output = (struct fd_user_output *)output_vaddr;
> > +	fd_output->number = num;
> > +	fd_hw->state = FD_CBD;
> > +	mutex_unlock(&fd_hw->fd_hw_lock);
> > +
> > +	mtk_fd_hw_job_finish(fd_hw, fd_param, VB2_BUF_STATE_DONE);
> > +	return 0;
> > +
> > +buf_err:
> > +	mutex_unlock(&fd_hw->fd_hw_lock);
> > +param_err:
> > +	mtk_fd_hw_job_finish(fd_hw, fd_param, VB2_BUF_STATE_ERROR);
> > +	return ret;
> > +}
> > +
> > +static int mtk_fd_vb2_buf_out_validate(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> > +
> > +	v4l2_buf->field = V4L2_FIELD_NONE;
> 
> We need to fail with -EINVAL if v4l2_buf->field was different than
> V4L2_FIELD_ANY or _NONE.
> 

Ok, I will refine as following:

if (v4l2_buf->field == V4L2_FIELD_ANY)
v4l2_buf->field = V4L2_FIELD_NONE;
if (v4l2_buf->field != V4L2_FIELD_NONE)
return -EINVAL;

> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_vb2_buf_prepare(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +	struct vb2_queue *vq = vb->vb2_queue;
> > +	struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > +	struct device *dev = ctx->dev;
> > +	struct v4l2_pix_format_mplane *pixfmt;
> > +
> > +	switch (vq->type) {
> > +	case V4L2_BUF_TYPE_META_CAPTURE:
> > +		if (vb2_plane_size(vb, 0) < ctx->dst_fmt.buffersize) {
> > +			dev_err(dev, "meta size %d is too small\n");
> 
> Please make this dev_dbg(), because userspace misbehavior must not trigger
> kernel error logs.
> 

Ok, fixed.

> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > +		pixfmt = &ctx->src_fmt;
> > +
> > +		if (vbuf->field == V4L2_FIELD_ANY)
> > +			vbuf->field = V4L2_FIELD_NONE;
> > +
> > +		if (vb->num_planes != 1 || vbuf->field != V4L2_FIELD_NONE) {
> > +			dev_err(dev, "plane or field %d not supported\n",
> > +				vb->num_planes, vbuf->field);
> 
> Ditto.
> 

Done.

> > +			return -EINVAL;
> > +		}
> > +		if (vb2_plane_size(vb, 0) < pixfmt->plane_fmt[0].sizeimage) {
> > +			dev_err(dev, "plane %d is too small\n");
> 
> Ditto.
> 

Done.

> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	default:
> > +		dev_err(dev, "invalid queue type: %d\n", vq->type);
> > +		return -EINVAL;
> 
> We shouldn't need to handle this.
> 

Ok, I will remove this.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtk_fd_vb2_buf_queue(struct vb2_buffer *vb)
> > +{
> > +	struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +
> > +	v4l2_m2m_buf_queue(ctx->fh.m2m_ctx, vbuf);
> > +}
> > +
> > +static int mtk_fd_vb2_queue_setup(struct vb2_queue *vq,
> > +				  unsigned int *num_buffers,
> > +				  unsigned int *num_planes,
> > +				  unsigned int sizes[],
> > +				  struct device *alloc_devs[])
> > +{
> > +	struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > +	struct device *dev = ctx->dev;
> > +	unsigned int size;
> > +
> > +	switch (vq->type) {
> > +	case V4L2_BUF_TYPE_META_CAPTURE:
> > +		size = ctx->dst_fmt.buffersize;
> > +		break;
> > +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > +		size = ctx->src_fmt.plane_fmt[0].sizeimage;
> > +		break;
> > +	default:
> > +		dev_err(dev, "invalid queue type: %d\n", vq->type);
> 
> We should need to handle this.
> 
Do you mean by giving a size instead of return -EINVAL?

> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!*num_planes) {
> > +		*num_planes = 1;
> > +		sizes[0] = size;
> > +	}
> 
> We need to handle the case when *num_planes is non-zero. We then need to
> check if it's 1 and *sizes >= size and fail with -EINVAL if not.
> 
Ok, I will refine as following:
if (*num_planes) {
if (sizes[0] < size || *num_planes != 1)
return -EINVAL;
} else {
*num_planes = 1;
sizes[0] = size;
} 
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_vb2_start_streaming(struct vb2_queue *vq, unsigned int count)
> > +{
> > +	struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > +
> > +	return mtk_fd_hw_connect(&ctx->fd_dev->fd_hw);
> 
> This would be called twice for every context, once for the VIDEO_OUTPUT
> queue and once for the META_CAPTURE queue. Perhaps it would make sense to
> just do this for the VIDEO_OUTPUT queue?
> 
Ok, I will refine as:
if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
return mtk_fd_hw_connect(&ctx->fd_dev->fd_hw);
else
return 0;

> > +}
> > +
> > +static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > +{
> > +	struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > +	struct vb2_buffer *vb;
> 
> How do we guarantee here that the hardware isn't still accessing the buffers
> removed below?
> 
Maybe we can check the driver state flag and aborting the unfinished
jobs?
(fd_hw->state == FD_ENQ)

> > +
> > +	if (V4L2_TYPE_IS_OUTPUT(vq->type))
> > +		vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > +	else
> > +		vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > +
> > +	while (vb) {
> > +		v4l2_m2m_buf_done(to_vb2_v4l2_buffer(vb), VB2_BUF_STATE_ERROR);
> > +		if (V4L2_TYPE_IS_OUTPUT(vq->type))
> > +			vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > +		else
> > +			vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > +	}
> 
> We can use v4l2_m2m_buf_remove(). Also we can move the call into the loop
> condition:
> 
> while ((vb == v4l2_m2m_buf_remove(...)))
> 	v4l2_m2m_buf_done(...);
> 
Ok, I will refine as following:

while ((vb = v4l2_m2m_buf_remove(V4L2_TYPE_IS_OUTPUT(vq->type)?
  &m2m_ctx->out_q_ctx :
  &m2m_ctx->cap_q_ctx)))
v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);

> > +
> > +	mtk_fd_hw_disconnect(&ctx->fd_dev->fd_hw);
> 
> This should also probably be done only for 1 queue. Since the VIDEO_OUTPUT
> queue supports requestes, I'd consider it the master one.
> 
Ok, only for output queue to trigger disconnect.

if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
mtk_fd_hw_disconnect(&ctx->fd_dev->fd_hw); 
> > +}
> > +
> > +static void mtk_fd_vb2_request_complete(struct vb2_buffer *vb)
> > +{
> > +	struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +
> > +	v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
> > +}
> > +
> > +static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
> > +				  const struct v4l2_pix_format_mplane *sfmt)
> > +{
> > +	dfmt->width = sfmt->width;
> > +	dfmt->height = sfmt->height;
> > +	dfmt->pixelformat = sfmt->pixelformat;
> > +	dfmt->field = sfmt->field;
> > +	dfmt->colorspace = sfmt->colorspace;
> > +	dfmt->num_planes = sfmt->num_planes;
> > +
> > +	/* Use default */
> > +	dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > +	dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > +	dfmt->xfer_func =
> > +		V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);
> > +	dfmt->plane_fmt[0].bytesperline = dfmt->width * 2;
> > +	dfmt->plane_fmt[0].sizeimage =
> > +		dfmt->height * dfmt->plane_fmt[0].bytesperline;
> > +	memset(dfmt->reserved, 0, sizeof(dfmt->reserved));
> > +}
> 
> Could we unify this function with mtk_fd_m2m_try_fmt_out_mp()? That function
> should be almost directly reusable for the default format initialization +/-
> the arguments passed to it.
> 
Ok, I will try to reuse it as following:

static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
  const struct v4l2_pix_format_mplane *sfmt)
{
dfmt->field = V4L2_FIELD_NONE;
dfmt->colorspace = V4L2_COLORSPACE_BT2020;
dfmt->num_planes = sfmt->num_planes;
dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
dfmt->xfer_func =
V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);

/* Keep user setting as possible */
dfmt->width = clamp(dfmt->width,
    MTK_FD_OUTPUT_MIN_WIDTH,
    MTK_FD_OUTPUT_MAX_WIDTH);
dfmt->height = clamp(dfmt->height,
     MTK_FD_OUTPUT_MIN_HEIGHT,
     MTK_FD_OUTPUT_MAX_HEIGHT);

if (sfmt->num_planes == 2) {
/* NV16M and NV61M has 1 byte per pixel */
dfmt->plane_fmt[0].bytesperline = dfmt->width;
dfmt->plane_fmt[1].bytesperline = dfmt->width;
} else {
/* 2 bytes per pixel */
dfmt->plane_fmt[0].bytesperline = dfmt->width * 2;
}

dfmt->plane_fmt[0].sizeimage =
dfmt->height * dfmt->plane_fmt[0].bytesperline;
}

> > +
> > +static const struct v4l2_pix_format_mplane *mtk_fd_find_fmt(u32 format)
> > +{
> > +	unsigned int i;
> > +	const struct v4l2_pix_format_mplane *dev_fmt;
> > +
> > +	for (i = 0; i < NUM_FORMATS; i++) {
> > +		dev_fmt = &in_img_fmts[i];
> > +		if (dev_fmt->pixelformat == format)
> > +			return dev_fmt;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static int mtk_fd_m2m_querycap(struct file *file, void *fh,
> > +			       struct v4l2_capability *cap)
> > +{
> > +	struct mtk_fd_dev *fd_dev = video_drvdata(file);
> > +	struct device *dev = &fd_dev->pdev->dev;
> > +
> > +	strscpy(cap->driver, dev->driver->name, sizeof(cap->driver));
> 
> Please use dev_driver_string().
> 
Done,

> > +	strscpy(cap->card, fd_dev->vfd.name, sizeof(cap->card));
> 
> I think we can also make this dev_driver_string().
> 
Done.

> > +	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> > +		 dev_name(&fd_dev->pdev->dev));
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_enum_fmt_out_mp(struct file *file, void *fh,
> > +				      struct v4l2_fmtdesc *f)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < NUM_FORMATS; ++i) {
> > +		if (i == f->index) {
> > +			f->pixelformat = in_img_fmts[i].pixelformat;
> > +			return 0;
> > +		}
> > +	}
> 
> Why don't we just check if f->index is within the [0, ARRAY_SIZE()-1] bounds
> and then just use it to index the array directly?
> 
I will refine as :

static int mtk_fd_m2m_enum_fmt_out_mp(struct file *file, void *fh,
      struct v4l2_fmtdesc *f)
{
if ((f->index >= 0) && (f->index < NUM_FORMATS)) {
f->pixelformat = in_img_fmts[f->index].pixelformat;
return 0;
}

return -EINVAL;
} 
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int mtk_fd_m2m_try_fmt_out_mp(struct file *file,
> > +				     void *fh,
> > +				     struct v4l2_format *f)
> 
> I think we could just shorten the function prefixes to "mtk_fd_".
> 
Do you mean by replace mtk_fd_m2m_* with mtk_fd_ ?

> > +{
> > +	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > +	const struct v4l2_pix_format_mplane *fmt;
> > +
> > +	fmt = mtk_fd_find_fmt(pix_mp->pixelformat);
> > +	if (!fmt) {
> > +		/* Get default img fmt */
> > +		fmt = &in_img_fmts[0];
> > +		f->fmt.pix.pixelformat = fmt->pixelformat;
> > +	}
> > +
> > +	/* Use default */
> > +	pix_mp->field = fmt->field;
> > +	pix_mp->colorspace = fmt->colorspace;
> > +	pix_mp->num_planes = fmt->num_planes;
> > +	pix_mp->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > +	pix_mp->quantization = V4L2_QUANTIZATION_DEFAULT;
> > +	pix_mp->xfer_func =
> > +		V4L2_MAP_XFER_FUNC_DEFAULT(pix_mp->colorspace);
> > +
> > +	/* Keep user setting as possible */
> > +	pix_mp->width = clamp(pix_mp->width,
> > +			      MTK_FD_OUTPUT_MIN_WIDTH,
> > +			      MTK_FD_OUTPUT_MAX_WIDTH);
> > +	pix_mp->height = clamp(pix_mp->height,
> > +			       MTK_FD_OUTPUT_MIN_HEIGHT,
> > +			       MTK_FD_OUTPUT_MAX_HEIGHT);
> > +
> > +	pix_mp->plane_fmt[0].bytesperline = pix_mp->width * 2;
> 
> Please add a comment explaining why this is always 2.
> 
Done.
/* 2 bytes per pixel */ 
> > +	pix_mp->plane_fmt[0].sizeimage =
> > +		pix_mp->plane_fmt[0].bytesperline * pix_mp->height;
> > +	memset(pix_mp->plane_fmt[0].reserved, 0,
> > +	       sizeof(pix_mp->plane_fmt[0].reserved));
> 
> No need to zero this here. The core will handle it.
> 
Ok, I will remove it.

> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_g_fmt_out_mp(struct file *file, void *fh,
> > +				   struct v4l2_format *f)
> > +{
> > +	struct mtk_fd_ctx *ctx = fh_to_ctx(fh);
> > +
> > +	f->fmt.pix_mp = ctx->src_fmt;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_s_fmt_out_mp(struct file *file, void *fh,
> > +				   struct v4l2_format *f)
> > +{
> > +	struct mtk_fd_ctx *ctx = fh_to_ctx(fh);
> > +	struct vb2_queue *vq;
> > +
> > +	/* Change not allowed if queue is streaming. */
> > +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
> > +	if (vb2_is_streaming(vq) || vb2_is_busy(vq)) {
> 
> vb2_is_busy() is a superset of vb2_is_streaming(), so it's enough to just
> check the former.
> 
Ok, I will just check the former.

> > +		dev_dbg(ctx->dev, "vb2_is_streaming or vb2_is_busy");
> > +		return -EBUSY;
> > +	}
> > +
> > +	mtk_fd_m2m_try_fmt_out_mp(file, fh, f);
> > +	ctx->src_fmt = f->fmt.pix_mp;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_enum_fmt_meta_cap(struct file *file, void *fh,
> > +					struct v4l2_fmtdesc *f)
> > +{
> > +	if (f->index)
> > +		return -EINVAL;
> > +
> > +	strscpy(f->description, "Face detection result",
> > +		sizeof(f->description));
> > +	f->pixelformat = fw_param_fmts[0].dataformat;
> > +	f->flags = 0;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_m2m_g_fmt_meta_cap(struct file *file, void *fh,
> > +				     struct v4l2_format *f)
> > +{
> > +	f->fmt.meta.dataformat = fw_param_fmts[0].dataformat;
> > +	f->fmt.meta.buffersize = fw_param_fmts[0].buffersize;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct vb2_ops mtk_fd_vb2_ops = {
> > +	.queue_setup = mtk_fd_vb2_queue_setup,
> > +	.buf_out_validate = mtk_fd_vb2_buf_out_validate,
> > +	.buf_prepare  = mtk_fd_vb2_buf_prepare,
> > +	.buf_queue = mtk_fd_vb2_buf_queue,
> > +	.start_streaming = mtk_fd_vb2_start_streaming,
> > +	.stop_streaming = mtk_fd_vb2_stop_streaming,
> > +	.wait_prepare = vb2_ops_wait_prepare,
> > +	.wait_finish = vb2_ops_wait_finish,
> > +	.buf_request_complete = mtk_fd_vb2_request_complete,
> > +};
> > +
> > +static const struct v4l2_ioctl_ops mtk_fd_v4l2_video_out_ioctl_ops = {
> > +	.vidioc_querycap = mtk_fd_m2m_querycap,
> > +	.vidioc_enum_fmt_vid_out_mplane = mtk_fd_m2m_enum_fmt_out_mp,
> > +	.vidioc_g_fmt_vid_out_mplane = mtk_fd_m2m_g_fmt_out_mp,
> > +	.vidioc_s_fmt_vid_out_mplane = mtk_fd_m2m_s_fmt_out_mp,
> > +	.vidioc_try_fmt_vid_out_mplane = mtk_fd_m2m_try_fmt_out_mp,
> > +	.vidioc_enum_fmt_meta_cap = mtk_fd_m2m_enum_fmt_meta_cap,
> > +	.vidioc_g_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> > +	.vidioc_s_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> > +	.vidioc_try_fmt_meta_cap = mtk_fd_m2m_g_fmt_meta_cap,
> > +	.vidioc_reqbufs = v4l2_m2m_ioctl_reqbufs,
> > +	.vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
> > +	.vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
> > +	.vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
> > +	.vidioc_querybuf = v4l2_m2m_ioctl_querybuf,
> > +	.vidioc_qbuf = v4l2_m2m_ioctl_qbuf,
> > +	.vidioc_dqbuf = v4l2_m2m_ioctl_dqbuf,
> > +	.vidioc_streamon = v4l2_m2m_ioctl_streamon,
> > +	.vidioc_streamoff = v4l2_m2m_ioctl_streamoff,
> > +	.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> > +	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> > +};
> > +
> > +static int
> > +mtk_fd_queue_init(void *priv, struct vb2_queue *src_vq,
> > +		  struct vb2_queue *dst_vq)
> > +{
> > +	struct mtk_fd_ctx *ctx = priv;
> > +	int ret;
> > +
> > +	src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> > +	src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> > +	src_vq->supports_requests = true;
> > +	src_vq->drv_priv = ctx;
> > +	src_vq->ops = &mtk_fd_vb2_ops;
> > +	src_vq->mem_ops = &vb2_dma_contig_memops;
> > +	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > +	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > +	src_vq->lock = &ctx->fd_dev->vfd_lock;
> > +	src_vq->dev = ctx->fd_dev->v4l2_dev.dev;
> > +
> > +	ret = vb2_queue_init(src_vq);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dst_vq->type = V4L2_BUF_TYPE_META_CAPTURE;
> > +	dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> > +	dst_vq->drv_priv = ctx;
> > +	dst_vq->ops = &mtk_fd_vb2_ops;
> > +	dst_vq->mem_ops = &vb2_dma_contig_memops;
> > +	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> > +	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > +	dst_vq->lock = &ctx->fd_dev->vfd_lock;
> > +	dst_vq->dev = ctx->fd_dev->v4l2_dev.dev;
> > +
> > +	return vb2_queue_init(dst_vq);
> > +}
> > +
> > +static int mtk_fd_dev_g_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct mtk_fd_ctx *ctx = ctrl->priv;
> > +	int i;
> > +
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_MTK_FD_SCALE_IMG_WIDTH:
> > +		for (i = 0; i < ctrl->elems; i++)
> > +			ctrl->p_new.p_u16[i] =
> > +				ctx->user_param.scale_img_width[i];
> > +		break;
> > +	case V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT:
> > +		for (i = 0; i < ctrl->elems; i++)
> > +			ctrl->p_new.p_u16[i] =
> > +				ctx->user_param.scale_img_height[i];
> > +		break;
> > +	case V4L2_CID_MTK_FD_DETECT_POSE:
> > +		ctrl->val = ctx->user_param.fd_pose;
> > +		break;
> > +	case V4L2_CID_MTK_FD_DETECT_SPEEDUP:
> > +		ctrl->val = ctx->user_param.fd_speedup;
> > +		break;
> > +	case V4L2_CID_MTK_FD_SCALE_IMG_NUM:
> > +		ctrl->val = ctx->user_param.scale_img_num;
> > +		break;
> > +	case V4L2_CID_MTK_FD_EXTRA_MODEL:
> > +		ctrl->val = ctx->user_param.fd_extra_model;
> > +		break;
> > +	default:
> > +		dev_dbg(ctx->dev, "%s: unexpected control: 0x%x:%d",
> > +			__func__, ctrl->id, ctrl->val);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_dev_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct mtk_fd_ctx *ctx = ctrl->priv;
> > +	int i;
> > +
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_MTK_FD_SCALE_IMG_WIDTH:
> > +		for (i = 0; i < ctrl->elems; i++)
> > +			ctx->user_param.scale_img_width[i] =
> > +				ctrl->p_new.p_u16[i];
> > +		break;
> > +	case V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT:
> > +		for (i = 0; i < ctrl->elems; i++)
> > +			ctx->user_param.scale_img_height[i] =
> > +				ctrl->p_new.p_u16[i];
> > +		break;
> > +	case V4L2_CID_MTK_FD_DETECT_POSE:
> > +		ctx->user_param.fd_pose = ctrl->val;
> > +		break;
> > +	case V4L2_CID_MTK_FD_DETECT_SPEEDUP:
> > +		ctx->user_param.fd_speedup = ctrl->val;
> > +		break;
> > +	case V4L2_CID_MTK_FD_SCALE_IMG_NUM:
> > +		ctx->user_param.scale_img_num = ctrl->val;
> > +		break;
> > +	case V4L2_CID_MTK_FD_EXTRA_MODEL:
> > +		ctx->user_param.fd_extra_model = ctrl->val;
> > +		break;
> > +	default:
> > +		dev_dbg(ctx->dev, "%s: unexpected control: 0x%x:%d",
> > +			__func__, ctrl->id, ctrl->val);
> > +		return -EINVAL;
> 
> No need to handle this, as the framework will only pass the controls we
> registered earlier to this function.
> 
Ok, I will remove this .

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops mtk_fd_dev_ctrl_ops = {
> > +	.g_volatile_ctrl = mtk_fd_dev_g_ctrl,
> 
> I don't see any volatile controls below. No need to implement this callback.
> 
Ok, I will remove this .

> > +	.s_ctrl = mtk_fd_dev_s_ctrl,
> 
> I think you might not even need to implement this function (or just provide
> a dummy one that returns 0 if its required) if you just use the controls
> directly when preparing the job for the hardware. Check v4l2_ctrl_find().
> 
I will remove the mtk_fd_dev_ctrl_ops, and use the following function to
get control values when preparing the job for HW,

static void mtk_fd_fill_user_param(struct user_param *user_param,
   struct v4l2_ctrl_handler *hdl)
{
struct v4l2_ctrl *ctrl;
int i;
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_SCALE_IMG_WIDTH);
if (ctrl)
for (i = 0; i < ctrl->elems; i++)
user_param->scale_img_width[i] = ctrl->p_new.p_u16[i];

ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT);
if (ctrl)
for (i = 0; i < ctrl->elems; i++)
user_param->scale_img_height[i] = ctrl->p_new.p_u16[i];

ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_DETECT_POSE);
if (ctrl)
user_param->fd_pose = ctrl->val;
pr_info("pose:%d\n", user_param->fd_pose);
ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_DETECT_SPEEDUP);
if (ctrl)
user_param->fd_speedup = ctrl->val;

ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_SCALE_IMG_NUM);
if (ctrl)
user_param->scale_img_num = ctrl->val;

ctrl = v4l2_ctrl_find(hdl, V4L2_CID_MTK_FD_EXTRA_MODEL);
if (ctrl)
user_param->fd_extra_model = ctrl->val;
}


> > +};
> > +
> > +struct v4l2_ctrl_config mtk_fd_controls[] = {
> > +	{
> > +	.ops = &mtk_fd_dev_ctrl_ops,
> > +	.id = V4L2_CID_MTK_FD_SCALE_IMG_WIDTH,
> > +	.name = "FD scale image widths",
> > +	.type = V4L2_CTRL_TYPE_U16,
> > +	.min = MTK_FD_OUTPUT_MIN_WIDTH,
> > +	.max = MTK_FD_OUTPUT_MAX_WIDTH,
> > +	.step = 1,
> > +	.def = MTK_FD_OUTPUT_MAX_WIDTH,
> > +	.dims = { FD_SCALE_NUM },
> 
> Something wrong with indentation here.
> 
Done.

> > +	},
> > +	{
> > +	.ops = &mtk_fd_dev_ctrl_ops,
> > +	.id = V4L2_CID_MTK_FD_SCALE_IMG_HEIGHT,
> > +	.name = "FD scale image heights",
> > +	.type = V4L2_CTRL_TYPE_U16,
> > +	.min = MTK_FD_OUTPUT_MIN_HEIGHT,
> > +	.max = MTK_FD_OUTPUT_MAX_HEIGHT,
> > +	.step = 1,
> > +	.def = MTK_FD_OUTPUT_MAX_HEIGHT,
> > +	.dims = { FD_SCALE_NUM },
> > +	},
> > +	{
> > +	.ops = &mtk_fd_dev_ctrl_ops,
> > +	.id = V4L2_CID_MTK_FD_SCALE_IMG_NUM,
> > +	.name = "FD scale size counts",
> > +	.type = V4L2_CTRL_TYPE_INTEGER,
> > +	.min = 0,
> > +	.max = FACE_SIZE_NUM_MAX,
> > +	.step = 1,
> > +	.def = 0,
> > +	},
> > +	{
> > +	.ops = &mtk_fd_dev_ctrl_ops,
> > +	.id = V4L2_CID_MTK_FD_DETECT_POSE,
> > +	.name = "FD detect face pose",
> > +	.type = V4L2_CTRL_TYPE_INTEGER,
> > +	.min = 0,
> > +	.max = 3,
> > +	.step = 1,
> > +	.def = 0,
> > +	},
> > +	{
> > +	.ops = &mtk_fd_dev_ctrl_ops,
> > +	.id = V4L2_CID_MTK_FD_DETECT_SPEEDUP,
> > +	.name = "FD detection speedup",
> > +	.type = V4L2_CTRL_TYPE_INTEGER,
> > +	.min = 0,
> > +	.max = FD_MAX_SPEEDUP,
> > +	.step = 1,
> > +	.def = 0,
> > +	},
> > +	{
> > +	.ops = &mtk_fd_dev_ctrl_ops,
> > +	.id = V4L2_CID_MTK_FD_EXTRA_MODEL,
> > +	.name = "FD use extra model",
> > +	.type = V4L2_CTRL_TYPE_BOOLEAN,
> > +	.min = 0,
> > +	.max = 1,
> > +	.step = 1,
> > +	.def = 0,
> > +	},
> > +};
> > +
> > +static int mtk_fd_ctrls_setup(struct mtk_fd_ctx *ctx)
> > +{
> > +	struct v4l2_ctrl_handler *hdl = &ctx->hdl;
> > +	struct v4l2_ctrl *ctl;
> > +	int i;
> > +
> > +	v4l2_ctrl_handler_init(hdl, V4L2_CID_MTK_FD_MAX);
> > +	if (hdl->error)
> > +		return hdl->error;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(mtk_fd_controls); i++) {
> > +		ctl = v4l2_ctrl_new_custom(hdl, &mtk_fd_controls[i], ctx);
> > +		if (hdl->error) {
> > +			v4l2_ctrl_handler_free(hdl);
> > +			dev_err(ctx->dev, "Failed to register controls:%d", i);
> > +			return hdl->error;
> > +		}
> > +	}
> > +
> > +	ctx->fh.ctrl_handler = &ctx->hdl;
> > +	v4l2_ctrl_handler_setup(hdl);
> > +
> > +	return 0;
> > +}
> > +
> > +static unsigned int get_fd_img_fmt(unsigned int fourcc)
> > +{
> > +	switch (fourcc) {
> > +	case V4L2_PIX_FMT_VYUY:
> > +		return FMT_VYUY;
> > +	case V4L2_PIX_FMT_YUYV:
> > +		return FMT_YUYV;
> > +	case V4L2_PIX_FMT_YVYU:
> > +		return FMT_YVYU;
> > +	case V4L2_PIX_FMT_UYVY:
> > +		return FMT_UYVY;
> > +	default:
> > +		return FMT_UNKNOWN;
> > +	}
> > +}
> > +
> > +static void init_ctx_fmt(struct mtk_fd_ctx *ctx)
> > +{
> > +	const struct v4l2_pix_format_mplane *fmt;
> > +
> > +	/* Initialize M2M source fmt */
> > +	fmt = &in_img_fmts[0];
> > +	mtk_fd_fill_pixfmt_mp(&ctx->src_fmt, fmt);
> > +
> > +	/* Initialize M2M destination fmt */
> > +	ctx->dst_fmt.buffersize = fw_param_fmts[0].buffersize;
> > +	ctx->dst_fmt.dataformat = fw_param_fmts[0].dataformat;
> 
> Why not just make dst_fmt a pointer and assign &fw_params_fmt[0] there?
> 
fw_params_fmt[] will be removed and assign the const value here.

ctx->dst_fmt.buffersize = sizeof(struct fd_user_output);
ctx->dst_fmt.dataformat = V4L2_META_FMT_MTFD_RESULT; 
> > +}
> > +
> > +/*
> > + * V4L2 file operations.
> > + */
> > +static int mtk_vfd_open(struct file *filp)
> > +{
> > +	struct mtk_fd_dev *fd_dev = video_drvdata(filp);
> > +	struct video_device *vdev = video_devdata(filp);
> > +	struct mtk_fd_ctx *ctx;
> > +	int ret;
> > +
> > +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx)
> > +		return -ENOMEM;
> > +
> > +	ctx->fd_dev = fd_dev;
> > +	ctx->dev = &fd_dev->pdev->dev;
> > +
> > +	v4l2_fh_init(&ctx->fh, vdev);
> > +	filp->private_data = &ctx->fh;
> > +
> > +	init_ctx_fmt(ctx);
> > +
> > +	ret = mtk_fd_ctrls_setup(ctx);
> > +	if (ret) {
> > +		dev_err(ctx->dev, "Failed to set up controls:%d\n", ret);
> > +		goto err_fh_ctrl;
> > +	}
> > +
> > +	ctx->fh.m2m_ctx = v4l2_m2m_ctx_init(fd_dev->m2m_dev, ctx,
> > +					    &mtk_fd_queue_init);
> > +	if (IS_ERR(ctx->fh.m2m_ctx)) {
> > +		ret = PTR_ERR(ctx->fh.m2m_ctx);
> > +		goto err_init_ctx;
> > +	}
> > +
> > +	v4l2_fh_add(&ctx->fh);
> > +
> > +	return 0;
> > +
> > +err_init_ctx:
> > +	v4l2_ctrl_handler_free(&ctx->hdl);
> > +err_fh_ctrl:
> > +	v4l2_fh_exit(&ctx->fh);
> > +	kfree(ctx);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mtk_vfd_release(struct file *filp)
> > +{
> > +	struct mtk_fd_ctx *ctx = container_of(filp->private_data,
> > +					      struct mtk_fd_ctx, fh);
> > +
> > +	v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> > +
> > +	v4l2_ctrl_handler_free(&ctx->hdl);
> > +	v4l2_fh_del(&ctx->fh);
> > +	v4l2_fh_exit(&ctx->fh);
> > +
> > +	kfree(ctx);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_file_operations fd_video_fops = {
> > +	.owner = THIS_MODULE,
> > +	.open = mtk_vfd_open,
> > +	.release = mtk_vfd_release,
> > +	.poll = v4l2_m2m_fop_poll,
> > +	.unlocked_ioctl = video_ioctl2,
> > +	.mmap = v4l2_m2m_fop_mmap,
> 
> We also need the compat_ioctl here for 32-bit userspace on 64-bit kernels.
> 
Done.

> > +};
> > +
> > +static void mtk_fd_device_run(void *priv)
> > +{
> > +	struct mtk_fd_ctx *ctx = priv;
> > +	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > +	struct fd_hw_param fd_param;
> > +	void *fd_result_vaddr;
> > +
> > +	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > +	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > +
> > +	memset(&fd_param, 0, sizeof(fd_param));
> > +
> > +	fd_param.src_img.dma_addr =
> > +		vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 0);
> > +	fd_param.user_result.dma_addr =
> > +		vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
> > +	fd_result_vaddr = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> > +
> > +	ctx->user_param.src_img_fmt = get_fd_img_fmt(ctx->src_fmt.pixelformat);
> > +	memcpy(&fd_param.user_param, &ctx->user_param, sizeof(ctx->user_param));
> > +
> > +	/* Complete request controls if any */
> > +	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, &ctx->hdl);
> > +
> > +	mtk_fd_hw_job_exec(&ctx->fd_dev->fd_hw, &fd_param, fd_result_vaddr);
> > +}
> > +
> > +static struct v4l2_m2m_ops fd_m2m_ops = {
> > +	.device_run = mtk_fd_device_run,
> > +};
> > +
> > +static int mtk_fd_request_validate(struct media_request *req)
> > +{
> > +	unsigned int count;
> > +
> > +	count = vb2_request_buffer_cnt(req);
> > +	if (!count)
> > +		return -ENOENT;
> 
> Why -ENOENT?
> 
Reference the return code in vb2_request_validate()
I consider refining as following:
static int mtk_fd_request_validate(struct media_request *req)
{
if (vb2_request_buffer_cnt(req) > 1)
return -EINVAL;

return vb2_request_validate(req);
}
or maybe I don't need to worry the request count greater than 1,
just remove this function and set vb2_request_validate as .req_validate
directly?

> > +	else if (count > 1)
> > +		return -EINVAL;
> > +
> > +	return vb2_request_validate(req);
> > +}
> > +
> > +static const struct media_device_ops fd_m2m_media_ops = {
> > +	.req_validate	= mtk_fd_request_validate,
> > +	.req_queue	= v4l2_m2m_request_queue,
> > +};
> > +
> > +static int mtk_fd_video_device_register(struct mtk_fd_dev *fd_dev)
> > +{
> > +	struct video_device *vfd = &fd_dev->vfd;
> > +	struct v4l2_m2m_dev *m2m_dev = fd_dev->m2m_dev;
> > +	struct device *dev = &fd_dev->pdev->dev;
> > +	int function, ret;
> > +
> > +	vfd->fops = &fd_video_fops;
> > +	vfd->release = video_device_release;
> > +	vfd->lock = &fd_dev->vfd_lock;
> > +	vfd->v4l2_dev = &fd_dev->v4l2_dev;
> > +	vfd->vfl_dir = VFL_DIR_M2M;
> > +	vfd->device_caps = V4L2_CAP_STREAMING  | V4L2_CAP_VIDEO_OUTPUT_MPLANE |
> > +		V4L2_CAP_META_CAPTURE;
> > +	vfd->ioctl_ops = &mtk_fd_v4l2_video_out_ioctl_ops;
> > +
> > +	strscpy(vfd->name, "MTK-FD-V4L2", sizeof(vfd->name));
> 
> Please use dev_driver_string().
> 
Done.

> > +	video_set_drvdata(vfd, fd_dev);
> > +
> > +	ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to register video device\n");
> > +		goto err_free_dev;
> > +	}
> > +
> > +	function = MEDIA_ENT_F_PROC_VIDEO_DECODER;
> 
> MEDIA_ENT_F_PROC_VIDEO_STATISTICS fits here much more.
> 
> Also nit: Any reason to have this assigned to this intermediate variable
> rather than just directly passed to the function?
> 
Too long for 80 columns, I will remove the intermediate variable and
refine as following:
ret = v4l2_m2m_register_media_controller(m2m_dev, vfd,
MEDIA_ENT_F_PROC_VIDEO_STATISTICS);

> > +	ret = v4l2_m2m_register_media_controller(m2m_dev, vfd, function);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to init mem2mem media controller\n");
> > +		goto err_unreg_video;
> > +	}
> > +	return 0;
> > +
> > +err_unreg_video:
> > +	video_unregister_device(vfd);
> > +err_free_dev:
> > +	video_device_release(vfd);
> > +	return ret;
> > +}
> > +
> > +static int mtk_fd_dev_v4l2_init(struct mtk_fd_dev *fd_dev)
> > +{
> > +	struct media_device *mdev = &fd_dev->mdev;
> > +	struct device *dev = &fd_dev->pdev->dev;
> > +	int ret;
> > +
> > +	ret = v4l2_device_register(dev, &fd_dev->v4l2_dev);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to register v4l2 device\n");
> > +		return ret;
> > +	}
> > +
> > +	fd_dev->m2m_dev = v4l2_m2m_init(&fd_m2m_ops);
> > +	if (IS_ERR(fd_dev->m2m_dev)) {
> > +		dev_err(dev, "Failed to init mem2mem device\n");
> > +		ret = PTR_ERR(fd_dev->m2m_dev);
> > +		goto fail_m2m_dev;
> 
> Please name the labels after the next clean-up step to be done, not the
> failure.
> 
Ok, I will refine as following:
err_unreg_vdev:
v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
video_unregister_device(&fd_dev->vfd);
video_device_release(&fd_dev->vfd);
err_cleanup_mdev:
media_device_cleanup(mdev);
v4l2_m2m_release(fd_dev->m2m_dev);
err_unreg_v4l2_dev:
v4l2_device_unregister(&fd_dev->v4l2_dev);

> > +	}
> > +
> > +	mdev->dev = dev;
> > +	strscpy(mdev->model, "MTK-FD-V4L2", sizeof(mdev->model));
> 
> Could we just use dev_driver_string() here instead?
> 
Done.

> > +	snprintf(mdev->bus_info, sizeof(mdev->bus_info),
> > +		 "platform:%s", dev_name(dev));
> > +	media_device_init(mdev);
> > +	mdev->ops = &fd_m2m_media_ops;
> > +	fd_dev->v4l2_dev.mdev = mdev;
> > +
> > +	ret = mtk_fd_video_device_register(fd_dev);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to register video device\n");
> > +		goto err_vdev;
> > +	}
> > +
> > +	ret = media_device_register(mdev);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to register mem2mem media device\n");
> > +		goto fail_mdev;
> > +	}
> > +
> > +	return 0;
> > +
> > +fail_mdev:
> > +	v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
> > +	video_unregister_device(&fd_dev->vfd);
> > +	video_device_release(&fd_dev->vfd);
> > +err_vdev:
> > +	media_device_cleanup(mdev);
> > +	v4l2_m2m_release(fd_dev->m2m_dev);
> > +fail_m2m_dev:
> > +	v4l2_device_unregister(&fd_dev->v4l2_dev);
> > +	return ret;
> > +}
> > +
> > +static void mtk_fd_dev_v4l2_release(struct mtk_fd_dev *fd_dev)
> > +{
> > +	v4l2_m2m_unregister_media_controller(fd_dev->m2m_dev);
> > +	video_unregister_device(&fd_dev->vfd);
> > +	video_device_release(&fd_dev->vfd);
> > +	media_device_cleanup(&fd_dev->mdev);
> > +	v4l2_m2m_release(fd_dev->m2m_dev);
> > +	v4l2_device_unregister(&fd_dev->v4l2_dev);
> > +}
> > +
> > +static irqreturn_t mtk_fd_irq(int irq, void *data)
> > +{
> > +	struct mtk_fd_hw *fd_hw = (struct mtk_fd_hw *)data;
> > +
> > +	fd_hw->fd_irq_result = readl(fd_hw->fd_base + FD_INT);
> 
> We should be able to just handle the job completion directly from here.
> 
Ok, I will try to handle the job completion from here.

> > +	wake_up_interruptible(&fd_hw->wq);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_fd_hw_get_scp_mem(struct mtk_fd_hw *fd_hw,
> > +				 struct fd_buffer *scp_mem)
> > +{
> > +	struct mtk_fd_dev *fd_dev = mtk_fd_hw_to_dev(fd_hw);
> > +	struct device *dev = &fd_dev->pdev->dev;
> > +	dma_addr_t addr;
> > +	u32 size;
> > +
> > +	scp_mem->scp_addr = scp_get_reserve_mem_phys(SCP_FD_MEM_ID);
> > +	size = scp_get_reserve_mem_size(SCP_FD_MEM_ID);
> > +	if (!scp_mem->scp_addr || !size)
> > +		return -EPROBE_DEFER;
> 
> Why -EPROBE_DEFER? Should we have some way to distinguish between the SCP
> driver not being initialized yet and some other error?
> 
Ok, I will fix it to -ENOMEM.
Not being initialized or some other error can be found during device
probe. 
(will also load scp stuff there)

> > +
> > +	/* get dma addr address */
> > +	addr = dma_map_page_attrs(dev, phys_to_page(scp_mem->scp_addr), 0,
> > +				  size, DMA_BIDIRECTIONAL,
> 
> Should this be really bidirectional?
> 
I will fix it to DMA_TO_DEVICE

> > +				  DMA_ATTR_SKIP_CPU_SYNC);
> > +	if (dma_mapping_error(dev, addr)) {
> > +		scp_mem->scp_addr = 0;
> > +		dev_err(dev, "Failed to map scp addr\n");
> > +		return -ENOMEM;
> > +	}
> > +	scp_mem->dma_addr = addr;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_probe(struct platform_device *pdev)
> > +{
> > +	struct mtk_fd_dev *fd_dev;
> 
> nit: Perhaps just fd, to have shorter code?
> 
Ok, I will refine as 
    struct mtk_fd_dev *fd; 
> > +	struct device *dev = &pdev->dev;
> > +	struct mtk_fd_hw *fd_hw;
> > +	struct resource *res;
> > +	int irq;
> > +	int ret;
> > +
> > +	fd_dev = devm_kzalloc(&pdev->dev, sizeof(*fd_dev), GFP_KERNEL);
> > +	if (!fd_dev)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(dev, fd_dev);
> > +	fd_hw = &fd_dev->fd_hw;
> > +	fd_dev->pdev = pdev;
> 
> Is pdev useful for anything? Perhaps you want to store &pdev->dev instead?
> 
Yes, I will store that instead. 
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		dev_err(dev, "no IRQ:%d resource info\n", irq);
> > +		return irq;
> > +	}
> > +	ret = devm_request_irq(dev, irq, mtk_fd_irq, IRQF_SHARED,
> > +			       dev_driver_string(dev),
> > +			       fd_hw);
> 
> Why not just fd_dev?
> 
Ok, I will use fd_dev here.

> > +	if (ret) {
> > +		dev_err(dev, "req_irq fail:%d\n", irq);
> > +		return ret;
> > +	}
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	fd_hw->fd_base = devm_ioremap_resource(dev, res);
> > +	if (!fd_hw->fd_base) {
> > +		dev_err(dev, "unable to get fd reg base\n");
> > +		return PTR_ERR(fd_hw->fd_base);
> > +	}
> > +
> > +	fd_hw->fd_clk = devm_clk_get(dev, "fd");
> > +	if (IS_ERR(fd_hw->fd_clk)) {
> > +		dev_err(dev, "cannot get fd_clk_img_fd clock\n");
> > +		return PTR_ERR(fd_hw->fd_clk);
> > +	}
> > +
> > +	ret = mtk_fd_hw_get_scp_mem(fd_hw, &fd_hw->scp_mem);
> > +	if (ret) {
> > +		dev_err(dev, "scp memory init failed: %d\n", ret);
> 
> nit: Could we make the error messages a bit more consistent? For example
> "Failed to get IRQ resource (%d)", "Failed to request IRQ (%d)", "Failed to
> ... (%d)", etc.
> 
Ok, I will fix it.

> > +		return ret;
> > +	}
> > +
> > +	atomic_set(&fd_hw->fd_user_cnt, 0);
> > +	init_waitqueue_head(&fd_hw->wq);
> > +	mutex_init(&fd_dev->vfd_lock);
> > +	mutex_init(&fd_hw->fd_hw_lock);
> > +	pm_runtime_enable(dev);
> > +
> > +	ret = mtk_fd_dev_v4l2_init(fd_dev);
> > +	if (ret) {
> > +		mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
> > +		mutex_destroy(&fd_dev->vfd_lock);
> > +		pm_runtime_disable(&pdev->dev);
> 
> Please move the clean-up to an error path on the bottom of the function.
> 
Done.

> > +		dev_err(dev, "v4l2 init failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_remove(struct platform_device *pdev)
> > +{
> > +	struct mtk_fd_dev *fd_dev = dev_get_drvdata(&pdev->dev);
> > +
> > +	mtk_fd_dev_v4l2_release(fd_dev);
> > +	mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
> > +	mutex_destroy(&fd_dev->vfd_lock);
> > +	pm_runtime_disable(&pdev->dev);
> 
> The order of calls in remove should normally be the opposite to probe.
> 
Ok, I will refine as following

mtk_fd_dev_v4l2_release(fd_dev);
pm_runtime_disable(&pdev->dev);
mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
mutex_destroy(&fd_dev->vfd_lock);
rproc_put(fd_dev->fd_hw.rproc_handle);

> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_suspend(struct device *dev)
> > +{
> > +	struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
> > +
> > +	if (pm_runtime_suspended(dev))
> > +		return 0;
> > +
> > +	/* suspend FD HW */
> > +	writel(0x0, fd_dev->fd_hw.fd_base + FD_HW_ENABLE);
> > +	writel(0x0, fd_dev->fd_hw.fd_base + FD_INT_EN);
> > +	clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
> > +	return 0;
> > +}
> > +
> > +static int mtk_fd_resume(struct device *dev)
> > +{
> > +	struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	if (pm_runtime_suspended(dev))
> > +		return 0;
> > +
> > +	ret = clk_prepare_enable(fd_dev->fd_hw.fd_clk);
> > +	if (ret < 0) {
> > +		dev_dbg(dev, "open fd clk failed\n");
> > +		clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
> > +	}
> > +
> > +	/* resume FD HW */
> > +	writel(ENABLE_FD, fd_dev->fd_hw.fd_base + FD_HW_ENABLE);
> > +	if (fd_dev->fd_hw.state == FD_ENQ)
> > +		writel(0x1, fd_dev->fd_hw.fd_base + FD_INT_EN);
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_fd_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(mtk_fd_suspend, mtk_fd_resume)
> > +	SET_RUNTIME_PM_OPS(mtk_fd_suspend, mtk_fd_resume, NULL)
> 
> We need separate runtime and system PM ops. Their behavior is expected to be
> different.
> 
> For runtime PM ops, you the functions should just unconditionally power on
> or power off the device.
> 
> For system PM ops, suspend needs to make sure that no further job is queued
> to the hardware and that any job being already processed by the hardware is
> completed. Resume needs to resume the processing if there are any further
> jobs to be queued to the hardware.
> 
Ok, for runtime suspend/resume I will add the following functions:
static int mtk_fd_runtime_suspend(struct device *dev)
{
struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);

dev_dbg(dev, "%s:disable clock\n", __func__);
clk_disable_unprepare(fd_dev->fd_hw.fd_clk);

return 0;
}

static int mtk_fd_runtime_resume(struct device *dev)
{
struct mtk_fd_dev *fd_dev = dev_get_drvdata(dev);
int ret;

dev_dbg(dev, "%s:enable clock\n", __func__);

ret = clk_prepare_enable(fd_dev->fd_hw.fd_clk);
if (ret < 0) {
dev_err(dev, "Failed to open fd clk:%d\n", ret);
return ret;
}

return 0;
}

Best regards,
Jerry 
> > +};
> > +
> > +static const struct of_device_id mtk_fd_of_ids[] = {
> > +	{ .compatible = "mediatek,mt8183-fd", },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_fd_of_ids);
> > +
> > +static struct platform_driver mtk_fd_driver = {
> > +	.probe   = mtk_fd_probe,
> > +	.remove  = mtk_fd_remove,
> > +	.driver  = {
> > +		.name  = "mtk-fd-4.0",
> > +		.of_match_table = of_match_ptr(mtk_fd_of_ids),
> > +		.pm = &mtk_fd_pm_ops,
> > +	}
> > +};
> > +module_platform_driver(mtk_fd_driver);
> > +
> > +MODULE_DESCRIPTION("Mediatek FD driver");
> > +MODULE_LICENSE("GPL");
> 
> GPL v2
> 
> Best regards,
> Tomasz
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH] ARM: dts: vf610-zii-scu4-aib: Use generic names for DT nodes
From: Shawn Guo @ 2019-08-25  7:18 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: Cory Tusar, Fabio Estevam, Chris Healy, linux-arm-kernel,
	linux-kernel
In-Reply-To: <20190824002747.14610-1-andrew.smirnov@gmail.com>

On Fri, Aug 23, 2019 at 05:27:47PM -0700, Andrey Smirnov wrote:
> The devicetree specification recommends using generic node names.
> 
> Some ZII dts files already follow such recommendation, but some don't,
> so use generic node names for consistency among the ZII dts files.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Cory Tusar <cory.tusar@zii.aero>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org

It doesn't apply to my branch.

Shawn

> ---
>  arch/arm/boot/dts/vf610-zii-scu4-aib.dts | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/vf610-zii-scu4-aib.dts b/arch/arm/boot/dts/vf610-zii-scu4-aib.dts
> index 45a978defbdc..6edd682dbd29 100644
> --- a/arch/arm/boot/dts/vf610-zii-scu4-aib.dts
> +++ b/arch/arm/boot/dts/vf610-zii-scu4-aib.dts
> @@ -417,7 +417,7 @@
>  	pinctrl-0 = <&pinctrl_dspi1>;
>  	status = "okay";
>  
> -	spi-flash@0 {
> +	flash@0 {
>  		#address-cells = <1>;
>  		#size-cells = <1>;
>  		compatible = "jedec,spi-nor";
> @@ -430,7 +430,7 @@
>  		};
>  	};
>  
> -	spi-flash@1 {
> +	flash@1 {
>  		#address-cells = <1>;
>  		#size-cells = <1>;
>  		compatible = "jedec,spi-nor";
> @@ -519,7 +519,7 @@
>  		#gpio-cells = <2>;
>  	};
>  
> -	lm75@48 {
> +	temp-sensor@48 {
>  		compatible = "national,lm75";
>  		reg = <0x48>;
>  	};
> @@ -534,7 +534,7 @@
>  		reg = <0x52>;
>  	};
>  
> -	ds1682@6b {
> +	elapsed-time-recorder@6b {
>  		compatible = "dallas,ds1682";
>  		reg = <0x6b>;
>  	};
> @@ -551,7 +551,7 @@
>  		reg = <0x38>;
>  	};
>  
> -	adt7411@4a {
> +	adc@4a {
>  		compatible = "adi,adt7411";
>  		reg = <0x4a>;
>  	};
> @@ -563,7 +563,7 @@
>  	pinctrl-0 = <&pinctrl_i2c2>;
>  	status = "okay";
>  
> -	gpio9: sx1503q@20 {
> +	gpio9: io-expander@20 {
>  		compatible = "semtech,sx1503q";
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&pinctrl_sx1503_20>;
> @@ -574,12 +574,12 @@
>  		interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
>  	};
>  
> -	lm75@4e {
> +	temp-sensor@4e {
>  		compatible = "national,lm75";
>  		reg = <0x4e>;
>  	};
>  
> -	lm75@4f {
> +	temp-sensor@4f {
>  		compatible = "national,lm75";
>  		reg = <0x4f>;
>  	};
> @@ -591,17 +591,17 @@
>  		reg = <0x23>;
>  	};
>  
> -	adt7411@4a {
> +	adc@4a {
>  		compatible = "adi,adt7411";
>  		reg = <0x4a>;
>  	};
>  
> -	at24c08@54 {
> +	eeprom@54 {
>  		compatible = "atmel,24c08";
>  		reg = <0x54>;
>  	};
>  
> -	tca9548@70 {
> +	i2c-mux@70 {
>  		compatible = "nxp,pca9548";
>  		pinctrl-names = "default";
>  		#address-cells = <1>;
> @@ -639,7 +639,7 @@
>  		};
>  	};
>  
> -	tca9548@71 {
> +	i2c-mux@71 {
>  		compatible = "nxp,pca9548";
>  		pinctrl-names = "default";
>  		reg = <0x71>;
> -- 
> 2.21.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH] ARM: dts: vf610-zii-scu4-aib: Configure IRQ line for GPIO expander
From: Shawn Guo @ 2019-08-25  7:15 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: Cory Tusar, Fabio Estevam, Chris Healy, linux-arm-kernel,
	linux-kernel
In-Reply-To: <20190824002703.13902-1-andrew.smirnov@gmail.com>

On Fri, Aug 23, 2019 at 05:27:03PM -0700, Andrey Smirnov wrote:
> Configure IRQ line for SX1503 GPIO expander. We already have
> appropriate pinmux entry and all that is missing is "interrupt-parent"
> and "interrupts" properties. Add them.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Cory Tusar <cory.tusar@zii.aero>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org

Applied, thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH] input: keyboard: snvs_pwrkey: Send press and release event for i.MX6 S,DL and Q
From: Shawn Guo @ 2019-08-25  7:10 UTC (permalink / raw)
  To: Robin van der Gracht
  Cc: Mark Rutland, devicetree, Adam Ford, Sascha Hauer,
	Dmitry Torokhov, linux-kernel, Rob Herring, NXP Linux Team,
	Pengutronix Kernel Team, linux-input, Robin Gong, Fabio Estevam,
	linux-arm-kernel
In-Reply-To: <20190823123002.10448-1-robin@protonic.nl>

On Fri, Aug 23, 2019 at 02:30:02PM +0200, Robin van der Gracht wrote:
> The older generation i.MX6 processors send a powerdown request interrupt if
> the powerkey is released before a hard shutdown (5 second press). This should
> allow software to bring down the SoC safely.
> 
> For this driver to work as a regular powerkey with the older SoCs, we need to
> send a keypress AND release when we get the powerdown request interrupt.
> 
> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
> ---
>  arch/arm/boot/dts/imx6qdl.dtsi       |  2 +-
>  arch/arm/boot/dts/imx6sll.dtsi       |  2 +-
>  arch/arm/boot/dts/imx6sx.dtsi        |  2 +-
>  arch/arm/boot/dts/imx6ul.dtsi        |  2 +-
>  arch/arm/boot/dts/imx7s.dtsi         |  2 +-

Please have dts changes in a separate patch.

>  drivers/input/keyboard/Kconfig       |  2 +-
>  drivers/input/keyboard/snvs_pwrkey.c | 59 +++++++++++++++++++++++-----
>  7 files changed, 56 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index b3a77bcf00d51..c10d12658743c 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -836,7 +836,7 @@
>  				};
>  
>  				snvs_pwrkey: snvs-powerkey {
> -					compatible = "fsl,sec-v4.0-pwrkey";
> +					compatible = "fsl,imx6qdl-sec-v4.0-pwrkey";
>  					regmap = <&snvs>;
>  					interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
>  					linux,keycode = <KEY_POWER>;
> diff --git a/arch/arm/boot/dts/imx6sll.dtsi b/arch/arm/boot/dts/imx6sll.dtsi
> index 1b4899f0fcded..91c7d5bdcc359 100644
> --- a/arch/arm/boot/dts/imx6sll.dtsi
> +++ b/arch/arm/boot/dts/imx6sll.dtsi
> @@ -571,7 +571,7 @@
>  				};
>  
>  				snvs_pwrkey: snvs-powerkey {
> -					compatible = "fsl,sec-v4.0-pwrkey";
> +					compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
>  					regmap = <&snvs>;
>  					interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
>  					linux,keycode = <KEY_POWER>;
> diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
> index b16a123990a26..b6736db65350f 100644
> --- a/arch/arm/boot/dts/imx6sx.dtsi
> +++ b/arch/arm/boot/dts/imx6sx.dtsi
> @@ -733,7 +733,7 @@
>  				};
>  
>  				snvs_pwrkey: snvs-powerkey {
> -					compatible = "fsl,sec-v4.0-pwrkey";
> +					compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
>  					regmap = <&snvs>;
>  					interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
>  					linux,keycode = <KEY_POWER>;
> diff --git a/arch/arm/boot/dts/imx6ul.dtsi b/arch/arm/boot/dts/imx6ul.dtsi
> index a7f6d1d58e20d..d4678c52b55db 100644
> --- a/arch/arm/boot/dts/imx6ul.dtsi
> +++ b/arch/arm/boot/dts/imx6ul.dtsi
> @@ -644,7 +644,7 @@
>  				};
>  
>  				snvs_pwrkey: snvs-powerkey {
> -					compatible = "fsl,sec-v4.0-pwrkey";
> +					compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
>  					regmap = <&snvs>;
>  					interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
>  					linux,keycode = <KEY_POWER>;
> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
> index 106711d2c01b0..bb68c23beb199 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -604,7 +604,7 @@
>  				};
>  
>  				snvs_pwrkey: snvs-powerkey {
> -					compatible = "fsl,sec-v4.0-pwrkey";
> +					compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
>  					regmap = <&snvs>;
>  					interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
>  					linux,keycode = <KEY_POWER>;
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 7c4f19dab34fd..937e58da5ce17 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -436,7 +436,7 @@ config KEYBOARD_SNVS_PWRKEY
>  	depends on OF
>  	help
>  	  This is the snvs powerkey driver for the Freescale i.MX application
> -	  processors that are newer than i.MX6 SX.
> +	  processors.
>  
>  	  To compile this driver as a module, choose M here; the
>  	  module will be called snvs_pwrkey.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 5342d8d45f811..c321e5f2d1087 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -29,6 +29,11 @@
>  #define DEBOUNCE_TIME 30
>  #define REPEAT_INTERVAL 60
>  
> +enum imx_snvs_hwtype {
> +	IMX6SX_SNVS,	/* i.MX6 SoloX and newer */
> +	IMX6QDL_SNVS,	/* i.MX6 Solo, DualLite adn Quad */
> +};
> +
>  struct pwrkey_drv_data {
>  	struct regmap *snvs;
>  	int irq;
> @@ -37,6 +42,19 @@ struct pwrkey_drv_data {
>  	int wakeup;
>  	struct timer_list check_timer;
>  	struct input_dev *input;
> +	enum imx_snvs_hwtype hwtype;
> +};
> +
> +static const struct platform_device_id imx_snvs_devtype[] = {
> +	{
> +		.name = "imx6sx-snvs-pwrkey",
> +		.driver_data = IMX6SX_SNVS,
> +	}, {
> +		.name = "imx6qdl-snvs-pwrkey",
> +		.driver_data = IMX6QDL_SNVS,
> +	}, {
> +		/* sentinel */
> +	}
>  };
>  
>  static void imx_imx_snvs_check_for_events(struct timer_list *t)
> @@ -67,13 +85,23 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
>  {
>  	struct platform_device *pdev = dev_id;
>  	struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> +	struct input_dev *input = pdata->input;
>  	u32 lp_status;
>  
> -	pm_wakeup_event(pdata->input->dev.parent, 0);
> +	pm_wakeup_event(input->dev.parent, 0);
>  
>  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> -	if (lp_status & SNVS_LPSR_SPO)
> -		mod_timer(&pdata->check_timer, jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> +	if (lp_status & SNVS_LPSR_SPO) {
> +		if (pdata->hwtype == IMX6QDL_SNVS) {
> +			input_report_key(input, pdata->keycode, 1);
> +			input_report_key(input, pdata->keycode, 0);
> +			input_sync(input);
> +			pm_relax(input->dev.parent);
> +		} else {
> +			mod_timer(&pdata->check_timer,
> +				jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> +		}
> +	}
>  
>  	/* clear SPO status */
>  	regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO);
> @@ -88,11 +116,24 @@ static void imx_snvs_pwrkey_act(void *pdata)
>  	del_timer_sync(&pd->check_timer);
>  }
>  
> +static const struct of_device_id imx_snvs_pwrkey_ids[] = {
> +	{
> +		.compatible = "fsl,imx6sx-sec-v4.0-pwrkey",

The new compatible should be documented.

> +		.data = &imx_snvs_devtype[IMX6SX_SNVS],
> +	}, {
> +		.compatible = "fsl,imx6qdl-sec-v4.0-pwrkey",
> +		.data = &imx_snvs_devtype[IMX6QDL_SNVS],
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids);
> +
>  static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  {
>  	struct pwrkey_drv_data *pdata = NULL;
>  	struct input_dev *input = NULL;
>  	struct device_node *np;
> +	const struct of_device_id *of_id;
>  	int error;
>  
>  	/* Get SNVS register Page */
> @@ -100,6 +141,11 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  	if (!np)
>  		return -ENODEV;
>  
> +	of_id = of_match_node(imx_snvs_pwrkey_ids, np);
> +	if (!of_id)
> +		return -ENODEV;
> +	pdev->id_entry = of_id->data;
> +
>  	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>  	if (!pdata)
>  		return -ENOMEM;
> @@ -116,6 +162,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  	}
>  
>  	pdata->wakeup = of_property_read_bool(np, "wakeup-source");
> +	pdata->hwtype = pdev->id_entry->driver_data;
>  
>  	pdata->irq = platform_get_irq(pdev, 0);
>  	if (pdata->irq < 0) {
> @@ -175,12 +222,6 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static const struct of_device_id imx_snvs_pwrkey_ids[] = {
> -	{ .compatible = "fsl,sec-v4.0-pwrkey" },

The compatible should be kept for not breaking existing DTBs.

Shawn

> -	{ /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids);
> -
>  static struct platform_driver imx_snvs_pwrkey_driver = {
>  	.driver = {
>  		.name = "snvs_pwrkey",
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH 7/7] arm64: dts: meson-gxm-khadas-vim2: use rc-khadas keymap
From: Christian Hewitt @ 2019-08-25  4:01 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
	linux-arm-kernel, linux-amlogic, linux-kernel
  Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>

Swap to the rc-khadas keymap that maps the mouse button to KEY_MUTE.

Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts b/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
index 989d33a..f25ddd1 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
@@ -299,7 +299,7 @@
 	status = "okay";
 	pinctrl-0 = <&remote_input_ao_pins>;
 	pinctrl-names = "default";
-	linux,rc-map-name = "rc-geekbox";
+	linux,rc-map-name = "rc-khadas";
 };
 
 &pwm_AO_ab {
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 6/7] arm64: dts: meson-gxl-s905w-tx3-mini: add rc-tx3mini keymap
From: Christian Hewitt @ 2019-08-25  4:01 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
	linux-arm-kernel, linux-amlogic, linux-kernel
  Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>

add the rc-tx3mini keymap to the ir node

Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts b/arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts
index 789c819..dd729ac 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts
@@ -20,3 +20,7 @@
 		reg = <0x0 0x0 0x0 0x40000000>; /* 1 GiB or 2 GiB */
 	};
 };
+
+&ir {
+	linux,rc-map-name = "rc-tanix-tx3mini";
+};
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 5/7] arm64: dts: meson-gxl-s905x-khadas-vim: use rc-khadas keymap
From: Christian Hewitt @ 2019-08-25  4:01 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
	linux-arm-kernel, linux-amlogic, linux-kernel
  Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>

Swap to the rc-khadas keymap that maps the mouse button to KEY_MUTE.

Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts
index 5499e8d..2a5cd30 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts
@@ -110,7 +110,7 @@
 };
 
 &ir {
-	linux,rc-map-name = "rc-geekbox";
+	linux,rc-map-name = "rc-khadas";
 };
 
 &gpio_ao {
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 4/7] arm64: dts: meson-gxbb-wetek-play2: add rc-wetek-play2 keymap
From: Christian Hewitt @ 2019-08-25  4:01 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
	linux-arm-kernel, linux-amlogic, linux-kernel
  Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>

add the rc-wetek-play2 keymap to the ir node

Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-play2.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-play2.dts b/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-play2.dts
index 0038522..1d32d1f 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-play2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-play2.dts
@@ -54,3 +54,7 @@
 &usb1 {
 	status = "okay";
 };
+
+&ir {
+	linux,rc-map-name = "rc-wetek-play2";
+};
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 3/7] arm64: dts: meson-gxbb-wetek-hub: add rc-wetek-hub keymap
From: Christian Hewitt @ 2019-08-25  4:01 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
	linux-arm-kernel, linux-amlogic, linux-kernel
  Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>

add the rc-wetek-hub keymap to the ir node

Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts b/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts
index 2bfe699..83b985b 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts
@@ -12,3 +12,7 @@
 	compatible = "wetek,hub", "amlogic,meson-gxbb";
 	model = "WeTek Hub";
 };
+
+&ir {
+	linux,rc-map-name = "rc-wetek-hub";
+};
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 2/7] arm64: dts: meson-g12a-x96-max: add rc-x96max keymap
From: Christian Hewitt @ 2019-08-25  4:01 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
	linux-arm-kernel, linux-amlogic, linux-kernel
  Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>

add the rc-x96max keymap to the ir node

Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
 arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts
index fe4013c..357d7dc 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts
@@ -277,6 +277,7 @@
 	status = "okay";
 	pinctrl-0 = <&remote_input_ao_pins>;
 	pinctrl-names = "default";
+	linux,rc-map-name = "rc-x96max";
 };
 
 &ext_mdio {
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 1/7] arm64: dts: meson-g12b-odroid-n2: add rc-odroid keymap
From: Christian Hewitt @ 2019-08-25  4:01 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
	linux-arm-kernel, linux-amlogic, linux-kernel
  Cc: Chrisitian Hewitt
In-Reply-To: <1566705688-18442-1-git-send-email-christianshewitt@gmail.com>

add the rc-odroid keymap to the ir node

Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
 arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
index 81780ff..35cef76 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
@@ -314,6 +314,7 @@
 	status = "okay";
 	pinctrl-0 = <&remote_input_ao_pins>;
 	pinctrl-names = "default";
+	linux,rc-map-name = "rc-odroid";
 };
 
 /* SD card */
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH 0/7] arm64: dts: meson: ir keymap updates
From: Christian Hewitt @ 2019-08-25  4:01 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
	linux-arm-kernel, linux-amlogic, linux-kernel
  Cc: Chrisitian Hewitt

This series adds keymaps for several box/board vendor IR remote devices
to respective device-tree files. The keymaps were submitted in [0] and
have been queued for inclusion in Linux 5.4.

The Khadas remote change swaps the rc-geekbox keymap for rc-khadas. The
Geekbox branded remote was only sold for a brief period when VIM(1) was
a new device. The Khadas branded remote that replaced it exchanged the
Geekbox full-screen key for an Android mouse button using a different IR
keycode. The rc-khadas keymap supports the mouse button keycode and maps
it to KEY_MUTE.

[0] https://patchwork.kernel.org/project/linux-media/list/?series=160309

Christian Hewitt (7):
  arm64: dts: meson-g12b-odroid-n2: add rc-odroid keymap
  arm64: dts: meson-g12a-x96-max: add rc-x96max keymap
  arm64: dts: meson-gxbb-wetek-hub: add rc-wetek-hub keymap
  arm64: dts: meson-gxbb-wetek-play2: add rc-wetek-play2 keymap
  arm64: dts: meson-gxl-s905x-khadas-vim: use rc-khadas keymap
  arm64: dts: meson-gxl-s905w-tx3-mini: add rc-tx3mini keymap
  arm64: dts: meson-gxm-khadas-vim2: use rc-khadas keymap

 arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts         | 1 +
 arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts       | 1 +
 arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts       | 4 ++++
 arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-play2.dts     | 4 ++++
 arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts   | 4 ++++
 arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts | 2 +-
 arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts      | 2 +-
 7 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* RE: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for ls1088a and ls2088a
From: Xiaowei Bao @ 2019-08-25  3:07 UTC (permalink / raw)
  To: christophe leroy, Andrew Murray
  Cc: mark.rutland@arm.com, Roy Zang, bhelgaas@google.com,
	lorenzo.pieralisi@arm.co, arnd@arndb.de,
	devicetree@vger.kernel.org, gregkh@linuxfoundation.org, Leo Li,
	linux-kernel@vger.kernel.org, kishon@ti.com, M.h. Lian,
	robh+dt@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-pci@vger.kernel.org, jingoohan1@gmail.com,
	shawnguo@kernel.org, gustavo.pimentel@synopsys.com,
	linuxppc-dev@lists.ozlabs.org, Mingkai Hu
In-Reply-To: <89c90732-5e42-f87e-73b1-8d615355afc4@c-s.fr>



> -----Original Message-----
> From: christophe leroy <christophe.leroy@c-s.fr>
> Sent: 2019年8月24日 14:45
> To: Xiaowei Bao <xiaowei.bao@nxp.com>; Andrew Murray
> <andrew.murray@arm.com>
> Cc: mark.rutland@arm.com; Roy Zang <roy.zang@nxp.com>;
> lorenzo.pieralisi@arm.co; arnd@arndb.de; devicetree@vger.kernel.org;
> gregkh@linuxfoundation.org; linuxppc-dev@lists.ozlabs.org;
> linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; kishon@ti.com; M.h.
> Lian <minghuan.lian@nxp.com>; robh+dt@kernel.org;
> gustavo.pimentel@synopsys.com; jingoohan1@gmail.com;
> bhelgaas@google.com; Leo Li <leoyang.li@nxp.com>; shawnguo@kernel.org;
> Mingkai Hu <mingkai.hu@nxp.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> ls1088a and ls2088a
> 
> 
> 
> Le 24/08/2019 à 02:18, Xiaowei Bao a écrit :
> >
> >
> >> -----Original Message-----
> >> From: Andrew Murray <andrew.murray@arm.com>
> >> Sent: 2019年8月23日 22:28
> >> To: Xiaowei Bao <xiaowei.bao@nxp.com>
> >> Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com;
> >> shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com;
> >> lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org;
> M.h.
> >> Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy
> >> Zang <roy.zang@nxp.com>; jingoohan1@gmail.com;
> >> gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org;
> >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
> >> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support
> >> for ls1088a and ls2088a
> >>
> >> On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> >>> Add PCIe EP mode support for ls1088a and ls2088a, there are some
> >>> difference between LS1 and LS2 platform, so refactor the code of the
> >>> EP driver.
> >>>
> >>> Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com>
> >>> ---
> >>> v2:
> >>>   - New mechanism for layerscape EP driver.
> >>
> >> Was there a v1 of this patch?
> >
> > Yes, but I don't know how to comments, ^_^
> 
> As far as I can see, in the previous version of the series
> (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.ozlabs.org%2Fproject%2Flinuxppc-dev%2Flist%2F%3Fseries%3D125315
> %26state%3D*&amp;data=02%7C01%7Cxiaowei.bao%40nxp.com%7C1befe9
> a67c8046f9535e08d7285eaab6%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C637022259387139020&amp;sdata=p4wbycd04Z7qRUfAoZtwc
> UP7pR%2FuA3%2FjVcWMz6YyQVQ%3D&amp;reserved=0),
> the 8/10 was something completely different, and I can't find any other patch
> in the series that could have been the v1 of this patch.

Thanks, I will correct it to v1 in next version patch.

> 
> Christophe
> 
> >
> >>
> >>>
> >>>   drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> >>> ++++++++++++++++++++------
> >>>   1 file changed, 58 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> >>> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> >>> index 7ca5fe8..2a66f07 100644
> >>> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> >>> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> >>> @@ -20,27 +20,29 @@
> >>>
> >>>   #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
> >>>
> >>> -struct ls_pcie_ep {
> >>> -	struct dw_pcie		*pci;
> >>> -	struct pci_epc_features	*ls_epc;
> >>> +#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> >>> +
> >>> +struct ls_pcie_ep_drvdata {
> >>> +	u32				func_offset;
> >>> +	const struct dw_pcie_ep_ops	*ops;
> >>> +	const struct dw_pcie_ops	*dw_pcie_ops;
> >>>   };
> >>>
> >>> -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> >>> +struct ls_pcie_ep {
> >>> +	struct dw_pcie			*pci;
> >>> +	struct pci_epc_features		*ls_epc;
> >>> +	const struct ls_pcie_ep_drvdata *drvdata; };
> >>>
> >>>   static int ls_pcie_establish_link(struct dw_pcie *pci)  {
> >>>   	return 0;
> >>>   }
> >>>
> >>> -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> >>> +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> >>>   	.start_link = ls_pcie_establish_link,  };
> >>>
> >>> -static const struct of_device_id ls_pcie_ep_of_match[] = {
> >>> -	{ .compatible = "fsl,ls-pcie-ep",},
> >>> -	{ },
> >>> -};
> >>> -
> >>>   static const struct pci_epc_features*
> >>> ls_pcie_ep_get_features(struct dw_pcie_ep *ep)  { @@ -82,10 +84,44
> >>> @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> >>>   	}
> >>>   }
> >>>
> >>> -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> >>> +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> >>> +						u8 func_no)
> >>> +{
> >>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >>> +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> >>> +	u8 header_type;
> >>> +
> >>> +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> >>> +
> >>> +	if (header_type & (1 << 7))
> >>> +		return pcie->drvdata->func_offset * func_no;
> >>> +	else
> >>> +		return 0;
> >>
> >> It looks like there isn't a PCI define for multi function, the
> >> nearest I could find was PCI_HEADER_TYPE_MULTIDEVICE in
> >> hotplug/ibmphp.h. A comment above the test might be helpful to explain
> the test.
> >
> > Yes, I have not find the PCI_HEADER_TYPE_MULTIDEVICE define. OK, I
> > will add The comments in next version patch.
> >
> >>
> >> As the ls_pcie_ep_drvdata structures are static, the unset
> >> .func_offset will be initialised to 0, so you could just drop the test above.
> >
> > OK, thanks
> >
> >>
> >> However something to the effect of the following may help spot
> >> misconfiguration:
> >>
> >> WARN_ON(func_no && !pcie->drvdata->func_offset); return
> >> pcie->drvdata->func_offset * func_no;
> >
> > Thanks a lot, this looks better.
> >
> >>
> >> The WARN is probably quite useful as if you are attempting to use
> >> non-zero functions and func_offset isn't set - then things may appear
> >> to work normally but actually will break horribly.
> >
> > got it, thanks.
> >
> >>
> >> Thanks,
> >>
> >> Andrew Murray
> >>
> >>> +}
> >>> +
> >>> +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> >>>   	.ep_init = ls_pcie_ep_init,
> >>>   	.raise_irq = ls_pcie_ep_raise_irq,
> >>>   	.get_features = ls_pcie_ep_get_features,
> >>> +	.func_conf_select = ls_pcie_ep_func_conf_select, };
> >>> +
> >>> +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> >>> +	.ops = &ls_pcie_ep_ops,
> >>> +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> >>> +
> >>> +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> >>> +	.func_offset = 0x20000,
> >>> +	.ops = &ls_pcie_ep_ops,
> >>> +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> >>> +
> >>> +static const struct of_device_id ls_pcie_ep_of_match[] = {
> >>> +	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> >>> +	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> >>> +	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> >>> +	{ },
> >>>   };
> >>>
> >>>   static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@ -98,7
> >>> +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
> >>>   	int ret;
> >>>
> >>>   	ep = &pci->ep;
> >>> -	ep->ops = &pcie_ep_ops;
> >>> +	ep->ops = pcie->drvdata->ops;
> >>>
> >>>   	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> >> "addr_space");
> >>>   	if (!res)
> >>> @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
> >> platform_device *pdev)
> >>>   	if (!ls_epc)
> >>>   		return -ENOMEM;
> >>>
> >>> -	dbi_base = platform_get_resource_byname(pdev,
> IORESOURCE_MEM,
> >> "regs");
> >>> -	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> >>> -	if (IS_ERR(pci->dbi_base))
> >>> -		return PTR_ERR(pci->dbi_base);
> >>> +	pcie->drvdata = of_device_get_match_data(dev);
> >>>
> >>> -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> >>>   	pci->dev = dev;
> >>> -	pci->ops = &ls_pcie_ep_ops;
> >>> +	pci->ops = pcie->drvdata->dw_pcie_ops;
> >>> +
> >>>   	pcie->pci = pci;
> >>>
> >>>   	ls_epc->linkup_notifier = false,
> >>> @@ -152,6 +185,13 @@ static int __init ls_pcie_ep_probe(struct
> >>> platform_device *pdev)
> >>>
> >>>   	pcie->ls_epc = ls_epc;
> >>>
> >>> +	dbi_base = platform_get_resource_byname(pdev,
> IORESOURCE_MEM,
> >> "regs");
> >>> +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> >>> +	if (IS_ERR(pci->dbi_base))
> >>> +		return PTR_ERR(pci->dbi_base);
> >>> +
> >>> +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> >>> +
> >>>   	platform_set_drvdata(pdev, pcie);
> >>>
> >>>   	ret = ls_add_pcie_ep(pcie, pdev);
> >>> --
> >>> 2.9.5
> >>>
> 
> ---
> L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel
> antivirus Avast.
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> avast.com%2Fantivirus&amp;data=02%7C01%7Cxiaowei.bao%40nxp.com%7
> C1befe9a67c8046f9535e08d7285eaab6%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C637022259387139020&amp;sdata=JAYds7X%2FHVxgtrg
> e%2F%2FvnP84zdb2yReXcctQUiSLC11I%3D&amp;reserved=0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v2 2/2] dma-contiguous: Use fallback alloc_pages for single pages
From: Christoph Hellwig @ 2019-08-25  1:10 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Ulf Hansson, Tony Lindgren, Catalin Marinas, Will Deacon,
	Linux Kernel Mailing List, Max Filippov, Christoph Hellwig,
	Marek Szyprowski, Stephen Rothwell, Joerg Roedel, Russell King,
	Thierry Reding, linux-xtensa, Kees Cook, Nicolin Chen,
	Andrew Morton, linux-arm-kernel, Chris Zankel, Wolfram Sang,
	Robin Murphy, linux-mmc, Adrian Hunter, iommu, iamjoonsoo.kim,
	David Woodhouse
In-Reply-To: <CAK7LNAQfYBCoChMV=MOwcUyVoqRkrPWs7DaWdzDqjBe18gGiAQ@mail.gmail.com>

On Fri, Aug 23, 2019 at 09:56:52PM +0900, Masahiro Yamada wrote:
> + linux-mmc, Ulf Hansson, Adrian Hunter,
> 
> 
> ADMA of SDHCI is not working
> since bd2e75633c8012fc8a7431c82fda66237133bf7e

Does it work for you with this commit:

http://git.infradead.org/users/hch/dma-mapping.git/commitdiff/90ae409f9eb3bcaf38688f9ec22375816053a08e

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH net-next v2 3/3] net: dsa: mt7530: Add support for port 5
From: David Miller @ 2019-08-24 23:19 UTC (permalink / raw)
  To: opensource
  Cc: andrew, f.fainelli, frank-w, netdev, sean.wang, linux-mips,
	linux-mediatek, john, matthias.bgg, vivien.didelot,
	linux-arm-kernel
In-Reply-To: <20190821144547.15113-4-opensource@vdorst.com>

From: René van Dorst <opensource@vdorst.com>
Date: Wed, 21 Aug 2019 16:45:47 +0200

> +	dev_info(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
> +		 val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));

This is debugging, at best.  Please make this a debugging message or
remove it entirely.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH V5 1/5] iommu/amd: Remove unnecessary locking from AMD iommu driver
From: Christoph Hellwig @ 2019-08-24 22:41 UTC (permalink / raw)
  To: Tom Murphy
  Cc: Heiko Stuebner, virtualization, Matthias Brugger, Thierry Reding,
	Will Deacon, Jean-Philippe Brucker, linux-samsung-soc,
	Krzysztof Kozlowski, Jonathan Hunter, Christoph Hellwig,
	linux-rockchip, Kukjin Kim, Andy Gross, linux-s390,
	Gerald Schaefer, linux-arm-msm, linux-mediatek, linux-tegra,
	linux-arm-kernel, Robin Murphy, Linux Kernel Mailing List, iommu,
	David Woodhouse
In-Reply-To: <CALQxJussiGDzWFT1xhko6no5jZNOezWCFuJQUCr4XwH4NHri3Q@mail.gmail.com>

Thank for the explanation Tom. It might make sense to add a condensed
version of it to commit log for the next iteration.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: cleanup the dma_pgprot handling
From: Christoph Hellwig @ 2019-08-24 22:34 UTC (permalink / raw)
  To: Paul Burton
  Cc: Shawn Anastasio, Will Deacon, linux-m68k@lists.linux-m68k.org,
	Catalin Marinas, linuxppc-dev@lists.ozlabs.org,
	linux-kernel@vger.kernel.org, Russell King,
	linux-mips@vger.kernel.org, iommu@lists.linux-foundation.org,
	Geert Uytterhoeven, James Hogan, Guan Xuetao, Christoph Hellwig,
	linux-arm-kernel@lists.infradead.org, Robin Murphy
In-Reply-To: <20190823215759.zprrwotlbva46y33@pburton-laptop>

On Fri, Aug 23, 2019 at 09:58:04PM +0000, Paul Burton wrote:
> So I believe uncached & uncached accelerated are another case like that
> described above - they're 2 different CCAs but the same "access type",
> namely uncached.
> 
> Section 4.9 then goes on to forbid mixing access types, but not CCAs.
> 
> It would be nice if the precise mapping from CCA to access type was
> provided, but I don't see that anywhere. I can check with the
> architecture team to be sure, but to my knowledge we're fine to mix
> access via kseg1 (ie. uncached) & mappings with CCA=7 (uncached
> accelerated).

Ok.  Looks like we can keep it then and I'll add a comment to the
code with the above reference.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH net-next v2 0/3] net: dsa: mt7530: Convert to PHYLINK and add support for port 5
From: Russell King - ARM Linux admin @ 2019-08-24 22:31 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: f.fainelli, frank-w, netdev, sean.wang, linux-mips,
	vivien.didelot, opensource, linux-mediatek, john, matthias.bgg,
	David Miller, linux-arm-kernel
In-Reply-To: <20190824221519.GF8251@lunn.ch>

On Sun, Aug 25, 2019 at 12:15:19AM +0200, Andrew Lunn wrote:
> 65;5402;1cOn Sat, Aug 24, 2019 at 02:18:03PM -0700, David Miller wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > Date: Fri, 23 Aug 2019 03:09:28 +0200
> > 
> > > That would be Russell.
> > > 
> > > We should try to improve MAINTAINER so that Russell King gets picked
> > > by the get_maintainer script.
> > 
> > Shoule he be added to the mt7530 entry?
> 
> Hi David
> 
> No. I think we need a phylink entry. And then make use of the K: line
> format to list keywords. I hope that even though changes like this
> don't touch any files listed as being part of phylink, they will match
> the keyword and pickup Russell.

Note that phylink itself is already covered by

"SFF/SFP/SFP+ MODULE SUPPORT"

but doesn't pick up on keywords.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH net-next v2 0/3] net: dsa: mt7530: Convert to PHYLINK and add support for port 5
From: Russell King - ARM Linux admin @ 2019-08-24 22:29 UTC (permalink / raw)
  To: René van Dorst
  Cc: Andrew Lunn, Florian Fainelli, Frank Wunderlich, netdev,
	Sean Wang, linux-mips, David S . Miller, linux-mediatek,
	John Crispin, Matthias Brugger, Vivien Didelot, linux-arm-kernel
In-Reply-To: <20190821144547.15113-1-opensource@vdorst.com>

On Wed, Aug 21, 2019 at 04:45:44PM +0200, René van Dorst wrote:
> 1. net: dsa: mt7530: Convert to PHYLINK API
>    This patch converts mt7530 to PHYLINK API.
> 2. dt-bindings: net: dsa: mt7530: Add support for port 5
> 3. net: dsa: mt7530: Add support for port 5
>    These 2 patches adding support for port 5 of the switch.
> 
> v1->v2:
>  * Mostly phylink improvements after review.
> rfc -> v1:
>  * Mostly phylink improvements after review.
>  * Drop phy isolation patches. Adds no value for now.
> René van Dorst (3):
>   net: dsa: mt7530: Convert to PHYLINK API
>   dt-bindings: net: dsa: mt7530: Add support for port 5
>   net: dsa: mt7530: Add support for port 5
> 
>  .../devicetree/bindings/net/dsa/mt7530.txt    | 218 ++++++++++
>  drivers/net/dsa/mt7530.c                      | 371 +++++++++++++++---
>  drivers/net/dsa/mt7530.h                      |  61 ++-
>  3 files changed, 577 insertions(+), 73 deletions(-)

Having looked through this set of patches, I don't see anything
from the phylink point of view that concerns me.  So, for the
series from the phylink perspective:

Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

Thanks.

I did notice a dev_info() in patch 3 that you may like to consider
whether they should be printed at info level or debug level.  You
may keep my ack on the patch when fixing that.

I haven't considered whether the patch passes davem's style
requirements for networking code; what I spotted did look like
the declarations were upside-down christmas tree.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH net-next v2 0/3] net: dsa: mt7530: Convert to PHYLINK and add support for port 5
From: Russell King - ARM Linux admin @ 2019-08-24 22:18 UTC (permalink / raw)
  To: David Miller
  Cc: andrew, f.fainelli, frank-w, netdev, sean.wang, linux-mips,
	opensource, linux-mediatek, john, matthias.bgg, vivien.didelot,
	linux-arm-kernel
In-Reply-To: <20190824.141803.1656753287804303137.davem@davemloft.net>

On Sat, Aug 24, 2019 at 02:18:03PM -0700, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Fri, 23 Aug 2019 03:09:28 +0200
> 
> > That would be Russell.
> > 
> > We should try to improve MAINTAINER so that Russell King gets picked
> > by the get_maintainer script.
> 
> Shoule he be added to the mt7530 entry?

Probably some way to make MAINTAINERS pick up on phylink-containing
patches.  Something like:

K:	phylink

maybe?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox