[v3,5/6] dt-bindings: i2c: Add Mediatek MT8183 i2c binding

Message ID 1548057574-8061-6-git-send-email-qii.wang@mediatek.com
State Superseded
Headers show
Series
  • add i2c support for mt7629 and mt8183
Related show

Commit Message

Qii Wang Jan. 21, 2019, 7:59 a.m.
Add MT8183 i2c binding to binding file. Compare to 2712 i2c
controller, MT8183 has different registers, offsets, clock,
and share i3c controller.

Signed-off-by: qii wang <qii.wang@mediatek.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/i2c/i2c-mtk.txt |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Wolfram Sang Feb. 5, 2019, 1:16 p.m. | #1
> +  - mediatek,share-i3c: i3c controller can share i2c function.

I am not happy with this binding. There must be a better way of using
the I3C controller in I2C mode. I think it would be easier to tell if we
had an I3C driver to see how it implements I2C fallback there. Is the
I3C driver on the way?
Qii Wang Feb. 14, 2019, 1:54 a.m. | #2
On Tue, 2019-02-05 at 14:16 +0100, Wolfram Sang wrote:
> > +  - mediatek,share-i3c: i3c controller can share i2c function.
> 
> I am not happy with this binding. There must be a better way of using
> the I3C controller in I2C mode. I think it would be easier to tell if we
> had an I3C driver to see how it implements I2C fallback there. Is the
> I3C driver on the way?
> 

I am very sorry for the late reply due to the Chinese New Year holiday.
After confirmation, for I2C mode in I3C controller, we will push an I3C
driver to implement I2C fallback there later, and we will remove
"mediatek,share-i3c" for i2c controller driver in the next patch.
Wolfram Sang Feb. 15, 2019, 9:02 a.m. | #3
On Thu, Feb 14, 2019 at 09:54:28AM +0800, Qii Wang wrote:
> On Tue, 2019-02-05 at 14:16 +0100, Wolfram Sang wrote:
> > > +  - mediatek,share-i3c: i3c controller can share i2c function.
> > 
> > I am not happy with this binding. There must be a better way of using
> > the I3C controller in I2C mode. I think it would be easier to tell if we
> > had an I3C driver to see how it implements I2C fallback there. Is the
> > I3C driver on the way?
> > 
> 
> I am very sorry for the late reply due to the Chinese New Year holiday.

No worries. I hope you enjoyed the holidays!

> After confirmation, for I2C mode in I3C controller, we will push an I3C
> driver to implement I2C fallback there later, and we will remove
> "mediatek,share-i3c" for i2c controller driver in the next patch.

The problem with DT bindings is that you need to support them forever
once you introduced them. Because there might be old device trees on
boards out there which need the property. So, I don't like the idea of
having a temporary binding which is meant to be removed anyway. My take
is that you should just send out the I3C driver which will handle the
I2C support then.

Adding Rob to CC in case he has a different opinion.
Qii Wang Feb. 16, 2019, 11:32 a.m. | #4
On Fri, 2019-02-15 at 10:02 +0100, Wolfram Sang wrote:
> On Thu, Feb 14, 2019 at 09:54:28AM +0800, Qii Wang wrote:
> > On Tue, 2019-02-05 at 14:16 +0100, Wolfram Sang wrote:
> > > > +  - mediatek,share-i3c: i3c controller can share i2c function.
> > > 
> > > I am not happy with this binding. There must be a better way of using
> > > the I3C controller in I2C mode. I think it would be easier to tell if we
> > > had an I3C driver to see how it implements I2C fallback there. Is the
> > > I3C driver on the way?
> > > 
> > 
> > I am very sorry for the late reply due to the Chinese New Year holiday.
> 
> No worries. I hope you enjoyed the holidays!
> 
> > After confirmation, for I2C mode in I3C controller, we will push an I3C
> > driver to implement I2C fallback there later, and we will remove
> > "mediatek,share-i3c" for i2c controller driver in the next patch.
> 
> The problem with DT bindings is that you need to support them forever
> once you introduced them. Because there might be old device trees on
> boards out there which need the property. So, I don't like the idea of
> having a temporary binding which is meant to be removed anyway. My take
> is that you should just send out the I3C driver which will handle the
> I2C support then.
> 
> Adding Rob to CC in case he has a different opinion.
> 

