| Message ID | 20200527153046.6172-3-Sergey.Semin@baikalelectronics.ru |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | i2c: designeware: Add Baikal-T1 System I2C support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 8 lines checked |
Rob, Could you pay attention to this patch? The patchset review procedure is nearly over, while the DT part is only partly reviewed by you. Thanks -Sergey On Wed, May 27, 2020 at 06:30:37PM +0300, Serge Semin wrote: > dtc currently doesn't support I2C_OWN_SLAVE_ADDRESS flag set in the > i2c "reg" property. If it is the compiler will print a warning: > > Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64: I2C bus unit address format error, expected "40000064" > Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64:reg: I2C address must be less than 10-bits, got "0x40000064" > > In order to silence dtc up let's discard the flag from the DW I2C DT > binding example for now. Just revert this commit when dtc is fixed. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > Cc: linux-mips@vger.kernel.org > > --- > > Changelog v3: > - This is a new patch created as a result of the Rob request to remove > the EEPROM-slave bit setting in the DT binndings example until the dtc > is fixed. > --- > Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml > index 4bd430b2b41d..101d78e8f19d 100644 > --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml > +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml > @@ -137,7 +137,7 @@ examples: > > eeprom@64 { > compatible = "linux,slave-24c02"; > - reg = <0x40000064>; > + reg = <0x64>; > }; > }; > - | > -- > 2.26.2 >
On Wed, May 27, 2020 at 06:33:51PM +0300, Serge Semin wrote: > Rob, > Could you pay attention to this patch? The patchset review procedure is > nearly over, while the DT part is only partly reviewed by you. Pretty sure I commented on this. Not sure what version, you're sending new versions too fast. Give people time to review. > > Thanks > -Sergey > > On Wed, May 27, 2020 at 06:30:37PM +0300, Serge Semin wrote: > > dtc currently doesn't support I2C_OWN_SLAVE_ADDRESS flag set in the > > i2c "reg" property. If it is the compiler will print a warning: > > > > Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64: I2C bus unit address format error, expected "40000064" > > Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64:reg: I2C address must be less than 10-bits, got "0x40000064" > > > > In order to silence dtc up let's discard the flag from the DW I2C DT > > binding example for now. Just revert this commit when dtc is fixed. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > > Cc: linux-mips@vger.kernel.org > > > > --- > > > > Changelog v3: > > - This is a new patch created as a result of the Rob request to remove > > the EEPROM-slave bit setting in the DT binndings example until the dtc > > is fixed. > > --- > > Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml > > index 4bd430b2b41d..101d78e8f19d 100644 > > --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml > > +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml > > @@ -137,7 +137,7 @@ examples: > > > > eeprom@64 { > > compatible = "linux,slave-24c02"; > > - reg = <0x40000064>; > > + reg = <0x64>; This is wrong though because "linux,slave-24c02" should have bit 30 set. (And either the unit-address was wrong or we can define the unit-address does not include the high bits.) Rob
On Fri, May 29, 2020 at 12:13:38PM -0600, Rob Herring wrote: > On Wed, May 27, 2020 at 06:33:51PM +0300, Serge Semin wrote: > > Rob, > > Could you pay attention to this patch? The patchset review procedure is > > nearly over, while the DT part is only partly reviewed by you. > > Pretty sure I commented on this. Not sure what version, you're sending > new versions too fast. Give people time to review. Yeah, you did. Sorry for sending the new versions very fast. Normally I prefer to keep up with comments so to past a particular maintainer review as fast as possible without long delays. In my experience the longer you wait, the lesser maintainers remember about your patchset, the harder for one to continue the next versions review. Regarding this patch the brand new version on is v6: [PATCH v6 02/11] dt-bindings: i2c: Convert DW I2C slave to the DW I2C master example Could you please find it in your email log? I've left a note there for you about options what we can do with this case. -Sergey > > > > > Thanks > > -Sergey > > > > On Wed, May 27, 2020 at 06:30:37PM +0300, Serge Semin wrote: > > > dtc currently doesn't support I2C_OWN_SLAVE_ADDRESS flag set in the > > > i2c "reg" property. If it is the compiler will print a warning: > > > > > > Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64: I2C bus unit address format error, expected "40000064" > > > Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64:reg: I2C address must be less than 10-bits, got "0x40000064" > > > > > > In order to silence dtc up let's discard the flag from the DW I2C DT > > > binding example for now. Just revert this commit when dtc is fixed. > > > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > > > Cc: linux-mips@vger.kernel.org > > > > > > --- > > > > > > Changelog v3: > > > - This is a new patch created as a result of the Rob request to remove > > > the EEPROM-slave bit setting in the DT binndings example until the dtc > > > is fixed. > > > --- > > > Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml > > > index 4bd430b2b41d..101d78e8f19d 100644 > > > --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml > > > +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml > > > @@ -137,7 +137,7 @@ examples: > > > > > > eeprom@64 { > > > compatible = "linux,slave-24c02"; > > > - reg = <0x40000064>; > > > + reg = <0x64>; > > This is wrong though because "linux,slave-24c02" should have bit 30 set. > (And either the unit-address was wrong or we can define the unit-address > does not include the high bits.) > > Rob
On Fri, May 29, 2020 at 09:22:56PM +0300, Serge Semin wrote: > On Fri, May 29, 2020 at 12:13:38PM -0600, Rob Herring wrote: > > On Wed, May 27, 2020 at 06:33:51PM +0300, Serge Semin wrote: > > you're sending > > new versions too fast. Give people time to review. > > Yeah, you did. Sorry for sending the new versions very fast. Normally I prefer > to keep up with comments so to past a particular maintainer review as fast as > possible without long delays. In my experience the longer you wait, the lesser > maintainers remember about your patchset, the harder for one to continue the > next versions review. Documentation/process/submitting-patches.rst: "Wait for a minimum of one week before resubmitting or pinging reviewers - possibly longer during busy times like merge windows."
On Fri, May 29, 2020 at 09:42:01PM +0300, Andy Shevchenko wrote: > On Fri, May 29, 2020 at 09:22:56PM +0300, Serge Semin wrote: > > On Fri, May 29, 2020 at 12:13:38PM -0600, Rob Herring wrote: > > > On Wed, May 27, 2020 at 06:33:51PM +0300, Serge Semin wrote: > > > > you're sending > > > new versions too fast. Give people time to review. > > > > Yeah, you did. Sorry for sending the new versions very fast. Normally I prefer > > to keep up with comments so to past a particular maintainer review as fast as > > possible without long delays. In my experience the longer you wait, the lesser > > maintainers remember about your patchset, the harder for one to continue the > > next versions review. > > Documentation/process/submitting-patches.rst: > > "Wait for a minimum of one week before resubmitting or pinging reviewers - > possibly longer during busy times like merge windows." Good to know what I already know.) How much do you personally wait before resubmitting? In my experience reviewing your DW APB GPIO patches, no longer than a few hours. -Sergey > > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, May 29, 2020 at 09:45:37PM +0300, Serge Semin wrote: > On Fri, May 29, 2020 at 09:42:01PM +0300, Andy Shevchenko wrote: > > On Fri, May 29, 2020 at 09:22:56PM +0300, Serge Semin wrote: > > > On Fri, May 29, 2020 at 12:13:38PM -0600, Rob Herring wrote: > > > > On Wed, May 27, 2020 at 06:33:51PM +0300, Serge Semin wrote: > > > > > > you're sending > > > > new versions too fast. Give people time to review. > > > > > > Yeah, you did. Sorry for sending the new versions very fast. Normally I prefer > > > to keep up with comments so to past a particular maintainer review as fast as > > > possible without long delays. In my experience the longer you wait, the lesser > > > maintainers remember about your patchset, the harder for one to continue the > > > next versions review. > > > > > Documentation/process/submitting-patches.rst: > > > > "Wait for a minimum of one week before resubmitting or pinging reviewers - > > possibly longer during busy times like merge windows." > > Good to know what I already know.) How much do you personally wait before > resubmitting? In my experience reviewing your DW APB GPIO patches, no longer > than a few hours. Moreover the statement you cited is about the patches, which doesn't get any attention from the maintainers/reviewers for quite some time. In this case I normally resubmit the patches no sooner than a week. I was talking about the situation when you get the review comments, which need to be addressed. -Sergey > > -Sergey > > > > > > > -- > > With Best Regards, > > Andy Shevchenko > > > >
On Fri, May 29, 2020 at 12:58 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Fri, May 29, 2020 at 09:45:37PM +0300, Serge Semin wrote: > > On Fri, May 29, 2020 at 09:42:01PM +0300, Andy Shevchenko wrote: > > > On Fri, May 29, 2020 at 09:22:56PM +0300, Serge Semin wrote: > > > > On Fri, May 29, 2020 at 12:13:38PM -0600, Rob Herring wrote: > > > > > On Wed, May 27, 2020 at 06:33:51PM +0300, Serge Semin wrote: > > > > > > > > you're sending > > > > > new versions too fast. Give people time to review. > > > > > > > > Yeah, you did. Sorry for sending the new versions very fast. Normally I prefer > > > > to keep up with comments so to past a particular maintainer review as fast as > > > > possible without long delays. In my experience the longer you wait, the lesser > > > > maintainers remember about your patchset, the harder for one to continue the > > > > next versions review. > > > > > > > > > Documentation/process/submitting-patches.rst: > > > > > > "Wait for a minimum of one week before resubmitting or pinging reviewers - > > > possibly longer during busy times like merge windows." > > > > Good to know what I already know.) How much do you personally wait before > > resubmitting? In my experience reviewing your DW APB GPIO patches, no longer > > than a few hours. > > Moreover the statement you cited is about the patches, which doesn't get any > attention from the maintainers/reviewers for quite some time. In this case I > normally resubmit the patches no sooner than a week. I was talking about the > situation when you get the review comments, which need to be addressed. There's not going to be any rule that always works. It takes judgement. I'd say the greater the rework from review comments, the sooner you can resend. But then if it's multiple subsystems/maintainers, you need to give all of them time. I go in date order mostly. You send a new version, then you go to the back of the queue. So if you want it reviewed the soonest, send it 2 weeks ago. ;) There's also the strategy of reviewing other patches in front of yours. Sometimes I go by version numbers, but send version 50 and I might be suspicious. And that's rewarding folks who are sloppy or keep sending broken stuff. The real solution is I just need more help reviewing things. Rob
diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml index 4bd430b2b41d..101d78e8f19d 100644 --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml @@ -137,7 +137,7 @@ examples: eeprom@64 { compatible = "linux,slave-24c02"; - reg = <0x40000064>; + reg = <0x64>; }; }; - |
dtc currently doesn't support I2C_OWN_SLAVE_ADDRESS flag set in the i2c "reg" property. If it is the compiler will print a warning: Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64: I2C bus unit address format error, expected "40000064" Warning (i2c_bus_reg): /example-2/i2c@1120000/eeprom@64:reg: I2C address must be less than 10-bits, got "0x40000064" In order to silence dtc up let's discard the flag from the DW I2C DT binding example for now. Just revert this commit when dtc is fixed. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: linux-mips@vger.kernel.org --- Changelog v3: - This is a new patch created as a result of the Rob request to remove the EEPROM-slave bit setting in the DT binndings example until the dtc is fixed. --- Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)