Re: [PATCH 04/11] ARM: shmobile: lager legacy: Add MSIOF support

[Magnus Damm]

Hi Geert,

On Fri, Feb 21, 2014 at 1:18 AM, Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
> Hi Magnus,
>
> On Thu, Feb 20, 2014 at 4:48 PM, Magnus Damm <magnus.damm@xxxxxxxxx> wrote:
>>> +/* MSIOF */
>>> +static const struct resource sh_msiof0_resources[] __initconst = {
>>> +       DEFINE_RES_MEM(0xe6e20000, 0x0064),
>>> +       DEFINE_RES_IRQ(gic_spi(156)),
>>> +};
>>> +
>>> +static const struct resource sh_msiof1_resources[] __initconst = {
>>> +       DEFINE_RES_MEM(0xe6e10000, 0x0064),
>>> +       DEFINE_RES_IRQ(gic_spi(157)),
>>> +};
>>> +
>>> +static const struct resource sh_msiof2_resources[] __initconst = {
>>> +       DEFINE_RES_MEM(0xe6e00000, 0x0064),
>>> +       DEFINE_RES_IRQ(gic_spi(158)),
>>> +};
>>> +
>>> +static const struct resource sh_msiof3_resources[] __initconst = {
>>> +       DEFINE_RES_MEM(0xe6c90000, 0x0064),
>>> +       DEFINE_RES_IRQ(gic_spi(159)),
>>> +};
>>> +
>>> +static const struct sh_msiof_spi_info sh_msiof_info __initconst = {
>>> +       .rx_fifo_override       = 256,
>>> +       .num_chipselect         = 1,
>>> +};
>>> +
>>> +#define r8a7790_register_msiof(idx)                                    \
>>> +       platform_device_register_resndata(&platform_bus,                \
>>> +                               "spi_r8a7790_msiof",                    \
>>> +                               (idx+1), sh_msiof##idx##_resources,     \
>>> +                               ARRAY_SIZE(sh_msiof##idx##_resources),  \
>>> +                               &sh_msiof_info,                         \
>>> +                               sizeof(struct sh_msiof_spi_info))
>>
>> That for your efforts - it's good to see the MSIOF being integrated as
>> well! I have one comment on this legacy board integration code.
>>
>> Since only MSIOF1 is used on Lager (correct me if i'm wrong), isn't it
>> best to omit the unused resources from above? In case of DT I think it
>> makes sense to define all channels in the SoC.dtsi and let the
>> SoC-board.dts just enable the channels that are used. But in this case
>> with legacy code  I think we should keep thing simple and small and
>> just enable the bits that are used on the particular board.
>>
>> The same obviously applies to the Koelsch legacy code as well. =)
>
> Note that while all resources are present, only MSIOF1 is registered on
> Lager (MSIOF0 on Koelsch). This is similar to i2c on Koelsch, which also
> has all resources, but only registers active devices.

Ok, I understand. Thanks for brining this to my attention.

I'd like to avoid having unused resources so I'll cook up a patch to
rework that myself.

> It's your preference, though, so I can adapt if you want.

Thanks.

Please rework this patch to only register a single MSIOF channel. I
think it makes sense to only enable hardware that is being used.

Another question: How about "bus_num" and the platform device id
mapping? I'd like them to be the same if possible, but you are having
this "(idx+1)" bit in your code which I assume is to add offset for
the QSPI bus.

Regarding the PFC configuration, can you please double check that the
PIN_MAP_MUX_GROUP_DEFAULT() is in sync with the platform device id? Is
it the "bus_num" or the platform device id that is being used in case
of SPI?

Thanks!

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