On Thu, Apr 18, 2024 at 02:47:18PM +0300, Andy Shevchenko wrote: > On Wed, Apr 17, 2024 at 11:11:46PM +0300, Serge Semin wrote: > > On Tue, Apr 16, 2024 at 10:04:42PM +0300, Andy Shevchenko wrote: > > > On Tue, Apr 16, 2024 at 07:28:57PM +0300, Serge Semin wrote: > > ... > > > > > + if (dwc->direction == DMA_MEM_TO_DEV) { > > > > + sms = dwc->dws.m_master; > > > > + smsize = 0; > > > > + dms = dwc->dws.p_master; > > > > + dmsize = sconfig->dst_maxburst; > > > > > > > > I would group it differently, i.e. > > > > > > sms = dwc->dws.m_master; > > > dms = dwc->dws.p_master; > > > smsize = 0; > > > dmsize = sconfig->dst_maxburst; > > > > Could you please clarify, why? From my point of view it was better to > > group the source master ID and the source master burst size inits > > together. > > Sure. The point here is that when you look at the DMA channel configuration > usually you operate with the semantically tied fields for source and > destination. At least this is my experience, I always check both sides > of the transfer for the same field, e.g., master setting, hence I want to > have them coupled. Ok. I see. Thanks for clarification. I normally do that in another order: group the functionally related fields together - all source-related configs first, then all destination-related configs. Honestly I don't have strong opinion about this part, it's just my personal preference. Am I right to think that from your experience in kernel it's normally done in the order you described? > > > > > + } else if (dwc->direction == DMA_DEV_TO_MEM) { > > > > + sms = dwc->dws.p_master; > > > > + smsize = sconfig->src_maxburst; > > > > + dms = dwc->dws.m_master; > > > > + dmsize = 0; > > > > + } else /* DMA_MEM_TO_MEM */ { > > > > + sms = dwc->dws.m_master; > > > > + smsize = 0; > > > > + dms = dwc->dws.m_master; > > > > + dmsize = 0; > > > > + } > > > > > > Ditto for two above cases. > > ... > > > > > static u32 idma32_prepare_ctllo(struct dw_dma_chan *dwc) > > > > { > > > > struct dma_slave_config *sconfig = &dwc->dma_sconfig; > > > > - u8 smsize = (dwc->direction == DMA_DEV_TO_MEM) ? sconfig->src_maxburst : 0; > > > > - u8 dmsize = (dwc->direction == DMA_MEM_TO_DEV) ? sconfig->dst_maxburst : 0; > > > > > + u8 smsize, dmsize; > > > > + > > > > + if (dwc->direction == DMA_MEM_TO_DEV) { > > > > + smsize = 0; > > > > + dmsize = sconfig->dst_maxburst; > > > > + } else if (dwc->direction == DMA_DEV_TO_MEM) { > > > > + smsize = sconfig->src_maxburst; > > > > + dmsize = 0; > > > > + } else /* DMA_MEM_TO_MEM */ { > > > > + smsize = 0; > > > > + dmsize = 0; > > > > + } > > > > > > u8 smsize = 0, dmsize = 0; > > > > > > if (dwc->direction == DMA_MEM_TO_DEV) > > > dmsize = sconfig->dst_maxburst; > > > else if (dwc->direction == DMA_DEV_TO_MEM) > > > smsize = sconfig->src_maxburst; > > > > > > ? > > > > > > Something similar also can be done in the Synopsys case above, no? > > > > As in case of the patch #1 the if-else statement here was designed > > like that intentionally: to signify that the else clause implies the > > DMA_MEM_TO_MEM transfer. Any other one (like DMA_DEV_TO_DEV) would > > need to have the statement alteration. > > My version as I read it: > - for M2D the dmsize is important > - for D2M the smsize is important > - for anything else use defaults (which are 0) > Ok. Let's follow your way in this case. After your how-to-read-it comment your version no longer look less readable than what is implemented by me. Thanks for clarification. > > Moreover even though your > > version looks smaller, but it causes one redundant store operation. > > Most likely not. Any assembler here? I can check on x86_64, but I believe it > simply assigns 0 for both u8 at once using xor r16,r16 or so. > > Maybe ARM or MIPS (what do you use?) sucks? :-) Interestingly, but asm-code in both cases match.) So the redundant store operation in your C-code gets to be optimized away. -Serge(y) > > > Do you think it still would be better to use your version despite of > > my reasoning? > > -- > With Best Regards, > Andy Shevchenko > >