Wolfram,
Apologies for the late reply; I've been quite busy in the summer and
couldn't address your comments earlier.
Please note that I am trying to be active and respectful as much as
possible, and I am sorry if I gave you a bad impression. That wasn't
my intention.
And again, thank you for having taken the time to review this change
and for the feedback.
>
> Hi Khalil,
>
> I am sorry for the long wait. An illness made a mess out of my schedule.
>
> On Fri, Mar 29, 2019 at 03:40:28PM -0400, Khalil Blaiech wrote:
> > Added BlueField I2C driver to offer master and slave support for
> > Mellanox BlueField SoCs. The driver implements an SMBus adapter
> > and interfaces to multiple busses that can be probed using both
> > ACPI and Device Tree infrastructures.
> >
> > Signed-off-by: Khalil Blaiech <kblaiech@xxxxxxxxxxxx>
>
> Are you or one of the authors willing to maintain this driver? Then, a
> MAINTAINERS entry would be great.
Yes, I am the author of the driver. I will add an entry to the MAINTAINERS
once we make progress with the review of the code.
>
> Please run 'checkpatch --strict' on this patch. I think the extra checks
> make all sense here.
Please note that I fixed most of the errors reported by "--strict" flag, but
couldn't see those issues.
>
> My code checkers say this, please check:
>
> CPPCHECK
> drivers/i2c/busses/i2c-mlx.c:474:17: warning: 'io' is of type 'void *'. When
> using void pointers in calculations, the behaviour is undefined.
> [arithOperationsOnVoidPointer]
> writel(val, io + reg);
> ^
> drivers/i2c/busses/i2c-mlx.c:479:18: warning: 'io' is of type 'void *'. When
> using void pointers in calculations, the behaviour is undefined.
> [arithOperationsOnVoidPointer]
> return readl(io + reg);
> ^
> drivers/i2c/busses/i2c-mlx.c:667:19: warning: Shifting signed 32-bit value by
> 31 bits is undefined behaviour [shiftTooManyBitsSigned]
> command |= 0x1 << MASTER_LOCK_BIT_OFF;
> ^
> drivers/i2c/busses/i2c-mlx.c:1975:26: warning: Shifting signed 32-bit value by
> 31 bits is undefined behaviour [shiftTooManyBitsSigned]
> control32 |= 0 << SLAVE_LOCK_BIT_OFF;
> ^
>
> CC drivers/i2c/busses/i2c-mlx.o
> drivers/i2c/busses/i2c-mlx.c: In function ‘mlx_slave_enable’:
> drivers/i2c/busses/i2c-mlx.c:1582:7: warning: variable ‘exist’ set but not used
> [-Wunused-but-set-variable]
>
> > ---
> > drivers/i2c/busses/Kconfig | 13 +
> > drivers/i2c/busses/Makefile | 1 +
> > drivers/i2c/busses/i2c-mlx.c | 2513
> ++++++++++++++++++++++++++++++++++++++++++
>
> This driver is huge and I am willing to assist with the I2C parts.
> However, there are some generic Kernel coding issues which I'd like to
> be sorted out first. I am adding two more people from Mellanox who are
> active in the I2C world, maybe they can help out here. No offence, yet I
> have to delegate such things to focus on I2C parts because my spare time
> is very limited.
>
I think it is a good idea. Thanks.
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Mellanox i2c bus driver
> > + *
> > + * Copyright (C) 2019 Mellanox Technologies, Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License v2.0 as published
> > + * by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
>
> You can skip this, you have a SPDX identifier already. It doesn't match
> with the MODULE_LICENSE at the end, though!
Done.
>
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/string.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/acpi.h>
> > +#include <linux/mutex.h>
> > +
> > +#define MLX_I2C_DRIVER_NAME "i2c-mlx"
>
> This is unused.
Removed.
>
> > +#define MLX_I2C_DRIVER_VERSION "1.0"
>
> We don't use this anymore.
Removed.
>
> > +#define MLX_I2C_DRIVER_DESCRIPTION "Mellanox I2C bus driver"
> > +
> > +#define MLX_I2C_BLUEFIELD1_COMPAT "mellanox,i2c-mlxbf1"
> > +#define MLX_I2C_BLUEFIELD2_COMPAT "mellanox,i2c-mlxbf2"
> > +
> > +#define MLX_I2C_BLUEFIELD1_ACPIHID "MLNXBF03"
> > +#define MLX_I2C_BLUEFIELD2_ACPIHID "MLNXBF23"
>
> I think those defines are not needed, see later.
Removed.
>
> > +
> > +#define I2C_SMBUS_INVALID 0xff
> > +#ifndef I2C_FUNC_SMBUS_HOST_NOTIFY
>
> ?? In what case is this not defined?
This has been removed since it is not really used and not fully tested.
>
> > +#define I2C_FUNC_SMBUS_HOST_NOTIFY 0x10000000
> > +#endif
> > +
> > +#define I2C_SMBUS_MAX 3
> > +
> > +/* Defines what functionality is present */
> > +#define MLX_I2C_FUNC_SMBUS_BLOCK \
> > + (I2C_FUNC_SMBUS_BLOCK_DATA |
> I2C_FUNC_SMBUS_BLOCK_PROC_CALL)
> > +
> > +#define MLX_I2C_FUNC_SMBUS_DEFAULT \
> > + (I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_BYTE_DATA | \
> > + I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_I2C_BLOCK |
> \
> > + I2C_FUNC_SMBUS_PROC_CALL |
> I2C_FUNC_SMBUS_HOST_NOTIFY)
> > +
> > +#define MLX_I2C_FUNC_ALL \
> > + (MLX_I2C_FUNC_SMBUS_DEFAULT |
> MLX_I2C_FUNC_SMBUS_BLOCK | \
> > + I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SLAVE)
> > +
> > +/*
> > + * TYU Shared resources in BlueField 1
> > + */
> > +
> > +#define BLUEFIELD1_TYU_COALESCE_ADDR 0x02801300
> > +#define BLUEFIELD1_TYU_COALESCE_SIZE 0x010
> > +
> > +#define BLUEFIELD1_TYU_GPIO_ADDR 0x02802000
> > +#define BLUEFIELD1_TYU_GPIO_SIZE 0x100
> > +
> > +#define BLUEFIELD1_TYU_COREPLL_ADDR 0x02800358
> > +#define BLUEFIELD1_TYU_COREPLL_SIZE 0x008
> > +
> > +/*
> > + * TYU Shared resources in BlueField 2
> > + */
> > +
> > +#define BLUEFIELD2_TYU_COALESCE_ADDR 0x02801680
> > +#define BLUEFIELD2_TYU_COALESCE_SIZE 0x010
> > +
> > +#define BLUEFIELD2_TYU_GPIO_ADDR 0x02802000 /* FIXME */
> > +#define BLUEFIELD2_TYU_GPIO_SIZE 0x100 /* FIXME */
>
> So, what about this FIXME?
Fixed.
>
> ... (skipping a lot of lines)
That's fine.
>
>
> > +static int mlx_i2c_of_probe(struct device *dev, struct mlx_i2c_priv *priv)
> > +{
> > + int bus_id = -1;
> > + int error;
> > +
> > + if (dev->of_node) {
> > + error = device_property_read_string(dev, "compatible",
> > + &priv->compat);
> > + if (error)
> > + return -EINVAL;
> > +
> > + if (!strcmp(priv->compat, i2c_bf1_compat))
> > + priv->devtype = MLX_I2C_BLUEFIELD1;
> > + else if (!strcmp(priv->compat, i2c_bf2_compat))
> > + priv->devtype = MLX_I2C_BLUEFIELD2;
>
> Why don't you populate the .data-member of the struct of_device_id and
> use then of_device_get_match_data() to get the devtype or some config
> data? I guess something similar could be done for ACPI. Then, you also
> wouldn't need #defines for the compatible string.
Done.
>
> > +
> > + bus_id = of_alias_get_id(dev->of_node, "i2c");
> > + if (bus_id >= 0)
> > + priv->bus = bus_id;
> > + }
> > +
> > + if (WARN(bus_id < 0, "couldn't get bus id"))
> > + return bus_id;
> > +
> > + return 0;
> > +}
> > +MODULE_LICENSE("GPL");
>
> This doesn't match the SPDX identifier!
My bad. Sorry about that.
>
> And from a glimpse, there are more generic things to be found in there.
> It will also be helpful to check recent reviews of new drivers and check
> if the issues found there are also relevant here.
>
> So, my impression is that this driver is not bad, yet it needs a general
> review first. With Mellanox being such a large contributor to the Kernel
> (cool!), I hope we can find some assistance "internally".
Agree.
Please note that the driver code has been reviewed and tested internally;
I also added Vadim, and Michael who might help/assist with the review
process.
>
> Thanks and kind regards,
>
> Wolfram
Thanks,
-Khalil
