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

Add support for I2C Slave Drivers #5224

Merged

Conversation

superna9999
Copy link
Collaborator

This PR adds support for I2C Slave by:

  • Adding proper I2C slave calls to I2C Controllers
  • Adding I2C Slave Drivers infrastructure and sample
  • Adding test with physical loopback
  • Adding support to STM32 V2 Driver

The Slave support is based on Linux I2C Slave support by Wolfram Sang and documented at https://www.kernel.org/doc/Documentation/i2c/slave-interface

The I2C Slave support is pretty simple and straightforward and it's desired.

No code has been copied, this PR only contains new code written from scratch.

This PR should solve #2956

@superna9999
Copy link
Collaborator Author

I would like @tbursztyka @erwango @ldts to look at this !

Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

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

I quickly went through, it looks quite nice to me at least. Let's see what others have to say.

Beware the necessary {} around one liners on if/else/while/for. That's basically the only difference we enforce in zephyr compared to Linux code style.

include/i2c.h Outdated
*/
__syscall bool i2c_slave_is_supported(struct device *dev);

static inline bool _impl_i2c_slave_is_supported(struct device *dev)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need this.
user/dev should know what the controller he's working against should support.

And, if he is really trying to mess with the wrong controller: i2c_slave_attach() should return -ENOTSUP (or -EBUSY if already used by another slave driver) and that's it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, this call is quite useless indeed

LL_I2C_AcknowledgeNextData(i2c, LL_I2C_NACK);
}

return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you move the logic from line 67 to here, into a dedicated function? just to get a clearer isr handler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was my plan, will do

include/i2c.h Outdated


