On Tue, May 7, 2019 at 6:11:36, Vinod Koul <vkoul@xxxxxxxxxx> wrote:
> On 06-05-19, 17:09, Gustavo Pimentel wrote:
> > 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.
>
> Hmm I guess I missed that one. I was actually looking at usage and not
> questioning split :)
>
> > > > +static int dw_edma_debugfs_u32_get(void *data, u64 *val)
> > > > +{
> > > > + if (dw->mode == EDMA_MODE_LEGACY &&
> > > > + data >= (void *)®s->type.legacy.ch) {
> > > > + void *ptr = (void *)®s->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, ®s->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.
>
> ptr is void, so there is no need for casts to or away from void.
>
> > > > +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.
>
> And one more things, we do not need to check errors on debugfs calls,
> and if it fails it fails...
Ok, makes sense. I'll remove all return validation relatively to debugfs
calls.
>
> --
> ~Vinod
