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

From: Nikita Kiryanov <nikita@compulab.co.il>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 09/12] IMX: add additional function for pinmux using an array
Date: Wed, 07 May 2014 19:59:26 +0300	[thread overview]
Message-ID: <536A666E.6030000@compulab.co.il> (raw)
In-Reply-To: <CAJ+vNU3QRZ_ircuZQn7m4TTXfOxuF0kEQOKMdK112A-aSFi2Nw@mail.gmail.com>

On 06/05/14 07:35, Tim Harvey wrote:
> On Tue, Apr 29, 2014 at 8:22 AM, Eric Nelson
> <eric.nelson@boundarydevices.com> wrote:
<snip>
>> 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.

Come to think of it, I don't think we need an _array() function at all. 
The list selection and
stride size are IOMUX_PADS implementation details. It's not something we 
should expose to
the function user. is_cpu_type() and ifdef(CONFIG_MX6QDL) can be used to 
decide the
list and stride values inside imx_iomux_v3_setup_multiple_pads(), and 
then this function
could be used for both single and multi cpu type situations.

>
> <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?

They can, but then we don't get to use the same code for both
situations.
If we define two versions of IOMUX_PADS: one for multi cpu type,
and one for single cpu type, then the pinmux arrays for both
situations will be syntactically similar.
When combined with my other suggestion, it will be very easy to
take a U-Boot configured for one CPU type, and reconfigure it to
support both CPU types.

>
>> 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
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
  --
Regards,
Nikita Kiryanov