[PATCH 0/4] i2c: nomadik: support >=1MHz & Mobileye EyeQ6H platform

[PATCH 0/4] i2c: nomadik: support >=1MHz & Mobileye EyeQ6H platform

[Théo Lebrun]

Hi,

First two patches are about adding Mobileye EyeQ6H support to the
Nomadik I2C controller driver, in the same vein as was done a few
months ago for EyeQ5.
 - dt-bindings wise, it is only a new compatible. EyeQ6H does NOT
   require the same mobileye,olb custom prop as EyeQ5.
 - driver wise, we are again on a 32bit memory bus, so reuse
   the .has_32b_bus flag.

Next two patches are about supporting higher speeds (fast-plus and
high-speed).
 - Fix computation of the bus rate clock divider (BRCR). It picks the
   smallest divider that gives a bus rate above target. Switch to
   picking the largest divider that gives a bus rate below target.
 - Then support high SM (speed-mode) values. This is not much work.

It works on EyeQ6H HW just fine. 1MHz has been tested but not 3.4MHz
because HW doesn't support it. The theory is there, and BRCR
computation has been checked to be valid with 3.4MHz clocks.

DTS patches are not provided because they depend on the platform's clock
series [0]. Lore being down at the moment, see Patchwork [1].

Have a nice day,
Théo

[0]: https://lore.kernel.org/lkml/20241007-mbly-clk-v5-0-e9d8994269cb@bootlin.com/
[1]: https://patchwork.kernel.org/project/linux-clk/cover/20241007-mbly-clk-v5-0-e9d8994269cb@bootlin.com/

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
Théo Lebrun (4):
      dt-bindings: i2c: nomadik: add mobileye,eyeq6h-i2c bindings
      i2c: nomadik: support Mobileye EyeQ6H I2C controller
      i2c: nomadik: fix BRCR computation
      i2c: nomadik: support >=1MHz speed modes

 .../devicetree/bindings/i2c/st,nomadik-i2c.yaml    |  6 +-
 drivers/i2c/busses/i2c-nomadik.c                   | 65 ++++++++++------------
 2 files changed, 35 insertions(+), 36 deletions(-)
---
base-commit: 6f1cfa7816af8b3286140f1b0476200d5e914eb9
change-id: 20241007-mbly-i2c-267c9d482b90

Best regards,
-- 
Théo Lebrun <theo.lebrun@bootlin.com>

[PATCH 1/4] dt-bindings: i2c: nomadik: add mobileye,eyeq6h-i2c bindings

[Théo Lebrun]

After EyeQ5, it is time for Mobileye EyeQ6H to reuse the Nomadik I2C
controller. Add a specific compatible because its HW integration is
slightly different from EyeQ5.

Do NOT add an example as it looks like EyeQ5 from a DT standpoint
(without the mobileye,olb property).

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
index 44c54b162bb10741ec7aac70d165403c28176eba..72ecb6efa733f7878bd807df277bfc13153bf71e 100644
--- a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
@@ -22,6 +22,7 @@ select:
         enum:
           - st,nomadik-i2c
           - mobileye,eyeq5-i2c
+          - mobileye,eyeq6h-i2c
   required:
     - compatible
 
@@ -38,6 +39,9 @@ properties:
       - items:
           - const: mobileye,eyeq5-i2c
           - const: arm,primecell
+      - items:
+          - const: mobileye,eyeq6h-i2c
+          - const: arm,primecell
 
   reg:
     maxItems: 1
@@ -54,7 +58,7 @@ properties:
       - items:
           - const: mclk
           - const: apb_pclk
-      # Clock name in DB8500 or EyeQ5
+      # Clock name in DB8500 or EyeQ
       - items:
           - const: i2cclk
           - const: apb_pclk

-- 
2.46.2

[PATCH 2/4] i2c: nomadik: support Mobileye EyeQ6H I2C controller

[Théo Lebrun]

Add EyeQ6H support to the nmk-i2c AMBA driver. It shares the same quirk
as EyeQ5: the memory bus only supports 32-bit accesses. Avoid writeb()
and readb() by reusing the same `priv->has_32b_bus` flag.

