On 21-06-18, 14:40, Srinivas Kandagatla wrote:
This patch adds support to SLIMbus stream apis for slimbus device. SLIMbus streaming involves adding support to Data Channel Management and channel Reconfiguration Messages to slim core plus few stream apis.
From slim device side the apis are very simple mostly inline with other
^^ Bad char >
+/**
- enum slim_port_direction: SLIMbus port direction
blank line here makes it more readable
+/**
- struct slim_presence_rate - Presense Rate table for all Natural Frequencies
- The Presense rate of a constant bitrate stram is mean flow rate of the
^^^^^ Do you mean stream?
+static struct slim_presence_rate {
- int rate;
- int pr_code;
+} prate_table[] = {
- {12000, 0x01},
- {24000, 0x02},
- {48000, 0x03},
- {96000, 0x04},
- {192000, 0x05},
- {384000, 0x06},
- {768000, 0x07},
- {110250, 0x09},
- {220500, 0x0a},
- {441000, 0x0b},
- {882000, 0x0c},
- {176400, 0x0d},
- {352800, 0x0e},
- {705600, 0x0f},
- {4000, 0x10},
- {8000, 0x11},
- {16000, 0x12},
- {32000, 0x13},
- {64000, 0x14},
- {128000, 0x15},
- {256000, 0x16},
- {512000, 0x17},
this table values are indices, so how about using only rate and removing pr_code and use array index for that, saves half the space..
+struct slim_stream_runtime *slim_stream_allocate(struct slim_device *dev,
const char *name)+{
- struct slim_stream_runtime *rt;
- unsigned long flags;
- rt = kzalloc(sizeof(*rt), GFP_KERNEL);
- if (!rt)
return ERR_PTR(-ENOMEM);- rt->name = kasprintf(GFP_KERNEL, "slim-%s", name);
- if (!rt->name) {
kfree(rt);return ERR_PTR(-ENOMEM);- }
- rt->dev = dev;
- rt->state = SLIM_STREAM_STATE_ALLOCATED;
- spin_lock_irqsave(&dev->stream_list_lock, flags);
- list_add_tail(&rt->node, &dev->stream_list);
- spin_unlock_irqrestore(&dev->stream_list_lock, flags);
Any reason for _irqsave variant? Do you expect stream APIs to be called from ISR?
+/*
- slim_stream_prepare() - Prepare a SLIMbus Stream
- @rt: instance of slim stream runtime to configure
- @cfg: new configuration for the stream
- This API will configure SLIMbus stream with config parameters from cfg.
- return zero on success and error code on failure. From ASoC DPCM framework,
- this state is linked to hw_params()/prepare() operation.
so would this be called from either.. btw prepare can be invoked multiple times, so that should be taken into consideration by caller.
- */
+int slim_stream_prepare(struct slim_stream_runtime *rt,
struct slim_stream_config *cfg)+{
- struct slim_controller *ctrl = rt->dev->ctrl;
- struct slim_port *port;
- int num_ports, i, port_id;
- num_ports = hweight32(cfg->port_mask);
- rt->ports = kcalloc(num_ports, sizeof(*port), GFP_ATOMIC);
since this is supposed to be invoked in hw_params()/prepare, why would we need GFP_ATOMIC here?
+static int slim_activate_channel(struct slim_stream_runtime *stream,
struct slim_port *port)+{
- struct slim_device *sdev = stream->dev;
- struct slim_val_inf msg = {0, 0, NULL, NULL};
- u8 mc = SLIM_MSG_MC_NEXT_ACTIVATE_CHANNEL;
- DEFINE_SLIM_LDEST_TXN(txn, mc, 5, stream->dev->laddr, &msg);
- u8 wbuf[1];
- txn.msg->num_bytes = 1;
- txn.msg->wbuf = wbuf;
- wbuf[0] = port->ch.id;
- port->ch.state = SLIM_CH_STATE_ACTIVE;
- return slim_do_transfer(sdev->ctrl, &txn);
+}
how about adding a macro for sending message, which fills slim_val_inf and you invoke that with required parameters to be filled.
+/*
- slim_stream_enable() - Enable a prepared SLIMbus Stream
Do you want to check if it is already prepared ..?
+/**
- slim_stream_direction: SLIMbus stream direction
- @SLIM_STREAM_DIR_PLAYBACK: Playback
- @SLIM_STREAM_DIR_CAPTURE: Capture
- */
+enum slim_stream_direction {
- SLIM_STREAM_DIR_PLAYBACK = 0,
- SLIM_STREAM_DIR_CAPTURE,
this is same as SNDRV_PCM_STREAM_PLAYBACK, so should we use that here?