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" > > > > ... > > > +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 = &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. 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 <= 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. Agreed. Will be named based on I2C mode names, eg standard, fast, high speed, fast plus. > > > + > > + 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... 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[] = { [0] = 0x0030, [1] = 0x00C0, [2] = 0x0300, [3] = 0x0C00, [4] = 0x3000, }; static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv) { // ... unsigned int id, mask, speed_mode; priv->has_32b_bus = true; olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id); // TODO: olb error checking // TODO: check id is valid if (priv->clk_freq <= 400000) speed_mode = NMK_I2C_EYEQ5_SPEED_MODE_FAST; else if (priv->clk_freq <= 1000000) speed_mode = NMK_I2C_EYEQ5_SPEED_MODE_FAST_PLUS; else speed_mode = NMK_I2C_EYEQ5_SPEED_MODE_HIGH_SPEED; mask = 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éo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com