Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support

[Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>]

[Adding other devicetree maintainers to Cc]

On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
> 
> ________________________________________
> >发件人: Mark Rutland [mark.rutland-5wv7dgnIgG8@public.gmane.org]
> >发送时间: 2013年8月10日 22:08
> >收件人: Lu Jingchang-B35083
> >抄送: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org; Estevam Fabio-R49496; Li Xiaochun-B41219; s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Jin Zhengxiong-R64188; shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XQ@public.gmane.orgorg
> >主题: Re: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
> 
> >On Fri, Aug 02, 2013 at 05:44:08AM +0100, Jingchang Lu wrote:
> >>   Add Freescale Vybrid VF610 I2C controller support to
> >> imx I2C driver framework.
> >>   Some operation is different from imx I2C controller.
> >> The register offset, the i2c clock divider value table,
> >> the module enabling(I2CR_IEN) which is just invert with imx,
> >> and the interrupt flag(I2SR) clearing opcode is w1c on VF610
> >> but w0c on imx.
> >>
> >> Signed-off-by: Jason Jin <Jason.jin-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >> Signed-off-by: Xiaochun Li <b41219-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >> Signed-off-by: Jingchang Lu <b35083-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >> ---
> >> changes in v3:
> >>   Using struct naming the i2c clock {div, regval} pair.
> >>  Using address shift handling registers address difference.
> >>
> >> changes in v2:
> >>   Fix building section mismatch(es) warning.
> >>
> >>  drivers/i2c/busses/i2c-imx.c | 146 ++++++++++++++++++++++++++++++++++++-------
> >>  1 file changed, 122 insertions(+), 24 deletions(-)
> 
> >[...]
> 
> >> @@ -145,6 +233,7 @@ MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
> >>  static const struct of_device_id i2c_imx_dt_ids[] = {
> >>         { .compatible = "fsl,imx1-i2c", .data = &imx_i2c_devtype[IMX1_I2C], },
> >>         { .compatible = "fsl,imx21-i2c", .data = &imx_i2c_devtype[IMX21_I2C], },
> >> +       { .compatible = "fsl,vf610-i2c", .data = &imx_i2c_devtype[VF610_I2C], },
> >>         { /* sentinel */ }
> >>  };
> 
> 
> >That string doesn't seem to be documented anywhere (from a quick grep of
> >Documentation/devicetree), and there's no binding update included
> >here. It would be nice for that to be fixed :)
> [Lu Jingchang]
> The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format
> of "- compatible : Should be "fsl,<chip>-i2c" " for device using this driver. Neither fsl,imx1-i2c nor fsl,imx21-i2c
> is described in the binding document. So I just leave the vf610 i2c compatible with this. 

I'm not a big fan on wildcards in bindings, as it leaves people free to
put anything in and claim it's a documented binding, and makes it far
harder for an os to actually implement drivers for said binding, as
there's no canonical reference for the set of valid variations.

Obviously there is some precedent, but I'm not sure it's something we
want to stick with, and we can prevent it my updating the documentation
now.

Does anyone else have an opinion?

Mark.