It does NOT need to write speed-mode specific value into a register;
therefore it does not depend on the mobileye,olb DT property.

Refactoring is done using is_eyeq5 and is_eyeq6h boolean local
variables. Sort variables in reverse christmas tree to try and
introduce some logic into the ordering.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index ad0f02acdb1215a1c04729f97bb14a4d93f88456..ea511d3a58073eaedb63850026e05b59427a69c6 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -6,10 +6,10 @@
  * I2C master mode controller driver, used in Nomadik 8815
  * and Ux500 platforms.
  *
- * The Mobileye EyeQ5 platform is also supported; it uses
+ * The Mobileye EyeQ5 and EyeQ6H platforms are also supported; they use
  * the same Ux500/DB8500 IP block with two quirks:
  *  - The memory bus only supports 32-bit accesses.
- *  - A register must be configured for the I2C speed mode;
+ *  - (only EyeQ5) A register must be configured for the I2C speed mode;
  *    it is located in a shared register region called OLB.
  *
  * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
@@ -1046,8 +1046,6 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
 	struct regmap *olb;
 	unsigned int id;
 
-	priv->has_32b_bus = true;
-
 	olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
 	if (IS_ERR(olb))
 		return PTR_ERR(olb);
@@ -1070,13 +1068,15 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
 
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 {
-	int ret = 0;
-	struct nmk_i2c_dev *priv;
-	struct device_node *np = adev->dev.of_node;
-	struct device *dev = &adev->dev;
-	struct i2c_adapter *adap;
 	struct i2c_vendor_data *vendor = id->data;
+	struct device_node *np = adev->dev.of_node;
+	bool is_eyeq6h = of_device_is_compatible(np, "mobileye,eyeq6h-i2c");
+	bool is_eyeq5 = of_device_is_compatible(np, "mobileye,eyeq5-i2c");
 	u32 max_fifo_threshold = (vendor->fifodepth / 2) - 1;
+	struct device *dev = &adev->dev;
+	struct nmk_i2c_dev *priv;
+	struct i2c_adapter *adap;
+	int ret = 0;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -1084,10 +1084,10 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 
 	priv->vendor = vendor;
 	priv->adev = adev;
-	priv->has_32b_bus = false;
+	priv->has_32b_bus = is_eyeq5 || is_eyeq6h;
 	nmk_i2c_of_probe(np, priv);
 
-	if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
+	if (is_eyeq5) {
 		ret = nmk_i2c_eyeq5_probe(priv);
 		if (ret)
 			return dev_err_probe(dev, ret, "failed OLB lookup\n");

-- 
2.46.2

[PATCH 3/4] i2c: nomadik: fix BRCR computation

[Théo Lebrun]

Current BRCR computation is:

    brcr = floor(i2cclk / (clkfreq * div))

With brcr: "baud rate counter", an internal clock divider,
 and i2cclk: input clock rate (24MHz, 38.4MHz or 48MHz),
 and clkfreq: desired bus rate,
 and div: speed-mode dependent divider (2 for standard, 3 otherwise).

Assume i2cclk=48MHz, clkfreq=3.4MHz, div=3,
  then brcr = floor(48MHz / (3.4MHz * 3)) = 4
   and resulting bus rate = 48MHz / (4 * 3) = 4MHz

Assume i2cclk=38.4MHz, clkfreq=1.0MHz, div=3,
  then brcr = floor(38.4MHz / (1.0MHz * 3)) = 12
   and resulting bus rate = 38.4MHz / (12 * 3) = 1066kHz

The current computation means we always pick the smallest divider that
gives a bus rate above target. We should instead pick the largest
divider that gives a bus rate below target, using:

    brcr = floor(i2cclk / (clkfreq * div)) + 1

If we redo the above examples:

Assume i2cclk=48MHz, clkfreq=3.4MHz, div=3,
  then brcr = floor(48MHz / (3.4MHz * 3)) + 1 = 5
   and resulting bus rate = 48MHz / (5 * 3) = 3.2MHz

Assume i2cclk=38.4MHz, clkfreq=1.0MHz, div=3,
  then brcr = floor(38.4MHz / (1.0MHz * 3)) + 1 = 13
   and resulting bus rate = 38.4MHz / (13 * 3) = 985kHz

This is much less of an issue with slower bus rates (ie those currently
supported), because the gap from one divider to the next is much
smaller. It however keeps us from always using bus rates superior to
the target.

This fix is required for later on supporting faster bus rates:
I2C_FREQ_MODE_FAST_PLUS (1MHz) and I2C_FREQ_MODE_HIGH_SPEED (3.4MHz).

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index ea511d3a58073eaedb63850026e05b59427a69c6..68ce39352d67477fa22424e2dc0f8d1741498cd1 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -453,9 +453,12 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 	 * operation, and the other is for std, fast mode, fast mode
 	 * plus operation. Currently we do not supprt high speed mode
 	 * so set brcr1 to 0.
+	 *
+	 * BRCR is a clock divider amount. Pick highest value that
+	 * leads to rate strictly below target.
 	 */
 	brcr1 = FIELD_PREP(I2C_BRCR_BRCNT1, 0);
-	brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2, i2c_clk / (priv->clk_freq * div));
+	brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2, i2c_clk / (priv->clk_freq * div) + 1);
 
 	/* set the baud rate counter register */
 	writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);

