From mboxrd@z Thu Jan 1 00:00:00 1970 From: l.stach@pengutronix.de (Lucas Stach) Date: Tue, 24 Jul 2018 11:06:54 +0200 Subject: [PATCH v6 5/5] mailbox: Add support for i.MX7D messaging unit In-Reply-To: References: <20180722063923.30222-1-o.rempel@pengutronix.de> <20180722063923.30222-6-o.rempel@pengutronix.de> <1532366380.3163.109.camel@pengutronix.de> Message-ID: <1532423214.32306.1.camel@pengutronix.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Am Dienstag, den 24.07.2018, 07:14 +0200 schrieb Oleksij Rempel: > Hi Lucas, > > here is more detailed response: > > On 23.07.2018 19:19, Lucas Stach wrote: [...] > > > +}; > > > + > > > +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox) > > > +{ > > > > + return container_of(mbox, struct imx_mu_priv, mbox); > > > > > > +} > > > + > > > +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) > > > +{ > > > + iowrite32(val, priv->base + offs); > > > > This driver is never going to be used on a device with port based IO, > > so iowrite doesn't make much sense here, just use writel. Same comment > > applies to the below read function. > > As before I don't really understand the difference. I took some time for > learning and here are my findings: > - historical background of ioread/iowrite functions. What I can find is > > this LWN article: https://lwn.net/Articles/102232/ . The main point > seems to be "The first of these is a new __iomem annotation used to mark > pointers to I/O memory" ... " As with __user, the __iomem marker serves > a documentation role in the kernel code;" > > In modern kernel the difference between ioread32 and readl seems to be > completely disappeared. > with this LWN article (2016): > https://lwn.net/Articles/698014/ > the readl and reade32 are always in the same group.. Okay, as discussed offline this is more a thing of personal preference, since this maps to the same thing internally when used on a architecture without IO ports. I slightly dislike those as they don't have a _relaxed counterpart, but other than that there should be no difference. As we don't use any of the relaxed stuff in this driver it's really about the color of the bikeshed, so feel free to keep it your way. > If there is some documentation which will say why I should use readl() > instead of ioread32() i would really love to read it. > > > Also, given that those functions are not really shortening the code in > > the user they may also be removed completely IMHO. > > Wrong assumption..??I use this functions for tracing. It is just to easy > to add two trace points...??From technical perspective I don't see any > advantage or disadvantage of having this functions. > If it is my personal preference, then I decide to keep it. That IMHO implies that I'm perfectly fine with you ignoring this comment. :) [...] > > > +static int imx_mu_startup(struct mbox_chan *chan) > > > +{ > > > > > > > > + struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); > > > > > > > > + struct imx_mu_con_priv *cp = chan->con_priv; > > > > + int ret; > > > > > > + > > > > > > > > + cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, "imx_mu_chan[%i]", > > > > > > > > + ??????cp->idx); > > > > > > > > + if (!cp->irq_desc) > > > > + return -ENOMEM; > > > > > > + > > > > + ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr, > > > > > > + ???????IRQF_SHARED, cp->irq_desc, chan); > > > > Using the devm_ variants of those functions doesn't make sense when the > > resources aren't tied to the device lifetime. As you are tearing them > > down manually in imx_mu_shutdown anyways, just use the raw variants of > > those functions. > > I have nothing against it. Thanks, as this is something I feel more strongly about. As the devm_ stuff is meant to tie the lifetime of an object to the device/driver lifetime using them in a context where there is no relation at all is kind of an API abuse to me. Regards, Lucas From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lucas Stach Subject: Re: [PATCH v6 5/5] mailbox: Add support for i.MX7D messaging unit Date: Tue, 24 Jul 2018 11:06:54 +0200 Message-ID: <1532423214.32306.1.camel@pengutronix.de> References: <20180722063923.30222-1-o.rempel@pengutronix.de> <20180722063923.30222-6-o.rempel@pengutronix.de> <1532366380.3163.109.camel@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Oleksij Rempel , Shawn Guo , Fabio Estevam , Rob Herring , Mark Rutland , "A.s. Dong" , Vladimir Zapolskiy Cc: kernel@pengutronix.de, devicetree@vger.kernel.org, dl-linux-imx , linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org QW0gRGllbnN0YWcsIGRlbiAyNC4wNy4yMDE4LCAwNzoxNCArMDIwMCBzY2hyaWViIE9sZWtzaWog UmVtcGVsOgo+IEhpIEx1Y2FzLAo+IAo+IGhlcmUgaXMgbW9yZSBkZXRhaWxlZCByZXNwb25zZToK PiAKPiBPbiAyMy4wNy4yMDE4IDE5OjE5LCBMdWNhcyBTdGFjaCB3cm90ZToKWy4uLl0KPiA+ID4g K307Cj4gPiA+ICsKPiA+ID4gK3N0YXRpYyBzdHJ1Y3QgaW14X211X3ByaXYgKnRvX2lteF9tdV9w cml2KHN0cnVjdCBtYm94X2NvbnRyb2xsZXIgKm1ib3gpCj4gPiA+ICt7Cj4gPiA+ID4gKwlyZXR1 cm4gY29udGFpbmVyX29mKG1ib3gsIHN0cnVjdCBpbXhfbXVfcHJpdiwgbWJveCk7Cj4gPiA+IAo+ ID4gPiArfQo+ID4gPiArCj4gPiA+ICtzdGF0aWMgdm9pZCBpbXhfbXVfd3JpdGUoc3RydWN0IGlt eF9tdV9wcml2ICpwcml2LCB1MzIgdmFsLCB1MzIgb2ZmcykKPiA+ID4gK3sKPiA+ID4gKwlpb3dy aXRlMzIodmFsLCBwcml2LT5iYXNlICsgb2Zmcyk7Cj4gPiAKPiA+IFRoaXMgZHJpdmVyIGlzIG5l dmVyIGdvaW5nIHRvIGJlIHVzZWQgb24gYSBkZXZpY2Ugd2l0aCBwb3J0IGJhc2VkIElPLAo+ID4g c28gaW93cml0ZSBkb2Vzbid0IG1ha2UgbXVjaCBzZW5zZSBoZXJlLCBqdXN0IHVzZSB3cml0ZWwu IFNhbWUgY29tbWVudAo+ID4gYXBwbGllcyB0byB0aGUgYmVsb3cgcmVhZCBmdW5jdGlvbi4KPiAK PiBBcyBiZWZvcmUgSSBkb24ndCByZWFsbHkgdW5kZXJzdGFuZCB0aGUgZGlmZmVyZW5jZS4gSSB0 b29rIHNvbWUgdGltZSBmb3IKPiBsZWFybmluZyBhbmQgaGVyZSBhcmUgbXkgZmluZGluZ3M6Cj4g LSBoaXN0b3JpY2FsIGJhY2tncm91bmQgb2YgaW9yZWFkL2lvd3JpdGUgZnVuY3Rpb25zLiBXaGF0 IEkgY2FuIGZpbmQgaXMKPiA+IHRoaXMgTFdOIGFydGljbGU6IGh0dHBzOi8vbHduLm5ldC9BcnRp Y2xlcy8xMDIyMzIvIC4gVGhlIG1haW4gcG9pbnQKPiBzZWVtcyB0byBiZSAiVGhlIGZpcnN0IG9m IHRoZXNlIGlzIGEgbmV3IF9faW9tZW0gYW5ub3RhdGlvbiB1c2VkIHRvIG1hcmsKPiBwb2ludGVy cyB0byBJL08gbWVtb3J5IiAuLi4gIiBBcyB3aXRoIF9fdXNlciwgdGhlIF9faW9tZW0gbWFya2Vy IHNlcnZlcwo+IGEgZG9jdW1lbnRhdGlvbiByb2xlIGluIHRoZSBrZXJuZWwgY29kZTsiCj4gCj4g SW4gbW9kZXJuIGtlcm5lbCB0aGUgZGlmZmVyZW5jZSBiZXR3ZWVuIGlvcmVhZDMyIGFuZCByZWFk bCBzZWVtcyB0byBiZQo+IGNvbXBsZXRlbHkgZGlzYXBwZWFyZWQuCj4gd2l0aCB0aGlzIExXTiBh cnRpY2xlICgyMDE2KToKPiBodHRwczovL2x3bi5uZXQvQXJ0aWNsZXMvNjk4MDE0Lwo+IHRoZSBy ZWFkbCBhbmQgcmVhZGUzMiBhcmUgYWx3YXlzIGluIHRoZSBzYW1lIGdyb3VwLi4KCk9rYXksIGFz IGRpc2N1c3NlZCBvZmZsaW5lIHRoaXMgaXMgbW9yZSBhIHRoaW5nIG9mIHBlcnNvbmFsIHByZWZl cmVuY2UsCnNpbmNlIHRoaXMgbWFwcyB0byB0aGUgc2FtZSB0aGluZyBpbnRlcm5hbGx5IHdoZW4g dXNlZCBvbiBhCmFyY2hpdGVjdHVyZSB3aXRob3V0IElPIHBvcnRzLiBJIHNsaWdodGx5IGRpc2xp a2UgdGhvc2UgYXMgdGhleSBkb24ndApoYXZlIGEgX3JlbGF4ZWQgY291bnRlcnBhcnQsIGJ1dCBv dGhlciB0aGFuIHRoYXQgdGhlcmUgc2hvdWxkIGJlIG5vCmRpZmZlcmVuY2UuIEFzIHdlIGRvbid0 IHVzZSBhbnkgb2YgdGhlIHJlbGF4ZWQgc3R1ZmYgaW4gdGhpcyBkcml2ZXIKaXQncyByZWFsbHkg YWJvdXQgdGhlIGNvbG9yIG9mIHRoZSBiaWtlc2hlZCwgc28gZmVlbCBmcmVlIHRvIGtlZXAgaXQK eW91ciB3YXkuCgo+IElmIHRoZXJlIGlzIHNvbWUgZG9jdW1lbnRhdGlvbiB3aGljaCB3aWxsIHNh eSB3aHkgSSBzaG91bGQgdXNlIHJlYWRsKCkKPiBpbnN0ZWFkIG9mIGlvcmVhZDMyKCkgaSB3b3Vs ZCByZWFsbHkgbG92ZSB0byByZWFkIGl0Lgo+IAo+ID4gQWxzbywgZ2l2ZW4gdGhhdCB0aG9zZSBm dW5jdGlvbnMgYXJlIG5vdCByZWFsbHkgc2hvcnRlbmluZyB0aGUgY29kZSBpbgo+ID4gdGhlIHVz ZXIgdGhleSBtYXkgYWxzbyBiZSByZW1vdmVkIGNvbXBsZXRlbHkgSU1ITy4KPiAKPiBXcm9uZyBh c3N1bXB0aW9uLi7CoMKgSSB1c2UgdGhpcyBmdW5jdGlvbnMgZm9yIHRyYWNpbmcuIEl0IGlzIGp1 c3QgdG8gZWFzeQo+IHRvIGFkZCB0d28gdHJhY2UgcG9pbnRzLi4uwqDCoEZyb20gdGVjaG5pY2Fs IHBlcnNwZWN0aXZlIEkgZG9uJ3Qgc2VlIGFueQo+IGFkdmFudGFnZSBvciBkaXNhZHZhbnRhZ2Ug b2YgaGF2aW5nIHRoaXMgZnVuY3Rpb25zLgo+IElmIGl0IGlzIG15IHBlcnNvbmFsIHByZWZlcmVu Y2UsIHRoZW4gSSBkZWNpZGUgdG8ga2VlcCBpdC4KClRoYXQgSU1ITyBpbXBsaWVzIHRoYXQgSSdt IHBlcmZlY3RseSBmaW5lIHdpdGggeW91IGlnbm9yaW5nIHRoaXMKY29tbWVudC4gOikKClsuLi5d Cj4gPiA+ICtzdGF0aWMgaW50IGlteF9tdV9zdGFydHVwKHN0cnVjdCBtYm94X2NoYW4gKmNoYW4p Cj4gPiA+ICt7Cj4gPiA+ID4gPiA+ID4gPiArCXN0cnVjdCBpbXhfbXVfcHJpdiAqcHJpdiA9IHRv X2lteF9tdV9wcml2KGNoYW4tPm1ib3gpOwo+ID4gPiA+ID4gPiA+ID4gKwlzdHJ1Y3QgaW14X211 X2Nvbl9wcml2ICpjcCA9IGNoYW4tPmNvbl9wcml2Owo+ID4gPiA+ICsJaW50IHJldDsKPiA+ID4g Cj4gPiA+ICsKPiA+ID4gPiA+ID4gPiA+ICsJY3AtPmlycV9kZXNjID0gZGV2bV9rYXNwcmludGYo cHJpdi0+ZGV2LCBHRlBfS0VSTkVMLCAiaW14X211X2NoYW5bJWldIiwKPiA+ID4gPiA+ID4gPiA+ ICsJCQkJwqDCoMKgwqDCoMKgY3AtPmlkeCk7Cj4gPiA+ID4gPiA+ID4gPiArCWlmICghY3AtPmly cV9kZXNjKQo+ID4gPiA+ICsJCXJldHVybiAtRU5PTUVNOwo+ID4gPiAKPiA+ID4gKwo+ID4gPiA+ ICsJcmV0ID0gZGV2bV9yZXF1ZXN0X2lycShwcml2LT5kZXYsIGNwLT5pcnEsIGlteF9tdV9pc3Is Cj4gPiA+IAo+ID4gPiArCQkJwqDCoMKgwqDCoMKgwqBJUlFGX1NIQVJFRCwgY3AtPmlycV9kZXNj LCBjaGFuKTsKPiA+IAo+ID4gVXNpbmcgdGhlIGRldm1fIHZhcmlhbnRzIG9mIHRob3NlIGZ1bmN0 aW9ucyBkb2Vzbid0IG1ha2Ugc2Vuc2Ugd2hlbiB0aGUKPiA+IHJlc291cmNlcyBhcmVuJ3QgdGll ZCB0byB0aGUgZGV2aWNlIGxpZmV0aW1lLiBBcyB5b3UgYXJlIHRlYXJpbmcgdGhlbQo+ID4gZG93 biBtYW51YWxseSBpbiBpbXhfbXVfc2h1dGRvd24gYW55d2F5cywganVzdCB1c2UgdGhlIHJhdyB2 YXJpYW50cyBvZgo+ID4gdGhvc2UgZnVuY3Rpb25zLgo+IAo+IEkgaGF2ZSBub3RoaW5nIGFnYWlu c3QgaXQuCgpUaGFua3MsIGFzIHRoaXMgaXMgc29tZXRoaW5nIEkgZmVlbCBtb3JlIHN0cm9uZ2x5 IGFib3V0LiBBcyB0aGUgZGV2bV8Kc3R1ZmYgaXMgbWVhbnQgdG8gdGllIHRoZSBsaWZldGltZSBv ZiBhbiBvYmplY3QgdG8gdGhlIGRldmljZS9kcml2ZXIKbGlmZXRpbWUgdXNpbmcgdGhlbSBpbiBh IGNvbnRleHQgd2hlcmUgdGhlcmUgaXMgbm8gcmVsYXRpb24gYXQgYWxsIGlzCmtpbmQgb2YgYW4g QVBJIGFidXNlIHRvIG1lLgoKUmVnYXJkcywKTHVjYXMKCl9fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fCmxpbnV4LWFybS1rZXJuZWwgbWFpbGluZyBsaXN0Cmxp bnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRwOi8vbGlzdHMuaW5mcmFkZWFk Lm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LWFybS1rZXJuZWwK