On Fri, Apr 24, 2015 at 06:11:40PM +0100, Mark Brown wrote:
> On Fri, Apr 17, 2015 at 06:46:02PM +0530, Vinod Koul wrote:
> 
> > +static int ssth_acquire_irq(struct ssth_lib *ctx)
> > +{
> > +	if (request_threaded_irq(ctx->irq, ssth_interrupt,
> > +			NULL, IRQF_SHARED, KBUILD_MODNAME, ctx)) {
> > +		dev_err(ctx->dev, "unable to grab threaded IRQ %d, disabling device\n", ctx->irq);
> > +		return -1;
> 
> Don't discard the return code, pass it back.  I'm pretty sure the error
> wasn't -EPERM anyway.  Though...
yes, we will fix it

> 
> > +
> > +	/* initialize IPC */
> > +	ctx->ipc = ssth_ipc_init(ctx->dev, ctx);
> > +	if (ctx->ipc == NULL)
> > +		ret = -ENODEV;
> > +
> > +	/* Now let's request the IRQ */
> > +	ssth_acquire_irq(ctx);
> 
> ..of course we ignore errors anyway.  Why not just inline this?
thats not right, we should do that...

> 
> > +int ssth_dsp_free0(struct ssth_lib *dsp)
> > +{
> > +	int ret = 0;
> > +
> > +	dev_dbg(dsp->dev, "In %s\n", __func__);
> > +
> > +	ssth_ipc_int_disable(dsp);
> > +
> > +	free_irq(dsp->irq, dsp);
> > +	ssth_ipc_free(dsp->ipc);
> > +	ssth_disable_dsp_core(dsp);
> > +	kfree(dsp);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(ssth_dsp_free0);
> 
> free0?
crap, will fix

> 
> > +bool ssth_dsp_is_running(struct ssth_lib *ctx)
> > +{
> > +	bool ret = 0;
> > +
> > +	mutex_lock(&ctx->sst_lock);
> > +	ret = (ctx->sst_state == SST_DSP_RUNNING) ? 1 : 0;
> > +	mutex_unlock(&ctx->sst_lock);
> 
> How does this get used?  The state could change immediately after
> returning.  Also no need for the ternery operator there, logic
> operations generate logic results anyway.
yes you are right, we should hold the lock and check, will fix this

Thanks
-- 
~Vinod