lkml.org 
[lkml]   [2016]   [Nov]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [patch v4 1/1] i2c: add master driver for mellanox systems
From
Date
Hi Vadim,

please find below some more nitpickings.

On 11/04/2016 11:15 AM, vadimp@mellanox.com wrote:
> From: Vadim Pasternak <vadimp@mellanox.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@mleia.com>

--
With best wishes,
Vladimir

\
 
 \ /
  Last update: 2016-11-07 01:47    [W:0.034 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site