From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 53EDEC433FE for ; Thu, 17 Feb 2022 07:39:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236424AbiBQHkA (ORCPT ); Thu, 17 Feb 2022 02:40:00 -0500 Received: from mxb-00190b01.gslb.pphosted.com ([23.128.96.19]:49172 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236370AbiBQHj7 (ORCPT ); Thu, 17 Feb 2022 02:39:59 -0500 Received: from codeconstruct.com.au (pi.codeconstruct.com.au [203.29.241.158]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 12EA62A39C7; Wed, 16 Feb 2022 23:39:43 -0800 (PST) Received: from [192.168.12.102] (unknown [159.196.94.94]) by mail.codeconstruct.com.au (Postfix) with ESMTPSA id 105532039E; Thu, 17 Feb 2022 15:39:37 +0800 (AWST) Message-ID: Subject: Re: [PATCH net-next v5 2/2] mctp i2c: MCTP I2C binding driver From: Matt Johnston To: Wolfram Sang Cc: "David S . Miller" , Jakub Kicinski , Jeremy Kerr , linux-i2c@vger.kernel.org, netdev@vger.kernel.org, Zev Weiss Date: Thu, 17 Feb 2022 15:39:37 +0800 In-Reply-To: References: <20220210063651.798007-1-matt@codeconstruct.com.au> <20220210063651.798007-3-matt@codeconstruct.com.au> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.40.4-1ubuntu2 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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