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
