You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/01/19 03:10:37 UTC

[GitHub] [incubator-nuttx] v01d opened a new pull request #2717: I2C bitbang driver (and nRF52 implementation)

v01d opened a new pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717


   ## Summary
   
   This adds a software implementation of I2C protocol via a hardware independent driver. A lower-half implementing required GPIO operations is used to interface with actual hardware. This includes a lower half for nRF52 arch.
   
   This is a work in progress, not yet tested.
   
   ## Impact
   
   Add feature
   
   ## Testing
   
   Not yet
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-763964384


   Ok, I have tested this on the logic analyzer and using an SSD1306. After some more optimization and a few fixes I managed to do I2C transfers (managed to turn on/off the display via I2C command line application). I also measured timings with delays on (if I configured for a 3uS GPIO overhead I got around 100kHZ as configured) and delays off (this went up to 480kHZ). 
   
   An important optimization was to configure pin for output open-drain but input buffer connected. This allows to read the pin without having to reconfigure it.
   
   If anyone wants to give it a go and test with some device it would be great. Otherwise I think this is good for merging until it gets more in depth testing in real scenario.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762591067


   > No, no address lines are exposed. The point is that I2C/SDA are not shared so I need five different pairs of SDA/SCL (or changing I2C/SDA pins of peripheral on the fly, as I mentioned, but not too happy about it).
   
   I am referring to the pins that select the match address, not SDA/SCL.  I2C parts usually have 1 to 3 of them to specify 1 to 3 bits of the full 7 bit address that the chip will respond to.  That hackaday reference talks about about using of these address match lines like a "chip select."
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-763964384


   Ok, I have tested this on the logic analyzer and using an SSD1306. After some more optimization and a few fixes I managed to do I2C transfers (managed to turn on/off the display via I2C command line application). I also measured timings with delays on (if I configured for a 3uS GPIO overhead I got around 100kHZ as configured) and delays off (this went up to 480kHZ). 
   
   An important optimization was to configure pin for output open-drain but input buffer connected. This allows to read the pin without having to reconfigure it.
   
   If anyone wants to give it a go and test with some device it would be great. Otherwise I think this is good for merging until it gets more in depth testing in real scenario.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] btashton commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-766509319


   I did a quick functional test.  I'll try and grab some scope shots later with a shunt for current as well.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r559894330



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =
+{
+  .transfer = i2c_bitbang_transfer
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: i2c_bitbang_transfer
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,

Review comment:
       Right. Is `spin_lock_irqsave` OK? Not entirely sure when to use it instead of `enter_critical_section`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] patacongo commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762591067


   > No, no address lines are exposed. The point is that I2C/SDA are not shared so I need five different pairs of SDA/SCL (or changing I2C/SDA pins of peripheral on the fly, as I mentioned, but not too happy about it).
   
   I am referring to the pins that select the address, not SDA/SCL.  I2C parts usually have 1 to 3 of them to specify 1 to 3 bits of the full 7 bit address that the chip will respond to.  That hackaday reference talks about about using of these address match lines like a "chip select."
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762591067


   > No, no address lines are exposed. The point is that I2C/SDA are not shared so I need five different pairs of SDA/SCL (or changing I2C/SDA pins of peripheral on the fly, as I mentioned, but not too happy about it).
   
   I am referring to the pins that select the match address, not SDA/SCL.  I2C parts usually have 1 to 3 of them to specify 1 to 3 bits of the full 7 bit address that the chip will respond to.  That hackaday reference talks about about using one of these address match lines like a "chip select."  Driving different address match bits can make different parts respond to the same address, for example.
   
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762575911


   > I'm curious why you wanted you would want to bit bang it instead of using the hardware?
   > (I'm not saying no, just curious why)
   
   I'm working on a device which has five I2C devices and they are all routed to different pins (:facepalm:). So I'm using the two I2C hardware peripherals and the other would have to be via software. I considered switching pins on the fly but I feel this is cleaner.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762587626


   > I'm working on a device which has five I2C devices and they are all routed to different pins (🤦). So I'm using the two I2C hardware peripherals and the other would have to be via software. I considered switching pins on the fly but I feel this is cleaner.
   
   Is driving the I2C address lines as in https://hackaday.com/2015/03/27/using-i2c-addresses-as-chip-selects/ an option?
   
   An I2C multiplexer would be a heavier weight solution.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] fjpanag commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-763080354


   > BTW, this is yet untested since I'm unsure about proper GPIO handling on nRF52. I understand that the pin should be set to open-drain, but I think nRF52 does not really support this.
   
   > https://devzone.nordicsemi.com/f/nordic-q-a/28373/nrf52810-open-drain-gpio
   
   I have never used these MCUs, but from your link above I understand that it does actually support open-drain configuration.  
   Instead of setting the GPIO as open-drain, as with other MCUs (like STM32 or LPC), you just disable the HIGH side driver. Thus the pin can sink current, but it is in high-Z when set to HIGH.  
   So I believe that this is OK.
   
   > So my reasoning is to using push-pull and switch between OUTPUT/INPUT when required. Although not sure if that is entirely safe (ie, if I send a 1 and the slave sends a 0). I think the drive setting should take care of that, but I've read something about this still being a bit dangerous.
   
   This is very possible in I2C, but you must guarantee that there will be no "conflict", having two devices driving the bus simultaneously. If they do, it is a short-circuit, and even if your prototype works, it is unknown how other devices will behave, or what will be its life-span.
   
   If you absolutely have to do this, then serial resistors are needed.
   
   Two drivers may operate simultaneously in one of the following cases:
   - Multi-master bus. Just forget about it in bit-banging implementations.
   - Clock-stretching. This is documented in the peripheral's datasheet. If the device supports it, then you must assume that it can always stretch the clock. Thus a simple GPIO solution won't do. Best-case scenario, the protocol breaks, worst scenario a part gets damaged.
   
   If the NuttX nRF52 GPIO implementation does not support open-drain mode but the chip does, I would suggest that this needs to be addressed first.
   
   
   > I will place 1.5k resistors on the line without a device, for now...
   These are on the "strong" side, and possibly unneeded for such a small PCB.
   I am afraid that they may get you very close to the maximum of the chip, based on [this](https://devzone.nordicsemi.com/f/nordic-q-a/12614/max-gpio-current-for-nrf52).
   Maybe start with weaker pull-ups?
   
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762588344


   No, no address lines are exposed. The point is that I2C/SDA are not shared so I need five different pairs of SDA/SCL (or changing I2C/SDA pins of peripheral on the fly, as I mentioned, but not too happy about it).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r559901483



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =
+{
+  .transfer = i2c_bitbang_transfer
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: i2c_bitbang_transfer
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,

Review comment:
       > Right. Is `spin_lock_irqsave` OK? Not entirely sure when to use it instead of `enter_critical_section`.
   
   You can use spin_lock_irqsave() when the thread cannot possibly suspend itself within the critical section.  If you can tolerate jitter from interrupt processing, sched_lock() will keep the thread in place, i.e., that it cannot be suspended.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762576221


   > > I'm curious why you wanted you would want to bit bang it instead of using the hardware?
   > > (I'm not saying no, just curious why)
   > 
   > I'm working on a device which has five I2C devices and they are all routed to different pins (facepalm). So I'm using the two I2C hardware peripherals and the other would have to be via software. I considered switching pins on the fly but I feel this is cleaner.
   
   If you feel extra curious, I'm working on this watch: https://hackaday.io/project/175577-hackable-nrf52840-smart-watch


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762591067


   > No, no address lines are exposed. The point is that I2C/SDA are not shared so I need five different pairs of SDA/SCL (or changing I2C/SDA pins of peripheral on the fly, as I mentioned, but not too happy about it).
   
   I am referring to the pins that select the match address, not SDA/SCL.  I2C parts usually have 1 to 3 of them to specify 1 to 3 bits of the full 7 bit address that the chip will respond to.  That hackaday reference talks about about using one of these address match lines like a "chip select."  Driving different address match bits can make different parts respond to the same address, for example.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r560153134



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>

Review comment:
       done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r560149625



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =
+{
+  .transfer = i2c_bitbang_transfer
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: i2c_bitbang_transfer
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count)
+{
+  FAR struct i2c_bitbang_dev_s *priv = (FAR struct i2c_bitbang_dev_s *)dev;
+  int ret = OK;
+  int i;
+
+  for (i = 0; i < count; i++)
+    {
+      uint8_t addr;
+      FAR struct i2c_msg_s *msg = &msgs[i];
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+      /* Compute delay from frequency */
+
+      priv->delay = ((1000000 / msg->frequency) / 2) -

Review comment:
       I think that it will reduce overhead of calling the pin handling functions if I use the variable from internal structure instead of passing via stack.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r559940718



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =
+{
+  .transfer = i2c_bitbang_transfer
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: i2c_bitbang_transfer
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,

Review comment:
       its' better to avoid sched_lock for SMP safety.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] btashton merged pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
btashton merged pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] patacongo commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762587626


   > I'm working on a device which has five I2C devices and they are all routed to different pins (🤦). So I'm using the two I2C hardware peripherals and the other would have to be via software. I considered switching pins on the fly but I feel this is cleaner.
   
   Is driving the I2C address lines as in https://hackaday.com/2015/03/27/using-i2c-addresses-as-chip-selects/ an option?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-765524666


   Anyone would like to take a look again? This is ready for merging.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r560149030



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>

Review comment:
       done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762591067


   > No, no address lines are exposed. The point is that I2C/SDA are not shared so I need five different pairs of SDA/SCL (or changing I2C/SDA pins of peripheral on the fly, as I mentioned, but not too happy about it).
   
   I am referring to the pins that select the match address, not SDA/SCL.  I2C parts usually have 1 to 3 of them to specify 1 to 3 bits of the full 7 bit address that the chip will respond to.  That hackaday reference talks about about using one of these address match lines like a "chip select."
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r560151107



##########
File path: arch/arm/src/nrf52/nrf52_i2c_bitbang.c
##########
@@ -0,0 +1,147 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c_bitbang.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+#include <nuttx/kmalloc.h>
+#include "nrf52_gpio.h"
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct nrf52_i2c_bitbang_dev_s
+{
+  struct i2c_bitbang_lower_dev_s lower;
+  nrf52_pinset_t sda_pin;
+  nrf52_pinset_t scl_pin;
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static void i2c_bb_initialize(FAR struct i2c_bitbang_lower_dev_s *lower);
+
+static void i2c_bb_set_scl(FAR struct i2c_bitbang_lower_dev_s *lower,
+                           bool high);
+static void i2c_bb_set_sda(FAR struct i2c_bitbang_lower_dev_s *lower,
+                           bool high);
+
+static bool i2c_bb_get_scl(FAR struct i2c_bitbang_lower_dev_s *lower);
+static bool i2c_bb_get_sda(FAR struct i2c_bitbang_lower_dev_s *lower);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+const static struct i2c_bitbang_lower_ops_s g_ops =
+{
+  .initialize = i2c_bb_initialize,
+  .set_scl    = i2c_bb_set_scl,
+  .set_sda    = i2c_bb_set_sda,
+  .get_scl    = i2c_bb_get_scl,
+  .get_sda    = i2c_bb_get_sda
+};
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+static void i2c_bb_initialize(FAR struct i2c_bitbang_lower_dev_s *lower)
+{
+  struct nrf52_i2c_bitbang_dev_s *dev = lower->priv;
+
+  nrf52_gpio_config(GPIO_INPUT | GPIO_DRIVE_S0D1 | dev->scl_pin);
+  nrf52_gpio_config(GPIO_INPUT | GPIO_DRIVE_S0D1 | dev->sda_pin);
+}
+
+static void i2c_bb_set_scl(FAR struct i2c_bitbang_lower_dev_s *lower,
+                           bool high)
+{
+  struct nrf52_i2c_bitbang_dev_s *dev = lower->priv;
+
+  nrf52_gpio_config(GPIO_OUTPUT | GPIO_DRIVE_S0D1 |
+                    (high ? GPIO_VALUE_ONE : GPIO_VALUE_ZERO) |
+                    dev->scl_pin);
+}
+
+static void i2c_bb_set_sda(FAR struct i2c_bitbang_lower_dev_s *lower,
+                           bool high)
+{
+  struct nrf52_i2c_bitbang_dev_s *dev = lower->priv;
+
+  nrf52_gpio_config(GPIO_OUTPUT | GPIO_DRIVE_S0D1 |
+                    (high ? GPIO_VALUE_ONE : GPIO_VALUE_ZERO) |
+                    dev->sda_pin);
+}
+
+static bool i2c_bb_get_scl(FAR struct i2c_bitbang_lower_dev_s *lower)
+{
+  struct nrf52_i2c_bitbang_dev_s *dev = lower->priv;
+
+  nrf52_gpio_config(GPIO_INPUT | GPIO_DRIVE_S0D1 | dev->scl_pin);

Review comment:
       I think that it is better to have the lower half be really lean since it will otherwise add overhead to pin toggling. Anyway, it could be considered later on and tested to compare if timings are affected or not.
   
   I still have to analyze timings of this implementation and see if I can actually reach required timings.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762815986


   > > No, no address lines are exposed. The point is that I2C/SDA are not shared so I need five different pairs of SDA/SCL (or changing I2C/SDA pins of peripheral on the fly, as I mentioned, but not too happy about it).
   > 
   > I am referring to the pins that select the match address, not SDA/SCL. I2C parts usually have 1 to 3 of them to specify 1 to 3 bits of the full 7 bit address that the chip will respond to. That hackaday reference talks about about using one of these address match lines like a "chip select." Driving different address match bits can make different parts respond to the same address, for example.
   
   Yes, I understand. But my case is not about being limited by overlapping address or too many devices on a bus, but the fact that I have five I2C devices on five separate buses and only have two hardware I2C peripherals.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r560151421



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =

Review comment:
       done 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r559951014



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c

Review comment:
       correct the file name

##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =

Review comment:
       add static const

##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>

Review comment:
       remove

##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =
+{
+  .transfer = i2c_bitbang_transfer
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: i2c_bitbang_transfer
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count)
+{
+  FAR struct i2c_bitbang_dev_s *priv = (FAR struct i2c_bitbang_dev_s *)dev;
+  int ret = OK;
+  int i;
+
+  for (i = 0; i < count; i++)
+    {
+      uint8_t addr;
+      FAR struct i2c_msg_s *msg = &msgs[i];
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+      /* Compute delay from frequency */
+
+      priv->delay = ((1000000 / msg->frequency) / 2) -

Review comment:
       why not change delay to local variable and pass it to i2c_bitbang_set_xxx instead?

##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>

Review comment:
       remove?

##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =
+{
+  .transfer = i2c_bitbang_transfer
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: i2c_bitbang_transfer
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count)
+{
+  FAR struct i2c_bitbang_dev_s *priv = (FAR struct i2c_bitbang_dev_s *)dev;
+  int ret = OK;
+  int i;
+
+  for (i = 0; i < count; i++)
+    {
+      uint8_t addr;
+      FAR struct i2c_msg_s *msg = &msgs[i];
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+      /* Compute delay from frequency */
+
+      priv->delay = ((1000000 / msg->frequency) / 2) -
+                    CONFIG_I2C_BITBANG_GPIO_OVERHEAD;
+
+      if (priv->delay < 0)
+        {
+          priv->delay = 0;
+        }
+#endif
+
+      /* Send start bit */
+
+      i2c_bitbang_set_scl(priv, true);
+      i2c_bitbang_set_sda(priv, false);
+      i2c_bitbang_set_scl(priv, false);
+
+      /* Send the address */
+
+      addr = (msg->flags & I2C_M_READ ? I2C_READADDR8(msg->addr) :
+                                        I2C_WRITEADDR8(msg->addr));
+
+      i2c_bitbang_send(priv, addr);
+
+      /* Wait for ACK */
+
+      ret = i2c_bitbang_wait_ack(priv);
+
+      if (ret < 0)
+        {
+          return ret;
+        }
+
+      i2c_bitbang_set_scl(priv, false);
+
+      if (msgs->flags & I2C_M_READ)
+        {
+          int j;
+
+          for (j = 0; j < msg->length; j++)
+            {
+              /* Send the data */
+
+              i2c_bitbang_send(priv, msg->buffer[j]);
+
+              ret = i2c_bitbang_wait_ack(priv);
+
+              if (ret < 0)
+                {
+                  return ret;
+                }
+
+              i2c_bitbang_set_scl(priv, false);
+            }
+        }
+      else
+        {
+          int j;
+          int k;
+
+          for (j = 0; j < msg->length; j++)
+            {
+              uint8_t data = 0;
+
+              i2c_bitbang_set_sda(priv, true);
+
+              msg->buffer[j] = 0;
+
+              for (k = 0; k < 8; k++)
+                {
+                  i2c_bitbang_set_scl(priv, true);
+                  data |= priv->lower->ops->get_sda(priv->lower) << (7 - k);

Review comment:
       add a wrapper function for get_sda?

##########
File path: arch/arm/src/nrf52/nrf52_i2c_bitbang.c
##########
@@ -0,0 +1,147 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c_bitbang.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+#include <nuttx/kmalloc.h>
+#include "nrf52_gpio.h"
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct nrf52_i2c_bitbang_dev_s
+{
+  struct i2c_bitbang_lower_dev_s lower;
+  nrf52_pinset_t sda_pin;
+  nrf52_pinset_t scl_pin;
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static void i2c_bb_initialize(FAR struct i2c_bitbang_lower_dev_s *lower);
+
+static void i2c_bb_set_scl(FAR struct i2c_bitbang_lower_dev_s *lower,
+                           bool high);
+static void i2c_bb_set_sda(FAR struct i2c_bitbang_lower_dev_s *lower,
+                           bool high);
+
+static bool i2c_bb_get_scl(FAR struct i2c_bitbang_lower_dev_s *lower);
+static bool i2c_bb_get_sda(FAR struct i2c_bitbang_lower_dev_s *lower);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+const static struct i2c_bitbang_lower_ops_s g_ops =
+{
+  .initialize = i2c_bb_initialize,
+  .set_scl    = i2c_bb_set_scl,
+  .set_sda    = i2c_bb_set_sda,
+  .get_scl    = i2c_bb_get_scl,
+  .get_sda    = i2c_bb_get_sda
+};
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+static void i2c_bb_initialize(FAR struct i2c_bitbang_lower_dev_s *lower)
+{
+  struct nrf52_i2c_bitbang_dev_s *dev = lower->priv;
+
+  nrf52_gpio_config(GPIO_INPUT | GPIO_DRIVE_S0D1 | dev->scl_pin);
+  nrf52_gpio_config(GPIO_INPUT | GPIO_DRIVE_S0D1 | dev->sda_pin);
+}
+
+static void i2c_bb_set_scl(FAR struct i2c_bitbang_lower_dev_s *lower,
+                           bool high)
+{
+  struct nrf52_i2c_bitbang_dev_s *dev = lower->priv;
+
+  nrf52_gpio_config(GPIO_OUTPUT | GPIO_DRIVE_S0D1 |
+                    (high ? GPIO_VALUE_ONE : GPIO_VALUE_ZERO) |
+                    dev->scl_pin);
+}
+
+static void i2c_bb_set_sda(FAR struct i2c_bitbang_lower_dev_s *lower,
+                           bool high)
+{
+  struct nrf52_i2c_bitbang_dev_s *dev = lower->priv;
+
+  nrf52_gpio_config(GPIO_OUTPUT | GPIO_DRIVE_S0D1 |
+                    (high ? GPIO_VALUE_ONE : GPIO_VALUE_ZERO) |
+                    dev->sda_pin);
+}
+
+static bool i2c_bb_get_scl(FAR struct i2c_bitbang_lower_dev_s *lower)
+{
+  struct nrf52_i2c_bitbang_dev_s *dev = lower->priv;
+
+  nrf52_gpio_config(GPIO_INPUT | GPIO_DRIVE_S0D1 | dev->scl_pin);

Review comment:
       should we write a generic gpio lower half for i2c bitbang driver thorugh ioexpander interface? So, other chip can share the same implementation.

##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =
+{
+  .transfer = i2c_bitbang_transfer
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: i2c_bitbang_transfer
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count)
+{
+  FAR struct i2c_bitbang_dev_s *priv = (FAR struct i2c_bitbang_dev_s *)dev;
+  int ret = OK;
+  int i;
+
+  for (i = 0; i < count; i++)
+    {
+      uint8_t addr;
+      FAR struct i2c_msg_s *msg = &msgs[i];
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+      /* Compute delay from frequency */
+
+      priv->delay = ((1000000 / msg->frequency) / 2) -
+                    CONFIG_I2C_BITBANG_GPIO_OVERHEAD;
+
+      if (priv->delay < 0)
+        {
+          priv->delay = 0;
+        }
+#endif
+
+      /* Send start bit */
+
+      i2c_bitbang_set_scl(priv, true);
+      i2c_bitbang_set_sda(priv, false);
+      i2c_bitbang_set_scl(priv, false);
+
+      /* Send the address */
+
+      addr = (msg->flags & I2C_M_READ ? I2C_READADDR8(msg->addr) :
+                                        I2C_WRITEADDR8(msg->addr));
+
+      i2c_bitbang_send(priv, addr);
+
+      /* Wait for ACK */
+
+      ret = i2c_bitbang_wait_ack(priv);
+
+      if (ret < 0)
+        {
+          return ret;
+        }
+
+      i2c_bitbang_set_scl(priv, false);
+
+      if (msgs->flags & I2C_M_READ)
+        {
+          int j;
+
+          for (j = 0; j < msg->length; j++)
+            {
+              /* Send the data */
+
+              i2c_bitbang_send(priv, msg->buffer[j]);
+
+              ret = i2c_bitbang_wait_ack(priv);
+
+              if (ret < 0)
+                {
+                  return ret;
+                }
+
+              i2c_bitbang_set_scl(priv, false);
+            }
+        }
+      else
+        {
+          int j;
+          int k;
+
+          for (j = 0; j < msg->length; j++)
+            {
+              uint8_t data = 0;
+
+              i2c_bitbang_set_sda(priv, true);
+
+              msg->buffer[j] = 0;
+
+              for (k = 0; k < 8; k++)
+                {
+                  i2c_bitbang_set_scl(priv, true);
+                  data |= priv->lower->ops->get_sda(priv->lower) << (7 - k);
+                  i2c_bitbang_set_scl(priv, false);
+                }
+
+              msg->buffer[j] = data;
+
+              if (j < msg->length - 1)
+                {
+                  /* Send ACK */
+
+                  i2c_bitbang_set_sda(priv, false);
+                  i2c_bitbang_set_scl(priv, true);
+                  i2c_bitbang_set_scl(priv, false);
+                }
+              else
+                {
+                  /* On the last byte send NAK */
+
+                  i2c_bitbang_set_sda(priv, true);
+                  i2c_bitbang_set_scl(priv, true);
+                  i2c_bitbang_set_scl(priv, false);
+                }
+            }
+        }
+
+      /* Send stop */
+
+      i2c_bitbang_set_sda(priv, false);
+      i2c_bitbang_set_scl(priv, true);
+      i2c_bitbang_set_sda(priv, true);
+    }
+
+  return ret;
+}
+
+/****************************************************************************
+ * Name: i2c_bitbang_wait_ack
+ ****************************************************************************/
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *priv)
+{
+  int ret = OK;
+  int i;
+
+  /* Wait for ACK */
+
+  i2c_bitbang_set_sda(priv, true);
+  i2c_bitbang_set_scl(priv, true);
+
+  for (i = 0; priv->lower->ops->get_sda(priv->lower) &&

Review comment:
       call the wrapper function mentioned at line 173




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] btashton merged pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
btashton merged pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r560148634



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c

Review comment:
       done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] btashton commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762575258


   I'm curious why you wanted you would want to bit bang it instead of using the hardware?
   (I'm not saying no, just curious why)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r559901483



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =
+{
+  .transfer = i2c_bitbang_transfer
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: i2c_bitbang_transfer
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,

Review comment:
       > Right. Is `spin_lock_irqsave` OK? Not entirely sure when to use it instead of `enter_critical_section`.
   
   You can use spin_lock_irqsave() when the thread cannot possibly suspend itself within the critical section.
   
   If you can tolerate jitter from interrupt processing, sched_lock() will also keep the thread in place, i.e., that it cannot be suspended.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] btashton commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-766295797


   @v01d I can take a look tomorrow. Would you like any scope shots for timing, I would be happy to grab some. What resistor configuration did you end up going with?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-766360051


   I used 2k just because that's what I had here. I also later saw that nRF SDK configured I2C pins with internal pullups, so that might be useful to try. I'm not particularly concerned with strict timing adherence but if you use your scope it would be nice to check how it looks.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-766509991


   Great, thanks


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-766509991


   Great, thanks


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762862610


   > > Thanks for the reviews so far. I will measure pin toggling timings with logic analyzer and see how it is looking right now. I will place 1.5k resistors on the line without a device, for now. If you can help me confirm the current pin handling is safe to hook up a device, I can test with something connected.
   > 
   > @v01d Do you have an analog analyser or a scope? The rise and fall times and may matter.
   
   Only logic analyzer for now. If that looks reasonable I was hoping to just try it and see if it works. If not, I can switch to H0D1 (high drive zero) which has around half rise/fall time according to manual.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r560152808



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =
+{
+  .transfer = i2c_bitbang_transfer
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: i2c_bitbang_transfer
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,

Review comment:
       I think spin lock is OK then. Suspending the thread would break the timings.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] davids5 commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762860109


   > Thanks for the reviews so far. I will measure pin toggling timings with logic analyzer and see how it is looking right now. I will place 1.5k resistors on the line without a device, for now. If you can help me confirm the current pin handling is safe to hook up a device, I can test with something connected.
   
   @v01d Do you have an analog analyser or a scope? The rise and fall times and may matter.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762575512


   BTW, this is yet untested since I'm unsure about proper GPIO handling on nRF52. I understand that the pin should be set to open-drain, but I think nRF52 does not really support this. So my reasoning is to using push-pull and switch between OUTPUT/INPUT when required. Although not sure if that is entirely safe (ie, if I send a 1 and the slave sends a 0). I think the drive setting should take care of that, but I've read something about this still being a bit dangerous:
   
   https://devzone.nordicsemi.com/f/nordic-q-a/28373/nrf52810-open-drain-gpio


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r559893100



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =
+{
+  .transfer = i2c_bitbang_transfer
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: i2c_bitbang_transfer
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,

Review comment:
       Seems like this will need to take a lock to have any guarantee of timing.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#discussion_r560149924



##########
File path: drivers/i2c/i2c_bitbang.c
##########
@@ -0,0 +1,340 @@
+/****************************************************************************
+ * arch/arm/src/nrf52/nrf52_i2c.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/kmalloc.h>
+#include <arch/board/board.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/i2c_bitbang.h>
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_bitbang_dev_s
+{
+  struct i2c_master_s i2c;
+  struct i2c_bitbang_lower_dev_s *lower;
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+  uint32_t delay;
+#endif
+};
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count);
+
+static int i2c_bitbang_set_scl(FAR struct i2c_bitbang_dev_s *dev,
+                               bool high);
+static void i2c_bitbang_set_sda(FAR struct i2c_bitbang_dev_s *dev,
+                                bool high);
+
+static int i2c_bitbang_wait_ack(FAR struct i2c_bitbang_dev_s *dev);
+static void i2c_bitbang_send(FAR struct i2c_bitbang_dev_s *dev,
+                             uint8_t data);
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+struct i2c_ops_s g_i2c_ops =
+{
+  .transfer = i2c_bitbang_transfer
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: i2c_bitbang_transfer
+ ****************************************************************************/
+
+static int i2c_bitbang_transfer(FAR struct i2c_master_s *dev,
+                                FAR struct i2c_msg_s *msgs, int count)
+{
+  FAR struct i2c_bitbang_dev_s *priv = (FAR struct i2c_bitbang_dev_s *)dev;
+  int ret = OK;
+  int i;
+
+  for (i = 0; i < count; i++)
+    {
+      uint8_t addr;
+      FAR struct i2c_msg_s *msg = &msgs[i];
+
+#ifdef CONFIG_I2C_BITBANG_NO_DELAY
+      /* Compute delay from frequency */
+
+      priv->delay = ((1000000 / msg->frequency) / 2) -
+                    CONFIG_I2C_BITBANG_GPIO_OVERHEAD;
+
+      if (priv->delay < 0)
+        {
+          priv->delay = 0;
+        }
+#endif
+
+      /* Send start bit */
+
+      i2c_bitbang_set_scl(priv, true);
+      i2c_bitbang_set_sda(priv, false);
+      i2c_bitbang_set_scl(priv, false);
+
+      /* Send the address */
+
+      addr = (msg->flags & I2C_M_READ ? I2C_READADDR8(msg->addr) :
+                                        I2C_WRITEADDR8(msg->addr));
+
+      i2c_bitbang_send(priv, addr);
+
+      /* Wait for ACK */
+
+      ret = i2c_bitbang_wait_ack(priv);
+
+      if (ret < 0)
+        {
+          return ret;
+        }
+
+      i2c_bitbang_set_scl(priv, false);
+
+      if (msgs->flags & I2C_M_READ)
+        {
+          int j;
+
+          for (j = 0; j < msg->length; j++)
+            {
+              /* Send the data */
+
+              i2c_bitbang_send(priv, msg->buffer[j]);
+
+              ret = i2c_bitbang_wait_ack(priv);
+
+              if (ret < 0)
+                {
+                  return ret;
+                }
+
+              i2c_bitbang_set_scl(priv, false);
+            }
+        }
+      else
+        {
+          int j;
+          int k;
+
+          for (j = 0; j < msg->length; j++)
+            {
+              uint8_t data = 0;
+
+              i2c_bitbang_set_sda(priv, true);
+
+              msg->buffer[j] = 0;
+
+              for (k = 0; k < 8; k++)
+                {
+                  i2c_bitbang_set_scl(priv, true);
+                  data |= priv->lower->ops->get_sda(priv->lower) << (7 - k);

Review comment:
       It would only be a direct call. I can make a define or inline to avoid extra overhead, but it would be mostly cosmetic.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-763085650


   Yes, I will start with the S0D1 mode (standard drive 0, disconnect 1). That should be safe.
   You're right about the pullup, I think 4k7 is the typical value. I think I confused it with the 1k5 pullup on USB.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] v01d commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-762821614


   Thanks for the reviews so far. I will measure pin toggling timings with logic analyzer and see how it is looking right now. I will place 1.5k resistors on the line without a device, for now. If you can help me confirm the current pin handling is safe to hook up a device, I can test with something connected.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] btashton commented on pull request #2717: I2C bitbang driver (and nRF52 implementation)

Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #2717:
URL: https://github.com/apache/incubator-nuttx/pull/2717#issuecomment-766509319


   I did a quick functional test.  I'll try and grab some scope shots later with a shunt for current as well.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org