linux-kernel - Re: [PATCH] pinctrl: sh-pfc: Initial r7s72100 support

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] [day] [month] [year] [list]

Message-ID: <CANqRtoTuev8Z0ZgNwWdvuPbLPdfkT2S6MmKYokA15KPVV_ZiQg@mail.gmail.com>
Date:	Wed, 18 Dec 2013 08:31:06 +0900
From:	Magnus Damm <magnus.damm@...il.com>
To:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	Wolfram Sang <wsa@...-dreams.de>,
	Linus Walleij <linus.walleij@...aro.org>,
	SH-Linux <linux-sh@...r.kernel.org>,
	"Simon Horman [Horms]" <horms@...ge.net.au>
Subject: Re: [PATCH] pinctrl: sh-pfc: Initial r7s72100 support

Hi Laurent,

Here are some replies to your comments, my apologies for the delay.

On Wed, Nov 27, 2013 at 7:54 PM, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Wednesday 27 November 2013 16:41:22 Magnus Damm wrote:
>> From: Magnus Damm <damm@...nsource.se>
>>
>> Add r7s72100 PINCTRL support via sh-pfc. At this point this code
>> provides enough support to allow use together with the gpio-rz
>> driver. Incremental per-device patches will in the future be
>> submitted on top of this base patch to allow PINCTRL enablement
>> for each individual device.
>>
>> Signed-off-by: Magnus Damm <damm@...nsource.se>
>> ---
>>
>>  drivers/pinctrl/sh-pfc/Kconfig        |    5
>>  drivers/pinctrl/sh-pfc/Makefile       |    1
>>  drivers/pinctrl/sh-pfc/core.c         |    9 +
>>  drivers/pinctrl/sh-pfc/core.h         |    1
>>  drivers/pinctrl/sh-pfc/pfc-r7s72100.c |  166 ++++++++++++++++++++++++++++++
>>  5 files changed, 182 insertions(+)
>
> [snip]
>
>> --- /dev/null
>> +++ work/drivers/pinctrl/sh-pfc/pfc-r7s72100.c        2013-11-27
> 15:20:28.000000000
>> +0900 @@ -0,0 +1,166 @@
>
> [snip]
>
>> +#define RZ_PORT_PIN(bank, pin) (((bank) * 16) + (pin))
>
> Could you move this below closer to the location where it will be used, right
> after the pinmux_pins array ?

Yes, I did in V2. I still prefer to have it in the GPIO header so it
can be shared between PFC and GPIO.

>> +#define PORT_P_1(bank, pin, fn, sfx) fn(bank, pin, P_##bank##_##pin, sfx)
>
> I'm tempted to call the pins GP_ instead of P_ to be able to reuse the
> existing macros. On the other hand I suppose the datasheet uses P, not GP.
> What's your opinion on that ?

The data sheet is using P so I prefer to stick to that. Also,
regarding code sharing, I think it makes sense to move the 16-bit
register support to a shared location when more than one SoC will use
it. For now this SoC is the only user so we may as well keep it here
for now.

>> +     PINMUX_FUNCTION_BEGIN,
>> +     P_ALL(PMC_0), P_ALL(PMC_1),
>> +     P_ALL(PFC_0), P_ALL(PFC_1),
>> +     P_ALL(PFCE_0), P_ALL(PFCE_1),
>> +     P_ALL(PFCAE_0), P_ALL(PFCAE_1),
>> +     P_ALL(PIBC_0), P_ALL(PIBC_1),
>> +     P_ALL(PBDC_0), P_ALL(PBDC_1),
>> +     P_ALL(PIPC_0), P_ALL(PIPC_1),
>> +     PINMUX_FUNCTION_END,
>
> Only the PMC_1 and PFC* registers seem to be used in your later patches. What
> are the other registers for ? Shouldn't they be omitted if unused ?

Um, all registers expect PIPC were used in this version unless I'm
mistaken. I started using PIPC to V2 but that turned out to cause
issues with the I2C pin functions.

>> +#define _P_ALL(n) CPU_ALL_PORT(n, unused)
>> +
>> +#define _P_GPIO(bank, _pin, _name, sfx)                                      \
>> +     [(bank * 16) + _pin] = {                                        \
>> +             .pin = (bank * 16) + _pin,                              \
>> +             .name = __stringify(_name),                             \
>> +             .enum_id = _name##_DATA,                                \
>> +     }
>
> What about renaming _GP_GPIO to _GP_GPIO32 in sh_pfc.h, and move this macro to
> that header, calling it _GP_GPIO16 ?

Done as a separate patch in the V2 patch series.

Thanks,

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