Hi Stefan,
> > ... and this into the for-loop? I'd think it is a tad more readable, but
> > it is a minor nit.
>
> Do you mean this way:
>
> for (len = pmsg->len; len > 0; len -= 8, j += 8) {
>
> ?
with 'j = 0' added, yes.
> > > +
> > > +static const struct i2c_algorithm mtk_i2c_algo = {
> > > + .master_xfer = mtk_i2c_master_xfer,
> > > + .functionality = mtk_i2c_func,
> > > +};
> > > +
> > > +static const struct of_device_id i2c_mtk_dt_ids[] = {
> > > + { .compatible = "mediatek,mt7621-i2c" },
> > > + { /* sentinel */ }
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(of, i2c_mtk_dt_ids);
> > > +
> > > +static void mtk_i2c_init(struct mtk_i2c *i2c)
> > > +{
> > > + i2c->clk_div = 40000000 / i2c->cur_clk - 1;
> >
> > What is 40000000? Maybe a define for this magic value?
>
> Seems to be the clock input frequency. I'll add a define.
Ah, I got confused because I thought cur_clk was the input clock and did
not double check. Renaming 'cur_clk' to 'bus_clk' or better
'bus_freq' seems like a good idea?
> > And no protection if cur_clk is 1 and thus the divider is 0!
>
> The divisor is "cur_clk" and not "(cur_clk - 1)".
Uhm, yes :)
> > > + adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> >
> > Why do you want the classes?
>
> No idea, sorry. I have no clue what the intention of the original
> authors might have been. Perhaps just some copy-and-paste? SPD is
> very unlikely as there surely are no DIMM's installed on any
> MT76xx platforms.
Then remove it. This is to *limit* potential clients and I don't see a
reason why to limit here. I guess you want to allow all.
> Many thanks for the review. Very much appreciated.
You are welcome. Thanks for keeping at it.
Kind regards,
Wolfram
Attachment:
signature.asc
Description: PGP signature
