Hi Frank,
> > +#define NDBSTLV0 0x398
> > +#define NDBSTLV0_RDBLV(x) (((x) >> 8) & 0xff)
>
> Can you use FILE_GET()?
You mean FIELD_GET? Probably yes.
> > +#define RENESAS_I3C_MAX_DEVS 8
> > +#define I2C_INIT_MSG -1
> > +
> > +/* Bus condition timing */
> > +#define I3C_BUS_THIGH_MIXED_NS 40 /* 40ns */
> > +#define I3C_BUS_FREE_TIME_NS 1300 /* 1.3us for Mixed Bus
> > with I2C FM Device*/
> > +#define I3C_BUS_AVAL_TIME_NS 1000 /* 1us */
> > +#define I3C_BUS_IDLE_TIME_NS 200000 /* 200us */
>
> Do you have document reference to such timeout value? If it is spec defined
> timeout, please move to master.h and add ref to spec sections number.
They are all in the specs. Will move them.
> > +#define XFER_TIMEOUT (msecs_to_jiffies(1000))
>
> Is it engineer choosen timeout or spec defined? add comments to show why
> choose this timeout value.
Consistency. All current I3C controller drivers use this value. If we
want to improve it, we should do it in a seperate series for all drivers
IMO.
> > + /* Wait for reset completion */
> > + return readl_relaxed_poll_timeout(i3c->regs + RSTCTL, val,
> > + !(val & RSTCTL_RI3CRST), 0, 1000);
>
> All you use customer's readl at other place. here, you should use
> read_poll_timeout(renesas_i3c_reg_read, ...) to keep consistent. check other
> place.
I will use regmap_read_poll_timeout().
> > + pp_high_ticks = DIV_ROUND_UP(I3C_BUS_THIGH_MIXED_NS,
> > + 1000000000 / rate);
>
> 100000000 use NSEC_PER_SEC, check other place.
Ack.
> > + /* Extended Bit Rate setting */
> > + renesas_i3c_reg_write(i3c->regs, EXTBR, EXTBR_EBRLO(od_low_ticks) |
> > + EXTBR_EBRHO(od_high_ticks) |
> > + EXTBR_EBRLP(pp_low_ticks) |
> > + EXTBR_EBRHP(pp_high_ticks));
>
> I feel renesas_i3c_reg_write is too long, renesas_write() should be enough.
It is long, but precise. renesas_write() could mean anything. It would
also be confusing if some functions start with renesas_* only and some
with renesas_i3c_*. But this is already too much bike-shedding for my
taste. I will do the extra work and switch to regmap and hope that the
overhead is not noticable. I hope I can squeeze this in today.
> > +static struct renesas_i3c_irq_desc renesas_i3c_irqs[] = {
>
> const?
Yes.
Kind regards,
Wolfram
