Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1214014 > unrolled thread

Re: [PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation

Started bySudeep Holla <sudeep.holla@arm.com>
First post2015-08-26 18:20 +0200
Last post2015-08-26 19:20 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/1] irqchip: imx-gpcv2:  Simplify the implemenation Sudeep Holla <sudeep.holla@arm.com> - 2015-08-26 18:20 +0200
    RE: [PATCH 1/1] irqchip: imx-gpcv2:  Simplify the implemenation Shenwei Wang <Shenwei.Wang@freescale.com> - 2015-08-26 18:50 +0200
      Re: [PATCH 1/1] irqchip: imx-gpcv2:  Simplify the implemenation Sudeep Holla <sudeep.holla@arm.com> - 2015-08-26 19:20 +0200

#1214014 — Re: [PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation

FromSudeep Holla <sudeep.holla@arm.com>
Date2015-08-26 18:20 +0200
SubjectRe: [PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation
Message-ID<q1LAC-2ow-15@gated-at.bofh.it>
typo in $subject

On 26/08/15 16:49, Shenwei Wang wrote:
> Based on Sudeep Holla's review comments, the implementation can
> be simplified by using the two flags: IRQCHIP_SKIP_SET_WAKE and
> IRQCHIP_MASK_ON_SUSPEND. This patch enables the flags in the
> struct irq_chip and removes the unnecessory syscore_ops callbacks.
>
> Signed-off-by: Shenwei Wang <shenwei.wang@freescale.com>
> ---
>   drivers/irqchip/irq-imx-gpcv2.c | 83 +++++++----------------------------------
>   1 file changed, 13 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
> index 4a97afa..e25df78 100644
> --- a/drivers/irqchip/irq-imx-gpcv2.c
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -22,7 +22,6 @@ struct gpcv2_irqchip_data {
>   	struct raw_spinlock    rlock;
>   	void __iomem       *gpc_base;
>   	u32 wakeup_sources[IMR_NUM];
> -	u32 saved_irq_mask[IMR_NUM];
>   	u32 cpu2wakeup;
>   };
>
> @@ -30,79 +29,25 @@ static struct gpcv2_irqchip_data *imx_gpcv2_instance;
>
>   u32 imx_gpcv2_get_wakeup_source(u32 **sources)

I assume this patch is against -next and I don't see any users of
imx_gpcv2_get_wakeup_source in -next.

If possible I would avoid exposing this function by implementing
suspend_ops just as before(just saving raw h/w reg values and restoring
then back on resume w/o tagging them as wakeup mask though they might be
indeed wakeup mask).

In that way, this driver is self-contained and whatever imx code calls
this function will not have dependency on this driver, no ? Do you need
access to imx_gpcv2_get_wakeup_source too early in resume much before
suspend_ops resume ? I would like to see the user of that function to
comment on that any further.

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1214036

FromShenwei Wang <Shenwei.Wang@freescale.com>
Date2015-08-26 18:50 +0200
Message-ID<q1M3E-2Xd-11@gated-at.bofh.it>
In reply to#1214014
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogU3VkZWVwIEhvbGxhIFtt
YWlsdG86c3VkZWVwLmhvbGxhQGFybS5jb21dDQo+IFNlbnQ6IDIwMTXE6jjUwjI2yNUgMTE6MTMN
Cj4gVG86IFdhbmcgU2hlbndlaS1CMzgzMzk7IGphc29uQGxha2VkYWVtb24ubmV0DQo+IENjOiBz
aGF3bi5ndW9AbGluYXJvLm9yZzsgdGdseEBsaW51dHJvbml4LmRlOyBTdWRlZXAgSG9sbGE7DQo+
IGxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZzsgbGludXgta2VybmVsQHZnZXIu
a2VybmVsLm9yZzsgSHVhbmcNCj4gWW9uZ2NhaS1CMjA3ODgNCj4gU3ViamVjdDogUmU6IFtQQVRD
SCAxLzFdIGlycWNoaXA6IGlteC1ncGN2MjogU2ltcGxpZnkgdGhlIGltcGxlbWVuYXRpb24NCj4g
DQo+IHR5cG8gaW4gJHN1YmplY3QNCg0KSnVzdCBub3RpY2VkLiBXaWxsIGNoYW5nZSBpdC4NCg0K
PiBPbiAyNi8wOC8xNSAxNjo0OSwgU2hlbndlaSBXYW5nIHdyb3RlOg0KPiA+IEJhc2VkIG9uIFN1
ZGVlcCBIb2xsYSdzIHJldmlldyBjb21tZW50cywgdGhlIGltcGxlbWVudGF0aW9uIGNhbiBiZQ0K
PiA+IHNpbXBsaWZpZWQgYnkgdXNpbmcgdGhlIHR3byBmbGFnczogSVJRQ0hJUF9TS0lQX1NFVF9X
QUtFIGFuZA0KPiA+IElSUUNISVBfTUFTS19PTl9TVVNQRU5ELiBUaGlzIHBhdGNoIGVuYWJsZXMg
dGhlIGZsYWdzIGluIHRoZSBzdHJ1Y3QNCj4gPiBpcnFfY2hpcCBhbmQgcmVtb3ZlcyB0aGUgdW5u
ZWNlc3Nvcnkgc3lzY29yZV9vcHMgY2FsbGJhY2tzLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTog
U2hlbndlaSBXYW5nIDxzaGVud2VpLndhbmdAZnJlZXNjYWxlLmNvbT4NCj4gPiAtLS0NCj4gPiAg
IGRyaXZlcnMvaXJxY2hpcC9pcnEtaW14LWdwY3YyLmMgfCA4MyArKysrKysrLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPiA+ICAgMSBmaWxlIGNoYW5nZWQsIDEzIGluc2VydGlv
bnMoKyksIDcwIGRlbGV0aW9ucygtKQ0KPiA+DQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvaXJx
Y2hpcC9pcnEtaW14LWdwY3YyLmMNCj4gPiBiL2RyaXZlcnMvaXJxY2hpcC9pcnEtaW14LWdwY3Yy
LmMgaW5kZXggNGE5N2FmYS4uZTI1ZGY3OCAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2lycWNo
aXAvaXJxLWlteC1ncGN2Mi5jDQo+ID4gKysrIGIvZHJpdmVycy9pcnFjaGlwL2lycS1pbXgtZ3Bj
djIuYw0KPiA+IEBAIC0yMiw3ICsyMiw2IEBAIHN0cnVjdCBncGN2Ml9pcnFjaGlwX2RhdGEgew0K
PiA+ICAgCXN0cnVjdCByYXdfc3BpbmxvY2sgICAgcmxvY2s7DQo+ID4gICAJdm9pZCBfX2lvbWVt
ICAgICAgICpncGNfYmFzZTsNCj4gPiAgIAl1MzIgd2FrZXVwX3NvdXJjZXNbSU1SX05VTV07DQo+
ID4gLQl1MzIgc2F2ZWRfaXJxX21hc2tbSU1SX05VTV07DQo+ID4gICAJdTMyIGNwdTJ3YWtldXA7
DQo+ID4gICB9Ow0KPiA+DQo+ID4gQEAgLTMwLDc5ICsyOSwyNSBAQCBzdGF0aWMgc3RydWN0IGdw
Y3YyX2lycWNoaXBfZGF0YQ0KPiA+ICppbXhfZ3BjdjJfaW5zdGFuY2U7DQo+ID4NCj4gPiAgIHUz
MiBpbXhfZ3BjdjJfZ2V0X3dha2V1cF9zb3VyY2UodTMyICoqc291cmNlcykNCj4gDQo+IEkgYXNz
dW1lIHRoaXMgcGF0Y2ggaXMgYWdhaW5zdCAtbmV4dCBhbmQgSSBkb24ndCBzZWUgYW55IHVzZXJz
IG9mDQo+IGlteF9ncGN2Ml9nZXRfd2FrZXVwX3NvdXJjZSBpbiAtbmV4dC4NCj4gDQo+IElmIHBv
c3NpYmxlIEkgd291bGQgYXZvaWQgZXhwb3NpbmcgdGhpcyBmdW5jdGlvbiBieSBpbXBsZW1lbnRp
bmcgc3VzcGVuZF9vcHMganVzdA0KPiBhcyBiZWZvcmUoanVzdCBzYXZpbmcgcmF3IGgvdyByZWcg
dmFsdWVzIGFuZCByZXN0b3JpbmcgdGhlbiBiYWNrIG9uIHJlc3VtZSB3L28NCj4gdGFnZ2luZyB0
aGVtIGFzIHdha2V1cCBtYXNrIHRob3VnaCB0aGV5IG1pZ2h0IGJlIGluZGVlZCB3YWtldXAgbWFz
aykuDQo+IA0KPiBJbiB0aGF0IHdheSwgdGhpcyBkcml2ZXIgaXMgc2VsZi1jb250YWluZWQgYW5k
IHdoYXRldmVyIGlteCBjb2RlIGNhbGxzIHRoaXMgZnVuY3Rpb24NCj4gd2lsbCBub3QgaGF2ZSBk
ZXBlbmRlbmN5IG9uIHRoaXMgZHJpdmVyLCBubyA/IERvIHlvdSBuZWVkIGFjY2VzcyB0bw0KPiBp
bXhfZ3BjdjJfZ2V0X3dha2V1cF9zb3VyY2UgdG9vIGVhcmx5IGluIHJlc3VtZSBtdWNoIGJlZm9y
ZSBzdXNwZW5kX29wcw0KPiByZXN1bWUgPyBJIHdvdWxkIGxpa2UgdG8gc2VlIHRoZSB1c2VyIG9m
IHRoYXQgZnVuY3Rpb24gdG8gY29tbWVudCBvbiB0aGF0IGFueQ0KPiBmdXJ0aGVyLg0KDQpJdCBp
cyBsaW51eC1uZXh0LiBUaGUgdXNlciBpcyBpbiB0aGUgZm9sbG93aW5nIHBhdGNoIHdoaWNoIGlz
IHVuZGVyIHJldmlldy4NCmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL3BpcGVybWFpbC9saW51
eC1hcm0ta2VybmVsLzIwMTUtSnVseS8zNjEzODguaHRtbA0KDQpUaGUgYWNjZXNzIHRvIHRoaXMg
ZnVuY3Rpb24gd2lsbCBvbmx5IGhhcHBlbiBpbiBzdXNwZW5kX29wcyBzbyBmYXIuDQoNClJlZ2Fy
ZHMsDQpTaGVud2VpDQoNCg0KPiBSZWdhcmRzLA0KPiBTdWRlZXANCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214064

FromSudeep Holla <sudeep.holla@arm.com>
Date2015-08-26 19:20 +0200
Message-ID<q1MwI-3Km-33@gated-at.bofh.it>
In reply to#1214036

On 26/08/15 17:44, Shenwei Wang wrote:
>
>
>> -----Original Message-----
>> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
>> Sent: 2015年8月26日 11:13
>> To: Wang Shenwei-B38339; jason@lakedaemon.net
>> Cc: shawn.guo@linaro.org; tglx@linutronix.de; Sudeep Holla;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Huang
>> Yongcai-B20788
>> Subject: Re: [PATCH 1/1] irqchip: imx-gpcv2: Simplify the implemenation
>>
>> typo in $subject
>
> Just noticed. Will change it.
>
>> On 26/08/15 16:49, Shenwei Wang wrote:
>>> Based on Sudeep Holla's review comments, the implementation can be
>>> simplified by using the two flags: IRQCHIP_SKIP_SET_WAKE and
>>> IRQCHIP_MASK_ON_SUSPEND. This patch enables the flags in the struct
>>> irq_chip and removes the unnecessory syscore_ops callbacks.
>>>
>>> Signed-off-by: Shenwei Wang <shenwei.wang@freescale.com>
>>> ---
>>>    drivers/irqchip/irq-imx-gpcv2.c | 83 +++++++----------------------------------
>>>    1 file changed, 13 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-imx-gpcv2.c
>>> b/drivers/irqchip/irq-imx-gpcv2.c index 4a97afa..e25df78 100644
>>> --- a/drivers/irqchip/irq-imx-gpcv2.c
>>> +++ b/drivers/irqchip/irq-imx-gpcv2.c
>>> @@ -22,7 +22,6 @@ struct gpcv2_irqchip_data {
>>>    	struct raw_spinlock    rlock;
>>>    	void __iomem       *gpc_base;
>>>    	u32 wakeup_sources[IMR_NUM];
>>> -	u32 saved_irq_mask[IMR_NUM];
>>>    	u32 cpu2wakeup;
>>>    };
>>>
>>> @@ -30,79 +29,25 @@ static struct gpcv2_irqchip_data
>>> *imx_gpcv2_instance;
>>>
>>>    u32 imx_gpcv2_get_wakeup_source(u32 **sources)
>>
>> I assume this patch is against -next and I don't see any users of
>> imx_gpcv2_get_wakeup_source in -next.
>>
>> If possible I would avoid exposing this function by implementing suspend_ops just
>> as before(just saving raw h/w reg values and restoring then back on resume w/o
>> tagging them as wakeup mask though they might be indeed wakeup mask).
>>
>> In that way, this driver is self-contained and whatever imx code calls this function
>> will not have dependency on this driver, no ? Do you need access to
>> imx_gpcv2_get_wakeup_source too early in resume much before suspend_ops
>> resume ? I would like to see the user of that function to comment on that any
>> further.
>
> It is linux-next. The user is in the following patch which is under review.
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/361388.html
>

I got lost trying to follow through that 1000 odd line of code with lots
of register accesses. I couldn't understand much, so I gave up.

Such details are abstracted well and hidden in firmware with PSCI(good
that it's enforced in ARM64, hopefully ARM32 also sees more adoption).

So, for the parts adding those 2 flags and removing the unnecessary
code, you can add:
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web