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

[Vinod Koul <vkoul@kernel.org>]

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?

> +#define RD_PERM					0444

lets not use a macro for these, 444 is more readable :)

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

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

> +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..?
-- 
~Vinod