/**
* @brief Get the I2C Slave device specific funcs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this be really needed?

The controller, once the driver slave is attached knows the functions, and the slave driver obviously too. What will need this? (besides the test app which uses it to initialize some data into the "eeprom").

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my mind it's useful, but there must be a simpler way to achieve the same goal

#
menuconfig I2C_SLAVE
bool
prompt "I2C Slave Drivers"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either use the prompt or not. Don't mix both in the same Kconfig.

Actually, my personal preference would go for just "message to prompt" and not using prompt keyword.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I copied it from i2c verbatim, i will fix this

- 3 INFO, write SYS_LOG_INF in addition to previous levels
- 4 DEBUG, write SYS_LOG_DBG in addition to previous levels

config I2C_SLAVE_EEPROM
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can move i²c slave eeprom stuff into its own Kconfig.eeprom.
If your api gets in - and it will - there will be a lot more i²c slaves I hope here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK

}

/* Program dummy bytes */
ret = funcs->program(eeprom, eeprom_data, TEST_DATA_SIZE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be a way to do this differently.

By exposing a way in the eeprom driver to initialize it with a memory address and the size it's pointing. (so the driver would not own the memory buffer by itself, but an address, etc...). You could have such function in include/drivers/i2c/slave/eeprom.h for instance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm I think I see what you mean, a eeprom_slave function only defined in the eeprom.h header. Indeed, it will simplify a lot.

typedef int (*eeprom_slave_read_t)(struct device *dev, u8_t *data,
unsigned int offset);

struct eeprom_slave_api {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to be inconsistent with

slave_eeprom
eeprom_slave

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, it was in my todo list

set(CONF_FILE "prj.conf ${APPLICATION_SOURCE_DIR}/boards/${BOARD}.conf")
else()
set(CONF_FILE "prj.conf")
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

There exists a convention that board-specific .conf files are named

${APPLICATION_SOURCE_DIR}/prj_${BOARD}.conf

The build system is aware of this convention and will use it automatically to detect board-specific .conf files.

So if you

mv boards/nucleo_f091rc.conf prj_nucleo_f091rc.conf

then you can get rid of this macro and it will just-work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's the same issue I want to solve as in #5200, where I want a commpon .conf and a board specific .conf fragment as @mbolivar suggested.

Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

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

Thanks @superna9999 for working on I2C slave support. I have some initial comments regarding your implementation.

include/i2c.h Outdated
struct i2c_slave_api {
i2c_slave_write_request_t write_request;
i2c_slave_read_request_t read_request;
i2c_slave_write_done_t write_done;
Copy link
Member

Choose a reason for hiding this comment

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

I find names used in the document you refer Linux I2C Slave support by Wolfram Sang more meaningful. What about re-using them. This function can be called write_received.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, seems simpler to reuse them.

include/i2c.h Outdated
i2c_slave_write_request_t write_request;
i2c_slave_read_request_t read_request;
i2c_slave_write_done_t write_done;
i2c_slave_read_done_t read_done;
Copy link
Member

Choose a reason for hiding this comment

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

This one read_processed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

include/i2c.h Outdated
typedef int (*i2c_slave_api_attach_t)(struct device *dev);
typedef int (*i2c_slave_api_detach_t)(struct device *dev);

struct i2c_slave_driver_api {
Copy link
Member

Choose a reason for hiding this comment

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

I believe we should not do this. This struct with all the related content belongs to the API of device drivers built on top of the new I2C slave functionality (e.g. eeprom), not here. By partitioning API like this we break the Zephyr convention. API of the eeprom driver shouldn't really be different (in design) than API of any other Zephyr driver.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking of moving this in a separate i2c-slave.h header. What were thinking ?

Copy link
Member

Choose a reason for hiding this comment

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

There are many other device drivers which relay on I2C API. Sensor drivers would be a good example. Yet, we do not place any sensor related functions in the main I2C API. The drivers which relay on the slave part of I2C API, like eeprom, are no different. We should keep APIs of different drivers clean and do not mix them.

Also, the next driver using I2C slave interface may choose to attach itself to the I2C bus driver during initialization phase and do not support dedicated attach/detach functions. I don't think we should enforce any specific API design on the device drivers using I2C slave interface. It's best to simply remove these functions from I2C API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I got your point, you think I should move these attach/detach to eeprom.h header and make them specific to the slave driver.

Yes I was thinking about auto-attach drivers.

@tbursztyka what do you think about that ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would keep all in i2c.h (slave or master API, it's still i²c APIs anyway)

I don't see how you will register/unregister (these names are nicer than attach/detach actually), if there is no such functions available on the i²c controller side. It's not a slave API here, actually name could be:

  • i2c_api_register_slave
  • i2c_api_unregister_slave

that's semantically better.

auto-attach, why not yes.

Question: it's not going to be possible for an i²c controller to serve 2+ i2c slave drivers, here it's one. Would that make sense to actually handle that use case?

If it does, you will have to take a look at how - for instance - I did that in gpio. The callbacks could be in a dedicated, with a sys_node_t in the beginning.
And the address it would serve as well.

So for the controller it would be a matter of going through that list and find a match on the address, to call the right slave callbacks. What do you think?

Also, it would save the need of passing a void *priv (or context) pointer to the callbacks. Just pass the callback sturct pointer itself, with CONTAINER_OF(), up to the slave driver to get the context back, as it's done in gpio.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes you are right, the controllers should be able to handle both master and slave at the same time.

Today, the API permits adding any number of slaves, it's a matter of the I2C controller driver to handle that, and it's proven in my i2c_virtual driver implementation.

The STM32 handle at least a maximum of 2 addresses, but I think there is some modes to ack a mask of adresses...

Copy link
Member

Choose a reason for hiding this comment

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

As I've already stated I believe we need to remove this structure and i2c_slave_driver_register(), i2c_slave_driver_unregister() functions from the I2C API. Only i2c_slave_register(), i2c_slave_unregister() should stay.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, but maybe you should decide with @tbursztyka !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In either case, would you want them to go in a separate ic2_slave.h header ?

Copy link
Member

Choose a reason for hiding this comment

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

@tbursztyka what's your opinion on presence of struct i2c_slave_driver_api in this file after my clarifying comments?

@superna9999 I personally wouldn't create i2c_slave.h header file but keep everything here. That said I'm not an authoritative voice here.

#ifndef I2C_EEPROM_SLAVE_H
#define I2C_EEPROM_SLAVE_H

int eeprom_slave_program(struct device *dev, u8_t *eeprom_data,
Copy link
Member

Choose a reason for hiding this comment

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

This file should resemble a typical device driver API/header file. E.g. it should contain doxygen documentation for the new EEPROM I2C Slave driver.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed

@superna9999
Copy link
Collaborator Author

Thanks for the initial review, I just pushed an updated version with:

  • cleaned up all the blocking issues
  • changed call names aligned with Linux names (read_processed, write_received)
  • added a new I2C Virtual driver that will communicate only with the attached I2C slaves (up to 128 !)
  • added Kconfig in the test to add configurations for device name for each target

The I2C slave permit to test the I2C Slave on any boards, but I failed to make it work on qemu_x86 since they use userspace threads by default.

@tbursztyka @mnkp about the I2C Slave Driver API and syscalls, I think it should go in a separate i2c-slave.h header, what do you think ?

Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

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

I have some further comments. However, I haven't been able to complete this review yet.

@@ -0,0 +1,214 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

This driver is not really useful for anything else but testing I2C slave EEPROM driver. I think we should move it to the tests/ directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, but where ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

tests/drivers/i2c/i2c_slave_api ?
Nothing prevents you from creating an i2c driver elsewhere than drivers/i2c

We actually do that in tests/net, creating dummy net device drivers to validate some use cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, seems fine for me, I'll move it

@@ -0,0 +1,6 @@
CONFIG_SYS_LOG=y
CONFIG_SYS_LOG_I2C_LEVEL=4
Copy link
Member

Choose a reason for hiding this comment

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

With level 4 - Debug drivers may not be fully functional, e.g. printing stuff from within ISR. Level 4 is only really useful for debugging. We should change it to 3.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Inded, but for the Virtual I2C, it's can be usefull to have them

CONFIG_BOOT_BANNER=y
CONFIG_I2C=y
CONFIG_I2C_SLAVE=y
CONFIG_I2C_EEPROM_SLAVE=y
Copy link
Member

Choose a reason for hiding this comment

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

We should add here

CONFIG_ZTEST=y

and use Zephyr test framework features within the testcase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll try to use this

@@ -0,0 +1,207 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

This file needs to be re-written as a testcase, i.e. it should use zassert_* macros such as zassert_true, zassert_equal, etc.

include/i2c.h Outdated
typedef int (*i2c_slave_read_processed_t)(void *priv, u8_t *val);
typedef int (*i2c_slave_stop_t)(void *priv);

struct i2c_slave_api {
Copy link
Member

Choose a reason for hiding this comment

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

This is not really API of the I2C slave driver. This structure contains a set of callbacks. I think it will be better to call it i2c_slave_callbacks. Current name is a bit confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, but "callbacks" does not seem very used in the zephyr codebase, maybe _funcs ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

callback is ok, gpio use it.

I would add _cb in above typedefs (before the _t, so it would be _cb_t )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

include/i2c.h Outdated
* Slave driver implementation.
*
* @param dev Pointer to the device structure for the driver instance.
* @param address Address of the I2C Slave to match on the bus
Copy link
Member

Choose a reason for hiding this comment

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

Let's shorten it to "Address of the I2C slave".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

include/i2c.h Outdated
*
* @param dev Pointer to the device structure for the driver instance.
* @param address Address of the I2C Slave to match on the bus
* @param funcs The Slave API funcs used by the controller to send bus events
Copy link
Member

Choose a reason for hiding this comment

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

What about changing it to "Callback functions used by the I2C driver to send bus events"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, but I'll keep the "by the I2C controller driver" because it's sent by the controller, through the controller

Copy link
Member

Choose a reason for hiding this comment

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

What about "by the I2C bus driver"? We usually talk about controller in a different context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok

include/i2c.h Outdated
* @retval -EINVAL If parameters are invalid
* @retval -EIO General input / output error.
*/
__syscall int i2c_slave_attach(struct device *dev, u8_t address,
Copy link
Member

Choose a reason for hiding this comment

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

We should use u16_t for the address parameter. Also, let's shorten address to addr to be consistent with existing I2C API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I must add the 10bit address support, and a flag is missing here somewhere, maybe in a dedicated struct containing the context, address and flags.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, actually we do need a flag to set the addressing mode.

include/i2c.h Outdated
/**
* @brief Attaches the provided funcs and address as Slave device
*
* This routine starts the I2C Controller in Slave mode and uses
Copy link
Member

Choose a reason for hiding this comment

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

I would write here:

Enable I2C slave mode for the 'dev' I2C bus driver. The I2C slave will be registered at the address provided as 'address' parameter. Addressing mode - 7 or 10 bit - depends on the current configuration of the I2C bus driver. Any I2C bus events related to the slave mode will be passed onto I2C slave device driver via a set of callback functions provided as 'funcs' parameter.

Most of the existing hardware allows simultaneous support for master and slave mode. This is however not guaranteed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

include/i2c.h Outdated
* @retval -EINVAL If parameters are invalid
* @retval -EIO General input / output error.
*/
__syscall int i2c_slave_attach(struct device *dev, u8_t address,
Copy link
Member

Choose a reason for hiding this comment

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

We also need to provide a proper _SYSCALL_HANDLER in drivers/i2c/i2c_handlers.c file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, even for the i2c driver ?

Copy link
Member

Choose a reason for hiding this comment

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

For all the functions that are preceded by __syscall.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

include/i2c.h Outdated
typedef int (*i2c_slave_read_processed_t)(void *priv, u8_t *val);
typedef int (*i2c_slave_stop_t)(void *priv);

struct i2c_slave_api {
Copy link
Collaborator

Choose a reason for hiding this comment

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

callback is ok, gpio use it.

I would add _cb in above typedefs (before the _t, so it would be _cb_t )

include/i2c.h Outdated
typedef int (*i2c_slave_api_attach_t)(struct device *dev);
typedef int (*i2c_slave_api_detach_t)(struct device *dev);

struct i2c_slave_driver_api {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would keep all in i2c.h (slave or master API, it's still i²c APIs anyway)

I don't see how you will register/unregister (these names are nicer than attach/detach actually), if there is no such functions available on the i²c controller side. It's not a slave API here, actually name could be:

  • i2c_api_register_slave
  • i2c_api_unregister_slave

that's semantically better.

auto-attach, why not yes.

Question: it's not going to be possible for an i²c controller to serve 2+ i2c slave drivers, here it's one. Would that make sense to actually handle that use case?

If it does, you will have to take a look at how - for instance - I did that in gpio. The callbacks could be in a dedicated, with a sys_node_t in the beginning.
And the address it would serve as well.

So for the controller it would be a matter of going through that list and find a match on the address, to call the right slave callbacks. What do you think?

Also, it would save the need of passing a void *priv (or context) pointer to the callbacks. Just pass the callback sturct pointer itself, with CONTAINER_OF(), up to the slave driver to get the context back, as it's done in gpio.

include/i2c.h Outdated
const struct i2c_driver_api *api = dev->driver_api;

if (!api->slave_detach)
return -ENOTSUP;
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't forget the {} ;) (check, but afaik there are many places that require this)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

damn, I must really look better for these

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tbursztyka do you mean here ? I see the {}

Copy link
Collaborator

Choose a reason for hiding this comment

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

now I do as well. nevermind

include/i2c.h Outdated
@@ -175,6 +204,105 @@ static inline int _impl_i2c_transfer(struct device *dev,
return api->transfer(dev, msgs, num_msgs, addr);
}

/**
* @brief Attaches the provided funcs and address as Slave device
Copy link
Collaborator

Choose a reason for hiding this comment

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

register/unregister sounds nicer actually yes.

@superna9999
Copy link
Collaborator Author

superna9999 commented Dec 13, 2017

Hi @mnkp @tbursztyka

I made a big rework to match all your comments:

  • renamed to register/unregister
  • use a single callbacks struct with a slist node like for gpio
  • renamed callbacks to *_requested
  • move i2c virtual to test dir
  • uses ztest now
  • implemented syscalls handlers (now runs into qemu_x86)

@superna9999 superna9999 force-pushed the for-pr/stm32_i2c_slave branch 3 times, most recently from 0bb7082 to 5c913bf Compare December 13, 2017 12:13
*/
#ifndef I2C_EEPROM_SLAVE_H
#define I2C_EEPROM_SLAVE_H

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a doxygen defgroup to contain these APIs (and for them to be displayed in the generated API docs)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

string
option env="ZEPHYR_BASE"

config APPLICATION_BASE
Copy link
Contributor

Choose a reason for hiding this comment

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

I just glanced over this PR without paying much attention to the details, but this APPLICATION_BASE config option should be removed. It's my fault, I added it to the documentation by mistake. I'm trying to fix it here:

#5399

Otherwise I have no issues with this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will fix !

@nashif nashif dismissed stale reviews from mnkp, galak, and andrewboie July 2, 2018 13:04

please re-review

@deepcube
Copy link

deepcube commented Jul 3, 2018

@dwagenk Thank you for your help. Your are correct, there is an error condition. It turns out we send a reset command to the peripheral, and the data sheet says that it will not ACK. If I don't reset the device everything works as it should.

However if I do reset the device, ignore the error, then try the next write, it hangs waiting for the semaphore in stm32_i2c_msg_write(). I see that the semaphore is given in the stm32_i2c_event() and stm32_i2c_error() which are called from the combined ISR. What I can't figure out is why this gets stuck. Here is a screenshot of the reset message in a protocol analyzer:

https://i.imgur.com/NDRAH2K.png

You can see the rising edge labeled as stop which is the error, then one after that which should be the stop. Do you recognize what would cause the problem in the restructured code after this?

Again thank you for your help and explanations.

EDIT: noticed I'm using the combined ISR
EDIT2: I'm stepping through the code now and realized there is much that I didn't understand. Hopefully I can figure it out, but I'd still greatly appreciate any direction.
EDIT3: I also realize this is not the proper place for this discussion. I'll take it to the Zephyr mailing list

@carlescufi
Copy link
Member

@galak @nashif I have just had a meeting with @anangl and @Mierunski. Mieszko will add DTS support for I2C slave once this PR is merged in the next weeks, as part of adding an implementation for our ICs.

@carlescufi
Copy link
Member

@superna9999 if you rebase this I believe we can merge it now.

Daniel Wagenknecht and others added 6 commits July 4, 2018 10:43
In preparation to implementing slave and multi-master
capabilities for STM32 I2C V2 driver move the checks,
whether all messages of a transfer are valid, before
starting the transaction.

This prevents having to abort a transmission, that is
already partly transmitted.

Signed-off-by: Daniel Wagenknecht <wagenknecht@clage.de>
This patch restructures stm32_i2c_v2 drivers Interrupts:
- NACK failures trigger an I2C event interrupt, so move
handling of NACK failure from error isr to event isr.
- Extract logic of interrupt handling to static functions. Use
isr functions (event & error OR combined) to call these. This
reduces duplication between error isr and combined isr.
- Restructure the error interrupt handling, so that it has no effect
when no errors occured and thus can be called by combined isr.
- Change interrupt logic from if/else to pure ifs for each flag.
This reduces code paths and leads to one call of the isr handling
multiple interrupt conditions, if there's more than one I2C
interrupt flags set.
This is the way it's done e.g. in linux kernels (since 4.14)
drivers/i2c/busses/i2c-stm32f7.c

Signed-off-by: Daniel Wagenknecht <wagenknecht@clage.de>
This patchs adds new I2C Syscalls for :
- I2C controllers slave support
- I2C Slave drivers support

I2C Controllers slave support adds 2 new ops to :
- register new slave driver
- unregister slave driver

Slave funcs consists on all the I2C phases :
- read or write request once address matches
- read or write done once the byte has been received/sent
- stop when the trasmit stops

I2C Slave drivers syscall are also added to make new
"I2C Slave" drivers to :
- register them to their I2C controller
- unregister them to their I2C controller

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
Add a new EEPROM simulated driver with all it's build infrastructure.

The EEPROM can be loaded/poked from applications by getting its funcs.

It is multi-instance capable, right now 2 instances are supported by
enabling them in KConfig.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
Signed-off-by: Daniel Wagenknecht <wagenknecht@clage.de>
Add a new test to make I2C transfers between an I2C controller and
an I2C slave driver, either by:
- Using a virtual I2C driver
- Using a physical link and 2 physical controllers

The Physical test uses :
- Two EEPROM Slave drivers each connected to one I2C controller
- Both I2C controllers also act as master and take turns writing
to the opposite EEPROM Slave.
All of this connected physically on the board.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
Signed-off-by: Daniel Wagenknecht <wagenknecht@clage.de>
This patch adds I2C Slave support conforming to the syscalls and funcs,
only for the STM32 V2 I2C Driver for the moment.

It is capable of handling multi-master bus setups.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
Signed-off-by: Daniel Wagenknecht <wagenknecht@clage.de>
@superna9999
Copy link
Collaborator Author

@carlescufi Just did, thanks @Mierunski for taking over, I'll have some time to review it anyway, so you can ping me and add me to the PR

@carlescufi
Copy link
Member

Merging after TSC meeting yesterday and update with @Mierunski today.

@carlescufi carlescufi merged commit c7875b7 into zephyrproject-rtos:master Jul 4, 2018
sslupsky added a commit to sslupsky/zephyr that referenced this pull request Apr 2, 2020
The existing sam0 i2c driver does not support slave mode.

This commit enables slave support for sam0 devices.

The Slave support is based on Linux I2C Slave support by Wolfram Sang
and documented at
https://www.kernel.org/doc/Documentation/i2c/slave-interface

The code for this commit is largely based on code taken from PR zephyrproject-rtos#5224
and the stm mcu implementation.  This was modified  to work with the
sam0 SERCOM perhiperal and integrated into the sam0 driver.

Fix whitespace and comments

Signed-off-by: Steven Slupsky <sslupsky@gmail.com>
sslupsky added a commit to sslupsky/zephyr that referenced this pull request Jul 10, 2020
The existing sam0 i2c driver does not support slave mode.

This commit enables slave support for sam0 devices.

The Slave support is based on Linux I2C Slave support by Wolfram Sang
and documented at
https://www.kernel.org/doc/Documentation/i2c/slave-interface

The code for this commit is largely based on code taken from PR zephyrproject-rtos#5224
and the stm mcu implementation. This was modified to work with the
sam0 SERCOM perhiperal and integrated into the sam0 driver.

A slave must wait for data ready when there is an address match
to ensure the data is valid.

Disable SCLSM (SCL stetch mode) when operating in slave mode
to ensure that i2c ACK's are handled properly.

Fix k_timeout_t.

Signed-off-by: Steven Slupsky <sslupsky@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.