Date:   Thu, 17 Nov 2016 02:20:11 +0200
From:   Vladimir Zapolskiy <vz@...ia.com>
To:     vadimp@...lanox.com, wsa@...-dreams.de
Cc:     linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        jiri@...nulli.us, Michael Shych <michaelsh@...lanox.com>
Subject: Re: [patch v5 repost 1/1] i2c: add master driver for mellanox systems

Hi Vadim,

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

On 11/16/2016 09:08 AM, vadimp@...lanox.com wrote:
> From: Vadim Pasternak <vadimp@...lanox.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@...lanox.com>
> Signed-off-by: Vadim Pasternak <vadimp@...lanox.com>
> Reviewed-by: Jiri Pirko <jiri@...lanox.com>
> Reviewed-by: Vladimir Zapolskiy <vz@...ia.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@...lanox.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

Powered by blists - more mailing lists