>> +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?
No, it's an implementation requirement.
We could move this to host-specific parts but then the codec drivers
will have to know about alignment requirements for each host they are
use with. IOW, it's more work for codec drivers if we don't have a
minimum bar for alignment requirement across all platforms.
>
>> +
>> + /* 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
sdw_bpt_open_stream() would be called by the peripheral driver (or
regmap as a proxy). The peripheral driver could also decide to check for
those callback during its probe, but that's beyond the scope of this
patchset.
These checks are just there for paranoia, in case a peripheral driver
uses BTP/BRA on a host where they are not supported.
It's not science-fiction, we see AMD- and INTEL-based platforms using
the same SoundWire-based codecs.
>> + 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..?
there's a refcount preventing multiples BTP streams from being opened.
> Re-iterating my comment on documentation patch, can we do with a async api
> and wait api, that makes symantics a lot simpler, right..?
see reply in previous email, combining open+send is weird IMHO.
>> +
>> +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
This can be called multiple times but it's useless: all the buffers are
prepared in the open() stage. This is the moral equivalent of a trigger
step, just enable data transfers.
>
>> 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
No disagreement. All the routines follow the same template, if we change
one we should also change the others.
The main question as discussed with Charles is whether we want to pass
the 'msg' argument in all routines.