-- 
2.46.2

[PATCH 4/4] i2c: nomadik: support >=1MHz speed modes

[Théo Lebrun]

 - BRCR value must go into the BRCR1 field when in high-speed mode.
   It goes into BRCR2 otherwise.

 - Remove fallback to standard mode if priv->sm > I2C_FREQ_MODE_FAST.

 - Set SM properly in probe; previously it only checked STANDARD versus
   FAST. Now we set STANDARD, FAST, FAST_PLUS or HIGH_SPEED.

 - Remove all comment sections saying we only support low-speeds.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 68ce39352d67477fa22424e2dc0f8d1741498cd1..82571983bbca5ebcd8a689d4d717ea96eb3d2ad2 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -396,7 +396,7 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags)
  */
 static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 {
-	u32 brcr1, brcr2;
+	u32 brcr;
 	u32 i2c_clk, div;
 	u32 ns;
 	u16 slsu;
@@ -443,7 +443,7 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 	/*
 	 * The spec says, in case of std. mode the divider is
 	 * 2 whereas it is 3 for fast and fastplus mode of
-	 * operation. TODO - high speed support.
+	 * operation.
 	 */
 	div = (priv->clk_freq > I2C_MAX_STANDARD_MODE_FREQ) ? 3 : 2;
 
@@ -451,33 +451,22 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 	 * generate the mask for baud rate counters. The controller
 	 * has two baud rate counters. One is used for High speed
 	 * operation, and the other is for std, fast mode, fast mode
-	 * plus operation. Currently we do not supprt high speed mode
-	 * so set brcr1 to 0.
+	 * plus operation.
 	 *
 	 * BRCR is a clock divider amount. Pick highest value that
 	 * leads to rate strictly below target.
 	 */
-	brcr1 = FIELD_PREP(I2C_BRCR_BRCNT1, 0);
-	brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2, i2c_clk / (priv->clk_freq * div) + 1);
+	brcr = i2c_clk / (priv->clk_freq * div) + 1;
+
+	if (priv->sm == I2C_FREQ_MODE_HIGH_SPEED)
+		brcr = FIELD_PREP(I2C_BRCR_BRCNT1, brcr);
+	else
+		brcr = FIELD_PREP(I2C_BRCR_BRCNT2, brcr);
 
 	/* set the baud rate counter register */
-	writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
+	writel(brcr, priv->virtbase + I2C_BRCR);
 
-	/*
-	 * set the speed mode. Currently we support
-	 * only standard and fast mode of operation
-	 * TODO - support for fast mode plus (up to 1Mb/s)
-	 * and high speed (up to 3.4 Mb/s)
-	 */
-	if (priv->sm > I2C_FREQ_MODE_FAST) {
-		dev_err(&priv->adev->dev,
-			"do not support this mode defaulting to std. mode\n");
-		brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2,
-				   i2c_clk / (I2C_MAX_STANDARD_MODE_FREQ * 2));
-		writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
-		writel(FIELD_PREP(I2C_CR_SM, I2C_FREQ_MODE_STANDARD),
-		       priv->virtbase + I2C_CR);
-	}
+	/* set the speed mode */
 	writel(FIELD_PREP(I2C_CR_SM, priv->sm), priv->virtbase + I2C_CR);
 
 	/* set the Tx and Rx FIFO threshold */
