On 07-12-23, 16:29, Pierre-Louis Bossart wrote:
> Add definitions and helpers for the BPT/BRA protocol. Peripheral
> drivers (aka ASoC codec drivers) can use this API to send bulk data
> such as firmware or tables.
>
> The API is only available when no other audio streams have been
> allocated, and only one BTP/BRA stream is allowed per link. To avoid
> the addition of yet another lock, the refcount tests are handled in
> the stream master_runtime alloc/free routines where the bus_lock is
> already held. Another benefit of this approach is that the same
> bus_lock is used to handle runtime and port linked lists, which
> reduces the potential for misaligned configurations.
>
> In addition to exclusion with audio streams, BPT transfers have a lot
> of overhead, specifically registers writes are needed to enable
> transport in DP0. In addition, most DMAs don't handle too well very
> small data sets.
>
> This patch suggests a minimum bound of 64 bytes, for smaller transfers
> codec drivers should rely on the regular read/write commands in
> Column0.
>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> ---
> drivers/soundwire/bus.c | 77 +++++++++++++++++++++++++++++++++++
> drivers/soundwire/bus.h | 18 ++++++++
> drivers/soundwire/stream.c | 30 ++++++++++++++
> include/linux/soundwire/sdw.h | 76 ++++++++++++++++++++++++++++++++++
> 4 files changed, 201 insertions(+)
>
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index f3fec15c3112..e5758d2ed88f 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -2015,3 +2015,80 @@ void sdw_clear_slave_status(struct sdw_bus *bus, u32 request)
> }
> }
> EXPORT_SYMBOL(sdw_clear_slave_status);
> +
> +int sdw_bpt_close_stream(struct sdw_bus *bus,
> + struct sdw_slave *slave,
> + enum sdw_bpt_type mode,
> + struct sdw_bpt_msg *msg)
> +{
> + int ret;
> +
> + ret = bus->ops->bpt_close_stream(bus, slave, mode, msg);
> + if (ret < 0)
> + dev_err(bus->dev, "BPT stream close, err %d\n", ret);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(sdw_bpt_close_stream);
> +
> +int sdw_bpt_open_stream(struct sdw_bus *bus,
> + struct sdw_slave *slave,
> + enum sdw_bpt_type mode,
> + struct sdw_bpt_msg *msg)
> +{
> + int ret;
> +
> + /* only Bulk Register Access (BRA) is supported for now */
> + if (mode != SDW_BRA)
> + return -EINVAL;
> +
> + if (msg->len < SDW_BPT_MSG_MIN_BYTES) {
> + dev_err(bus->dev, "BPT message length %d, min supported %d\n",
> + msg->len, SDW_BPT_MSG_MIN_BYTES);
> + return -EINVAL;
> + }
> +
> + if (msg->len % SDW_BPT_MSG_BYTE_ALIGNMENT) {
> + dev_err(bus->dev, "BPT message length %d is not a multiple of %d bytes\n",
> + msg->len, SDW_BPT_MSG_BYTE_ALIGNMENT);
> + return -EINVAL;
> + }
Is this a protocol requirement?
> +
> + /* check device is enumerated */
> + if (slave->dev_num == SDW_ENUM_DEV_NUM ||
> + slave->dev_num > SDW_MAX_DEVICES)
> + return -ENODEV;
> +
> + /* make sure all callbacks are defined */
> + if (!bus->ops->bpt_open_stream ||
> + !bus->ops->bpt_close_stream ||
> + !bus->ops->bpt_send_async ||
> + !bus->ops->bpt_wait)
> + return -ENOTSUPP;
should this not be checked at probe time, if device declares the support
> +
> + ret = bus->ops->bpt_open_stream(bus, slave, mode, msg);
> + if (ret < 0)
> + dev_err(bus->dev, "BPT stream open, err %d\n", ret);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(sdw_bpt_open_stream);
can we open multiple times (i dont see a check preventing that), how do
we close..?
Re-iterating my comment on documentation patch, can we do with a async api
and wait api, that makes symantics a lot simpler, right..?
> +
> +int sdw_bpt_send_async(struct sdw_bus *bus,
> + struct sdw_slave *slave,
> + struct sdw_bpt_msg *msg)
> +{
> + if (msg->len > SDW_BPT_MSG_MAX_BYTES)
> + return -EINVAL;
> +
> + return bus->ops->bpt_send_async(bus, slave, msg);
> +}
> +EXPORT_SYMBOL(sdw_bpt_send_async);
Can we call this multiple times after open, it is unclear to me. Can you
please add kernel-doc comments about the APIs here as well
> struct sdw_master_ops {
> int (*read_prop)(struct sdw_bus *bus);
> @@ -869,6 +913,20 @@ struct sdw_master_ops {
> void (*new_peripheral_assigned)(struct sdw_bus *bus,
> struct sdw_slave *slave,
> int dev_num);
> + int (*bpt_open_stream)(struct sdw_bus *bus,
> + struct sdw_slave *slave,
> + enum sdw_bpt_type mode,
> + struct sdw_bpt_msg *msg);
> + int (*bpt_close_stream)(struct sdw_bus *bus,
> + struct sdw_slave *slave,
> + enum sdw_bpt_type mode,
> + struct sdw_bpt_msg *msg);
> + int (*bpt_send_async)(struct sdw_bus *bus,
> + struct sdw_slave *slave,
> + struct sdw_bpt_msg *msg);
> + int (*bpt_wait)(struct sdw_bus *bus,
> + struct sdw_slave *slave,
> + struct sdw_bpt_msg *msg);
do we need both bus and slave, that was a mistake in orignal design IMO.
We should fix that for bpt_ apis
--
~Vinod
