Received-SPF: pass (google.com: domain of linux-kernel+bounces-90759-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Mon, 04 Mar 2024 15:53:52 +0100
Message-Id: <CZL1VED24SZ0.7ETRO4YZ70CF@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>
To: "Andi Shyti" <andi.shyti@kernel.org>
From: =?utf-8?q?Th=C3=A9o_Lebrun?= <theo.lebrun@bootlin.com>
Subject: Re: [PATCH v2 09/11] i2c: nomadik: support Mobileye EyeQ5 I2C
 controller
References: <20240229-mbly-i2c-v2-0-b32ed18c098c@bootlin.com>
 <20240229-mbly-i2c-v2-9-b32ed18c098c@bootlin.com>
 <yqyhu3qsrfyj52sraeo76jnpbgq6wi4o66hfqepxwwwupaggoa@7t5bah3qqcwb>
In-Reply-To: <yqyhu3qsrfyj52sraeo76jnpbgq6wi4o66hfqepxwwwupaggoa@7t5bah3qqcwb>

Hello,

On Mon Mar 4, 2024 at 3:08 PM CET, Andi Shyti wrote:
> 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.

Will do.

>
> >  #define DRIVER_NAME "nmk-i2c"
> > =20
>
> ...
>
> > +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).

Indeed, the useless else block can be removed. Will do.

> > +}
> > +
> > +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 =3D &priv->adev->dev;
> > +	struct device_node *np =3D dev->of_node;
> > +	unsigned int shift, speed_mode;
> > +	struct regmap *olb;
> > +	unsigned int id;
> > +
> > +	priv->has_32b_bus =3D true;
> > +
> > +	olb =3D syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &=
id);
> > +	if (IS_ERR_OR_NULL(olb)) {
> > +		if (!olb)
> > +			olb =3D 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.

Good catch. In previous revisions nmk_i2c_eyeq5_probe() had multiple
error cases so it had to be the one doing the logging. Now that there
is a single possible error parent can do it. It should simplify code.

>
> > +	}
> > +
> > +	if (priv->clk_freq <=3D 400000)
> > +		speed_mode =3D 0b00;
> > +	else if (priv->clk_freq <=3D 1000000)
> > +		speed_mode =3D 0b01;
> > +	else
> > +		speed_mode =3D 0b10;
>
> would be nice to have these as defines.

Agreed. Will be named based on I2C mode names, eg standard, fast, high
speed, fast plus.

>
> > +
> > +	shift =3D 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...

0b11 is a two-bit mask. What do you mean by "these values"? Something
like:


#define NMK_I2C_EYEQ5_SPEED_MODE_FAST		0b00
#define NMK_I2C_EYEQ5_SPEED_MODE_FAST_PLUS	0b01
#define NMK_I2C_EYEQ5_SPEED_MODE_HIGH_SPEED	0b10

static const u8 nmk_i2c_eyeq5_masks[] =3D {
	[0] =3D 0x0030,
	[1] =3D 0x00C0,
	[2] =3D 0x0300,
	[3] =3D 0x0C00,
	[4] =3D 0x3000,
};

static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
{
	// ...
	unsigned int id, mask, speed_mode;

	priv->has_32b_bus =3D true;

	olb =3D syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
	// TODO: olb error checking
	// TODO: check id is valid

	if (priv->clk_freq <=3D 400000)
		speed_mode =3D NMK_I2C_EYEQ5_SPEED_MODE_FAST;
	else if (priv->clk_freq <=3D 1000000)
		speed_mode =3D NMK_I2C_EYEQ5_SPEED_MODE_FAST_PLUS;
	else
		speed_mode =3D NMK_I2C_EYEQ5_SPEED_MODE_HIGH_SPEED;

	mask =3D nmk_i2c_eyeq5_masks[id];
	regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
			   mask, speed_mode << __fls(mask));
	return 0;
}

Else I do not see what you are suggesting by "please define these
values".

Have a nice day,

--
Th=C3=A9o Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com