From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758206AbcHWV5j (ORCPT ); Tue, 23 Aug 2016 17:57:39 -0400 Received: from mail-by2nam03on0085.outbound.protection.outlook.com ([104.47.42.85]:34112 "EHLO NAM03-BY2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755008AbcHWV5g (ORCPT ); Tue, 23 Aug 2016 17:57:36 -0400 X-Greylist: delayed 833 seconds by postgrey-1.27 at vger.kernel.org; Tue, 23 Aug 2016 17:57:35 EDT Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Jan.Glauber@cavium.com; Date: Tue, 23 Aug 2016 23:22:09 +0200 From: Jan Glauber To: Wolfram Sang CC: , , David Daney , Subject: Re: [PATCH v10 3/8] i2c: thunderx: Add i2c driver for ThunderX SOC Message-ID: <20160823212209.GB28936@hardcore> References: <1ead8831e4dd879d452e8bc1a2a33ccba3a74b3d.1465997604.git.jglauber@cavium.com> <20160823203628.GB20872@katana> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20160823203628.GB20872@katana> User-Agent: Mutt/1.5.21 (2010-09-15) X-Originating-IP: [50.233.148.156] X-ClientProxiedBy: CY1PR15CA0011.namprd15.prod.outlook.com (10.163.14.21) To CY1PR07MB2586.namprd07.prod.outlook.com (10.167.16.136) X-MS-Office365-Filtering-Correlation-Id: ea971029-1265-452c-ac02-08d3cb9b8c6d X-Microsoft-Exchange-Diagnostics: 1;CY1PR07MB2586;2:u6a/qQ1Lz0yYHcC/Xsa6wkX+6iZVvGW4rov9PLBv6EAxSF60Z9HIMvEa0aTutYw894/6TLz+6Njy60ygRIvGLx/hHJf0ZwJWd6TZr85oLNbmKfKH2tPFrCLnlqG/ZKH3uY/9ZrkSwfg2XZLd95CjwUspaZBXm1DrRt6GDPJGKQs21O+QjXv6YBEhdz6DNm+q;3:s6helAQjNxGWoav+6iL/gFaleEfTdYDS77m7BVJL4cYi4myJAz46edyrhpY2iFQKz/E668M33sVIJHEgCczefPkUBp+rwhscp0/pgOlBkpsEIMu1WOqtxpMfoEp5A3sd X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:CY1PR07MB2586; X-Microsoft-Exchange-Diagnostics: 1;CY1PR07MB2586;25:yP1SkYJ0QEpY/0NH+a9267xvvhz5N1uypN/O8kygiGIgdTnn5GlDVsKXT8OOQ7jphmJ1bAnQgfSbtQMALRaSSVuZ43+lfaOD/UCZjBlhR91C6G91d6IcwU7lK80bqEU7VuvMtkTlRbCZGa8TXjq1NWWtaAQZyo8om/nbHnO0rKant5j3ePFYN6RWNr8SceEvgzpxOmL6EwxI9eTZ5OrmtQ39EmAWcZC0W3Qnsu/yu5TtRrgL4Z69IgevWfNMLzWNwZjXpQf3imSMCsv4Lgbc2joTddn8Smv/MSiW0wbRC0ylJvTuUrFJFC6LQcp78WBwN/4YsCztNWOEGMs8+Z8Xi0YDRNyZ6EpjEhRJg8GSmHRLj64yLnDhqs69CHvnH83gNiAL9+fizvirIH7lxzc29NcP78xKPKwi+n9tvGkmDNN/timl/5fdg6usWvBDzfGd8/lDTeKnTr+SmpU5/g59Zc5EmO9U0klpC5aJ0n1jYNItQnlzd0+Nf8q4/p7kWfDS70DhUBxfwAzIYP5qvVoLeEMUJ4Iykg1OlxBLno7ztoDheeCGGAtFf7QYlELikJnXpGpTbm0yKjZOs4FkoyFidZnpSxDsLbQA7yerCzmiymaj0acAi1d5pKs1Z/2CBSbJyHxRAxgdAxjW1nh4L/JRKKA+3+iu2ZQwdHVAB63WdV1DpfW4qgBPG7lX0y6L/UHahkOa1lpp7FFyiGSl2x8m9Q==;31:GYzg3fJiVR4QKu1+C3nq2mHITESW7T63Us9UF2STlBJRzs/M1E5aQc2wAjnYvQlBMLpLi6BXBSY3SzbIJ5FIUVUZSQN/P8FTrkmE820Fx927sADhQcxD3EbyQXZyoVONaFoSIr24Xm9ZSLu3QBl2tg18y7+6+uDCFqBuDGho5fLT2LNF4RFK0ArrEU1ydaW8gR+tpxJn6XSNsHfsVU2kX+S1IXUtF1jHJ9+zbul1Izk= X-Microsoft-Exchange-Diagnostics: 1;CY1PR07MB2586;20:w+f3Y9WtAreBeVmD37Yz7cDIw14+VT6Ykq/UZB09GVXnDWOar7pOblfXCF1F7zX8HAKEi8RGrLQLMIvdsA0HAoOLxbgnkskLy1wJPIDQT4DzpbjZYKM0DhG/psaZRdmhx02NvepEPpmaAucxbxy45p0Dt7SziAOW0/qqai/23lKqD4UbuXBdoGzw83UWU7llJFjDEt0hOeCfNERBotmyuJk2hoe4ti5TsyXeOJpWU2SW0UutPSXpPwaz/eXvouXaFcl0M+ymQ7ALH/ed+nPlvdzi1ys/gLPKLNA1Uv8k4Ig5TEi4YC+RGaj7k7+Uq21/Zk07cBV3EgYOMffxrshqTOdp8k9rkxY5vD+pE2tf7F6KM9uFz92pJE3dBBGR+cifLe2L+qNpuIqDGTCv/dPCWHyaQJfMtFAKZ4W+ywu2IOw6hHsf40n6B2m4gp+FpnGWk+ups28IOqsl5HqJ2fdS/8IwcybthwvZ+7bMzR/C36mX9qoMMn3wGleejFzhJRGloPS9NwIPyRccxMIOtQvmI22np57JI/nv0Z/NP3HQEqxOSMaW2iOePGUm5hiZAKkAMnA9gfDGCFDszdkjS+W09h7zZCmdTms3BrbjqRrbd6M= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(211171220733660); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040176)(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046);SRVR:CY1PR07MB2586;BCL:0;PCL:0;RULEID:;SRVR:CY1PR07MB2586; X-Microsoft-Exchange-Diagnostics: 1;CY1PR07MB2586;4:hyJXaJxcC7C7e4FDx+6HEFxcZbpy5qvCnbcP1s0Sy5XzJaWR5WQVvLDy21ueZweW9Ynv+NZIQL9qGaTme3bHxNAZYYWjFeHmMOhGxkpbHDR6ZOcsFww0JhCFk1Pvpktq3e0A/DSYA7pktqUnLbKRBimp+D8FXfjGVJZcTLOR1K/9wf+XakrW16CdaGtt6xaV1BmRtuEbd84B2Aw87gzuqT8gKaxqUAasVjNhQqROo9rnlXFM5esRAgDRmlqmuZPqSraHowk0/ZcYbKwAZUxqxgkqCU5SY5FKShVakpjDhy+hqloiXUmegGIT+XVtEnmXY5Ixq7UbZG0E3XmYbA5DSuaDXofMVh5DItbJHAr/+pbDea1YLY7XZLn+IqHXlC7h4hBXt64aucd/uqcVS7Qwmflw54Ep48GMX0nUeTGeIV00m+NaYpJ1kH1twCqiuA3u X-Forefront-PRVS: 004395A01C X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4630300001)(6009001)(7916002)(55674003)(199003)(189002)(24454002)(81156014)(8676002)(586003)(101416001)(3846002)(4001430100002)(92566002)(5660300001)(97756001)(83506001)(106356001)(68736007)(9686002)(81166006)(76176999)(50986999)(110136002)(4326007)(7736002)(42186005)(7846002)(2906002)(305945005)(47776003)(33716001)(2950100001)(4001350100001)(19580395003)(33656002)(19580405001)(97736004)(107886002)(46406003)(77096005)(105586002)(54356999)(6116002)(189998001)(66066001)(50466002)(23726003)(1076002)(341764005)(18370500001);DIR:OUT;SFP:1101;SCL:1;SRVR:CY1PR07MB2586;H:hardcore;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;CY1PR07MB2586;23:CE6+fX3H/2D9PZvR5VD+srxZXuKtR/mBSIRKYQio7?= =?us-ascii?Q?IX0f95xxfHobt6jq32GNqLtHUvpsP86bgC+pQ4R3YHsUlXSrYBhUaVtavCI4?= =?us-ascii?Q?kwXF4IcHb51NfplgZSsdgtAnqB2lmoFPQ2Mfc56itZwLDhfcTxwZCmcqVUjH?= =?us-ascii?Q?+cEkn1RXBAEpYqTvnRdGCRldYTvjIBFYcEKhP1j5WPgx4BPeW9+aWbCji2bb?= =?us-ascii?Q?8Zz82/cQaUYEBAw6BiFyh3r4iIYqMjHZl28dFTYWP9WK4lC/m7qZ0v0gMBGm?= =?us-ascii?Q?3ehcMAJ1RgAP8fWeyu9sckVepduABckgHYYLVDID+HjCj+uEOqhdTPQkY6mh?= =?us-ascii?Q?PHNxHB/ZkRbiGY3Li5xMB3G9MW0zJKbf0WnvVc50Eib2aI2441plqjF9x6wl?= =?us-ascii?Q?UHt8jQwrVJse0lF/lSa/5SVjxoTk+piw4IINEsRduPtSSmhjG8Yzn0zcc/9L?= =?us-ascii?Q?6+W+frzi1qdqgYMrV6QfTL2bTmpS+Z+Kg/SewUzqsnPyrgb4N5mMO2ZnrNTH?= =?us-ascii?Q?lLSQXjvsftlhS/l8wbjeEZGzZ15mGXsFL1NwuYnjzP4waUdHZOQCW8S43scx?= =?us-ascii?Q?KZ2H/mxhOLL9BAr4k0ePlcQa7C1b4WhUYOTdClAouqfmAs7erNmSfcfpfNqB?= =?us-ascii?Q?7kM8C7qvhlC8GGWiFPBft2TM3YVefPFHtlYDtewhfPt/JyEl8ZzsxKX4AKCj?= =?us-ascii?Q?sTYKjw03gyflTKfxShp2/WxN7KzWATmb+6W8X0QO6r3MPpHVhJQVfhTz6Pz2?= =?us-ascii?Q?IB0864xr7JZl8Jm43SvnMmxA4LvIwb1dVBxwmK0Sxli92UreoUCSjzqN/gxu?= =?us-ascii?Q?ZjjtiYb8SVe4mKwGvCDU2dy1MctCwB6wA8sa2Bcn+f0ZxlujWvoTthLNnfxz?= =?us-ascii?Q?XfZH5MahUuAj4+SsS0bMVPy+AI6YbUbkckNOpIUXqvKOQ4AfmgDENuT5yqlu?= =?us-ascii?Q?ceOOS4X1ejKO57xoD9pk5bhTfkYI6qOBer/E3Gs3DqoxPHVq7J+xAI+4Vb5D?= =?us-ascii?Q?XrtJrPZQ0Ijt+K71u9E7jHrP2h9HW7eXcQMNszP2+qBS3xJoJRQAehbAX4T8?= =?us-ascii?Q?O6lyG2VSDSNo/8Z/sNiq/W/jpjIo0FP+gJZ4K/eKy+vgzDYuhfEHZ0s29mp9?= =?us-ascii?Q?6zyJZT6mBuYPgKIWjOfZ+i/MOeEtk+H1uwtm5AonPXFIElDpqLGhJYCs4xXC?= =?us-ascii?Q?rQBGnupOiQ/Ll3MEfFzMRP31ahuSYfarSpAhzSzLBDpPVJIt0rmAVGjIGd8t?= =?us-ascii?Q?nUyeub7U7NtV/xLN2ZHQx6WyhWUomc38iWyB5UUTz2UjZFQmmMqzWaPqxi7N?= =?us-ascii?B?Zz09?= X-Microsoft-Exchange-Diagnostics: 1;CY1PR07MB2586;6:qy4/YdP39RXiVL3L74SDSRHsUVMX/cwGcxdmmXZatUld7vh12FzTLJH98fY3J8J6ff6C+Kzu8xZCDT0Tz/l8s0OuusYRWwN2JqldWFnZXZVYGd2J/LPwNqP0/eO30eb+fscfm6LzrF9GQUd5VrqxwCNvQkgXdeeExzT6jwK+rm6yP3PwjN3ukw7tuA8DFN4IqBB86LAeRa5VTJH/qv95GpNxuLC2OJDHNz0gQLUAiHTAdehb3NBtrjQqQ6wt/gndiRM2X+8Lx45DS544sH0yK7uoGGULSB8vuegynuoLwDE=;5:VWE6B82q6uSNolpJnQC+prqbWTBV+NViTHK0fUlLl5BglKz4aH6/NdkNaxO76nwoV/QAJYo3QC2vai+uXULH76aPLpTNuSIGGs4qmtfw8empOFrJyr3t+VzcO/StmD/HTOQxHYkFJ0s22so9dJNajw==;24:FJL8u9uOjM7eeMKR5fpHrU9IU2QVP2CFQ04CYHezTz1Xuymuob95hV39YYG0/mGYP7wvsLNN7Tl/kGYFnhXaUyZMVZr95mG/fTg+2SewYYI=;7:69uW1lWjIWTTSndaO4ZXFCM/J46cynEAOAfFUI+CeXvrp/LJS/mtjjqcPoTdzx9hozacfTenGry4PY6mF+ZZch7rta0Fj0Zi9jDDkblSgi5cOgqFtkIiLy7QH7t+usMCZ7J5mxuEvdG9b1/HZtHNQVDqNxB5yFfwvbnS8mT+DhTZGnZINN6UnIaq3ZuFERsDGmkbl7lo1w/IjeSw9YfD4QswbcgzfIMp/YwY2aVqfSF1uTkvFMK0a+TAcOQ2np0Y SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: caviumnetworks.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Aug 2016 21:22:12.3096 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR07MB2586 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Aug 23, 2016 at 10:36:29PM +0200, Wolfram Sang wrote: > > > i2c-octeon-objs := i2c-cavium.o i2c-octeon-core.o > > obj-$(CONFIG_I2C_OCTEON) += i2c-octeon.o > > +i2c-thunderx-objs := i2c-cavium.o i2c-thunderx-core.o > > +obj-$(CONFIG_I2C_THUNDERX) += i2c-thunderx.o > > Shouldn't that rather be "i2c-cavium-core.o", "i2c-octeon-platdrv.o", > and "i2c-thunderx-pcidrv.o" for the -objs? I mean, the cavium part is > the core-part... > > > +skip: > > + if (!i2c->sys_freq) > > + i2c->sys_freq = SYS_FREQ_DEFAULT; > > + > > + dev_info(dev, "Set system clock to %u\n", i2c->sys_freq); > > Too many dev_info IMO, see later. OK. I was working on an update where I dropped most of the messages. > > + i2c->adap.class = I2C_CLASS_HWMON | I2C_CLASS_SPD; > > Do you really need that? Not sure, just copy'n paste from what most of the other bus drivers do. Looking at i2c-core.c it might be a legacy thing, so I can probably remove it. > > + ret = i2c_add_adapter(&i2c->adap); > > + if (ret < 0) { > > + dev_err(dev, "Failed to add i2c adapter\n"); > > I2C core does error reporting for you meanwhile. OK. > > + goto out_irq; > > + } > > + > > + dev_info(i2c->dev, "probed\n"); > > I'd think all nice to know dev_info should go here in condensed form. > > > +out_irq: > > + devm_free_irq(dev, i2c->i2c_msix.vector, i2c); > > If you need to free irq manually here anyhow, then you don't need the > devm variant. Yes. I paid attention to the devm documentation in the meantime and switched all probing to the managed fucntions and got rid of the goto's completely. > > +out_msix: > > + pci_disable_msix(pdev); > > +out_unmap: > > + iounmap(i2c->twsi_base); > > + thunder_i2c_clock_disable(dev, i2c->clk); > > +out_release_regions: > > + pci_release_regions(pdev); > > +out_disable_device: > > + pci_disable_device(pdev); > > +out_free_i2c: > > + pci_set_drvdata(pdev, NULL); > > Similar to devm_*, there is also pcim_* for PCI devices. You might want > to have a look for those. See above. > > + devm_kfree(dev, i2c); > > Not needed. Yes. > > + return ret; > > +} > > + > > +static void thunder_i2c_remove_pci(struct pci_dev *pdev) > > +{ > > + struct octeon_i2c *i2c = pci_get_drvdata(pdev); > > + struct device *dev; > > + > > + if (!i2c) > > + return; > > How should that happen? Just a safety net, can be removed. > > + > > +module_pci_driver(thunder_i2c_pci_driver); > > + > > +MODULE_LICENSE("GPL"); > > Please add a minimal GPL header at the top of the file, so it is clear > if you mean "v2" or "v2 or later". OK. > > +MODULE_AUTHOR("Fred Martin "); > > +MODULE_DESCRIPTION("I2C-Bus adapter for Cavium ThunderX SOC"); > > -- > > 1.9.1 > >