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

[Vinod Koul <vkoul@kernel.org>]

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?

> +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?

> +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()

> +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...

> +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?

> +
> +	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
 */
-- 
~Vinod