lkml.org 
[lkml]   [2018]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] slimbus: stream: add stream support
From
Date
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!

>

\
 
 \ /
  Last update: 2018-06-25 12:13    [W:0.050 / U:2.956 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site