linux-kernel - Re: [PATCH v2 3/7] pinctrl: samsung: Add the support the multiple IORESOURCE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Aug 2016 23:41:04 +0900
From:   Tomasz Figa <tomasz.figa@...il.com>
To:     Chanwoo Choi <cw00.choi@...sung.com>
Cc:     Krzysztof Kozłowski <k.kozlowski@...sung.com>,
        Kukjin Kim <kgene@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        "linux-samsung-soc@...r.kernel.org" 
        <linux-samsung-soc@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Jaehoon Chung <jh80.chung@...sung.com>,
        "sw0312.kim" <sw0312.kim@...sung.com>,
        Joonyoung Shim <jy0922.shim@...sung.com>,
        InKi Dae <inki.dae@...sung.com>,
        Jonghwa Lee <jonghwa3.lee@...sung.com>,
        Beomho Seo <beomho.seo@...sung.com>, jaewon02.kim@...sung.com,
        human.hwang@...sung.com, Inha Song <ideal.song@...sung.com>,
        ingi2.kim@...sung.com, Marek Szyprowski <m.szyprowski@...sung.com>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Sylwester Nawrocki <s.nawrocki@...sung.com>,
        chanwoo@...nel.org, Linus Walleij <linus.walleij@...aro.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v2 3/7] pinctrl: samsung: Add the support the multiple
 IORESOURCE_MEM for one pin-bank

2016-08-25 23:30 GMT+09:00 Tomasz Figa <tomasz.figa@...il.com>:
>> +       }
>> +
>> +#define EXYNOS_PIN_BANK_EINTN_EXT(pins, reg, id, pctl_idx, eint_idx)   \
>> +       {                                               \
>> +               .type           = &bank_type_off,       \
>> +               .pctl_offset    = reg,                  \
>> +               .nr_pins        = pins,                 \
>> +               .eint_type      = EINT_TYPE_NONE,       \
>> +               .name           = id,                   \
>> +               .pctl_res_idx   = pctl_idx,             \
>> +               .eint_res_idx   = eint_dix              \
>> +       }
>
> Your patch 4/7 doesn't seem to use this one, so this is dead code for
> the time being. Please add when there is real need for it.
>
> Also it doesn't really make much sense to have index for both pctl and
> eint. Please define first entry of regs property as always pointing to
> pctl registers and by also eint registers for usual controllers. Then
> second regs entry would be eint registers for controllers with
> separate register blocks. Then there is only a need to have
> eint_res_idx in the driver and no need for pctl_res_idx, because it
> would be always 0.

Ah, sorry, I got confused again by which registers are where in these
GPF banks. Let's make it the other way around and make DT contain eint
registers in first regs entry and hardcode eint_res_idx to 0 for the
time being. However it should be still beneficial to refactor the code
and calculate per-bank eint_base to avoid adding the offset every
time, similarly to pctl_base/offset, from my suggestion below.

>> @@ -345,7 +346,8 @@ static void pin_to_reg_bank(struct samsung_pinctrl_drv_data *drvdata,
>>                         ((b->pin_base + b->nr_pins - 1) < pin))
>>                 b++;
>>
>> -       *reg = drvdata->virt_base + b->pctl_offset;
>> +       pctl_res_idx = b->pctl_res_idx;
>> +       *reg = drvdata->virt_base[pctl_res_idx] + b->pctl_offset;
>
> I suggested something slightly different. Instead of
> bank::pctl_res_idx, I proposed bank::pctl_base.
> bank_info::pctl_res_idx would be specified only in init driver data
> and bank::pctl_base would be calculated at probe time as
> drvdata->virt_base[bank_info->pctl_res_idx] + bank_info->pctl_offset.
> This would eliminate the need to do any indexing and adding further in
> the code and make things simpler.
>
> Taking my other comments above, pctl part would be unchanged and only
> eint addresses and offsets would be affected.

Ah, scratch this one sentence. I got confused with the register layout
again, sorry. Please refactor both eint and pctl as I suggested in the
upper paragraph.

Best regards,
Tomasz