Hi Vadim,

I've noticed two more minor issues, please consider to fix them.

On 11/16/2016 09:08 AM, vad...@mellanox.com wrote:
From: Vadim Pasternak <vad...@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 <michae...@mellanox.com>
Signed-off-by: Vadim Pasternak <vad...@mellanox.com>
Reviewed-by: Jiri Pirko <j...@mellanox.com>
Reviewed-by: Vladimir Zapolskiy <v...@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 <micha...@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

Reply via email to