Re: [PATCH v8 5/9] dmaengine: dw: dmamux: Introduce RZN1 DMA router support

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Ilpo, Andy,

ilpo.jarvinen@linux.intel.com wrote on Fri, 8 Apr 2022 15:38:48 +0300
(EEST):

> On Fri, 8 Apr 2022, Andy Shevchenko wrote:
> 
> > On Fri, Apr 08, 2022 at 12:55:47PM +0300, Ilpo Järvinen wrote:  
> > > On Wed, 6 Apr 2022, Miquel Raynal wrote:
> > >   
> > > > The Renesas RZN1 DMA IP is based on a DW core, with eg. an additional
> > > > dmamux register located in the system control area which can take up to
> > > > 32 requests (16 per DMA controller). Each DMA channel can be wired to
> > > > two different peripherals.
> > > > 
> > > > We need two additional information from the 'dmas' property: the channel
> > > > (bit in the dmamux register) that must be accessed and the value of the
> > > > mux for this channel.  
> >   
> > > > +	mask = BIT(map->req_idx);
> > > > +	mutex_lock(&dmamux->lock);
> > > > +	dmamux->used_chans |= mask;
> > > > +	ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0);
> > > > +	if (ret)
> > > > +		goto release_chan_and_unlock;
> > > > +
> > > > +	mutex_unlock(&dmamux->lock);
> > > > +
> > > > +	return map;
> > > > +
> > > > +release_chan_and_unlock:
> > > > +	dmamux->used_chans &= ~mask;  
> > > 
> > > Now that I check this again, I'm not sure why dmamux->used_chans |= mask; 
> > > couldn't be done after r9a06g032_sysctrl_set_dmamux() call so this 
> > > rollback of it wouldn't be necessary.  
> > 
> > I would still need the mutex unlock which I believe is down path there under
> > some other label. Hence you are proposing something like
> > 
> > 	mask = BIT(map->req_idx);
> > 
> > 	mutex_lock(&dmamux->lock);
> > 	ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0);
> > 	if (ret)
> > 		goto err_unlock; // or whatever label is
> > 
> > 	dmamux->used_chans |= mask;
> > 	mutex_unlock(&dmamux->lock);
> > 
> > 	return map;
> > 
> > Is that correct? If so, I don't see impediments either.  
> 
> Yes, and yes, the mutex still has to be unlocked on that error path.
> 
> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>  

I've done the modification, thanks for your feedback.

Thanks,
Miquèl