Hi Frank,
thanks again for the super-fast review.
> > - RENESAS_I3C_MAX_DEVS is a constant currently. So, we could get rid of
> > the 'maxdevs' variable. I tend to keep it this way in case future
> > controllers may use a different value, then we can change it easily.
>
> It is similar ADI version. Can you simple descript hardware behavor to show
> why need RENESAS_I3C_MAX_DEVS,
Yes, for example this register has basic status info per target:
> > +#define DATBAS(x) (0x224 + 0x8 * (x))
And there are only 8 of these registers. So, there is a maximum of 8 for
this controller. We could hardcode 8. But we could leave the handling as
is, just in case a future controller has more or less of these
registers.
> > - For accessing the FIFOs, this driver uses the same code as existing
> > upstream drivers or the recenlty submitted "ADI" driver. There, the
> > question came up, if this could be a helper function?
>
> Add common header in drivers/i3c/master/core.h implement inline helper
> function
Sure thing. I think I didn't get feedback on my original suggestion so
far. If I now know you are positive about it, I will give it a try.
> #define PRTS 0x00
> #define PRTS_PRTMD BIT(0)
>
> Add extra space help distinguish register and field define.
Okay.
>
> > +
> > +#define BCTL 0x14
> > +#define BCTL_HJACKCTL BIT(8)
> > +#define BCTL_ABT BIT(29)
> > +#define BCTL_BUSE BIT(31)
> > +
> > +#define MSDVAD 0x18
> > +#define MSDVAD_MDYAD(x) (((x) & 0x3f) << 16)
>
> Use GEN_MASK
Sigh, if you think this is more readable, then OK.
> > +#define STDBR 0x74
> > +#define STDBR_SBRLO(cond, x) ((((x) >> (cond)) & 0xff) << 0)
> > +#define STDBR_SBRHO(cond, x) ((((x) >> (cond)) & 0xff) << 8)
>
> FIELD_GET FIELD_PREP
Ditto.
> > +static inline void i3c_reg_update(u32 mask, u32 val, void __iomem *reg)
> > +{
> > + u32 data = readl(reg);
> > +
> > + data &= ~mask;
> > + data |= (val & mask);
> > + writel(data, reg);
> > +}
>
> can you move reg to first argument to align below help function?
Yup, coccinelle should make that easy.
> > +out:
> > + kfree(xfer);
>
> you can __free(kfree) xfer = NULL at declear to avoid this goto branch.
I'll give it a try...
> > + if (!i3c_xfers[i].rnw && i3c_xfers[i].len > 4) {
>
> Only pre fill fifo when len >4? what happen if only write 1 byte?
Ehrm, good catch. I will check this in more detail.
> > + renesas_i3c_write_to_tx_fifo(i3c, cmd->tx_buf, cmd->len);
> > + if (cmd->len > NTDTBP0_DEPTH * sizeof(u32))
> > + i3c_reg_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
> > + }
> > +
> > + renesas_i3c_enqueue_xfer(i3c, xfer);
> > + if (!wait_for_completion_timeout(&xfer->comp, XFER_TIMEOUT))
> > + renesas_i3c_dequeue_xfer(i3c, xfer);
>
> This may common problem, I3C spec have 100us timeout, target side may
> timeout when driver wait for irq. So target side treat "repeat start" as
> "start" and issue address arbitration.
There is a specified timeout? I couldn't find one in the specs, can you
kindly point me to it? So, the solution is to use 100us as timeout?
> > + i3c->addrs[pos] = dev->info.dyn_addr ? : dev->info.static_addr;
>
> Are there extension of "?" operator in C ? I remember
>
> dev->info.dyn_addr ? dev->info.dyn_addr : dev->info.static_addr;
Dunno if it made it into the standard these days, but as a GCC extension
it is used in the kernel for ages. I encourage its use in I2C, other
maintainers don't like it much. Mileages vary.
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to request irq %s\n",
> > + renesas_i3c_irqs[i].desc);
> > + return ret;
>
> return dev_err_probe()
OK.
Thanks and happy hacking,
Wolfram
Attachment:
signature.asc
Description: PGP signature
