Re: [PATCH v3 2/2] i2c: Support for Netlogic XLP9XX/5XX I2C controller.

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hello,

On Sat, Mar 14, 2015 at 05:18:37PM +0530, Jayachandran C. wrote:
> On Fri, Mar 13, 2015 at 11:24:06AM +0100, Uwe Kleine-König wrote:
> > On Fri, Mar 13, 2015 at 11:59:58AM +0530, Jayachandran C wrote:
> > > diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
> > > new file mode 100644
> > > index 00000000..2f303ad
> > > --- /dev/null
> > > +++ b/drivers/i2c/busses/i2c-xlp9xx.c
> > > @@ -0,0 +1,446 @@
> > > [...]
> > > +
> > > +/*
> > > + * see Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt for device
> > > + * tree bindings documentation
> > > + */
> > When I asked for documentation here, I didn't meant the device tree
> > binding, but the hardware reference manual.
> 
> Unfortunately there is no standalone documentation for the I2C controller,
> this block is used in XLP9xx and XLP5xx SoCs and you can get the documentation
> for the whole SoC from support.broadcom.com if you have an account there.
I don't. According to the instructions to get such an account I have to
contact my "Sales/Engineering contacts at Broadcom or its
Distributors/Manufacturer's representatives directly". Who would that be
for me? You?

> > > [...]
> > > +static void xlp9xx_i2c_mask_irq(struct xlp9xx_i2c_dev *priv, u32 mask)
> > > +{
> > > +	mask = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_INTEN) & ~mask;
> > > +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, mask);
> > You didn't comment on my suggestion to add a variable here for improved
> > readability:
> > 
> > 	u32 inten = xlp9xx_read_i2c_reg(...);
> > 	inten &= ~mask;
> > 	xlp9xx_write_i2c_reg(...);
> > 
> 
> Thought it was not really needed, but it is a little better than the
> current code, will update.
Yeah, for the generated code it probably doesn't make much difference---if
at all. Readability and so maintainability is a good reason to update
code if there is no overhead in the compiled result. (And sometimes even
although there is overhead.)

> > > [...]
> > > +static int xlp9xx_i2c_init(struct xlp9xx_i2c_dev *priv)
> > > +{
> > > +	u32 prescale;
> > > +
> > > +	/*
> > > +	 * The controller uses 5 * SCL clock internally.
> > > +	 * So prescale value should be divided by 5.
> > > +	 */
> > > +	prescale = (((XLP9XX_I2C_IP_CLK_FREQ / priv->clk_hz) - 8) / 5) - 1;
> > I still wonder if you should round differently here.
> 
> Are you thinking of making sure that we don't exceed the given clock freq
> because of rounding? I don't think this is an issue.
Right, that and also in the other direction not to give up speed because
the calculation is not optimal.

> > > +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, XLP9XX_I2C_CTRL_RST);
> > > +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, XLP9XX_I2C_CTRL_EN |
> > > +			     XLP9XX_I2C_CTRL_MASTER);
> > > +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_DIV, prescale);
> > > +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, 0);
> > > +
> > > +	return 0;
> > > +}
> 
> I will post a v4 (which takes care of another comment I have seen in this
> thread as well).
BTW, I fully agree to Arnd's request to use the full SoC's name in the
compatible string. Pick the first SoC to fix the name, and for the later
SoCs use both strings. This method proved to be flexible enough to work
even if Broadcom might release a SoC next year with a name that
matches 9xx but an incompatible i2c device.
For Freescale devices it works exactly like this. There are (to the best
of our knowledge) 3 similar implementations. On i.MX51 the device is
used that first appeared in the i.MX21 and imx51.dtsi has:

	compatible = "fsl,imx51-i2c", "fsl,imx21-i2c";

.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |