[DNM] [RFC] drivers: i2c: Add i2c_async for asynchronous transfers

[nordic-krch]

PR content:

• Proposed extension to i2c API:

  • It's is mainly addition of i2c_single_transfer(dev, msg, add, callback, user_data).
  • Extending i2c_msg flags to contain milliseconds delay field which can be used within sequence to delay message execution.
  • Added helper macros for initializing set of message, e.g. I2C_MSG_TXRX()
  • Added function for getting i2c_async instance associated with given device: i2c_get_async(dev)

• Added new module drivers/i2c/i2c_async which wraps queue_operation_manager and async_seq_manager to provide safe and managed, asynchronous access to i2c.

  • Module is a fairly simple wrapper which binds those two modules together and finally uses i2c_single_transfer() to execute item within the sequence.
  • I2C sequence may consist of many i2c_async_msg items and may be constant, e.g. whole sensor initialization procedure can be stored in flash.
  • There are 3 message types:
    • transfer which contains struct i2c_msg
    • delay - sequence is delayed for milliseconds
    • pause - user provides function which is called when message is executed. Sequence is resumed using i2c_async_resume().
  • i2c_async is a generic part and vendor specific driver implements only single transfer functionality (along with configure and getting async instance). This way specific driver is only focused on it's core functionality, all access management, semaphores, etc. is removed from the driver. It is in opposition to current state where each driver is implementing own mechanisms for access control.
  • i2c_async implements z_i2c_async_sync_transfer which is used to provide backward compatibility (it is called by i2c_transfer).

• Modified i2c_nrfx_twim driver to implement i2c_single_transfer

• Modified hts221 sensor driver to show how to use i2c_async. This is temporary of course, just to show new way.

How to control I2C using i2c_async?

Asynchronous access to i2c does not happen through i2c API because there can be multiple context accessing same i2c device. Access must be managed. queued operation manager is created to handle that generic task (see #23333) thus user is actually talking to queued operation manager API using handle to the manager associated with given i2c device. The manager works with queued operations object so i2c operation must contain queued operation structure. i2c operation consists of one or more i2c_msg. Messages build a sequence which is executed by the queue operation manager. Completion of the i2c operation is notified using async notify object (see #23229) which is part of the i2c operation. async notify standardizes methods of notification of completion of asynchronous operation.

Scheduling of asynchronous i2c is performed in following steps:

• create i2c operation (see struct i2c_async_op in i2c_async.h). Helper macros can be used to create it. Operation contains queued operation object and pointer to the sequence (pointer because sequence can be in flash and operation must be in rw memory).
• get i2c_async object to use. Use i2c_get_async(dev).
• initialize async_notify. Use i2c_async_get_notify(op) to get async notify from the operation.
• submit operation queue_operation_submit(mngr, op, PRI)
• wait for notification about the completion

Example code:

/* create an operation with sequence */
I2C_ASYNC_OP_DEFINE(op, I2C_SENSOR_ADDRESS,
		I2C_ASYNC_MSG_TXRX(&reg_who_i_am, 1, &who_i_am, 1),
		I2C_ASYNC_MSG_PAUSE(who_i_am_validate),
		I2C_ASYNC_MSG_TX(reg_ctrl1, 2),
		I2C_ASYNC_MSG_DELAY(3),
		I2C_ASYNC_MSG_TXRX(&reg_conv, 1, conv_data, sizeof(conv_data))
	);

/* async instance */
struct i2c_async *i2c_async = i2c_get_async(i2c_dev);
struct async_notify *anotify = i2c_async_get_notify(&op);

/* get callback notification */
async_notify_init_callback(anotify, i2c_callback);

/* submit operation */
err = queued_operation_submit(i2c_async_get_qop_mgr(i2c_async), &op.op,
					QUEUED_OPERATION_PRIORITY_APPEND);
if (err < 0) {
	return err;
}

Benefits

• operations can be submitted from interrupt context
• backward compatibility
• driver code focused on its functionality (single transfer)
• no need to extend i2c API with generic API (queued operation manager API used directly)
• similar approach can be applied to spi
• standardized asynchronous notification (async_notify)
• possibility to combine mulitple i2c operations into a sequence (e.g. init)
• possibility to pause a sequence (not added yet to i2c_async) which allows to combine operation like trigger sensor, wait for data rdy pin, read sensor into a single sequence.
• possibility to have single sensor implementation for spi or i2c bus with wrapper for creating messages (e.g. SENSOR_BUS_MSG_TXRX(), SENSOR_BUS_OP_DEFINE)

misc

PR is based on unmerged work:

• async_notify refactor onoff to separate async notification, standardize terminology, and add functionality #23229
• queued_operation add queued operation manager #23333
• async_seq Asynchronous sequence manager #23075

hts221 sensor changes commit will be removed from final PR

[tbursztyka]

The example is maybe not the best one (at least the init process: it looses the error check on chip id afaik)
And I did not realize this meant that much change in user code to switch to async, and it will - at this stage - make it painful for maintainers to support both modes (do not forget that some users may not need such advanced feature). I think the most important part to improve is this then.

[tbursztyka]

I would avoid this. Primarily because it's going to be called in interrupt context (I know, gpio does it, but we agreed about that when it was designed to do so). In SPI we use struct k_poll_signal instead for notifying when a call is done (the only difference being that the call does not return -EBUSY but waits until it gets its turn but that's another story). Using such kernel object is I think better since it provides more flexibility and robustness as well as avoiding to run a cb in interrupt context.

