Bcf: Fix sensor shell example

[Ayush1325]

The following commits adds support for samples/sensor/sensor_shell for beagleconnect freedom.

Currently, it seems hdc2010-humidity sensor does not show up on sensor info command but works fine with sensor get so help on that will be appreciated.

[fabiobaltieri]

I think it would make a lot more sense if this was written as a generic "GPIO controlled I2C bus switch" rather than pointing out a specific general purpose analog switch IC.

How about rewording the whole thing so it sounds generic?

[Ayush1325]

I think it would make a lot more sense if this was written as a generic "GPIO controlled I2C bus switch" rather than pointing out a specific general purpose analog switch IC.

How about rewording the whole thing so it sounds generic?

Just to confirm you are talking about the driver right (not the dt-bindings)?
Any naming suggestions (I am terrible at those) ? Maybe gpio_i2c_switch?

[fabiobaltieri]

Just to confirm you are talking about the driver right (not the dt-bindings)?

Well the whole thing, binding name and compatible, driver name, function prefixes, board definition etc. I the board dts should not have been submitted as is in the first place - we normally don't take trees with off-tree bindings but this slipped through I guess.

[Ayush1325]

Well the whole thing, binding name and compatible, driver name, function prefixes, board definition etc. I the board dts should not have been submitted as is in the first place - we normally don't take trees with off-tree bindings but this slipped through I guess.

Well, ti,ts5a2066 compatible string is already present in the bcf dts (which is definitely weird), so not sure whether it is worth changing that.
I guess Zephyr doesn't follow the whole bindings should describe the actual hardware thing from the Linux kernel?
I am happy to make any changes that will suit upstream zephyr. I would just prefer to fix the half broken state of upstream support for bcf.

[fabiobaltieri]

Well, ti,ts5a2066 compatible string is already present in the bcf dts (which is definitely weird), so not sure whether it is worth changing that.
I guess Zephyr doesn't follow the whole bindings should describe the actual hardware thing from the Linux kernel?

Yeah I think it should be changed, you could add multiple compatibles if you really want to but I don't really see a point, it's a generic analog switch, what's the point of having a driver referring to it by part number? It would still describe the actual hardware.

[Ayush1325]

Well, ti,ts5a2066 compatible string is already present in the bcf dts (which is definitely weird), so not sure whether it is worth changing that.
I guess Zephyr doesn't follow the whole bindings should describe the actual hardware thing from the Linux kernel?

Yeah I think it should be changed, you could add multiple compatibles if you really want to but I don't really see a point, it's a generic analog switch, what's the point of having a driver referring to it by part number? It would still describe the actual hardware.

Well, the linux kernel actually does pretty much require the part thing (even in cases where hardware is not important is running some sort of special firmware), but I digress.

I am not familiar enough to say if there is or will be other parts that work exactly the same as this, but I am happy to make the changes if that's what upstream expects.

[fabiobaltieri]

Well, the linux kernel actually does pretty much require the part thing (even in cases where hardware is not important is running some sort of special firmware), but I digress.

At some extent, it's not like you'd expect stuff like a gpio-keys entry to have the compatible for the exact switch p/n for example. But to your point, it's certainly more common in the Linux case, mainly because there ther's a solid infrastructure to handle multiple compatibles for the same drivers and introduce changes in behavior down the road, that's unlikely be the case for something as simple as this though. You have a more specific example to look for? Happy to dig in that direction if you feel like.

I am not familiar enough to say if there is or will be other parts that work exactly the same as this, but I am happy to make the changes if that's what upstream expects.

Dual channel SPST analog switches? I would lay towards a "yes". :-)

[Ayush1325]

Dual channel SPST analog switches? I would lay towards a "yes". :-)

Ok. doesdual_chan_spst_analog_switch seem fine for all terminology? Or maybe dchan_spst_aswitch?

[fabiobaltieri]

Ok. doesdual_chan_spst_analog_switch seem fine for all terminology? Or maybe dchan_spst_aswitch?

An analog switch can be used for many different things, your driver is specifically about switching an i2c line using a gpio based switch regardless of how the thing is implemented so I would go with something like gpio-i2c-switch. @teburd any opinions about this?

[fabiobaltieri]

Cool,few comments, from the design standpoint, I'd be tempted to suggest that this should be on-bus: i2c and be instantiated as a child of the i2c device. That would place it in the right point in the i2c hierarchy, the only problem is that doing that would force you to assign a dummy address to the device, so I'm not too sure about it. @teburd what do you think?

[yperess]

Can anyone tell me why Footprint Delta is failing?

I'm re-running with debug logging

[yperess]

That's a pretty long timeout, can you make it configurable via the Kconfig maybe?

[Ayush1325]

I talked to the orignial authors and it seems the reason for the abnormally long timeout was that they weren't sure about the other delays at the time. I have now made it dependent on the transfer delay (2 * delay + 100us). Should it still be configurable?

[jadonk]

I think the dependency is on the MCU and switch, so a configuration option makes sense with a default definition provided by the platform. For the base configuration, it should be provided and not given a default as it is platform usage dependent.

[fabiobaltieri]

hey thinking about the logic, is this wait needed? I understand the one before the transaction if the switch has any settling time, but then why would you wait BEFORE setting it back? This should be placed after gpio_i2c_switch_disable.

[Ayush1325]

Yes, it can be removed. I have also reduced the gpio toggle delay to 1 us after checking the data sheet according to which the max on/off time is 12.9 ns.

[jadonk]

I believe there was a delay in case there was any propagation time through the controller. The intent was to avoid cutting the bus off before the transaction is completed at the hardware. As long as we are past the ACK, all should be good and no wait after the transaction before flipping the I/O is needed. And, as it seems, the delay time from flipping the I/O and having it propagate to the switch before new I2C transfers can happen seems negligable. Thanks for going through all this effort to clean up the hacks and sorry it slipped through the initial upstream submission.

[fabiobaltieri]

Of cousre, it's a good addition, sure there's more board around that could need this. Also now that I look at it again I realized I missed a misuse of the GPIO API :-) let me put together a quick followup for that.