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?
--
~Vinod
