boards: beagle_bcf: Move gpio-i2c-switch to i2c bus #65031

Ayush1325 · 2023-11-09T17:24:08Z

#64881 breaks #64693

Instead of reverting, I thought it might be better to move the whole switch to i2c.

jadonk · 2023-11-09T18:49:11Z

Looks good. Thanks @Ayush1325

Ayush1325 · 2023-11-29T11:46:22Z

Just pinging some people who might already be familiar with this
Cc: @vaishnavachath @cfriedt

boards/arm/beagle_bcf/beagleconnect_freedom.dts

cfriedt · 2023-11-29T11:59:08Z

boards/arm/beagle_bcf/beagleconnect_freedom.dts

This node is missing a @N peripheral address.

Hi, is the peripheral address supposed to signify something or is it supposed to just be unique for the same types of nodes?

It signifies the i2c address of the peripheral. In most cases, each peripheral address on an i2c bus should be unique. If the peripheral is being multiplexed (may be the case here?) then it might be possible to use duplicate addresses.

Some i2c peripherals are capable of supporting one of several possible i2c peripheral addresses (configured via wiring specific pins to ground). That way, a particular board could have more than 1 of the same type of i2c peripheral even on the same i2c bus.

@cfriedt I am not sure if it has an i2c address (and if it does, what it might be). The switch in question is a 2 channel analog switch ts5a2066.

cc @jadonk

It looks to not be an i2c device. Probably more of a platform device, because it looks to be connected via gpio.

In any case, if it's not on the i2c bus, then it should probably not be a child of i2c0.

I would suggest undoing this part.

Well, the reason I moved it to i2c was because #64881 commit moved the sensors to i2c.
I think the sensors need to be children of gpio switch (unless I am getting that part wrong). So should I move the sensors back to their original place as well?

Well, the reason I moved it to i2c was because #64881 commit moved the sensors to i2c. I think the sensors need to be children of gpio switch (unless I am getting that part wrong). So should I move the sensors back to their original place as well?

I would definitely say leave i2c sensors as children of the i2c controller.

Devicetree is a bus-centric abstraction. By bus, I mean basically a group of tightly related copper traces on a board, or interconnects in an IC.

The main bus of the Devicetree is memory - namely the addressable range(s) of memory that have meaning to a particular CPU inside of a particular SoC.

PCIe is a bus as well, and so is SPI, and so is I2C. PCIe's address space has a 1:1 mapping to main memory, whereas I2C addresses are either the 7 or 10-bit numbers we use regularly. e.g. 0x44 or @44 or something for i2c. SPI "addresses" are just indexes into an array of GPIO that are used as chip-enables.

Sensors that are connected over the I2C bus would naturally be children of a i2c node (like i2c0).

Technically speaking, GPIO is also a bus, so the gpio-i2c-switch node could potentially be a child of the GPIO device that it is connected to.

Does that make sense?

There isn't really a way to specify an address for a device that is connected via GPIO though (although multiple devices could technically drive the same wire with proper arbitration). Actually, that's kind of the point of this analog switch, I would imagine. To prevent multiple devices from driving voltages on the same wire.

@cfriedt I think I somewhat understand, i2c senors should be defined as children of i2c controller, and the analog switch should be either child of gpio node or be in its original position.
However, I am still not sure how to show that the gpio switch drives the i2c sensors without making sensors as child nodes to the switch. Is there some primitive in device tree or maybe a new property needs to be added to gpio i2c switch bindings.

However, I am still not sure how to show that the gpio switch drives the i2c sensors

Unless there is some dedicated binding for a chip-enable pin (which isn't part of the i2c spec, but may be present on certain sensor bindings), and unless that pin is wired in to the same GPIO in this case, I'm not sure if it's possible to have software (e.g. i2c controller driver, hypothetically) automatically enable it.

It's really easy to have application code set or clear the gpio though - and there is minimal Devicetree involved. In the case that you still want a binding for the gpio i2c switch, you could give it a phandle to the gpio controller and an index representing the offset of the particular driver for the pin.

It's kind of weird that this node is an i2c controller though.

I would just have application code get a phandle to the gpio device and flip it as necessary.

I think that is prone to error and potential race conditions. By defining a bus, we avoid mistakes and enable even shell tools to see the sensor i2c bus properly. Originally, I was looking at this as a bridge, but I can see that by not having an I2C address, it does not look like a peripheral to the primary I2C bus controller. To that end, I think we could use a different property to define the sensor I2C bus controller, rather than make it look like a peripheral. Does that make sense?

dts/bindings/i2c/gpio-i2c-switch.yaml

zephyrproject-rtos#64881 breaks zephyrproject-rtos#64693 Instead of reverting, I thought it might be better to move the whole switch to i2c. Signed-off-by: Ayush Singh <[email protected]>

jadonk · 2023-12-11T17:58:35Z

boards/arm/beagle_bcf/beagleconnect_freedom.dts

 		};
 	};
