Skip to content
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.

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

i2c slave driver removed #30557

Closed
sslupsky opened this issue Dec 9, 2020 · 8 comments · Fixed by #30748
Closed

i2c slave driver removed #30557

sslupsky opened this issue Dec 9, 2020 · 8 comments · Fixed by #30748
Assignees
Labels
Breaking API Change Breaking changes to stable, public APIs bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug

Comments

@sslupsky
Copy link
Contributor

sslupsky commented Dec 9, 2020

@pabigot
Reviewing the 2.4 release notes I noticed issue #27303 removed the i2c slave driver. This was the only i2c slave driver available in the tree and its removal without a replacement is particularly troubling since I use this driver. Please put it back.

@sslupsky sslupsky added the bug The issue is a bug, or the PR is fixing a bug label Dec 9, 2020
@pabigot pabigot added Enhancement Changes/Updates/Additions to existing features and removed bug The issue is a bug, or the PR is fixing a bug labels Dec 9, 2020
@pabigot
Copy link
Collaborator

pabigot commented Dec 9, 2020

What do you use it for? Something similar is still present in a sample, you can move it into your project and use it there. Also there's an i2c emulator API that can implements a similar capability.

I don't think it's likely this will be restored as a supported driver because its usefulness is extremely limited and its presence was confusing to many people. If you can provide a more compelling justification for why it belongs in Zephyr that would help.

@sslupsky
Copy link
Contributor Author

sslupsky commented Dec 9, 2020

Well, there is no i2c slave driver or API at present so that is a problem. This driver attempted to address that. I suspect the original author thought that some attention would be given to the i2c slave side of things but that doesn't appear to have ever happened.

In my case, I have two boards running Zephyr that talk to each other using i2c. One is the master and the other is a slave. So, I use this driver for that purpose. It is by no means perfect but until there is something else (better) I highly recommend we leave it in the tree.

@sslupsky
Copy link
Contributor Author

sslupsky commented Dec 9, 2020

Also, this issue is most definitely a bug, not an enhancement. This driver was removed which has broken my application.

@pabigot pabigot added bug The issue is a bug, or the PR is fixing a bug Breaking API Change Breaking changes to stable, public APIs and removed Enhancement Changes/Updates/Additions to existing features labels Dec 9, 2020
@pabigot
Copy link
Collaborator

pabigot commented Dec 9, 2020

Well, there is no i2c slave driver or API at present so that is a problem

? There is an I2C "slave" API, it's implemented in five I2C drivers where the hardware supports it.

What there isn't anymore is a supported "i2c eeprom slave" driver which does nothing but act like an eeprom. But the entire implementation is still in-tree in a sample, where people can use it.

Also, this issue is most definitely a bug, not an enhancement.

It went through a removal process that included community notifications before the removal in #27387. I'm not going to argue whether that was a violation of process. It seems that the "eeprom slave API" was marked as stable in 1.13. If that means it didn't follow process this is a bug. If it did, then restoring it is an enhancement. IMO.

In any case, we should discuss this in the API meeting.

@sslupsky
Copy link
Contributor Author

sslupsky commented Dec 9, 2020

I think you are referring to the four functions that register and unregister a slave driver? I would suggest those are not a slave API, rather, an API for implementing a custom i2c slave API?

The sam0 has i2c slave hardware. I am not aware of any implementation of a slave driver for the sam0 other than the eeprom emulation driver. Is there something else I can use that I am not aware of?

IMO, we should implement an API similar to what TI has done to standardize the interface of their i2c devices. But that is beyond the scope of this particular issue.

@henrikbrixandersen
Copy link
Member

@sslupsky Could you please explain how the I2C EEPROM "slave" driver is being used in your application? As far as I can tell, the I2C EEPROM "slave" driver only exposes an EEPROM-like device to the I2C "master", but provides no API for interaction with the rest of the application running on the I2C "slave" CPU?

@sslupsky
Copy link
Contributor Author

Hi @henrikbrixandersen, the driver also provides an api for the slave. The app running on the i2c slave cpu uses the AT_24 api to "program some data to the eeprom memory". The app running on the i2c master cpu can pull that data from the slave device over the i2c bus.

@henrikbrixandersen
Copy link
Member

Hi @henrikbrixandersen, the driver also provides an api for the slave. The app running on the i2c slave cpu uses the AT_24 api to "program some data to the eeprom memory". The app running on the i2c master cpu can pull that data from the slave device over the i2c bus.

That actually makes the driver usable in a generic way. I missed that part of the functionality back when we discussed moving the driver to a sample.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change Breaking changes to stable, public APIs bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants