RE: [RFC v6 3/6] dmaengine: Add Synopsys eDMA IP version 0 debugfs support

[Gustavo Pimentel <Gustavo.Pimentel@xxxxxxxxxxxx>]

Hi Vinod,

On Mon, May 6, 2019 at 13:7:10, Vinod Koul <vkoul@xxxxxxxxxx> wrote:

> On 23-04-19, 20:30, Gustavo Pimentel wrote:
> 
> >  int dw_edma_v0_core_debugfs_on(struct dw_edma_chip *chip)
> >  {
> > -	return 0;
> > +	return dw_edma_v0_debugfs_on(chip);
> 
> who calls this?

The main driver. This was done like this for 2 reasons:
1) To not break the patch #1 compilation
2) Since the code it's to extensive, I decided to break it in another 
patch.

> 
> > +#define RD_PERM					0444
> 
> lets not use a macro for these, 444 is more readable :)

Ok.

> 
> > +static int dw_edma_debugfs_u32_get(void *data, u64 *val)
> > +{
> > +	if (dw->mode == EDMA_MODE_LEGACY &&
> > +	    data >= (void *)&regs->type.legacy.ch) {
> > +		void *ptr = (void *)&regs->type.legacy.ch;
> > +		u32 viewport_sel = 0;
> > +		unsigned long flags;
> > +		u16 ch;
> > +
> > +		for (ch = 0; ch < dw->wr_ch_cnt; ch++)
> > +			if (lim[0][ch].start >= data && data < lim[0][ch].end) {
> > +				ptr += (data - lim[0][ch].start);
> > +				goto legacy_sel_wr;
> > +			}
> > +
> > +		for (ch = 0; ch < dw->rd_ch_cnt; ch++)
> > +			if (lim[1][ch].start >= data && data < lim[1][ch].end) {
> > +				ptr += (data - lim[1][ch].start);
> > +				goto legacy_sel_rd;
> > +			}
> > +
> > +		return 0;
> > +legacy_sel_rd:
> > +		viewport_sel = BIT(31);
> > +legacy_sel_wr:
> > +		viewport_sel |= FIELD_PREP(EDMA_V0_VIEWPORT_MASK, ch);
> > +
> > +		raw_spin_lock_irqsave(&dw->lock, flags);
> > +
> > +		writel(viewport_sel, &regs->type.legacy.viewport_sel);
> > +		*val = readl((u32 *)ptr);
> 
> why do you need the cast?

I can't tell from my head, but I think checkpatch or some code analysis 
tool was complaining about not having that.

> 
> > +static int dw_edma_debugfs_regs(void)
> > +{
> > +	const struct debugfs_entries debugfs_regs[] = {
> > +		REGISTER(ctrl_data_arb_prior),
> > +		REGISTER(ctrl),
> > +	};
> > +	struct dentry *regs_dir;
> > +	int nr_entries, err;
> > +
> > +	regs_dir = debugfs_create_dir(REGISTERS_STR, base_dir);
> > +	if (!regs_dir)
> > +		return -EPERM;
> > +
> > +	nr_entries = ARRAY_SIZE(debugfs_regs);
> > +	err = dw_edma_debugfs_create_x32(debugfs_regs, nr_entries, regs_dir);
> > +	if (err)
> > +		return err;
> > +
> > +	err = dw_edma_debugfs_regs_wr(regs_dir);
> > +	if (err)
> > +		return err;
> > +
> > +	err = dw_edma_debugfs_regs_rd(regs_dir);
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> 
> single return err would suffice right?

By looking now to this code, I decided to remove the err variable and 
perform the if immediately on the function, if the result is different 
from 0 it goes directly to a return -EPERM. 

> 
> > +int dw_edma_v0_debugfs_on(struct dw_edma_chip *chip)
> > +{
> > +	struct dentry *entry;
> > +	int err;
> > +
> > +	dw = chip->dw;
> > +	if (!dw)
> > +		return -EPERM;
> > +
> > +	regs = (struct dw_edma_v0_regs *)dw->rg_region.vaddr;
> > +	if (!regs)
> > +		return -EPERM;
> > +
> > +	base_dir = debugfs_create_dir(dw->name, 0);
> > +	if (!base_dir)
> > +		return -EPERM;
> > +
> > +	entry = debugfs_create_u32("version", RD_PERM, base_dir, &dw->version);
> > +	if (!entry)
> > +		return -EPERM;
> > +
> > +	entry = debugfs_create_u32("mode", RD_PERM, base_dir, &dw->mode);
> > +	if (!entry)
> > +		return -EPERM;
> > +
> > +	entry = debugfs_create_u16("wr_ch_cnt", RD_PERM, base_dir,
> > +				   &dw->wr_ch_cnt);
> > +	if (!entry)
> > +		return -EPERM;
> > +
> > +	entry = debugfs_create_u16("rd_ch_cnt", RD_PERM, base_dir,
> > +				   &dw->rd_ch_cnt);
> > +	if (!entry)
> > +		return -EPERM;
> > +
> > +	err = dw_edma_debugfs_regs();
> > +	return err;
> 
> return dw_edma_debugfs_regs() perhpaps..?

Sure, small traces of old code.

> -- 
> ~Vinod

Regards,
Gustavo