Messages in this thread | | | Subject | Re: [patch v5 repost 1/1] i2c: add master driver for mellanox systems | From | Vladimir Zapolskiy <> | Date | Thu, 17 Nov 2016 02:20:11 +0200 |
| |
Hi Vadim,
I've noticed two more minor issues, please consider to fix them.
On 11/16/2016 09:08 AM, vadimp@mellanox.com wrote: > From: Vadim Pasternak <vadimp@mellanox.com> > > Device driver for Mellanox I2C controller logic, implemented in Lattice > CPLD device. > Device supports: > - Master mode > - One physical bus > - Polling mode > > The Kconfig currently controlling compilation of this code is: > drivers/i2c/busses/Kconfig:config I2C_MLXCPLD > > Signed-off-by: Michael Shych <michaelsh@mellanox.com> > Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> > Reviewed-by: Jiri Pirko <jiri@mellanox.com> > Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
Still valid.
[snip]
> diff --git a/drivers/i2c/busses/i2c-mlxcpld.c b/drivers/i2c/busses/i2c-mlxcpld.c > new file mode 100644 > index 0000000..f7a6f3a > --- /dev/null > +++ b/drivers/i2c/busses/i2c-mlxcpld.c > @@ -0,0 +1,551 @@ > +/* > + * drivers/i2c/busses/i2c-mlxcpld.c
You can drop this line with the path above.
> + * Copyright (c) 2016 Mellanox Technologies. All rights reserved. > + * Copyright (c) 2016 Michael Shych <michaels@mellanox.com> > + *
[snip]
> + > +/** > + * struct mlxcpld_i2c_priv - private controller data: > + * @lpc_gen_dec_reg: register space;
This field is gone, right?
> + * @adap: i2c adapter; > + * @dev_id: device id; > + * @base_addr: base IO address; > + * @lock: bus access lock; > + * @xfer: current i2c transfer block; > + * @dev: device handle; > + */ > + > +struct mlxcpld_i2c_priv { > + struct i2c_adapter adap; > + u16 dev_id;
This field is unused, right?
> + u16 base_addr;
For in[bwl]/out[bwl] IO addr has u32 type, it might be better to have it u32 here as well.
> + struct mutex lock; > + struct mlxcpld_i2c_curr_transf xfer; > + struct device *dev; > +}; > +
[snip]
> + > +/* Check validity of current i2c message and all transfer. > + * Calculate also coomon length of all i2c messages in transfer.
Typo, s/coomon/common/
> + */ > +static int mlxcpld_i2c_invalid_len(struct mlxcpld_i2c_priv *priv, > + const struct i2c_msg *msg, u8 *comm_len)
[snip]
> + > +/* Make sure the CPLD is ready to start transmitting. > + * Return 0 if it is, -EBUSY if it is not. > + */
While it has no runtime effect, the function returns -EIO if bus is busy. If you add a comment (by the way you may consider to remove it), please make it aligned with the function.
> +static int mlxcpld_i2c_check_busy(struct mlxcpld_i2c_priv *priv) > +{ > + u8 val; > + > + mlxcpld_i2c_read_comm(priv, MLXCPLD_LPCI2C_STATUS_REG, &val, 1); > + > + if (val & MLXCPLD_LPCI2C_TRANS_END) > + return 0; > + > + return -EIO; > +} > +
[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);
Common practice to avoid hypothetical problems in future after struct renamings etc. is to get sizeof() of the target storage, here it should be changed to:
priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
There is no dereferencing, so it is a safe expression.
> + 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) > + mutex_destroy(&priv->lock); > + > + return err; > +}
-- With best wishes, Vladimir
| |