Received-SPF: pass (google.com: domain of linux-kernel+bounces-90710-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 4 Mar 2024 15:08:37 +0100
From: Andi Shyti <andi.shyti@kernel.org>
To: =?utf-8?B?VGjDqW8=?= Lebrun <theo.lebrun@bootlin.com>
Cc: Linus Walleij <linus.walleij@linaro.org>, 
	Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, Thomas Bogendoerfer <tsbogend@alpha.franken.de>, 
	linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org, 
	Gregory Clement <gregory.clement@bootlin.com>, Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>, 
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>, Tawfik Bayouk <tawfik.bayouk@mobileye.com>, 
	Andi Shyti <andi.shyti@kernel.org>
Subject: Re: [PATCH v2 09/11] i2c: nomadik: support Mobileye EyeQ5 I2C
 controller
Message-ID: <yqyhu3qsrfyj52sraeo76jnpbgq6wi4o66hfqepxwwwupaggoa@7t5bah3qqcwb>
References: <20240229-mbly-i2c-v2-0-b32ed18c098c@bootlin.com>
 <20240229-mbly-i2c-v2-9-b32ed18c098c@bootlin.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240229-mbly-i2c-v2-9-b32ed18c098c@bootlin.com>

Hi Theo,

..

> +#include <linux/amba/bus.h>
>  #include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
>  #include <linux/init.h>
> -#include <linux/module.h>
> -#include <linux/amba/bus.h>
> -#include <linux/slab.h>
>  #include <linux/interrupt.h>
> -#include <linux/i2c.h>
> -#include <linux/err.h>
> -#include <linux/clk.h>
>  #include <linux/io.h>
> -#include <linux/pm_runtime.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>

Please reorder the headers in a different patch.

>  #define DRIVER_NAME "nmk-i2c"
>  

..

> +static inline u8 nmk_i2c_readb(const struct nmk_i2c_dev *priv,
> +			       unsigned long reg)
> +{
> +	if (priv->has_32b_bus)
> +		return readl(priv->virtbase + reg);
> +	else
> +		return readb(priv->virtbase + reg);

nit: no need for the else (your choice though, if you want to
have ti coherent with the write counterpart).

> +}
> +
> +static inline void nmk_i2c_writeb(const struct nmk_i2c_dev *priv, u32 val,
> +				unsigned long reg)
> +{
> +	if (priv->has_32b_bus)
> +		writel(val, priv->virtbase + reg);
> +	else
> +		writeb(val, priv->virtbase + reg);
> +}

..

> +static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
> +{
> +	struct device *dev = &priv->adev->dev;
> +	struct device_node *np = dev->of_node;
> +	unsigned int shift, speed_mode;
> +	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_OR_NULL(olb)) {
> +		if (!olb)
> +			olb = ERR_PTR(-ENOENT);
> +		return dev_err_probe(dev, PTR_ERR(olb),
> +				     "failed OLB lookup: %lu\n", PTR_ERR(olb));

just return PTR_ERR(olb) and use dev_err_probe() in the upper
layer probe.

> +	}
> +
> +	if (priv->clk_freq <= 400000)
> +		speed_mode = 0b00;
> +	else if (priv->clk_freq <= 1000000)
> +		speed_mode = 0b01;
> +	else
> +		speed_mode = 0b10;

would be nice to have these as defines.

> +
> +	shift = NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id);
> +	regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
> +			   0b11 << shift, speed_mode << shift);

please define these values and for hexadecimals use 0x...

> +	return 0;
> +}
> +
>  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>  {
>  	int ret = 0;
> @@ -1001,8 +1065,17 @@ 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;
>  	nmk_i2c_of_probe(np, priv);
>  
> +	if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
> +		ret = nmk_i2c_eyeq5_probe(priv);
> +		if (ret) {
> +			dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);
> +			return ret;
> +		}

as said above, use dev_err_probe here.

Andi

> +	}
> +
>  	if (priv->tft > max_fifo_threshold) {
>  		dev_warn(dev, "requested TX FIFO threshold %u, adjusted down to %u\n",
>  			 priv->tft, max_fifo_threshold);
> 
> -- 
> 2.44.0
>