On Fri, Jul 18, 2025 at 12:48:31PM +0200, Wolfram Sang wrote:
> 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.
You can put it at your first patch. Then your driver is first user.
we can create new clean up patch for other drivers after your patch merged.
patch1: Add standard TIMEMOUT value in master.h
patch2: binding doc
Patch3: drivers
new series:
cleanup other drivers.
Frank
>
> > > + /* 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
>
