From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753290AbcGYR0H (ORCPT ); Mon, 25 Jul 2016 13:26:07 -0400 Received: from mail-by2nam01on0070.outbound.protection.outlook.com ([104.47.34.70]:21818 "EHLO NAM01-BY2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753181AbcGYR0E (ORCPT ); Mon, 25 Jul 2016 13:26:04 -0400 X-Greylist: delayed 4778 seconds by postgrey-1.27 at vger.kernel.org; Mon, 25 Jul 2016 13:26:04 EDT Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Jan.Glauber@cavium.com; Date: Mon, 25 Jul 2016 17:51:22 +0200 From: Jan Glauber To: Mark Brown CC: , , "Steven J. Hill" , David Daney Subject: Re: [PATCH 6/6] spi: octeon: Add thunderx driver Message-ID: <20160725155122.GA2710@hardcore> References: <20160724210452.GB6345@sirena.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20160724210452.GB6345@sirena.org.uk> User-Agent: Mutt/1.5.21 (2010-09-15) X-Originating-IP: [109.193.46.58] X-ClientProxiedBy: AM3PR05CA018.eurprd05.prod.outlook.com (10.141.192.28) To SN2PR07MB2591.namprd07.prod.outlook.com (10.167.15.21) X-MS-Office365-Filtering-Correlation-Id: e451cacd-dc3b-4f67-e59e-08d3b4a38d19 X-Microsoft-Exchange-Diagnostics: 1;SN2PR07MB2591;2:bzUjkuPXsOV9bNgSAJwN/tRSi6bi2Ewvtv2OUtjmtcZJ871sCjngUeG8onNe8MQPhfR/Kd426cFl12+CTc4g6Ve8+4OaJyFYcaQwSfgjK4dsaMeB3p9CqOI24xCVv3fWqRqnvs0QBrOcW4mr0slBXZ504Aklk5VaiGKUYUavJzIDvvo/RwPkm1Vh6htXGC0y;3:RaX5BDJ+hMm3BgwDiN1j7BM37hLfkK2joYAYfKwa2eBSnnoALSgC3vCl+mkO9zOX1+MyQI8Hkk90kAS7M7RYOKsv58G1px5ueFf0scTwEbYUGrtZTO37D377rIgaJl0J X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:SN2PR07MB2591; X-Microsoft-Exchange-Diagnostics: 1;SN2PR07MB2591;25:6D4zMOdjo9Cx0bhVVBJ1aiJAu+eZi/lkjX3BCxIbG2LZlZAVaOW10Rr15oQnxo4556Fu27eT0YURAx0KrM2vQEicY5j5+7Ze7n03+MVuXPGfatwUfOXIuzIVS+kO8Kjh3vwqHkAa6wg0do+0pONiGanl7r1VRe71TrOTDu3duYfTQldu49orhCcaYr0PNG2FMdV2XY64Q1Gij9tWFBoHKMUCb9rSKgk8Z00PW7bnigGv2Gw8UdiYMDlv2+24cJZZMj63lvQRIjBrlvwIgwanLArsdFP1Wwadla1qyp3m8pkQDxs1NJFu8avfwmNraej3YQUCNHqEMTsIrNVaPJiI55Vjn/YuT1JTuEC1se2dSpmVGTDh8vb5FeAh8bd7hckhPmLcdBoAhI7dDBMoPlr6wb07623p23n3wVCGb69dEYUQa/yEzm/MJ3MDcU32ZCgQaH7qgNaoHozLoiLnfn27NR8MS+/Pxgdgu4/hvG+Cy1T/HHylrzSiZg7qW599LLpbzjee2Eqw1VwLiAHv3pBkwMB/xSmdjJ/kPmmt7JxMlfhhw+euwH5yyIBAEfbEC7JfwqZmiIJQZPO7t14OWG1utA1qdGKz2ca03sdn8GFxr2pZZMNylFJx2B8YXP6rcLDWZ3bY6RFl9NyHIy7pA2qJ3rT+S3I8ON2UNeonumgqU7kwMGHlyQHKovSv/sN5Hzu7XXedUO8uuC8Es1TideGJ8g==;31:29h22gJISGjBf9baTLViw/jr/5kZ2Hc0le+Fpi/NOi74C5QUkxuOq3jjxp4tevArJWLy0ZYXFGXe0Nq7V7t046I+JAk8llCXsiMMQmx6eCf2Kh/YhHEOc8+ogbga09M/bXiAX8iG5A3zzOA9CI8qJf9WYAkeHBeKA4BRpKk/IyCu6kuqIZoNbxihuxKTuf8Znbqkrlth5H0Abr8yq2rhvQ== X-Microsoft-Exchange-Diagnostics: 1;SN2PR07MB2591;20:s1f84dZdD9UKFvaELiJYmdN87nGzaOXDp7AMe9u94cFCfF5un+QJRpyj0mqt64iy3P+zmXmAFtiCudxfqF46Lu8+SYmUS40s6RilhvFwzIZYqPXI3m0JgJzQbNXSDK+hz5AtXkQEc0srOkWVXhs5/Oe7Po9PrjAyKy/RsleGYg0AmpYn+Fnhr/t/ASZkezc7GLAWltIo389Q19dVaK2QcI4u3+GB/x6hENUU4vvotwxPcgpx26MSuzCYXCm6Ft6KfK2q4T7ndF/bltprn6jWqzAnSglS3q8KPI6NE3oc1mYNlpoYv5gCmjjje9ob6QwiM9ff7U7nlfJ6/Iy2y2Bi49T5F+GZ9VIX4AHwUeuTpZeHGO4rSZj+tETFsKqdNwWT6SU114w5d26hCraUE9yvsWf28qtFAoTzaG06WrN4kd47dwdycNxyB/bubZKSj7Ibd8mCbsHUDDHHn6UuQdBv3aqvffTA+Y/b7bzdcVGS+y4CcxHYugRoasworNoStdGEsaZWNSDPrX35GHV+fN802f11y+vCA1cq6GzkRBFWRrXGKsEnRUxeyIn4a0Hh09kfV/b+p3fbYwwJ+/0KrcsUVWQRll+zQ1+puTaY3h/P+vU= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(8121501046)(5005006)(3002001)(10201501046);SRVR:SN2PR07MB2591;BCL:0;PCL:0;RULEID:;SRVR:SN2PR07MB2591; X-Microsoft-Exchange-Diagnostics: 1;SN2PR07MB2591;4:rNDpddEEE3HWUnrnb8KQMCjlWdscA0Q0B+0Vo69asAD+fGpbh//4MUBxBWO8J+ooe8Yu+oxihFExa7Mp+bIJRAfYZ4F1jdbw1lYcyxryouRcTNqDv5rUY2PZLyOGjYOxlGgmW6Gh+jwRWXKHP/n21EpGzyUMDSLaU67aSQeB2sW/uZPXAK4NkG74Ba8HlptVLy+jdwQ8XboRW//Dsh8XWH4FRdLBWAB/ECNuaXqJVvYoc+GfmjoMRN9PbQAV7hwxNxf+r9Two2d21NW1hXGUXTN+iUhZZWvfxzYs5qTNtf88sqXOPJvtB/BfUvOznRwx/F3InvPGDHdMOv0pdv8O99s3NCA0liZdCPv1sLjhTXVANbA9bmYsRfhyM48BHXHS X-Forefront-PRVS: 0014E2CF50 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4630300001)(6009001)(7916002)(189002)(199003)(24454002)(2906002)(23726003)(46406003)(106356001)(6116002)(1076002)(68736007)(54356999)(76176999)(97756001)(77096005)(92566002)(3846002)(189998001)(110136002)(4001350100001)(2950100001)(4001430100002)(107886002)(305945005)(97736004)(33656002)(33716001)(7846002)(47776003)(4326007)(66066001)(50986999)(586003)(7736002)(101416001)(83506001)(50466002)(81166006)(81156014)(8676002)(9686002)(105586002)(42186005)(18370500001);DIR:OUT;SFP:1101;SCL:1;SRVR:SN2PR07MB2591;H:hardcore;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;SN2PR07MB2591;23:vf/l3ftQ9zS5hwhdbooUa3/N6XZosaSvSRa5q7hfY?= =?us-ascii?Q?xSKf9NhhclFGA838A5PubXYO3Xl/t7us0/ICM4Qt39Lx2KaKHmffZjtn58Sd?= =?us-ascii?Q?3KZAvWt2SWQjmbDCM3YaTRlc8Zb7eBQCeXveduhV74QG9nTIktl2nJCOT12v?= =?us-ascii?Q?w8PkBB0e2sd5eOZQ0q930/O8bL/0oFc7r5ytxt6+CQ+U6ZUmneZPTakPjI0g?= =?us-ascii?Q?g7TwdMT1ep4f9U/6eOPYsgrpj/KA0KgAEZo+YKH7KjOZzLQR1OcgNZun3g7f?= =?us-ascii?Q?kbchMBnmHn7q98Gl52RGYomnVnTEKpGSku9KRp+b3xwFpV8oS9LlB4SeIgnK?= =?us-ascii?Q?dFghLct1csg4ISRzzaaCwVengDB7+3WHtSMISt8J7A3rRc+UFoefnfmOwlvs?= =?us-ascii?Q?rmrCM93DgzZ3+ZrhrszeJIl0ItdqeqOH2Iz1y9NU5vxqo79Jjf3mCLtX4WRy?= =?us-ascii?Q?mUby4sxYuDc1icI3oQYUQoy8DRZu/AFXeEJakNezY1XPXZvJVBtZWOwABRyl?= =?us-ascii?Q?buNkY574skty9aoT1yFQwX+3t5oEyf3HlwDsAIN1h9/+XFHfynzidajVY2Fw?= =?us-ascii?Q?NFuJy/3uSy0+ymissxdP7XQMlJkhadeDD93FL/2tXxJf8GI2BjcveUomOtJd?= =?us-ascii?Q?qChEQeZf54g6wfARW1+nUJGnBAyxnXgEvzXzI6XypbApIzMQUHn6R50SmGwM?= =?us-ascii?Q?R1zbbqtVETxM9kxvBUCS60NCd9VtuaHPL/sf7POz5sRsOLPL7FbQ1dJaHuGt?= =?us-ascii?Q?IUB/d/M2OwvunNGrQSGrMETbHKdWl4t6BbcZfwWLx8+1u7ZaJPKmIEHwV4ZB?= =?us-ascii?Q?Nm6auUoMDWg6wNykWHZxZpiU8uUVIpxNthGyMuinsqlk7ZYYZukAITsQ3ojo?= =?us-ascii?Q?59uymj5f6QsetV6JE767qICTC+d+AMBbajfGkLzBOZ7hsv79H2VV5V/WkJja?= =?us-ascii?Q?x5kqxXB2QXIBBLHZzuJVlkF+NpGlMb2n/BjmCmXjWNqRHoBNvS1PYZfyNnXO?= =?us-ascii?Q?aGiQCQrzQF5B1iUPxGk/CtTq0FH457Zmbp30uYzKHc9812UAJ4CtfCtav+29?= =?us-ascii?Q?ew5NJkz9CzxsYL7hs1Ys8S4uMw7Lc9bIItaA9xzPgXJXgT3rBUZ8zfKtoNMG?= =?us-ascii?Q?NW8zoG66h85YRSVzGaPJI40UrxpnTzVgaHr12Kq1bCNqjoiSTs6Eg=3D=3D?= X-Microsoft-Exchange-Diagnostics: 1;SN2PR07MB2591;6:h8HQghrdfellt70FMRT32Zyt33aGxHP8VfSlrNl0IfaF7GfkVt7M+XNhqEUhRtQjL0cw7hhL++HyYZHggc+nscMVQuziVH3KNNwpZrFEf7Io1298yPTzSdBqxiFczxwo/dkNl2H/Iaf2u36+TzmrwCjsDwiKLwdDFeocNeU5wfpIo0tayHGtQsZgQyJyJ6rX9E6tnvNfnvu/5wDoApY+N4LRg5Q+ncfcwe3JSEJMenKD8udkpSDJI380s0x2zO6ubdbbPBvu8K06wya5gMe9dLXqF236bCVdejerqh8KlL8=;5:7Z8l1sq8eoLf+1MTFe8daAVwzGihIkBCypTP2sxbYjxveWRoqMOM+C9bEMyCvbWTesU2oPceJLNeboLu+UbKizZZlE0HEAzseEAhPP1S3QjG9VHcQ3iWLnG+lrsfhFZfaZ9NW7qB/d/GFCV4e28LEw==;24:QBO2BtiadvzpRl0RZ4OPH4A7rlBu1qVVH2tCD+CIdEJKVHSZQ+bdEOOSdPYVQZhwACGGf+MBvl4yME0WLI4MuIuJa+MjB8jTd+WOIylKgW4=;7:1wOo065Au17Inm+UEw/8bz9epO3Y+QY6Q6s2bIHUwdW3M8guce/Wrv3LqMb7lirYDSHN3HD/M5CM5om9CmhMKmrbkFW61MYV6SWkwvOSm8eJaWBIifgYT9TQno+WxC+DLuZkqZ6wX1ZZT+dUAEABySCP8fi8OUmvgaDjI8g8n/9vHoNrOuSwJNIKeS/yjSvPTBb/hHXdRRz1PCozhdAGN5V55dfkm9ph8O+rNWxMxdZEC4Ho6UZSXNJdsUFt/rZB SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: caviumnetworks.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Jul 2016 15:51:31.9427 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN2PR07MB2591 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Jul 24, 2016 at 10:04:52PM +0100, Mark Brown wrote: > On Sat, Jul 23, 2016 at 12:42:55PM +0200, Jan Glauber wrote: > > > +config SPI_THUNDERX > > + tristate "Cavium ThunderX SPI controller" > > + depends on 64BIT && PCI && !CAVIUM_OCTEON_SOC > > This is a *weird* and most likely broken set of dependencies - why > exclude this if we're on Octeon (or Octeon happens to have been enabled > in a config)? I agree that it looks weird, the reasoning is that we would like to avoid making the driver depend on something like ARCH_THUNDER. So I made the driver depend on the things it actually uses (PCI for probing and 64BIT because of readq/writeq) and don't care if it compiles on other platforms too (like x86). That said, I can remove the !CAVIUM_OCTEON_SOC, it compiles without errors on MIPS too. Would that be ok? > > +static void thunderx_spi_clock_enable(struct device *dev, struct octeon_spi *p) > > +{ > > + int ret; > > + > > + p->clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(p->clk)) { > > + p->clk = NULL; > > + goto skip; > > + } > > This is really not clever - we should be requesting clocks on probe, not > only when we're trying to enable them, and using devm_ outside of probe > paths is usually a warning sign too. Now, this is actually called from > probe so it works out fine but obviously it'd be better to improve the > power management to only enable the clock when needed and at that point > this function will be used and we'll fall into a bad pattern. > > Given how tiny this function is and that we've not bothered splitting > out any of the other resource acquisition it's probably better to just > inline it into probe. OK, I'll merge it into the probe function. > > + dev_info(&pdev->dev, "Cavium SPI bus driver probed\n"); > > Again, this is just adding noise to the boot log. > > > +#define PCI_DEVICE_ID_THUNDERX_SPI 0xa00b > > + > > +static const struct pci_device_id thunderx_spi_pci_id_table[] = { > > + { PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, PCI_DEVICE_ID_THUNDERX_SPI) }, > > + { 0, } > > +}; > > The define for the device ID doesn't seem to be adding much here. I find it more readable instead of PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, 0xa00b), or did I miss your point? thanks, Jan