Hi Wolfram,
On Wed, 2022-02-16 at 17:15 +0100, Wolfram Sang wrote:
> So, I did a high level review regardings the I2C stuff. I did not check
> locking, device lifetime, etc. My biggest general remark is the mixture
> of multi-comment styles, like C++ style or no empty "/*" at the
> beginning as per Kernel coding style. Some functions have nice
> explanations in the header but not proper kdoc formatting. And also on
> the nitbit side, I don't think '__func__' helps here on the error
> messages. But that's me, I'll leave it to the netdev maintainers.
I'll tidy up the comments. A filled /* first line is part of the netdev
style.
>
> Now for the I2C part. It looks good. I have only one remark:
>
> > +static const struct i2c_device_id mctp_i2c_id[] = {
> > + { "mctp-i2c", 0 },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(i2c, mctp_i2c_id);
>
> ...
>
> > +static struct i2c_driver mctp_i2c_driver = {
> > + .driver = {
> > + .name = "mctp-i2c",
> > + .of_match_table = mctp_i2c_of_match,
> > + },
> > + .probe_new = mctp_i2c_probe,
> > + .remove = mctp_i2c_remove,
> > + .id_table = mctp_i2c_id,
> > +};
>
> I'd suggest to add 'slave' to the 'mctp-i2c' string somewhere to make it
> easily visible that this driver does not manage a remote device but
> processes requests to its own address.
I think 'slave' might be a bit unclear - the driver's acting as an I2C master
too. It also is more baggage moving to inclusive naming. Maybe mctp-i2c-
transport or mctp-i2c-interface would suit?
Cheers,
Matt