Maybe you think that MT8183 SoC only has the I3C controllers. Actually,
there are I2C and I3C controllers on MT8183 SoC. We will remove
"mediatek,share-i3c" for I2C controller driver, so these patches are
just to support the I2C controllers on MT8183 SoC. For the I3C
controller we will send some new patches which are based on i3c
framework directories(kernel/drivers/i3c), not based on
i2c-mt65xx.c.
Wolfram Sang Feb. 16, 2019, 12:29 p.m. | #5
> Maybe you think that MT8183 SoC only has the I3C controllers. Actually,
> there are I2C and I3C controllers on MT8183 SoC. We will remove
> "mediatek,share-i3c" for I2C controller driver, so these patches are
> just to support the I2C controllers on MT8183 SoC. For the I3C
> controller we will send some new patches which are based on i3c
> framework directories(kernel/drivers/i3c), not based on
> i2c-mt65xx.c.

Sounds perfect to me. If it turns out that the I3C driver can share
things with the I2C driver, we can discuss it then. But unless we have
an I3C driver and know for sure, the above strategy seems good to me.
Qii Wang Feb. 19, 2019, 1:13 p.m. | #6
On Sat, 2019-02-16 at 13:29 +0100, Wolfram Sang wrote:
> > Maybe you think that MT8183 SoC only has the I3C controllers. Actually,
> > there are I2C and I3C controllers on MT8183 SoC. We will remove
> > "mediatek,share-i3c" for I2C controller driver, so these patches are
> > just to support the I2C controllers on MT8183 SoC. For the I3C
> > controller we will send some new patches which are based on i3c
> > framework directories(kernel/drivers/i3c), not based on
> > i2c-mt65xx.c.
> 
> Sounds perfect to me. If it turns out that the I3C driver can share
> things with the I2C driver, we can discuss it then. But unless we have
> an I3C driver and know for sure, the above strategy seems good to me.
> 

Can I push the V4 patches of I2C controller driver now? or you just want
an I3C driver that is compatible with the I2C controller?

According to our plan, I2C controller will use
i2c-mt65xx.c(kernel/drivers/i2c/busses/)and I3C controller will use
i3c-xxx.c(kernel/drivers/i3c/master/)
Wolfram Sang Feb. 19, 2019, 1:27 p.m. | #7
> > > Maybe you think that MT8183 SoC only has the I3C controllers. Actually,
> > > there are I2C and I3C controllers on MT8183 SoC. We will remove
> > > "mediatek,share-i3c" for I2C controller driver, so these patches are
> > > just to support the I2C controllers on MT8183 SoC. For the I3C
> > > controller we will send some new patches which are based on i3c
> > > framework directories(kernel/drivers/i3c), not based on
> > > i2c-mt65xx.c.
> > 
> > Sounds perfect to me. If it turns out that the I3C driver can share
> > things with the I2C driver, we can discuss it then. But unless we have
> > an I3C driver and know for sure, the above strategy seems good to me.
> > 
> 
> Can I push the V4 patches of I2C controller driver now? or you just want
> an I3C driver that is compatible with the I2C controller?

If the V4 series does not use the "mediatek,share-i3c" binding anymore,
please resend it. From what I recall, it should be OK then.

> According to our plan, I2C controller will use
> i2c-mt65xx.c(kernel/drivers/i2c/busses/)and I3C controller will use
> i3c-xxx.c(kernel/drivers/i3c/master/)

Sounds OK from what I understood. The I2C fallback from I3C will be
interesting.

Patch

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mtk.txt b/Documentation/devicetree/bindings/i2c/i2c-mtk.txt
index ee4c324..0d29a5b 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mtk.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mtk.txt
@@ -12,14 +12,15 @@  Required properties:
       "mediatek,mt7623-i2c", "mediatek,mt6577-i2c": for MediaTek MT7623
       "mediatek,mt7629-i2c", "mediatek,mt2712-i2c": for MediaTek MT7629
       "mediatek,mt8173-i2c": for MediaTek MT8173
+      "mediatek,mt8183-i2c": for MediaTek MT8183
   - reg: physical base address of the controller and dma base, length of memory
     mapped region.
   - interrupts: interrupt number to the cpu.
   - clock-div: the fixed value for frequency divider of clock source in i2c
     module. Each IC may be different.
   - clocks: clock name from clock manager
-  - clock-names: Must include "main" and "dma", if enable have-pmic need include
-    "pmic" extra.
+  - clock-names: Must include "main" and "dma", "arb" is optional, if enable
+    have-pmic need include "pmic" extra.
 
 Optional properties:
   - clock-frequency: Frequency in Hz of the bus when transfer, the default value
@@ -27,6 +28,7 @@  Optional properties:
   - mediatek,have-pmic: platform can control i2c form special pmic side.
     Only mt6589 and mt8135 support this feature.
   - mediatek,use-push-pull: IO config use push-pull mode.
+  - mediatek,share-i3c: i3c controller can share i2c function.
 
 Example: