RE: [PATCH v3 2/4] drivers: dma: sh: Add DMAC driver for RZ/G2L SoC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Vinod,

Thanks for the feedback.

> Subject: Re: [PATCH v3 2/4] drivers: dma: sh: Add DMAC driver for RZ/G2L
> SoC
> 
> On 02-07-21, 11:05, Biju Das wrote:
> 
> > +static void rz_dmac_set_dmars_register(struct rz_dmac *dmac, int nr,
> > +				       u32 dmars)
> > +{
> > +	u32 dmars_offset = (nr / 2) * 4;
> > +	u32 dmars32;
> > +
> > +	dmars32 = rz_dmac_ext_readl(dmac, dmars_offset);
> > +	if (nr % 2) {
> > +		dmars32 &= 0x0000ffff;
> > +		dmars32 |= dmars << 16;
> > +	} else {
> > +		dmars32 &= 0xffff0000;
> > +		dmars32 |= dmars;
> > +	}
> 
> how about using upper_16_bits() and lower_16_bits() for extracting above?

OK Good point. Geert suggested a logic for this and looks fine.

> 
> > +static void rz_dmac_prepare_desc_for_memcpy(struct rz_dmac_chan
> > +*channel) {
> > +	struct dma_chan *chan = &channel->vc.chan;
> > +	struct rz_dmac *dmac = to_rz_dmac(chan->device);
> > +	struct rz_lmdesc *lmdesc = channel->lmdesc.base;
> > +	struct rz_dmac_desc *d = channel->desc;
> > +	u32 chcfg = CHCFG_MEM_COPY;
> > +	u32 dmars = 0;
> > +
> > +	lmdesc = channel->lmdesc.tail;
> > +
> > +	/* prepare descriptor */
> > +	lmdesc->sa = d->src;
> > +	lmdesc->da = d->dest;
> > +	lmdesc->tb = d->len;
> > +	lmdesc->chcfg = chcfg;
> > +	lmdesc->chitvl = 0;
> > +	lmdesc->chext = 0;
> > +	lmdesc->header = HEADER_LV;
> > +
> > +	rz_dmac_set_dmars_register(dmac, channel->index, dmars);
> 
> why not pass 0 as last arg and remove dmars?

OK.

> 
> > +static enum dma_status rz_dmac_tx_status(struct dma_chan *chan,
> > +					 dma_cookie_t cookie,
> > +					 struct dma_tx_state *txstate)
> > +{
> > +	return dma_cookie_status(chan, cookie, txstate); }
> 
> why not assign status as dma_cookie_status and remove
> rz_dmac_tx_status()

OK. will remove rz_dmac_tx_status

> 
> > +static int rz_dmac_config(struct dma_chan *chan,
> > +			  struct dma_slave_config *config) {
> > +	struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> > +	u32 *ch_cfg;
> > +	u32 val;
> > +
> > +	if (config->direction == DMA_DEV_TO_MEM) {
> 
> config->direction is deprecated, pls save the dma_slave_config here and
> then use based on txn direction...

OK, will do.

> 
> > +static bool rz_dmac_chan_filter(struct dma_chan *chan, void *arg) {
> > +	struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> > +	struct rz_dmac *dmac = to_rz_dmac(chan->device);
> > +	struct of_phandle_args *dma_spec = arg;
> > +
> > +	if (chan->device->device_config != rz_dmac_config)
> > +		return false;
> 
> which cases would this be false?

OK. Will remove this as it not needed.
> 
> > +
> > +	channel->mid_rid = dma_spec->args[0];
> > +
> > +	return !test_and_set_bit(dma_spec->args[0], dmac->modules); }
> > +
> > +static struct dma_chan *rz_dmac_of_xlate(struct of_phandle_args
> *dma_spec,
> > +					 struct of_dma *ofdma)
> > +{
> > +	dma_cap_mask_t mask;
> > +
> > +	if (dma_spec->args_count != 1)
> > +		return NULL;
> > +
> > +	/* Only slave DMA channels can be allocated via DT */
> > +	dma_cap_zero(mask);
> > +	dma_cap_set(DMA_SLAVE, mask);
> > +
> > +	return dma_request_channel(mask, rz_dmac_chan_filter, dma_spec); }
> > +
> > +/*
> > +---------------------------------------------------------------------
> > +--------
> > + * Probe and remove
> > + */
> 
> we use
> /*
>  * this style
>  * multi-line comments
>  */

Will change multiline comments like this.

Regards,
Biju





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux