Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions boards/arm/beagle_bcf/beagleconnect_freedom.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,6 @@
<&gpio0 30 GPIO_ACTIVE_HIGH>; // SubG TX/RX 0dB
};
};

sens_i2c: sensor-switch {
Copy link
Member

@jadonk jadonk Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

status = "okay";
compatible = "gpio-i2c-switch";
#address-cells = <1>;
#size-cells = <0>;
controller = <&i2c0>;
gpios = <&gpio0 14 GPIO_ACTIVE_HIGH>;
};
};

&flash0 {
Expand Down Expand Up @@ -127,16 +118,25 @@
reg = <0x4>;
};

light: opt3001-light@44 {
Copy link
Member

@jadonk jadonk Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sens_i2c: sensor-switch {
Copy link
Member

@cfriedt cfriedt Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This node is missing a @N peripheral address.

Copy link
Member Author

@Ayush1325 Ayush1325 Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@cfriedt cfriedt Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@Ayush1325 Ayush1325 Dec 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

@cfriedt cfriedt Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@Ayush1325 Ayush1325 Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@cfriedt cfriedt Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@Ayush1325 Ayush1325 Dec 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@cfriedt cfriedt Dec 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jadonk jadonk Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

status = "okay";
compatible = "ti,opt3001";
reg = <0x44>;
};
compatible = "gpio-i2c-switch";
#address-cells = <1>;
#size-cells = <0>;
controller = <&i2c0>;
gpios = <&gpio0 14 GPIO_ACTIVE_HIGH>;

humidity: hdc2010-humidity@41 {
status = "okay";
compatible = "ti,hdc2010";
reg = <0x41>;
light: opt3001-light@44 {
status = "okay";
compatible = "ti,opt3001";
reg = <0x44>;
};

humidity: hdc2010-humidity@41 {
status = "okay";
compatible = "ti,hdc2010";
reg = <0x41>;
};
};
};

Expand Down