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
