Re: [PATCH] i2c: add Renesas R-Car I2C driver

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

 



Hi Phil

Thank you for your comments

> > +static void rcar_i2c_write(struct rcar_i2c_priv *priv, int reg, u32 
> val)
> > +{
> > +   __raw_writel(val, priv->io + reg);
> > +}
> > +
> > +static u32 rcar_i2c_read(struct rcar_i2c_priv *priv, int reg)
> > +{
> > +   return __raw_readl(priv->io + reg);
> > +}
> I think we should use writel/readl here. We have a number of devices where 
> the virtual address map conflicts with register addresses for other 
> peripherals.

OK. I will fix it.


> > +static void rcar_i2c_bus_phase(struct rcar_i2c_priv *priv, int phase)
> > +{
> > +   switch (phase) {
> > +   case RCAR_BUS_PHASE_ADDR:
> > +      rcar_i2c_write(priv, ICMCR, MDBS | MIE | ESG);
> > +      break;
> > +   case RCAR_BUS_PHASE_DATA:
> > +      rcar_i2c_write(priv, ICMCR, MDBS | MIE);
> > +      break;
> > +   case RCAR_BUS_PHASE_STOP:
> > +      rcar_i2c_write(priv, ICMCR, MDBS | MIE | FSB);
> > +      break;
> > +   }
> > +
> > +   return;
> > +}
> Don't need the return.

indeed. thanks

> > +   /*
> > +    * use 95% bus speed for safety.
> > +    */
> > +   bus_speed = bus_speed * 95 / 100;
> Why do you use 95% of the bus speed? Surely, either the hardware supports 
> a specific speed or it doesn't.
(snip)
> > +   /*
> > +    * it is impossible to calculate large scale
> > +    * number on u32
> > +    *
> > +    * F[(ticf + tr + intd) * ick]
> > +    *  = F[(35 + 200 + 50)ns * ick]
> > +    *  = F[285 * ick / 1000000000]
> > +    *  = F[(ick / 1000000) * 285 / 1000]
> > +    */
> > +   round = (ick + 500000) / 1000000 * 285;
> > +   round = (round + 500) / 1000;
> Now that I see this, I guess that you used 95% bus clock due to rounding 
> errors here. If so, maybe it would be better to try to improve this 
> calculation.

Indeed, thanks.

> typo: happend => happened 
> typo: finised => finished
> typo: happend => happened 

Haha :)
Sorry for my English.

> > +static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
> > +            struct i2c_msg *msgs,
> > +            int num)
> > +{
> > +   struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
> > +   struct device *dev = rcar_i2c_priv_to_dev(priv);
> > +   unsigned long flags;
> > +   int i, ret, timeout;
> > +
> > +   /*================== enable i2c device ===================*/
> > +   pm_runtime_get_sync(dev);
(snip)
> > +   pm_runtime_put_sync(dev);
> > +   /*================== disable i2c device ===================*/
> This comment should be above the previous line, unless you the comment is 
> meant to say "i2c device disabled"

I see.


Best regards
---
Kuninori Morimoto
--
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


[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