Re: [PATCH 1/2] slimbus: stream: add stream support

[Srinivas Kandagatla]

Thanks Vinod for the Review,

On 22/06/18 13:50, Vinod wrote:
> 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 >
Yep, will fix it.
> > +/**
> > + * enum slim_port_direction: SLIMbus port direction
>
> blank line here makes it more readable
Sure it makes sense.
> > +/**
> > + * 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?
Yep will fix it.
> > +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..
look like I over done it, I will fix this in next version.

> > +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?We can move to non irqsave variant here, as i do not see a case where 
this list would be interrupted from irq context.


> > +/*
> > + * 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.

This should be just hw_params() where we have more information on the 
audio parameters, I will make this more clear in the doc about this.

> > + */
> > +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?
No, we do not need this to be ATOMIC, will remove this!

> > +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.
Sounds sensible thing, I will give that a try and see!

> > +/*
> > + * slim_stream_enable() - Enable a prepared SLIMbus Stream
>
> Do you want to check if it is already prepared ..?
Yep, I think most of the code needs similar state machine check, I will 
add this in next version.
> > +/**
> > + * 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?
Sure will do, makes it clear!

>