-
-	sens_i2c: sensor-switch {


OK, it looks like the way I had it before made more sense. I just listed a controller to define this new bus. I think @Ayush1325 might have had a different perspective on it, but I think the previous instance method was fine.

jadonk · 2023-12-11T18:00:57Z

boards/arm/beagle_bcf/beagleconnect_freedom.dts

 		reg = <0x4>;
 	};

-	light: opt3001-light@44 {


It looks like this was in the wrong place. It should go under the sens_i2c or i2c0s I think it was called on other versions.

…m board" This reverts commit 3769938. Now that gpio_i2c_switch is upstream, this hack should be removed. This had the unintended effect of disabling the on-board sensors. The issue was that the board was merged before this driver was upstream, resulting in the issue this commit "fixed". This revert also does not move the bus driver under the controller used by the driver as the sensor bus I2C driver is not an I2C peripheral. See: * zephyrproject-rtos#64881 * zephyrproject-rtos#64693 * zephyrproject-rtos#65031 Signed-off-by: Jason Kridner <[email protected]>

jadonk · 2023-12-12T13:42:34Z

Better to just revert: #66400

cfriedt · 2023-12-12T16:23:49Z

Better to just revert: #66400

Agreed - that actually makes things much clearer. Thanks!

@Ayush1325 - OK to close this PR?

Ayush1325 · 2023-12-13T00:14:11Z

Closing this in favor of #66400

…m board" This reverts commit 3769938. Now that gpio_i2c_switch is upstream, this hack should be removed. This had the unintended effect of disabling the on-board sensors. The issue was that the board was merged before this driver was upstream, resulting in the issue this commit "fixed". This revert also does not move the bus driver under the controller used by the driver as the sensor bus I2C driver is not an I2C peripheral. See: * #64881 * #64693 * #65031 Signed-off-by: Jason Kridner <[email protected]>

…m board" This reverts commit 3769938. Now that gpio_i2c_switch is upstream, this hack should be removed. This had the unintended effect of disabling the on-board sensors. The issue was that the board was merged before this driver was upstream, resulting in the issue this commit "fixed". This revert also does not move the bus driver under the controller used by the driver as the sensor bus I2C driver is not an I2C peripheral. See: * zephyrproject-rtos/zephyr#64881 * zephyrproject-rtos/zephyr#64693 * zephyrproject-rtos/zephyr#65031 (cherry picked from commit 547a75d) Original-Signed-off-by: Jason Kridner <[email protected]> GitOrigin-RevId: 547a75d Change-Id: Ib6a6a0f71d249bc19a312d469f6e41224b70524b Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5119865 Reviewed-by: Yuval Peress <[email protected]> Tested-by: Yuval Peress <[email protected]> Commit-Queue: Yuval Peress <[email protected]> Tested-by: ChromeOS Prod (Robot) <[email protected]>

Ayush1325 requested review from MaureenHelm and galak as code owners November 9, 2023 17:24

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: I2C labels Nov 9, 2023

zephyrbot requested a review from teburd November 9, 2023 17:24

zephyrbot assigned galak Nov 9, 2023

cfriedt reviewed Nov 29, 2023

View reviewed changes

cfriedt reviewed Dec 4, 2023

View reviewed changes

dts/bindings/i2c/gpio-i2c-switch.yaml Outdated Show resolved Hide resolved

boards: beagle_bcf: Move gpio-i2c-switch to i2c

b07e5cb

zephyrproject-rtos#64881 breaks zephyrproject-rtos#64693 Instead of reverting, I thought it might be better to move the whole switch to i2c. Signed-off-by: Ayush Singh <[email protected]>

Ayush1325 force-pushed the bcf-sensor-shell-fix branch from ee5939a to b07e5cb Compare December 8, 2023 05:00

jadonk reviewed Dec 11, 2023

View reviewed changes

jadonk mentioned this pull request Dec 11, 2023

Revert "boards: arm: Fix sensor shell sample for beagleconnect_freedo… #66400

Merged

Ayush1325 closed this Dec 13, 2023

Ayush1325 deleted the bcf-sensor-shell-fix branch December 13, 2023 14:40

boards: beagle_bcf: Move gpio-i2c-switch to i2c bus #65031

boards: beagle_bcf: Move gpio-i2c-switch to i2c bus #65031

Uh oh!

Conversation

Ayush1325 commented Nov 9, 2023

Uh oh!

jadonk commented Nov 9, 2023

Uh oh!

Ayush1325 commented Nov 29, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush1325 Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfriedt Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfriedt Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jadonk commented Dec 12, 2023

Uh oh!

cfriedt commented Dec 12, 2023

Uh oh!

Ayush1325 commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ayush1325 Dec 1, 2023 •

edited

Loading

cfriedt Dec 1, 2023 •

edited

Loading

cfriedt Dec 4, 2023 •

edited

Loading