@@ -1018,11 +1007,14 @@ static void nmk_i2c_of_probe(struct device_node *np,
 	if (of_property_read_u32(np, "clock-frequency", &priv->clk_freq))
 		priv->clk_freq = I2C_MAX_STANDARD_MODE_FREQ;
 
-	/* This driver only supports 'standard' and 'fast' modes of operation. */
 	if (priv->clk_freq <= I2C_MAX_STANDARD_MODE_FREQ)
 		priv->sm = I2C_FREQ_MODE_STANDARD;
-	else
+	else if (priv->clk_freq <= I2C_MAX_FAST_MODE_FREQ)
 		priv->sm = I2C_FREQ_MODE_FAST;
+	else if (priv->clk_freq <= I2C_MAX_FAST_MODE_PLUS_FREQ)
+		priv->sm = I2C_FREQ_MODE_FAST_PLUS;
+	else
+		priv->sm = I2C_FREQ_MODE_HIGH_SPEED;
 	priv->tft = 1; /* Tx FIFO threshold */
 	priv->rft = 8; /* Rx FIFO threshold */
 

-- 
2.46.2

Re: [PATCH 1/4] dt-bindings: i2c: nomadik: add mobileye,eyeq6h-i2c bindings

[Krzysztof Kozlowski]

On Tue, Oct 08, 2024 at 12:29:40PM +0200, Théo Lebrun wrote:
> After EyeQ5, it is time for Mobileye EyeQ6H to reuse the Nomadik I2C
> controller. Add a specific compatible because its HW integration is
> slightly different from EyeQ5.
> 
> Do NOT add an example as it looks like EyeQ5 from a DT standpoint
> (without the mobileye,olb property).
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
> index 44c54b162bb10741ec7aac70d165403c28176eba..72ecb6efa733f7878bd807df277bfc13153bf71e 100644
> --- a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml
> @@ -22,6 +22,7 @@ select:
>          enum:
>            - st,nomadik-i2c
>            - mobileye,eyeq5-i2c
> +          - mobileye,eyeq6h-i2c
>    required:
>      - compatible
>  
> @@ -38,6 +39,9 @@ properties:
>        - items:
>            - const: mobileye,eyeq5-i2c

This should be enum in such case.

>            - const: arm,primecell
> +      - items:
> +          - const: mobileye,eyeq6h-i2c
> +          - const: arm,primecell

Best regards,
Krzysztof

Re: [PATCH 2/4] i2c: nomadik: support Mobileye EyeQ6H I2C controller

[Krzysztof Kozlowski]

On Tue, Oct 08, 2024 at 12:29:41PM +0200, Théo Lebrun wrote:
> Add EyeQ6H support to the nmk-i2c AMBA driver. It shares the same quirk
> as EyeQ5: the memory bus only supports 32-bit accesses. Avoid writeb()
> and readb() by reusing the same `priv->has_32b_bus` flag.
> 
> It does NOT need to write speed-mode specific value into a register;
> therefore it does not depend on the mobileye,olb DT property.
> 
> Refactoring is done using is_eyeq5 and is_eyeq6h boolean local
> variables. Sort variables in reverse christmas tree to try and
> introduce some logic into the ordering.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/i2c/busses/i2c-nomadik.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index ad0f02acdb1215a1c04729f97bb14a4d93f88456..ea511d3a58073eaedb63850026e05b59427a69c6 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -6,10 +6,10 @@
>   * I2C master mode controller driver, used in Nomadik 8815
>   * and Ux500 platforms.
>   *
> - * The Mobileye EyeQ5 platform is also supported; it uses
> + * The Mobileye EyeQ5 and EyeQ6H platforms are also supported; they use
>   * the same Ux500/DB8500 IP block with two quirks:
>   *  - The memory bus only supports 32-bit accesses.
> - *  - A register must be configured for the I2C speed mode;
> + *  - (only EyeQ5) A register must be configured for the I2C speed mode;
>   *    it is located in a shared register region called OLB.
>   *
>   * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
> @@ -1046,8 +1046,6 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
>  	struct regmap *olb;
>  	unsigned int id;
>  
> -	priv->has_32b_bus = true;
> -
>  	olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
>  	if (IS_ERR(olb))
>  		return PTR_ERR(olb);
> @@ -1070,13 +1068,15 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
>  
>  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>  {
> -	int ret = 0;
> -	struct nmk_i2c_dev *priv;
> -	struct device_node *np = adev->dev.of_node;
> -	struct device *dev = &adev->dev;
> -	struct i2c_adapter *adap;
>  	struct i2c_vendor_data *vendor = id->data;
> +	struct device_node *np = adev->dev.of_node;
> +	bool is_eyeq6h = of_device_is_compatible(np, "mobileye,eyeq6h-i2c");
> +	bool is_eyeq5 = of_device_is_compatible(np, "mobileye,eyeq5-i2c");

You should use match data, not add compatibles in the middle of code.
That's preferred, scallable pattern. What you added here last time does
not scale and above change is a proof for that.

Best regards,
Krzysztof

Re: [PATCH 2/4] i2c: nomadik: support Mobileye EyeQ6H I2C controller

[Théo Lebrun]

Hello Krzysztof,

On Tue Oct 8, 2024 at 3:39 PM CEST, Krzysztof Kozlowski wrote:
> On Tue, Oct 08, 2024 at 12:29:41PM +0200, Théo Lebrun wrote:
> > Add EyeQ6H support to the nmk-i2c AMBA driver. It shares the same quirk
> > as EyeQ5: the memory bus only supports 32-bit accesses. Avoid writeb()
> > and readb() by reusing the same `priv->has_32b_bus` flag.
> > 
> > It does NOT need to write speed-mode specific value into a register;
> > therefore it does not depend on the mobileye,olb DT property.
> > 
> > Refactoring is done using is_eyeq5 and is_eyeq6h boolean local
> > variables. Sort variables in reverse christmas tree to try and
> > introduce some logic into the ordering.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  drivers/i2c/busses/i2c-nomadik.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> > index ad0f02acdb1215a1c04729f97bb14a4d93f88456..ea511d3a58073eaedb63850026e05b59427a69c6 100644
> > --- a/drivers/i2c/busses/i2c-nomadik.c
> > +++ b/drivers/i2c/busses/i2c-nomadik.c
> > @@ -6,10 +6,10 @@
> >   * I2C master mode controller driver, used in Nomadik 8815
> >   * and Ux500 platforms.
> >   *
> > - * The Mobileye EyeQ5 platform is also supported; it uses
> > + * The Mobileye EyeQ5 and EyeQ6H platforms are also supported; they use
> >   * the same Ux500/DB8500 IP block with two quirks:
> >   *  - The memory bus only supports 32-bit accesses.
> > - *  - A register must be configured for the I2C speed mode;
> > + *  - (only EyeQ5) A register must be configured for the I2C speed mode;
> >   *    it is located in a shared register region called OLB.
> >   *
> >   * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
> > @@ -1046,8 +1046,6 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
> >  	struct regmap *olb;
> >  	unsigned int id;
> >  
> > -	priv->has_32b_bus = true;
> > -
> >  	olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
> >  	if (IS_ERR(olb))
> >  		return PTR_ERR(olb);
> > @@ -1070,13 +1068,15 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
> >  
> >  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
> >  {
> > -	int ret = 0;
> > -	struct nmk_i2c_dev *priv;
> > -	struct device_node *np = adev->dev.of_node;
> > -	struct device *dev = &adev->dev;
> > -	struct i2c_adapter *adap;
> >  	struct i2c_vendor_data *vendor = id->data;
> > +	struct device_node *np = adev->dev.of_node;
> > +	bool is_eyeq6h = of_device_is_compatible(np, "mobileye,eyeq6h-i2c");
> > +	bool is_eyeq5 = of_device_is_compatible(np, "mobileye,eyeq5-i2c");
>
> You should use match data, not add compatibles in the middle of code.
> That's preferred, scallable pattern. What you added here last time does
> not scale and above change is a proof for that.

I would have used match data if the driver struct had a .of_match_table
field. `struct amba_driver` does not. Are you recommending the approach
below?

I don't see how it brings much to the driver but I do get the scaling
issue as the number of support compatibles increases. This is a fear
based on what *could* happen in the future though.

------------------------------------------------------------------------

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 82571983bbca..98fc40dfcbfc 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -26,6 +26,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
@@ -1061,28 +1062,46 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
 	return 0;
 }

+#define NMK_I2C_EYEQ_FLAG_32B_BUS	BIT(0)
+#define NMK_I2C_EYEQ_FLAG_IS_EYEQ5	BIT(1)
+
+static const struct of_device_id nmk_i2c_eyeq_match_table[] = {
+	{
+		.compatible = "mobileye,eyeq5-i2c",
+		.data = (void*)(NMK_I2C_EYEQ_FLAG_32B_BUS | NMK_I2C_EYEQ_FLAG_IS_EYEQ5),
+	},
+	{
+		.compatible = "mobileye,eyeq6h-i2c",
+		.data = (void*)(NMK_I2C_EYEQ_FLAG_32B_BUS),
+	},
+};
+
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	struct i2c_vendor_data *vendor = id->data;
 	struct device_node *np = adev->dev.of_node;
-	bool is_eyeq6h = of_device_is_compatible(np, "mobileye,eyeq6h-i2c");
-	bool is_eyeq5 = of_device_is_compatible(np, "mobileye,eyeq5-i2c");
 	u32 max_fifo_threshold = (vendor->fifodepth / 2) - 1;
+	const struct of_device_id *match;
 	struct device *dev = &adev->dev;
+	unsigned long match_flags = 0;
 	struct nmk_i2c_dev *priv;
 	struct i2c_adapter *adap;
 	int ret = 0;

+	match = of_match_device(nmk_i2c_eyeq_match_table, dev);
+	if (match)
+		match_flags = (unsigned long)match->data;
+
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;

 	priv->vendor = vendor;
 	priv->adev = adev;
-	priv->has_32b_bus = is_eyeq5 || is_eyeq6h;
+	priv->has_32b_bus = match_flags & NMK_I2C_EYEQ_FLAG_32B_BUS;
 	nmk_i2c_of_probe(np, priv);

-	if (is_eyeq5) {
+	if (match_flags & NMK_I2C_EYEQ_FLAG_IS_EYEQ5) {
 		ret = nmk_i2c_eyeq5_probe(priv);
 		if (ret)
 			return dev_err_probe(dev, ret, "failed OLB lookup\n");

------------------------------------------------------------------------

Thanks Krzysztof,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/4] i2c: nomadik: support Mobileye EyeQ6H I2C controller

[Krzysztof Kozlowski]

On 08/10/2024 16:43, Théo Lebrun wrote:
> Hello Krzysztof,
> 
> On Tue Oct 8, 2024 at 3:39 PM CEST, Krzysztof Kozlowski wrote:
>> On Tue, Oct 08, 2024 at 12:29:41PM +0200, Théo Lebrun wrote:
>>> Add EyeQ6H support to the nmk-i2c AMBA driver. It shares the same quirk
>>> as EyeQ5: the memory bus only supports 32-bit accesses. Avoid writeb()
>>> and readb() by reusing the same `priv->has_32b_bus` flag.
>>>
>>> It does NOT need to write speed-mode specific value into a register;
>>> therefore it does not depend on the mobileye,olb DT property.
>>>
>>> Refactoring is done using is_eyeq5 and is_eyeq6h boolean local
>>> variables. Sort variables in reverse christmas tree to try and
>>> introduce some logic into the ordering.
>>>
>>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>>> ---
>>>  drivers/i2c/busses/i2c-nomadik.c | 22 +++++++++++-----------
>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
>>> index ad0f02acdb1215a1c04729f97bb14a4d93f88456..ea511d3a58073eaedb63850026e05b59427a69c6 100644
>>> --- a/drivers/i2c/busses/i2c-nomadik.c
>>> +++ b/drivers/i2c/busses/i2c-nomadik.c
>>> @@ -6,10 +6,10 @@
>>>   * I2C master mode controller driver, used in Nomadik 8815
>>>   * and Ux500 platforms.
>>>   *
>>> - * The Mobileye EyeQ5 platform is also supported; it uses
>>> + * The Mobileye EyeQ5 and EyeQ6H platforms are also supported; they use
>>>   * the same Ux500/DB8500 IP block with two quirks:
>>>   *  - The memory bus only supports 32-bit accesses.
>>> - *  - A register must be configured for the I2C speed mode;
>>> + *  - (only EyeQ5) A register must be configured for the I2C speed mode;
>>>   *    it is located in a shared register region called OLB.
>>>   *
>>>   * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
>>> @@ -1046,8 +1046,6 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
>>>  	struct regmap *olb;
>>>  	unsigned int id;
>>>  
>>> -	priv->has_32b_bus = true;
>>> -
>>>  	olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
>>>  	if (IS_ERR(olb))
>>>  		return PTR_ERR(olb);
>>> @@ -1070,13 +1068,15 @@ static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
>>>  
>>>  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>>>  {
>>> -	int ret = 0;
>>> -	struct nmk_i2c_dev *priv;
>>> -	struct device_node *np = adev->dev.of_node;
>>> -	struct device *dev = &adev->dev;
>>> -	struct i2c_adapter *adap;
>>>  	struct i2c_vendor_data *vendor = id->data;
>>> +	struct device_node *np = adev->dev.of_node;
>>> +	bool is_eyeq6h = of_device_is_compatible(np, "mobileye,eyeq6h-i2c");
>>> +	bool is_eyeq5 = of_device_is_compatible(np, "mobileye,eyeq5-i2c");
>>
>> You should use match data, not add compatibles in the middle of code.
>> That's preferred, scallable pattern. What you added here last time does
>> not scale and above change is a proof for that.
> 
> I would have used match data if the driver struct had a .of_match_table
> field. `struct amba_driver` does not. Are you recommending the approach
> below?
> 
> I don't see how it brings much to the driver but I do get the scaling
> issue as the number of support compatibles increases. This is a fear
> based on what *could* happen in the future though.

You still have adev->dev.of_node, which you can use for matching in
probe. See for example of_match_device() (and add a note so people will
not convert it to device_get_match_data() blindly).

Best regards,
Krzysztof

Re: [PATCH 2/4] i2c: nomadik: support Mobileye EyeQ6H I2C controller

[Théo Lebrun]

On Tue Oct 8, 2024 at 6:03 PM CEST, Krzysztof Kozlowski wrote:
> On 08/10/2024 16:43, Théo Lebrun wrote:
> > On Tue Oct 8, 2024 at 3:39 PM CEST, Krzysztof Kozlowski wrote:
> >> On Tue, Oct 08, 2024 at 12:29:41PM +0200, Théo Lebrun wrote:
> >>> +	bool is_eyeq6h = of_device_is_compatible(np, "mobileye,eyeq6h-i2c");
> >>> +	bool is_eyeq5 = of_device_is_compatible(np, "mobileye,eyeq5-i2c");
> >>
> >> You should use match data, not add compatibles in the middle of code.
> >> That's preferred, scallable pattern. What you added here last time does
> >> not scale and above change is a proof for that.
> > 
> > I would have used match data if the driver struct had a .of_match_table
> > field. `struct amba_driver` does not. Are you recommending the approach
> > below?
> > 
> > I don't see how it brings much to the driver but I do get the scaling
> > issue as the number of support compatibles increases. This is a fear
> > based on what *could* happen in the future though.
>
> You still have adev->dev.of_node, which you can use for matching in
> probe. See for example of_match_device() (and add a note so people will
> not convert it to device_get_match_data() blindly).

Just sent the new revision [0]. It uses of_match_device() in the same
way as was shown in my previous answer to this thread [1]. Followed
your recommendation and added a comment to avoid conversions to
device_get_match_data().

Thanks!

[0]: https://lore.kernel.org/lkml/20241009-mbly-i2c-v2-0-ac9230a8dac5@bootlin.com/
[1]: https://lore.kernel.org/lkml/D4QI63B6YQU5.3UPKA7G75J445@bootlin.com/

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com