Hi Vadim,
I've noticed two more minor issues, please consider to fix them.
On 11/16/2016 09:08 AM, vadimp@xxxxxxxxxxxx wrote:
From: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
Reviewed-by: Jiri Pirko <jiri@xxxxxxxxxxxx>
Reviewed-by: Vladimir Zapolskiy <vz@xxxxxxxxx>
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@xxxxxxxxxxxx>
+ *
[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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
