RE: [patch v4 1/1] i2c: add master driver for mellanox systems

[Vadim Pasternak]

Hi Vladimir,

Thank you very much for reviews.

Thanks,
Vadim.

> -----Original Message-----
> From: Vladimir Zapolskiy [mailto:vz@xxxxxxxxx]
> Sent: Monday, November 07, 2016 2:47 AM
> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>; wsa@xxxxxxxxxxxxx
> Cc: linux-i2c@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; jiri@xxxxxxxxxxx;
> Michael Shych <michaelsh@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxx wrote:
> > From: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> >
> 
> [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@xxxxxxxxx>
> 
> --
> With best wishes,
> Vladimir