Re: [PATCH net-next v5 2/2] mctp i2c: MCTP I2C binding driver

[Matt Johnston <matt@codeconstruct.com.au>]

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