Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
> I vote for having the exact SoC revision in the binding documentation > rather than wildcards or references to the list of i.MX SoCs. Otherwise > only the driver code gives a clue that the i2c driver matches imx1-i2c, > imx21-i2c and vf610-i2c, but not imx31-i2c. Dunno if I got all right, so adding my 2 cents: Yes to adding each SoC to the binding docs. No to adding each SoC to the driver as a seperate 'compatible' entry if not really needed to distinguish IP versions. I mean imx31 should have two compatible entries anyhow, one for imx31 and one for imx21 as fallback, no? That all being said: Unless somebody objects, I'll pick the most recent VF610 series today and leave the doc fixup for later. Thanks, Wolfram signature.asc Description: Digital signature
Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
On Tue, Aug 13, 2013 at 06:12:14PM +0200, s.ha...@pengutronix.de wrote: > I vote for having the exact SoC revision in the binding documentation > rather than wildcards or references to the list of i.MX SoCs. Otherwise > only the driver code gives a clue that the i2c driver matches imx1-i2c, > imx21-i2c and vf610-i2c, but not imx31-i2c. +1 Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
On Tue, Aug 13, 2013 at 09:48:40AM -0600, Stephen Warren wrote: > On 08/13/2013 01:46 AM, s.ha...@pengutronix.de wrote: > > On Mon, Aug 12, 2013 at 05:23:35PM -0600, Stephen Warren wrote: > >> On 08/12/2013 10:43 AM, Mark Rutland wrote: > The binding string for i2c-imx driver in > Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format > of "- compatible : Should be "fsl,-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? > >> > >> I suppose technically we should list out every exact string in the > >> binding, but it's a little annoying to have to update the binding doc > >> every time a new chip comes out (and I expect that'll happen more and > >> more!) just to add a new compatible value since all the differences are > >> known internally to the driver and don't impact the binding... > > > > We would only have to update the the docs when an incompatible SoC comes > > out. For this particular driver this would be all marked with a star: > > > > * i.MX1 > > * i.MX21 > > i.MX25 > > i.MX27 > > i.MX31 > > i.MX35 > > i.MX51 > > i.MX53 > > i.MX6 > > * Vybrid > > > > That's not too many updates to the binding docs since 2001. > > (The SPI core changed with nearly every SoC version though) > > So, the SPI core changed its HW implementation, or changed its > SW-visible interface? If the latter, then you need a separate compatible > value for each, which was my point. It changed the SW-visible interface. I vote for having the exact SoC revision in the binding documentation rather than wildcards or references to the list of i.MX SoCs. Otherwise only the driver code gives a clue that the i2c driver matches imx1-i2c, imx21-i2c and vf610-i2c, but not imx31-i2c. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
On 08/13/2013 01:46 AM, s.ha...@pengutronix.de wrote: > On Mon, Aug 12, 2013 at 05:23:35PM -0600, Stephen Warren wrote: >> On 08/12/2013 10:43 AM, Mark Rutland wrote: The binding string for i2c-imx driver in Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format of "- compatible : Should be "fsl,-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? >> >> I suppose technically we should list out every exact string in the >> binding, but it's a little annoying to have to update the binding doc >> every time a new chip comes out (and I expect that'll happen more and >> more!) just to add a new compatible value since all the differences are >> known internally to the driver and don't impact the binding... > > We would only have to update the the docs when an incompatible SoC comes > out. For this particular driver this would be all marked with a star: > > * i.MX1 > * i.MX21 > i.MX25 > i.MX27 > i.MX31 > i.MX35 > i.MX51 > i.MX53 > i.MX6 > * Vybrid > > That's not too many updates to the binding docs since 2001. > (The SPI core changed with nearly every SoC version though) So, the SPI core changed its HW implementation, or changed its SW-visible interface? If the latter, then you need a separate compatible value for each, which was my point. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
On Mon, Aug 12, 2013 at 05:23:35PM -0600, Stephen Warren wrote: > On 08/12/2013 10:43 AM, Mark Rutland wrote: > >> The binding string for i2c-imx driver in > >> Documentation/devicetree/bindings/i2c/i2c-imx.txt use a wildcard format > >> of "- compatible : Should be "fsl,-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? > > I suppose technically we should list out every exact string in the > binding, but it's a little annoying to have to update the binding doc > every time a new chip comes out (and I expect that'll happen more and > more!) just to add a new compatible value since all the differences are > known internally to the driver and don't impact the binding... We would only have to update the the docs when an incompatible SoC comes out. For this particular driver this would be all marked with a star: * i.MX1 * i.MX21 i.MX25 i.MX27 i.MX31 i.MX35 i.MX51 i.MX53 i.MX6 * Vybrid That's not too many updates to the binding docs since 2001. (The SPI core changed with nearly every SoC version though) Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
On 08/12/2013 10:43 AM, Mark Rutland wrote:
> [Adding other devicetree maintainers to Cc]
>
> On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
>>
>>
>>> 发件人: Mark Rutland [mark.rutl...@arm.com]
>>> 发送时间: 2013年8月10日 22:08
>>> 收件人: Lu Jingchang-B35083
>>> 抄送: w...@the-dreams.de; Estevam Fabio-R49496; Li Xiaochun-B41219;
>>> s.ha...@pengutronix.de; linux-i2c@vger.kernel.org; Jin Zhengxiong-R64188;
>>> shawn@linaro.org; linux-arm-ker...@lists.infradead.org
>>> 主题: 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.
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], },
>>>
>>> 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,-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?
I suppose technically we should list out every exact string in the
binding, but it's a little annoying to have to update the binding doc
every time a new chip comes out (and I expect that'll happen more and
more!) just to add a new compatible value since all the differences are
known internally to the driver and don't impact the binding...
Kumar's idea could be a reasonable compromise, although I guess it
doesn't technically cover the case of there being 10 chips, each of
which has *some* new/different IP block, yet some IP blocks not changing
in every chip, and hence drivers might only directly binding to 5 of the
10 possible compatible values in some cases...
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
On Monday 12 of August 2013 17:43:54 Mark Rutland wrote:
> [Adding other devicetree maintainers to Cc]
>
> On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
> >
> >
> > >发件人: Mark Rutland [mark.rutl...@arm.com]
> > >发送时间: 2013年8月10日 22:08
> > >收件人: Lu Jingchang-B35083
> > >抄送: w...@the-dreams.de; Estevam Fabio-R49496; Li Xiaochun-B41219;
> > >s.ha...@pengutronix.de; linux-i2c@vger.kernel.org; Jin
> > >Zhengxiong-R64188; shawn@linaro.org;
> > >linux-arm-ker...@lists.infradead.org 主题: 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
> > >> Signed-off-by: Xiaochun Li
> > >> Signed-off-by: Jingchang Lu
> > >> ---
> > >>
> > >> 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,-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?
In case of Samsung platforms we decided to always use the name of first
SoC in which given IP appeared and list all compatible SoCs in binding
documentation. This is IMHO the most consistent way, as there is no
confusion about IP versions (not always listed in documentation, not even
saying about unavailable documentation) or potential problems with new
SoCs matching the wildcard, but having different IP.
In this particular case, the wildcard can be easily transformed
into a non-wildcard binding, by listing all supported values, i.e.
adding following text to binding documentation:
8<--
The can be one of:
- imx1 - for i.MX1 and compatible SoCs,
- imx21 - for i.MX21 and compatible SoCs,
- vf610 - for Vybrid VF610 and compatible SoCs.
-->8
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 答复: [PATCH v3 2/2] i2c: imx: Add Vybrid VF610 I2C controller support
[Adding other devicetree maintainers to Cc]
On Mon, Aug 12, 2013 at 01:56:07PM +0100, Lu Jingchang-B35083 wrote:
>
>
> >发件人: Mark Rutland [mark.rutl...@arm.com]
> >发送时间: 2013年8月10日 22:08
> >收件人: Lu Jingchang-B35083
> >抄送: w...@the-dreams.de; Estevam Fabio-R49496; Li Xiaochun-B41219;
> >s.ha...@pengutronix.de; linux-i2c@vger.kernel.org; Jin Zhengxiong-R64188;
> >shawn@linaro.org; linux-arm-ker...@lists.infradead.org
> >主题: 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
> >> Signed-off-by: Xiaochun Li
> >> Signed-off-by: Jingchang Lu
> >> ---
> >> 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,-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.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
