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.pinch...@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Wednesday 27 November 2013 16:41:22 Magnus Damm wrote:
>> From: Magnus Damm <d...@opensource.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 <d...@opensource.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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
