Re: [PATCH 3/5] i2c: emev2: add driver

[Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]


> > +static inline void em_clear_set_bit(struct em_i2c_device *priv, u8 clear,
> > u8 set, u8 reg)
> 
> Maybe em_i2c_clear_set_bit for consistency ? I'd prefer having the reg 
> argument before clear and set, but maybe that's just me.

This is such a generic function that I decided to skip 'i2c' in the
function name. Can add it, don't have a strong preference.

> I would have also introduced em_i2c_read and em_i2c_write. That's entirely up 
> to you.

I don't like such wrappers around standard read/write functions. What's
the gain?

> > +		retr = 1000;
> > +		while (readb(priv->base + I2C_OFS_IICACT0) == 1 && retr)
> 
> How about adding a cpu_relax() here ?

Can do, but I think it is overkill.

> 
> > +	/* Extra setup for read transactions */
> > +	if (!(status & I2C_BIT_TRC0)) {
> 
> How about checking msg->flags & I2C_M_RD here ? It should be equivalent but 
> would make the code more readable by not requiring knowledge of the hardware. 

I buy this argument. Will change.

> > +	priv = devm_kzalloc(&pdev->dev, sizeof(struct em_i2c_device), 
> GFP_KERNEL);
> 
> I'd use sizeof(*priv).

Yes, definately.

> > +	irq = platform_get_irq(pdev, 0);
> 
> I'd move this call right before devm_request_irq() below. devm_request_irq() 
> should handle invalid IRQs, but won't print an error message. I'd let you 
> decide whether that's a problem.

Will check.

> 
> > +	priv->adap.timeout = msecs_to_jiffies(100);
> > +	priv->adap.retries = 5;
> 
> Is there a particular reason for setting the number of retries to 5 ?

It should be non-zero at least, so bus access will be retried if -EAGAIN
is returned because of a busy bus.

> > +	if (ret)
> > +		goto exit_clk;
> 
> Nitpicking, I'd call this error_clk to show that the label is used in case of 
> error only. You could also just call it error as there's no other error-
> related label.

Yup.

> > +	dev_info(&pdev->dev, "Added i2c controller %d irq %d @ 0x%p\n",
> > +		priv->adap.nr, irq, priv->base);
> 
> Is priv->base useful here ? The physical address of the registers block could 
> be, but its kernel virtual address doesn't seem very interesting to me.

Agreed.

Thanks for the review!

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]