RE: [PATCH v2 2/2] i2c: Add Renesas RZ/V2M controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy,

Thanks for your review!

On 28 June 2022 22:08 Andy Shevchenko wrote:
> On Tue, Jun 28, 2022 at 08:45:26PM +0100, Phil Edworthy wrote:
> > Yet another i2c controller from Renesas that is found on the RZ/V2M
> > (r9a09g011) SoC. It can support only 100kHz and 400KHz operation.
> 
> ...
> 
> > +#include <linux/of_device.h>
> 
> No user of this header.
> 
> But missed mod_devicetable.h (Okay, for I2C drivers we usually rely on
> i2c.h to
> include it for us, but it's cleaner to include directly).
Ok

> > +#define rzv2m_i2c_priv_to_dev(p)	((p)->adap.dev.parent)
> 
> It's longer than the actual expression. Why do you need this?!
Ok, no need for it

> > +static const struct bitrate_config bitrate_configs[] = {
> > +	[RZV2M_I2C_100K] = { 47, 3450 },
> > +	[RZV2M_I2C_400K] = { 52, 900},
> 
> Missed space.
Ok

> > +	if (priv->iicb0wl > 0x3ff)
> 
> GENMASK() ?
> Or (BIT(x) - 1) in case to tell that there is an HW limitation of x bits?
Ok, BIT makes sense here

> > +	if (priv->iicb0wh > 0x3ff)
> 
> Ditto.
Ok

> > +	if (!last) {
> 
> Why not positive conditional?
Ok

> > +static int rzv2m_i2c_send(struct rzv2m_i2c_priv *priv, struct i2c_msg
> *msg,
> > +			  unsigned int *count)
> > +{
> > +	unsigned int i;
> > +	int ret = 0;
> 
> Redundant assignment, you may return 0 directly.
> Ditto for other similar cases in other functions.
Ok

> > +		ret = rzv2m_i2c_read_with_ACK(priv, &msg->buf[i],
> > +					      ((msg->len - 1) == i));
> 
> Too many parentheses.
Ok

> > +static int rzv2m_i2c_send_address(struct rzv2m_i2c_priv *priv,
> > +				  struct i2c_msg *msg)
> > +{
> > +	u32 addr;
> > +	int ret;
> > +
> > +	if (msg->flags & I2C_M_TEN) {
> > +		/* 10-bit address
> > +		 *   addr_1: 5'b11110 | addr[9:8] | (R/nW)
> > +		 *   addr_2: addr[7:0]
> > +		 */
> > +		addr = 0xf0 | ((msg->addr >> 7) & 0x06);
> > +		addr |= !!(msg->flags & I2C_M_RD);
> > +		/* Send 1st address(extend code) */
> > +		ret = rzv2m_i2c_write_with_ack(priv, addr);
> 
> 	if (ret)
> 		return ret;
Ok


> > +	ret = rzv2m_i2c_send_address(priv, msg);
> > +	if (ret == 0) {
> 
> This is a bit confusing if it's only comparison with "no error code"
> condition.
> Use if (!ret) if there is no meaning of positive value. Same applies to
> other
> cases in the code.
> 
> > +		if (read)
> > +			ret = rzv2m_i2c_receive(priv, msg, &count);
> > +		else
> > +			ret = rzv2m_i2c_send(priv, msg, &count);
> > +
> > +		if ((ret == 0) && stop)
> 
> Like here.
Sorry, I don't follow you. How is (ret == 0) any different to (!ret) ?



> > +static const struct of_device_id rzv2m_i2c_ids[] = {
> > +	{ .compatible = "renesas,rzv2m-i2c", },
> 
> Inner comma is not needed.
Ok

> > +	priv->clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(priv->clk)) {
> > +		dev_err_probe(dev, PTR_ERR(priv->clk), "Can't get clock\n");
> > +		return PTR_ERR(priv->clk);
> > +	}
> 
> Why not
> 
> 	return dev_err_probe(...);
> 
> ?
> 
> Ditto for the rest cases like this.
Ok

> > +	adap->dev.of_node = dev->of_node;
> 
> device_set_node()
Since we don't need to set dev->fwnode, why is device_set_node() any
better?


Thanks
Phil




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux