linux-kernel - Re: [patch v4 1/1] i2c: add master driver for mellanox systems

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fe5f890f-6be1-130f-3978-2d73b4a24e7a@mleia.com>
Date:   Mon, 7 Nov 2016 02:47:07 +0200
From:   Vladimir Zapolskiy <vz@...ia.com>
To:     vadimp@...lanox.com, wsa@...-dreams.de
Cc:     linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        jiri@...nulli.us, Michael Shych <michaelsh@...lanox.com>
Subject: Re: [patch v4 1/1] i2c: add master driver for mellanox systems

Hi Vadim,

please find below some more nitpickings.

On 11/04/2016 11:15 AM, vadimp@...lanox.com wrote:
> From: Vadim Pasternak <vadimp@...lanox.com>
>

[snip]

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index d252276..b9e1c4c 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1150,6 +1150,18 @@ config I2C_ELEKTOR
>  	  This support is also available as a module.  If so, the module
>  	  will be called i2c-elektor.
>
> +config I2C_MLXCPLD
> +	tristate "Mellanox I2C driver"
> +	depends on X86_64
> +	default n

Please just remove 'default n' line, unset default value in Kconfig
is the same as 'default n'.

[snip]

> +/* General defines */
> +#define MLXPLAT_CPLD_LPC_I2C_BASE_ADDR	0x2000
> +#define MLXCPLD_I2C_DEVICE_NAME		"i2c_mlxcpld"
> +#define MLXCPLD_I2C_VALID_FLAG		(I2C_M_RECV_LEN | I2C_M_RD)
> +#define MLXCPLD_I2C_BUS_NUM		1
> +#define MLXCPLD_I2C_DATA_REG_SZ		36
> +#define MLXCPLD_I2C_MAX_ADDR_LEN	4
> +#define MLXCPLD_I2C_RETR_NUM		2
> +#define MLXCPLD_I2C_XFER_TO		500000 /* msec */
> +#define MLXCPLD_I2C_POLL_TIME		2000   /* msec */

Two lines above define macro values with the microsecond unit, its
short form is denoted as "usec" and "msec" or "ms" is used to
denote milliseconds. Please correct it, transfer timeout of "500
seconds" is confusing.

[snip]

> +
> +static int mlxcpld_i2c_probe(struct platform_device *pdev)
> +{
> +	struct mlxcpld_i2c_priv *priv;
> +	int err;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(struct mlxcpld_i2c_priv),
> +			    GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	mutex_init(&priv->lock);
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->dev = &pdev->dev;
> +
> +	/* Register with i2c layer */
> +	mlxcpld_i2c_adapter.timeout = usecs_to_jiffies(MLXCPLD_I2C_XFER_TO);
> +	priv->adap = mlxcpld_i2c_adapter;
> +	priv->adap.dev.parent = &pdev->dev;
> +	priv->base_addr = MLXPLAT_CPLD_LPC_I2C_BASE_ADDR;
> +	i2c_set_adapdata(&priv->adap, priv);
> +
> +	err = i2c_add_numbered_adapter(&priv->adap);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to add %s adapter (%d)\n",
> +			MLXCPLD_I2C_DEVICE_NAME, err);
> +		goto fail_adapter;
> +	}
> +
> +	return 0;
> +
> +fail_adapter:
> +	mutex_destroy(&priv->lock);
> +	return err;

Here you can save some more lines of code by changing it to

	err = i2c_add_numbered_adapter(&priv->adap);
	if (err)
		mutex_destroy(&priv->lock);

	return err;

Note, dev_err() is not needed here, because printing of the error
message is delegated to the I2C core.

> +}
> +

Please feel free to add a tag to the next version:

Reviewed-by: Vladimir Zapolskiy <vz@...ia.com>

--
With best wishes,
Vladimir