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 2022/01/30 15:46:02 UTC
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5376: i2c: Add TCA9548A I2C Multiplexer
pkarashchenko commented on a change in pull request #5376:
URL: https://github.com/apache/incubator-nuttx/pull/5376#discussion_r795203101
##########
File path: boards/xtensa/esp32/common/src/esp32_tca9548a.c
##########
@@ -0,0 +1,145 @@
+/****************************************************************************
+ * boards/xtensa/esp32/common/src/esp32_tca9548a.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 <stdio.h>
+#include <debug.h>
+
+#include <nuttx/arch.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/tca9548a.h>
+
+#include "esp32_board_i2c.h"
+#include "esp32_i2c.h"
+#include "esp32_tca9548a.h"
+
+FAR struct tca9548a_dev_s *g_tca9448a_devs[8] =
+ {
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL
+ };
Review comment:
```suggestion
FAR struct tca9548a_dev_s *g_tca9448a_devs[8];
```
Is zero initialized by a standard. Only relevant in case if `NULL` is not defined to zero.
Or remove extra spaces and make like:
```
{
NULL,
NULL,
...
};
```
##########
File path: drivers/i2c/tca9548a.c
##########
@@ -0,0 +1,404 @@
+/****************************************************************************
+ * drivers/i2c/tca9548a.c
+ * Driver for the TCA9448A i2c multiplexer
+ *
+ * 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 <stdlib.h>
+#include <fixedmath.h>
+#include <assert.h>
+#include <errno.h>
+#include <debug.h>
+
+#include <nuttx/kmalloc.h>
+#include <nuttx/fs/fs.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/tca9548a.h>
+
+#ifdef CONFIG_I2CMULTIPLEXER_TCA9548A
+
+#ifndef CONFIG_TCA9548A_I2C_FREQUENCY
+# define CONFIG_TCA9548A_I2C_FREQUENCY 400000
+#endif
+
+/****************************************************************************
+ * Private Function Prototypes
+ ****************************************************************************/
+
+/* I2C Helpers */
+
+static int tca9548a_write_config(FAR struct tca9548a_dev_s *priv,
+ FAR uint8_t regvalue);
+static int tca9548a_read_config(FAR struct tca9548a_dev_s *priv,
+ FAR uint8_t *regvalue);
+
+/* Other helpers */
+
+static int tca9548a_select_channel(FAR struct tca9548a_dev_s *priv,
+ uint8_t val);
+
+/* I2C multiplexer vtable */
+
+static int tca9548a_transfer_on_channel (FAR struct i2c_master_s *dev,
+ FAR struct i2c_msg_s *msgs,
+ int count);
+#ifdef CONFIG_I2C_RESET
+static int tca9548a_reset_on_channel (FAR struct i2c_master_s *dev);
+#endif
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static const struct i2c_ops_s g_i2cmux_ops =
+{
+ tca9548a_transfer_on_channel
+#ifdef CONFIG_I2C_RESET
+ , tca9548a_reset_on_channel
+#endif
+};
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+struct i2c_channel_dev_s
+{
+ FAR struct i2c_master_s vi2c; /* Nested structure to allow casting as
+ * public i2c master */
+ uint8_t channel; /* Associated channel on the mux */
+ FAR struct tca9548a_dev_s *dev; /* Associated device */
+};
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: tca9548a_write_config
+ *
+ * Description:
+ * Write to the mux register of TCA9548A.
+ *
+ * Input Parameters:
+ * priv - A pointer to the TCA9548A device structure.
+ * regvalue - The value that will be written.
+ *
+ * Returned Value:
+ * OK on success or a negative error.
+ *
+ ****************************************************************************/
+
+static int tca9548a_write_config(FAR struct tca9548a_dev_s *priv,
+ FAR uint8_t regvalue)
+{
+ struct i2c_config_s iconf;
+ int ret;
+
+ iconf.frequency = CONFIG_TCA9548A_I2C_FREQUENCY;
+ iconf.address = priv->addr;
+ iconf.addrlen = 7;
+
+ ret = i2c_write(priv->i2c, &iconf, ®value, 1);
+
+ i2cinfo("Write to address 0x%02X; register value: 0x%02x ret: %d\n",
+ priv->addr, regvalue, ret);
+ return ret;
+}
+
+/****************************************************************************
+ * Name: tca9548a_read_config
+ *
+ * Description:
+ * Read the mux register from TCA9548A.
+ *
+ * Input Parameters:
+ * priv - A pointer to the TCA9548A device structure.
+ * regvalue - A pointer to a buffer into which data will be received.
+ *
+ * Returned Value:
+ * OK on success or a negative error.
+ *
+ ****************************************************************************/
+
+static int tca9548a_read_config(FAR struct tca9548a_dev_s *priv,
+ FAR uint8_t *regvalue)
+{
+ struct i2c_config_s iconf;
+ int ret;
+
+ iconf.frequency = CONFIG_TCA9548A_I2C_FREQUENCY;
+ iconf.address = priv->addr;
+ iconf.addrlen = 7;
+
+ ret = i2c_read(priv->i2c, &iconf, regvalue, 1);
+
+ i2cinfo("Read from address: 0x%02X; register value: 0x%02x ret: %d\n",
+ priv->addr, *regvalue, ret);
+ return ret;
+}
+
+/****************************************************************************
+ * Name: tca9548a_select_channel
+ *
+ * Description:
+ * Helper function to select a channel in the TCA9548A. The TCA9548A allows
+ * to select more than 1 channel at same time, but here we are forcing it
+ * to only enable a single channel by time. It will avoid collision in case
+ * where two devices with same address are connected to two channels.
+ *
+ * Input Parameters:
+ * dev - Pointer to the (virtual) i2c_master_s.
+ * val - The channel to be selected.
+ *
+ * Returned Value:
+ * OK on success or a negative error.
+ *
+ ****************************************************************************/
+
+static int tca9548a_select_channel(FAR struct tca9548a_dev_s *priv,
+ uint8_t val)
+{
+ if (val > TCA9548A_SEL_CH7)
+ {
+ /* channel not existent/supported */
+
+ return -EINVAL;
+ }
+
+ if ((priv->state & (1 << val)) != 0)
+ {
+ /* channel already selected */
+
+ return OK;
+ }
+
+ /* Modify state and write it to the mux. Selecting a channel always enables
+ * the device
+ */
+
+ priv->state = (1 << val);
Review comment:
```suggestion
priv->state = 1 << val;
```
##########
File path: boards/xtensa/esp32/common/src/esp32_tca9548a.c
##########
@@ -0,0 +1,145 @@
+/****************************************************************************
+ * boards/xtensa/esp32/common/src/esp32_tca9548a.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 <stdio.h>
+#include <debug.h>
+
+#include <nuttx/arch.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/tca9548a.h>
+
+#include "esp32_board_i2c.h"
+#include "esp32_i2c.h"
+#include "esp32_tca9548a.h"
+
+FAR struct tca9548a_dev_s *g_tca9448a_devs[8] =
+ {
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL
+ };
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: board_tca9548a_initialize
+ *
+ * Description:
+ * Initialize and register the TCA9548A Multiplexer.
+ *
+ * Input Parameters:
+ * devno - The device number associated to I2C address, 0=0x70 ... 7=0x77
+ * busno - The I2C bus number
+ *
+ * Returned Value:
+ * Zero (OK) on success; a negated errno value on failure.
+ *
+ ****************************************************************************/
+
+int board_tca9548a_initialize(int devno, int busno)
+{
+ struct i2c_master_s *i2c;
+ uint8_t addr;
+
+ i2cinfo("Initializing TCA9548A!\n");
+
+ /* TCA9548A valid addresses are 0:0x70 - 7:0x77 */
+
+ if (devno < TCA9548A_SEL_CH0 && devno > TCA9548A_SEL_CH7)
+ {
+ i2cerr("Invalid devno: %d! Valid range: 0 to 7!\n", devno);
+ return -EINVAL;
+ }
+
+ addr = TCA9548A_BASEADDR0 + devno;
+
+ /* Initialize TCA9548A */
+
+ i2c = esp32_i2cbus_initialize(busno);
+ if (i2c != NULL)
+ {
+ /* Then try to initialize the TCA9548A */
+
+ g_tca9448a_devs[devno] = tca9548a_initialize(i2c, addr);
Review comment:
should we check if `g_tca9448a_devs[devno]` is not `NULL` before calling init to avoid double init or not?
##########
File path: boards/xtensa/esp32/common/src/esp32_tca9548a.c
##########
@@ -0,0 +1,145 @@
+/****************************************************************************
+ * boards/xtensa/esp32/common/src/esp32_tca9548a.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 <stdio.h>
+#include <debug.h>
+
+#include <nuttx/arch.h>
+#include <nuttx/i2c/i2c_master.h>
+#include <nuttx/i2c/tca9548a.h>
+
+#include "esp32_board_i2c.h"
+#include "esp32_i2c.h"
+#include "esp32_tca9548a.h"
+
+FAR struct tca9548a_dev_s *g_tca9448a_devs[8] =
+ {
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL
+ };
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: board_tca9548a_initialize
+ *
+ * Description:
+ * Initialize and register the TCA9548A Multiplexer.
+ *
+ * Input Parameters:
+ * devno - The device number associated to I2C address, 0=0x70 ... 7=0x77
+ * busno - The I2C bus number
+ *
+ * Returned Value:
+ * Zero (OK) on success; a negated errno value on failure.
+ *
+ ****************************************************************************/
+
+int board_tca9548a_initialize(int devno, int busno)
+{
+ struct i2c_master_s *i2c;
+ uint8_t addr;
+
+ i2cinfo("Initializing TCA9548A!\n");
+
+ /* TCA9548A valid addresses are 0:0x70 - 7:0x77 */
+
+ if (devno < TCA9548A_SEL_CH0 && devno > TCA9548A_SEL_CH7)
+ {
+ i2cerr("Invalid devno: %d! Valid range: 0 to 7!\n", devno);
+ return -EINVAL;
+ }
+
+ addr = TCA9548A_BASEADDR0 + devno;
Review comment:
This part and part in `esp32_i2cmux_getmaster` are really confusing to me. All the time compare with `TCA9548A_SEL_CH`.
I would suggest to rework this into:
```suggestion
if (devno < 0 || devno > (TCA9548A_BASEADDR7 - TCA9548A_BASEADDR0))
{
i2cerr("Invalid devno: %d! Valid range: 0 to 7!\n", devno);
return -EINVAL;
}
addr = TCA9548A_BASEADDR0 + devno;
```
And the same in `esp32_i2cmux_getmaster`
or
```
#define TCA9548A_DEV_CNT (TCA9548A_BASEADDR7 - TCA9548A_BASEADDR0 + 1)
```
and use it in `FAR struct tca9548a_dev_s *g_tca9448a_devs[TCA9548A_DEV_CNT]` and here in check condition `if (devno < 0 || devno >= TCA9548A_DEV_CNT)`.
Anyway even with current logic need a change from `&&` to `||`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org