From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from host.buserror.net ([209.198.135.123]:59473 "EHLO host.buserror.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752041AbcIIDrv (ORCPT ); Thu, 8 Sep 2016 23:47:51 -0400 Message-ID: <1473392840.30217.170.camel@buserror.net> From: Scott Wood To: Yangbo Lu , linux-mmc@vger.kernel.org, ulf.hansson@linaro.org, Arnd Bergmann Cc: linuxppc-dev@lists.ozlabs.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, linux-i2c@vger.kernel.org, iommu@lists.linux-foundation.org, netdev@vger.kernel.org, Mark Rutland , Rob Herring , Russell King , Jochen Friedrich , Joerg Roedel , Claudiu Manoil , Bhupesh Sharma , Qiang Zhao , Kumar Gala , Santosh Shilimkar , leoyang.li@nxp.com, xiaobo.xie@nxp.com Date: Thu, 08 Sep 2016 22:47:20 -0500 In-Reply-To: <1473150503-9550-6-git-send-email-yangbo.lu@nxp.com> References: <1473150503-9550-1-git-send-email-yangbo.lu@nxp.com> <1473150503-9550-6-git-send-email-yangbo.lu@nxp.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms Sender: linux-clk-owner@vger.kernel.org List-ID: On Tue, 2016-09-06 at 16:28 +0800, Yangbo Lu wrote: > The global utilities block controls power management, I/O device > enabling, power-onreset(POR) configuration monitoring, alternate > function selection for multiplexed signals,and clock control. > > This patch adds a driver to manage and access global utilities block. > Initially only reading SVR and registering soc device are supported. > Other guts accesses, such as reading RCW, should eventually be moved > into this driver as well. > > Signed-off-by: Yangbo Lu > Signed-off-by: Scott Wood Don't put my signoff on patches that I didn't put it on myself.  Definitely don't put mine *after* yours on patches that were last modified by you. If you want to mention that the soc_id encoding was my suggestion, then do so explicitly. > +/* SoC attribute definition for QorIQ platform */ > +static const struct soc_device_attribute qoriq_soc[] = { > +#ifdef CONFIG_PPC > + /* > +  * Power Architecture-based SoCs T Series > +  */ > + > + /* SoC: T1024/T1014/T1023/T1013 Rev: 1.0 */ > + { .soc_id = "svr:0x85400010,name:T1024,die:T1024", > +   .revision = "1.0", > + }, > + { .soc_id = "svr:0x85480010,name:T1024E,die:T1024", > +   .revision = "1.0", > + }, Revision could be computed from the low 8 bits of SVR (just as you do for unknown SVRs). We could move the die name into .family: { .soc_id = "svr:0x85490010,name:T1023E,", .family = "QorIQ T1024", } I see you dropped svre (and the trailing comma), though I guess the vast majority of potential users will be looking at .family.  In which case do we even need name?  If we just make the soc_id be "svr:0xnnnnnnnn" then we could shrink the table to an svr+mask that identifies each die.  I'd still want to keep the "svr:" even if we're giving up on the general tagging system, to make it clear what the number refers to, and to provide some defense against users who match only against soc_id rather than soc_id+family.  Or we could go further and format soc_id as "QorIQ SVR 0xnnnnnnnn" so that soc_id-only matches are fully acceptable rather than just less dangerous. > +static const struct soc_device_attribute *fsl_soc_device_match( > + unsigned int svr, const struct soc_device_attribute *matches) > +{ > + char svr_match[50]; > + int n; > + > + n = sprintf(svr_match, "*%08x*", svr); n = sprintf(svr_match, "svr:0x%08x,*", svr); (according to the current encoding) > + > + do { > + if (!matches->soc_id) > + return NULL; > + if (glob_match(svr_match, matches->soc_id)) > + break; > + } while (matches++); Are you expecting "matches++" to ever evaluate as false? > + /* Register soc device */ > + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL); > + if (!soc_dev_attr) { > + ret = -ENOMEM; > + goto out_unmap; > + } Couldn't this be statically allocated? > + > + machine = of_flat_dt_get_machine_name(); > + if (machine) > + soc_dev_attr->machine = kasprintf(GFP_KERNEL, "%s", > machine); > + > + soc_dev_attr->family = kasprintf(GFP_KERNEL, "QorIQ"); > + > + svr = fsl_guts_get_svr(); > + fsl_soc = fsl_soc_device_match(svr, qoriq_soc); > + if (fsl_soc) { > + soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s", > +  fsl_soc->soc_id); You can use kstrdup() if you're just copying the string as is. > + soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%s", > +    fsl_soc->revision); > + } else { > + soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%08x", > svr); kasprintf(GFP_KERNEL, "svr:0x%08x,", svr); > + > + soc_dev = soc_device_register(soc_dev_attr); > + if (IS_ERR(soc_dev)) { > + ret = -ENODEV; Why are you changing the error code? > + goto out; > + } else { Unnecessary "else". > + pr_info("Detected: %s\n", soc_dev_attr->machine); Machine: %s > + pr_info("Detected SoC family: %s\n", soc_dev_attr->family); > + pr_info("Detected SoC ID: %s, revision: %s\n", > + soc_dev_attr->soc_id, soc_dev_attr->revision); s/Detected //g > + } > + return 0; > +out: > + kfree(soc_dev_attr->machine); > + kfree(soc_dev_attr->family); > + kfree(soc_dev_attr->soc_id); > + kfree(soc_dev_attr->revision); > + kfree(soc_dev_attr); > +out_unmap: > + iounmap(guts->regs); > +out_free: > + kfree(guts); devm > +static int fsl_guts_remove(struct platform_device *dev) > +{ > + kfree(soc_dev_attr->machine); > + kfree(soc_dev_attr->family); > + kfree(soc_dev_attr->soc_id); > + kfree(soc_dev_attr->revision); > + kfree(soc_dev_attr); > + soc_device_unregister(soc_dev); > + iounmap(guts->regs); > + kfree(guts); > + return 0; > +} Don't free the memory before you unregister the device that uses it (moot if you use devm). >   > +#ifdef CONFIG_FSL_GUTS > +unsigned int fsl_guts_get_svr(void); > +#endif Don't ifdef prototypes (unless you're going to provide a stub alternative). -Scott From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Subject: Re: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms Date: Thu, 08 Sep 2016 22:47:20 -0500 Message-ID: <1473392840.30217.170.camel@buserror.net> References: <1473150503-9550-1-git-send-email-yangbo.lu@nxp.com> <1473150503-9550-6-git-send-email-yangbo.lu@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1473150503-9550-6-git-send-email-yangbo.lu-3arQi8VN3Tc@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Yangbo Lu , linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, Arnd Bergmann Cc: Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Russell King , Bhupesh Sharma , netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Santosh Shilimkar , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jochen Friedrich , xiaobo.xie-3arQi8VN3Tc@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, Rob Herring , linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Claudiu Manoil , Kumar Gala , leoyang.li-3arQi8VN3Tc@public.gmane.org, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Qiang Zhao List-Id: linux-i2c@vger.kernel.org T24gVHVlLCAyMDE2LTA5LTA2IGF0IDE2OjI4ICswODAwLCBZYW5nYm8gTHUgd3JvdGU6Cj4gVGhl IGdsb2JhbCB1dGlsaXRpZXMgYmxvY2sgY29udHJvbHMgcG93ZXIgbWFuYWdlbWVudCwgSS9PIGRl dmljZQo+IGVuYWJsaW5nLCBwb3dlci1vbnJlc2V0KFBPUikgY29uZmlndXJhdGlvbiBtb25pdG9y aW5nLCBhbHRlcm5hdGUKPiBmdW5jdGlvbiBzZWxlY3Rpb24gZm9yIG11bHRpcGxleGVkIHNpZ25h bHMsYW5kIGNsb2NrIGNvbnRyb2wuCj4gCj4gVGhpcyBwYXRjaCBhZGRzIGEgZHJpdmVyIHRvIG1h bmFnZSBhbmQgYWNjZXNzIGdsb2JhbCB1dGlsaXRpZXMgYmxvY2suCj4gSW5pdGlhbGx5IG9ubHkg cmVhZGluZyBTVlIgYW5kIHJlZ2lzdGVyaW5nIHNvYyBkZXZpY2UgYXJlIHN1cHBvcnRlZC4KPiBP dGhlciBndXRzIGFjY2Vzc2VzLCBzdWNoIGFzIHJlYWRpbmcgUkNXLCBzaG91bGQgZXZlbnR1YWxs eSBiZSBtb3ZlZAo+IGludG8gdGhpcyBkcml2ZXIgYXMgd2VsbC4KPiAKPiBTaWduZWQtb2ZmLWJ5 OiBZYW5nYm8gTHUgPHlhbmdiby5sdUBueHAuY29tPgo+IFNpZ25lZC1vZmYtYnk6IFNjb3R0IFdv b2QgPG9zc0BidXNlcnJvci5uZXQ+CgpEb24ndCBwdXQgbXkgc2lnbm9mZiBvbiBwYXRjaGVzIHRo YXQgSSBkaWRuJ3QgcHV0IGl0IG9uIG15c2VsZi4gwqBEZWZpbml0ZWx5CmRvbid0IHB1dCBtaW5l ICphZnRlciogeW91cnMgb24gcGF0Y2hlcyB0aGF0IHdlcmUgbGFzdCBtb2RpZmllZCBieSB5b3Uu CgpJZiB5b3Ugd2FudCB0byBtZW50aW9uIHRoYXQgdGhlIHNvY19pZCBlbmNvZGluZyB3YXMgbXkg c3VnZ2VzdGlvbiwgdGhlbiBkbyBzbwpleHBsaWNpdGx5LgoKPiArLyogU29DIGF0dHJpYnV0ZSBk ZWZpbml0aW9uIGZvciBRb3JJUSBwbGF0Zm9ybSAqLwo+ICtzdGF0aWMgY29uc3Qgc3RydWN0IHNv Y19kZXZpY2VfYXR0cmlidXRlIHFvcmlxX3NvY1tdID0gewo+ICsjaWZkZWYgQ09ORklHX1BQQwo+ ICsJLyoKPiArCcKgKiBQb3dlciBBcmNoaXRlY3R1cmUtYmFzZWQgU29DcyBUIFNlcmllcwo+ICsJ wqAqLwo+ICsKPiArCS8qIFNvQzogVDEwMjQvVDEwMTQvVDEwMjMvVDEwMTMgUmV2OiAxLjAgKi8K PiArCXsgLnNvY19pZAk9ICJzdnI6MHg4NTQwMDAxMCxuYW1lOlQxMDI0LGRpZTpUMTAyNCIsCj4g KwnCoMKgLnJldmlzaW9uCT0gIjEuMCIsCj4gKwl9LAo+ICsJeyAuc29jX2lkCT0gInN2cjoweDg1 NDgwMDEwLG5hbWU6VDEwMjRFLGRpZTpUMTAyNCIsCj4gKwnCoMKgLnJldmlzaW9uCT0gIjEuMCIs Cj4gKwl9LAoKUmV2aXNpb24gY291bGQgYmUgY29tcHV0ZWQgZnJvbSB0aGUgbG93IDggYml0cyBv ZiBTVlIgKGp1c3QgYXMgeW91IGRvIGZvciB1bmtub3duIFNWUnMpLgoKV2UgY291bGQgbW92ZSB0 aGUgZGllIG5hbWUgaW50byAuZmFtaWx5OgoKCXsKCQkuc29jX2lkID0gInN2cjoweDg1NDkwMDEw LG5hbWU6VDEwMjNFLCIsCgkJLmZhbWlseSA9ICJRb3JJUSBUMTAyNCIsCgl9CgpJIHNlZSB5b3Ug ZHJvcHBlZCBzdnJlIChhbmQgdGhlIHRyYWlsaW5nIGNvbW1hKSwgdGhvdWdoIEkgZ3Vlc3MgdGhl IHZhc3QKbWFqb3JpdHkgb2YgcG90ZW50aWFsIHVzZXJzIHdpbGwgYmUgbG9va2luZyBhdCAuZmFt aWx5LiDCoEluIHdoaWNoIGNhc2UgZG8gd2UKZXZlbiBuZWVkIG5hbWU/IMKgSWYgd2UganVzdCBt YWtlIHRoZSBzb2NfaWQgYmUgInN2cjoweG5ubm5ubm5uIiB0aGVuIHdlIGNvdWxkCnNocmluayB0 aGUgdGFibGUgdG8gYW4gc3ZyK21hc2sgdGhhdCBpZGVudGlmaWVzIGVhY2ggZGllLiDCoEknZCBz dGlsbCB3YW50IHRvCmtlZXAgdGhlICJzdnI6IiBldmVuIGlmIHdlJ3JlIGdpdmluZyB1cCBvbiB0 aGUgZ2VuZXJhbCB0YWdnaW5nIHN5c3RlbSwgdG8gbWFrZQppdCBjbGVhciB3aGF0IHRoZSBudW1i ZXIgcmVmZXJzIHRvLCBhbmQgdG8gcHJvdmlkZSBzb21lIGRlZmVuc2UgYWdhaW5zdCB1c2Vycwp3 aG8gbWF0Y2ggb25seSBhZ2FpbnN0IHNvY19pZCByYXRoZXIgdGhhbiBzb2NfaWQrZmFtaWx5LiDC oE9yIHdlIGNvdWxkIGdvCmZ1cnRoZXIgYW5kIGZvcm1hdCBzb2NfaWQgYXMgIlFvcklRIFNWUiAw eG5ubm5ubm5uIiBzbyB0aGF0IHNvY19pZC1vbmx5Cm1hdGNoZXMgYXJlIGZ1bGx5IGFjY2VwdGFi bGUgcmF0aGVyIHRoYW4ganVzdCBsZXNzIGRhbmdlcm91cy4KCj4gK3N0YXRpYyBjb25zdCBzdHJ1 Y3Qgc29jX2RldmljZV9hdHRyaWJ1dGUgKmZzbF9zb2NfZGV2aWNlX21hdGNoKAo+ICsJdW5zaWdu ZWQgaW50IHN2ciwgY29uc3Qgc3RydWN0IHNvY19kZXZpY2VfYXR0cmlidXRlICptYXRjaGVzKQo+ ICt7Cj4gKwljaGFyIHN2cl9tYXRjaFs1MF07Cj4gKwlpbnQgbjsKPiArCj4gKwluID0gc3ByaW50 ZihzdnJfbWF0Y2gsICIqJTA4eCoiLCBzdnIpOwoKbiA9IHNwcmludGYoc3ZyX21hdGNoLCAic3Zy OjB4JTA4eCwqIiwgc3ZyKTsKCihhY2NvcmRpbmcgdG8gdGhlIGN1cnJlbnQgZW5jb2RpbmcpCgo+ ICsKPiArCWRvIHsKPiArCQlpZiAoIW1hdGNoZXMtPnNvY19pZCkKPiArCQkJcmV0dXJuIE5VTEw7 Cj4gKwkJaWYgKGdsb2JfbWF0Y2goc3ZyX21hdGNoLCBtYXRjaGVzLT5zb2NfaWQpKQo+ICsJCQli cmVhazsKPiArCX0gd2hpbGUgKG1hdGNoZXMrKyk7CgpBcmUgeW91IGV4cGVjdGluZyAibWF0Y2hl cysrIiB0byBldmVyIGV2YWx1YXRlIGFzIGZhbHNlPwoKPiArCS8qIFJlZ2lzdGVyIHNvYyBkZXZp Y2UgKi8KPiArCXNvY19kZXZfYXR0ciA9IGt6YWxsb2Moc2l6ZW9mKCpzb2NfZGV2X2F0dHIpLCBH RlBfS0VSTkVMKTsKPiArCWlmICghc29jX2Rldl9hdHRyKSB7Cj4gKwkJcmV0ID0gLUVOT01FTTsK PiArCQlnb3RvIG91dF91bm1hcDsKPiArCX0KCkNvdWxkbid0IHRoaXMgYmUgc3RhdGljYWxseSBh bGxvY2F0ZWQ/Cgo+ICsKPiArCW1hY2hpbmUgPSBvZl9mbGF0X2R0X2dldF9tYWNoaW5lX25hbWUo KTsKPiArCWlmIChtYWNoaW5lKQo+ICsJCXNvY19kZXZfYXR0ci0+bWFjaGluZSA9IGthc3ByaW50 ZihHRlBfS0VSTkVMLCAiJXMiLAo+IG1hY2hpbmUpOwo+ICsKPiArCXNvY19kZXZfYXR0ci0+ZmFt aWx5ID0ga2FzcHJpbnRmKEdGUF9LRVJORUwsICJRb3JJUSIpOwo+ICsKPiArCXN2ciA9IGZzbF9n dXRzX2dldF9zdnIoKTsKPiArCWZzbF9zb2MgPSBmc2xfc29jX2RldmljZV9tYXRjaChzdnIsIHFv cmlxX3NvYyk7Cj4gKwlpZiAoZnNsX3NvYykgewo+ICsJCXNvY19kZXZfYXR0ci0+c29jX2lkID0g a2FzcHJpbnRmKEdGUF9LRVJORUwsICIlcyIsCj4gKwkJCQkJCcKgZnNsX3NvYy0+c29jX2lkKTsK CllvdSBjYW4gdXNlIGtzdHJkdXAoKSBpZiB5b3UncmUganVzdCBjb3B5aW5nIHRoZSBzdHJpbmcg YXMgaXMuCgo+ICsJCXNvY19kZXZfYXR0ci0+cmV2aXNpb24gPSBrYXNwcmludGYoR0ZQX0tFUk5F TCwgIiVzIiwKPiArCQkJCQkJwqDCoMKgZnNsX3NvYy0+cmV2aXNpb24pOwo+ICsJfSBlbHNlIHsK PiArCQlzb2NfZGV2X2F0dHItPnNvY19pZCA9IGthc3ByaW50ZihHRlBfS0VSTkVMLCAiMHglMDh4 IiwKPiBzdnIpOwoKCWthc3ByaW50ZihHRlBfS0VSTkVMLCAic3ZyOjB4JTA4eCwiLCBzdnIpOwoK Cj4gKwo+ICsJc29jX2RldiA9IHNvY19kZXZpY2VfcmVnaXN0ZXIoc29jX2Rldl9hdHRyKTsKPiAr CWlmIChJU19FUlIoc29jX2RldikpIHsKPiArCQlyZXQgPSAtRU5PREVWOwoKV2h5IGFyZSB5b3Ug Y2hhbmdpbmcgdGhlIGVycm9yIGNvZGU/Cgo+ICsJCWdvdG8gb3V0Owo+ICsJfSBlbHNlIHsKClVu bmVjZXNzYXJ5ICJlbHNlIi4KCj4gKwkJcHJfaW5mbygiRGV0ZWN0ZWQ6ICVzXG4iLCBzb2NfZGV2 X2F0dHItPm1hY2hpbmUpOwoKTWFjaGluZTogJXMKCj4gKwkJcHJfaW5mbygiRGV0ZWN0ZWQgU29D IGZhbWlseTogJXNcbiIsIHNvY19kZXZfYXR0ci0+ZmFtaWx5KTsKPiArCQlwcl9pbmZvKCJEZXRl Y3RlZCBTb0MgSUQ6ICVzLCByZXZpc2lvbjogJXNcbiIsCj4gKwkJCXNvY19kZXZfYXR0ci0+c29j X2lkLCBzb2NfZGV2X2F0dHItPnJldmlzaW9uKTsKCnMvRGV0ZWN0ZWQgLy9nCgoKPiArCX0KPiAr CXJldHVybiAwOwo+ICtvdXQ6Cj4gKwlrZnJlZShzb2NfZGV2X2F0dHItPm1hY2hpbmUpOwo+ICsJ a2ZyZWUoc29jX2Rldl9hdHRyLT5mYW1pbHkpOwo+ICsJa2ZyZWUoc29jX2Rldl9hdHRyLT5zb2Nf aWQpOwo+ICsJa2ZyZWUoc29jX2Rldl9hdHRyLT5yZXZpc2lvbik7Cj4gKwlrZnJlZShzb2NfZGV2 X2F0dHIpOwo+ICtvdXRfdW5tYXA6Cj4gKwlpb3VubWFwKGd1dHMtPnJlZ3MpOwo+ICtvdXRfZnJl ZToKPiArCWtmcmVlKGd1dHMpOwoKZGV2bQoKPiArc3RhdGljIGludCBmc2xfZ3V0c19yZW1vdmUo c3RydWN0IHBsYXRmb3JtX2RldmljZSAqZGV2KQo+ICt7Cj4gKwlrZnJlZShzb2NfZGV2X2F0dHIt Pm1hY2hpbmUpOwo+ICsJa2ZyZWUoc29jX2Rldl9hdHRyLT5mYW1pbHkpOwo+ICsJa2ZyZWUoc29j X2Rldl9hdHRyLT5zb2NfaWQpOwo+ICsJa2ZyZWUoc29jX2Rldl9hdHRyLT5yZXZpc2lvbik7Cj4g KwlrZnJlZShzb2NfZGV2X2F0dHIpOwo+ICsJc29jX2RldmljZV91bnJlZ2lzdGVyKHNvY19kZXYp Owo+ICsJaW91bm1hcChndXRzLT5yZWdzKTsKPiArCWtmcmVlKGd1dHMpOwo+ICsJcmV0dXJuIDA7 Cj4gK30KCkRvbid0IGZyZWUgdGhlIG1lbW9yeSBiZWZvcmUgeW91IHVucmVnaXN0ZXIgdGhlIGRl dmljZSB0aGF0IHVzZXMgaXQgKG1vb3QgaWYKeW91IHVzZSBkZXZtKS4KCj4gwqAKPiArI2lmZGVm IENPTkZJR19GU0xfR1VUUwo+ICt1bnNpZ25lZCBpbnQgZnNsX2d1dHNfZ2V0X3N2cih2b2lkKTsK PiArI2VuZGlmCgpEb24ndCBpZmRlZiBwcm90b3R5cGVzICh1bmxlc3MgeW91J3JlIGdvaW5nIHRv IHByb3ZpZGUgYSBzdHViIGFsdGVybmF0aXZlKS4KCi1TY290dAoKX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX18KaW9tbXUgbWFpbGluZyBsaXN0CmlvbW11QGxp c3RzLmxpbnV4LWZvdW5kYXRpb24ub3JnCmh0dHBzOi8vbGlzdHMubGludXhmb3VuZGF0aW9uLm9y Zy9tYWlsbWFuL2xpc3RpbmZvL2lvbW11 From mboxrd@z Thu Jan 1 00:00:00 1970 From: oss@buserror.net (Scott Wood) Date: Thu, 08 Sep 2016 22:47:20 -0500 Subject: [v11, 5/8] soc: fsl: add GUTS driver for QorIQ platforms In-Reply-To: <1473150503-9550-6-git-send-email-yangbo.lu@nxp.com> References: <1473150503-9550-1-git-send-email-yangbo.lu@nxp.com> <1473150503-9550-6-git-send-email-yangbo.lu@nxp.com> Message-ID: <1473392840.30217.170.camel@buserror.net> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, 2016-09-06 at 16:28 +0800, Yangbo Lu wrote: > The global utilities block controls power management, I/O device > enabling, power-onreset(POR) configuration monitoring, alternate > function selection for multiplexed signals,and clock control. > > This patch adds a driver to manage and access global utilities block. > Initially only reading SVR and registering soc device are supported. > Other guts accesses, such as reading RCW, should eventually be moved > into this driver as well. > > Signed-off-by: Yangbo Lu > Signed-off-by: Scott Wood Don't put my signoff on patches that I didn't put it on myself. ?Definitely don't put mine *after* yours on patches that were last modified by you. If you want to mention that the soc_id encoding was my suggestion, then do so explicitly. > +/* SoC attribute definition for QorIQ platform */ > +static const struct soc_device_attribute qoriq_soc[] = { > +#ifdef CONFIG_PPC > + /* > + ?* Power Architecture-based SoCs T Series > + ?*/ > + > + /* SoC: T1024/T1014/T1023/T1013 Rev: 1.0 */ > + { .soc_id = "svr:0x85400010,name:T1024,die:T1024", > + ??.revision = "1.0", > + }, > + { .soc_id = "svr:0x85480010,name:T1024E,die:T1024", > + ??.revision = "1.0", > + }, Revision could be computed from the low 8 bits of SVR (just as you do for unknown SVRs). We could move the die name into .family: { .soc_id = "svr:0x85490010,name:T1023E,", .family = "QorIQ T1024", } I see you dropped svre (and the trailing comma), though I guess the vast majority of potential users will be looking at .family. ?In which case do we even need name? ?If we just make the soc_id be "svr:0xnnnnnnnn" then we could shrink the table to an svr+mask that identifies each die. ?I'd still want to keep the "svr:" even if we're giving up on the general tagging system, to make it clear what the number refers to, and to provide some defense against users who match only against soc_id rather than soc_id+family. ?Or we could go further and format soc_id as "QorIQ SVR 0xnnnnnnnn" so that soc_id-only matches are fully acceptable rather than just less dangerous. > +static const struct soc_device_attribute *fsl_soc_device_match( > + unsigned int svr, const struct soc_device_attribute *matches) > +{ > + char svr_match[50]; > + int n; > + > + n = sprintf(svr_match, "*%08x*", svr); n = sprintf(svr_match, "svr:0x%08x,*", svr); (according to the current encoding) > + > + do { > + if (!matches->soc_id) > + return NULL; > + if (glob_match(svr_match, matches->soc_id)) > + break; > + } while (matches++); Are you expecting "matches++" to ever evaluate as false? > + /* Register soc device */ > + soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL); > + if (!soc_dev_attr) { > + ret = -ENOMEM; > + goto out_unmap; > + } Couldn't this be statically allocated? > + > + machine = of_flat_dt_get_machine_name(); > + if (machine) > + soc_dev_attr->machine = kasprintf(GFP_KERNEL, "%s", > machine); > + > + soc_dev_attr->family = kasprintf(GFP_KERNEL, "QorIQ"); > + > + svr = fsl_guts_get_svr(); > + fsl_soc = fsl_soc_device_match(svr, qoriq_soc); > + if (fsl_soc) { > + soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s", > + ?fsl_soc->soc_id); You can use kstrdup() if you're just copying the string as is. > + soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%s", > + ???fsl_soc->revision); > + } else { > + soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%08x", > svr); kasprintf(GFP_KERNEL, "svr:0x%08x,", svr); > + > + soc_dev = soc_device_register(soc_dev_attr); > + if (IS_ERR(soc_dev)) { > + ret = -ENODEV; Why are you changing the error code? > + goto out; > + } else { Unnecessary "else". > + pr_info("Detected: %s\n", soc_dev_attr->machine); Machine: %s > + pr_info("Detected SoC family: %s\n", soc_dev_attr->family); > + pr_info("Detected SoC ID: %s, revision: %s\n", > + soc_dev_attr->soc_id, soc_dev_attr->revision); s/Detected //g > + } > + return 0; > +out: > + kfree(soc_dev_attr->machine); > + kfree(soc_dev_attr->family); > + kfree(soc_dev_attr->soc_id); > + kfree(soc_dev_attr->revision); > + kfree(soc_dev_attr); > +out_unmap: > + iounmap(guts->regs); > +out_free: > + kfree(guts); devm > +static int fsl_guts_remove(struct platform_device *dev) > +{ > + kfree(soc_dev_attr->machine); > + kfree(soc_dev_attr->family); > + kfree(soc_dev_attr->soc_id); > + kfree(soc_dev_attr->revision); > + kfree(soc_dev_attr); > + soc_device_unregister(soc_dev); > + iounmap(guts->regs); > + kfree(guts); > + return 0; > +} Don't free the memory before you unregister the device that uses it (moot if you use devm). > ? > +#ifdef CONFIG_FSL_GUTS > +unsigned int fsl_guts_get_svr(void); > +#endif Don't ifdef prototypes (unless you're going to provide a stub alternative). -Scott