On 22.09.2014 08:17, Abhilash Kesavan wrote:
> Hi Tomasz,
>
> On Sat, Sep 13, 2014 at 4:57 PM, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote:
>> Hi Abhilash,
>>
>> Please see my comments inline.
>>
>> On 13.09.2014 10:50, Abhilash Kesavan wrote:
>>> Exynos7 uses different offsets for wakeup interrupt configuration registers.
>>> So a new irq_chip instance for Exynos7 wakeup interrupts is added. The irq_chip
>>> selection is now based on the wakeup interrupt controller compatible string.
>>
>> [snip]
>>
>>> @@ -328,9 +322,11 @@ static int exynos_wkup_irq_set_wake(struct irq_data *irqd, unsigned int on)
>>> /*
>>> * irq_chip for wakeup interrupts
>>> */
>>> -static struct exynos_irq_chip exynos_wkup_irq_chip = {
>>> +static struct exynos_irq_chip exynos_wkup_irq_chip;
>>> +
>>
>> Why do you still need this, if you have both variants below?
>
> After adding __initdata to the two variants, I will require to have a
> copy of one of them.
>
>>
>>> +static struct exynos_irq_chip exynos4210_wkup_irq_chip = {
>>> .chip = {
>>> - .name = "exynos_wkup_irq_chip",
>>> + .name = "exynos4210_wkup_irq_chip",
>>> .irq_unmask = exynos_irq_unmask,
>>> .irq_mask = exynos_irq_mask,
>>> .irq_ack = exynos_irq_ack,
>>> @@ -342,6 +338,29 @@ static struct exynos_irq_chip exynos_wkup_irq_chip = {
>>> .eint_pend = EXYNOS_WKUP_EPEND_OFFSET,
>>> };
>>>
>>> +static struct exynos_irq_chip exynos7_wkup_irq_chip = {
>>> + .chip = {
>>> + .name = "exynos7_wkup_irq_chip",
>>> + .irq_unmask = exynos_irq_unmask,
>>> + .irq_mask = exynos_irq_mask,
>>> + .irq_ack = exynos_irq_ack,
>>> + .irq_set_type = exynos_irq_set_type,
>>> + .irq_set_wake = exynos_wkup_irq_set_wake,
>>> + },
>>> + .eint_con = EXYNOS7_WKUP_ECON_OFFSET,
>>> + .eint_mask = EXYNOS7_WKUP_EMASK_OFFSET,
>>> + .eint_pend = EXYNOS7_WKUP_EPEND_OFFSET,
>>> +};
>>> +
>>> +/* list of external wakeup controllers supported */
>>> +static const struct of_device_id exynos_wkup_irq_ids[] = {
>>> + { .compatible = "samsung,exynos4210-wakeup-eint",
>>> + .data = &exynos4210_wkup_irq_chip },
>>> + { .compatible = "samsung,exynos7-wakeup-eint",
>>> + .data = &exynos7_wkup_irq_chip },
>>> + { }
>>> +};
>>> +
>>> /* interrupt handler for wakeup interrupts 0..15 */
>>> static void exynos_irq_eint0_15(unsigned int irq, struct irq_desc *desc)
>>> {
>>> @@ -434,7 +453,12 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
>>> int idx, irq;
>>>
>>> for_each_child_of_node(dev->of_node, np) {
>>> - if (of_match_node(exynos_wkup_irq_ids, np)) {
>>> + const struct of_device_id *match;
>>> +
>>> + match = of_match_node(exynos_wkup_irq_ids, np);
>>> + if (match) {
>>> + memcpy(&exynos_wkup_irq_chip, match->data,
>>> + sizeof(struct exynos_irq_chip));
>>
>> Hmm, this doesn't look correct to me. You are modifying a static struct
>> here. Why couldn't you simply use the exynos irq chip pointed by
>> match->data in further registration code?
>
> That will not be available later once I use __initdata.
>
Then either __initdata shouldn't be necessary or kmemdup() should be
used to allocate a copy.
>>
>>> wkup_np = np;
>>> break;
>>> }
>>> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.h b/drivers/pinctrl/samsung/pinctrl-exynos.h
>>> index e060722..0db1e52 100644
>>> --- a/drivers/pinctrl/samsung/pinctrl-exynos.h
>>> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.h
>>> @@ -25,6 +25,9 @@
>>> #define EXYNOS_WKUP_ECON_OFFSET 0xE00
>>> #define EXYNOS_WKUP_EMASK_OFFSET 0xF00
>>> #define EXYNOS_WKUP_EPEND_OFFSET 0xF40
>>> +#define EXYNOS7_WKUP_ECON_OFFSET 0x700
>>> +#define EXYNOS7_WKUP_EMASK_OFFSET 0x900
>>> +#define EXYNOS7_WKUP_EPEND_OFFSET 0xA00
>>
>> Interestingly enough, the offsets look just like the normal GPIO
>> interrupt controller of previous Exynos SoCs. Are you sure those are
>> correct? Also if somehow the controller now resembles the normal one,
>> doesn't it have the SVC register making it possible to reuse the non
>> wake-up code instead?
>
> The wakeup interrupt register offsets are the same as the GPIO
> interrupt offsets in earlier Exynos SoCs. There is no SVC register for
> the wakeup interrupt block.
OK, your code is fine then.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
