> > +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:
signature.asc
Description: Digital signature