[pabigot]

If this is supposed to be an asynchronous API, I'd really like to see it use async_notify rather than assume a callback is the user's desired interface for results.

[tbursztyka]

async_notify would mean such async function would be usable only within the async library. It may not be what all users want. They might want to use a simpler system, thus why using k_poll_signal might be more relevant.

[pabigot]

async_notify (from #23229) is simply an abstraction that describes how the caller wants to be notified that an operation completes. It allows callback, poll (signal), or spin-wait notifications, so can be used from a variety of contexts with or without POLL support.

[tbursztyka]

Sure, however result and flags attribute seem useless (signal owns already a result attribute, and for the cb it could just take a result as parameter). Would be worth having an intermediate structure just for signal or cb then.

[tbursztyka]

async_notify holds 2 useless attributes (result and flags), it can be another one.

[pabigot]

They are not useless. See #23229 (comment). Please provide feedback there if you are unconvinced so we don't have to track discussions in two places.

[nordic-krch]

flags are needed to distinguish between callback or signal. and i advocate for moving result into union (it's needed for spinwait mode): #23229 (comment)

[tbursztyka]

oh yes, missed that entirely. Well, do you really need a interrupt context cb then? :)

[nordic-krch]

yes, i do. I don't want dedicated thread to talk to i2c. If i have signal then i need thread context to wait for the completion and i want to be able to do that from any context (interrupts).

[tbursztyka]

I see why you are doing this: it's easier then to manage async object memory space, as each and every driver would own it directly. But on the other hand, it is an object the driver will never use by itself.
I think this needs to be reconsidered, finding a way to manager async objects directly into the async manager.

[nordic-krch]

i agree. currently i had to do something to get this object. I already made a proposal of different approach: #22409

[tbursztyka]

Could you have a separate patch for all the above? (Am I wrong if I say it could be used on i2c_transfer()? thus why I think it deserves its own patch.)

[nordic-krch]

yes, except for delay parameter which was added to the flags. but you are right, i will extract it to separate patch.

[tbursztyka]

remove 'data'

[pabigot]

• Extending i2c_msg flags to contain milliseconds delay field which can be used within sequence to delay message execution.

I'm not a fan of packing up many features into a single API that would have be be independently supported in multiple drivers. Why should a single-transaction API include a start delay?

Better to have a multi-operation description that steps through a sequence of operations, one of which could be a generic "delay" (for time, or for event) that is interpreted by an service-agnostic multi-operation sequencing engine, while service-specific operations are passed off to a common driver function like process_operation.

[nordic-krch]

@tbursztyka

The example is maybe not the best one (at least the init process: it looses the error check on chip id afaik)

yes, it might be that chip id would need to be taken out, alternatively it can be checked after whole sequence. Another option that sequence manager is supporting is pausing. sequence can be configured so that after chip id read there is a pause callback. User checks chip id and aborts sequence if it is wrong or resumes. This is not yet implemented in i2c_async (supported by async_seq)

[nordic-krch]

@pabigot

Better to have a multi-operation description that steps through a sequence of operations, one of which could be a generic "delay"

I will look into that.

[nordic-krch]

@tbursztyka

And I did not realize this meant that much change in user code to switch to async,

Note that legacy API will keep work and hts221 change was done only to showcase new api. I expect that asynchronous API will go in without any changes in sensors. It will serve for those who wants to use i2c asynchronously (custom driver). It's hard to tell if other vendors switch to that api. Since legacy and async can be mixed, sensor can have most parts common and for example two versions of read out call. However, this is too far into the future as that would require sensor API to be asynchronous.

[pabigot]

API 2020-07-07: Moved from triage to in-progress pending further activity per #23371 (comment)