[U-Boot] [PATCH 09/12] IMX: add additional function for pinmux using an array

[Tim Harvey]

On Tue, Apr 29, 2014 at 8:22 AM, Eric Nelson
<eric.nelson at boundarydevices.com> wrote:
> Hi Tim,
>

Hi Eric,

>
> On 04/28/2014 01:17 PM, Tim Harvey wrote:
>>
>>
>> Add new function that can take an array of iomux configs, an index, and
>> a stride to allow a multi-dimentional array of pinmux values to be used
>> to define pinmux values per cpu-type.
>>
>> This takes a different approach to previously proposed solutions which
>> used
>> multiple arrays of pad lists. The goal is to eliminate having these
>> multiple
>> arrays such as 'mx6q_uart1_pads' and 'mx6dl_uart1_pads' which are almost
>> identical copies of each other except for the MX6Q/MX6DL prefix on the
>> PAD.
>>
> I don't think a blind reference to the previously proposed solution
> helps in understanding this patch, and an example would be more helpful.

agreed - will remove

>
<snip>
>
>> -void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list,
>> -                                     unsigned count)
>
>
> I think 'start_offs' would be clearer than 'list'.

agreed - will change to 'start_offs'

>
> The function name ..._array() also doesn't really capture what's
> going on here. Naming is hard though, and I'm not coming up
> with something else.
>
> Perhaps 'sparse', 'skip', or alternate?

ya, I'm not sure anything else is more explanatory when we are doing
something like this. Its bad enough that its likely difficult for
someone to understand their first time through that we are doing this
to eliminate multiple structs.

>
<snip>
>
>> +/* macros for declaring and using pinmux array */
>> +#define IOMUX_PADS(x) (MX6Q_##x), (MX6DL_##x)
>
>
> In a similar vein to my comment about Patch 8, I do wonder if a
> minor extension of this will allow use with a single-variant
> board though.

for a single-variant one would just use the original
IOMUX_PAD/imx_iomux_v3_setup_pad/imx_iomux_v3_setup_pad right?

>
> We have some custom designs that really only function with
> one variant (usually i.MX6DQ) and it seems wrong to have
> the other variant included.
>
>
>> +#define SETUP_IOMUX_PAD(def)                                   \
>> +if (is_cpu_type(MXC_CPU_MX6Q)) {                               \
>> +       imx_iomux_v3_setup_pad(MX6Q_##def);                     \
>> +} else {                                                       \
>> +       imx_iomux_v3_setup_pad(MX6DL_##def);                    \
>> +}
>> +#define SETUP_IOMUX_PADS(x)                                    \
>> +       imx_iomux_v3_setup_multiple_pads_array(x,               \
>> +       ARRAY_SIZE(x)/2, is_cpu_type(MXC_CPU_MX6Q) ? 0 : 1, 2)
>> +
>>   #endif        /* __MACH_IOMUX_V3_H__*/
>>
>
> Please don't mis-interpret my comments as any form of Nack.
>
> This patch moves the ball forward, and the approach of building
> two lists into one prevents duplication of tables quite nicely.
>
> Regards,
>
>
> Eric
>

Thanks for the review!

Tim