On 09/02/2010 09:13 AM, Daniel Walker wrote:
> On Wed, 2010-09-01 at 13:59 -0500, H Hartley Sweeten wrote:
>> On Monday, August 30, 2010 5:18 PM, Gregory Bean wrote:
>>> Subject: [PATCH 1/2] msm: Install the Google-Android gpio driver.
>>>
>>> From: Arve Hjønnevåg <[email protected]>
>>>
>>> As part of the ongoing effort to converge on a common code base,
>>> adopt the Google-Android msmgpio driver, as it has a stronger pedigree
>>> than the previous submission from codeaurora.
>>>
>>> Cc: Arve Hjønnevåg <[email protected]>
>>> Signed-off-by: Gregory Bean <[email protected]>
>>
>> Hello Greg,
>>
>> A couple comments on this.
>>
>>> +struct msm_gpio_chip msm_gpio_chips[] = {
>>> + {
>>> + .regs = {
>>> + .out = GPIO_OUT_0,
>>> + .in = GPIO_IN_0,
>>> + .int_status = GPIO_INT_STATUS_0,
>>> + .int_clear = GPIO_INT_CLEAR_0,
>>> + .int_en = GPIO_INT_EN_0,
>>> + .int_edge = GPIO_INT_EDGE_0,
>>> + .int_pos = GPIO_INT_POS_0,
>>> + .oe = GPIO_OE_0,
>>> + },
>>> + .chip = {
>>> + .base = 0,
>>> + .ngpio = 16,
>>> + .get = msm_gpio_get,
>>> + .set = msm_gpio_set,
>>> + .direction_input = msm_gpio_direction_input,
>>> + .direction_output = msm_gpio_direction_output,
>>> + .to_irq = msm_gpio_to_irq,
>>> + }
>>> + },
>>
>> This is a bit ugly... A 204 line struct definition is a bit hard to follow,
>> especially the way it's broken up with all the #if defined stuff.
>>
>> Each gpio "bank" is code duplication other than the .base and .ngpio. The
>> whole thing can be reduced using a helper macro. Something like this:
>>
>> #define MSM_GPIO_BANK(bank, base_gpio, num_gpio) \
>> {
>> \
>> .regs = {
>> \
>> .out = GPIO_OUT_##bank,
>> \
>> .in = GPIO_IN_##bank,
>> \
>> .int_status = GPIO_INT_STATUS_##bank,
>> \
>> .int_clear = GPIO_INT_CLEAR_##bank,
>> \
>> .int_en = GPIO_INT_EN_##bank, \
>> .int_edge = GPIO_INT_EDGE_##bank,
>> \
>> .int_pos = GPIO_INT_POS_##bank,
>> \
>> .oe = GPIO_OE_##bank,
>> \
>> },
>> \
>> .chip = {
>> \
>> .direction_input = msm_gpio_direction_input,
>> \
>> .direction_output = msm_gpio_direction_output,
>> \
>> .get = msm_gpio_get,
>> \
>> .set = msm_gpio_set,
>> \
>> .to_irq = msm_gpio_to_irq, \
>> .base = base_gpio,
>> \
>> .ngpio = num_gpio,
>> \
>> },
>> \
>> }
>>
>> Then the struct definition can be reduced to this:
>>
>> struct msm_gpio_chip msm_gpio_chips[] = {
>> #if defined(CONFIG_ARCH_MSM7X30)
>> MSM_GPIO_BANK(0, 0, 16), /* gpio 0-15 */
>> MSM_GPIO_BANK(1, 16, 28), /* gpio 16-43 */
>> MSM_GPIO_BANK(2, 44, 24), /* gpio 44-67 */
>> MSM_GPIO_BANK(3, 68, 27), /* gpio 68-94 */
>> MSM_GPIO_BANK(4, 95, 12), /* gpio 95-106 */
>> MSM_GPIO_BANK(5, 107, 27), /* gpio 107-133 */
>> MSM_GPIO_BANK(6, 134, 17), /* gpio 134-150 */
>> MSM_GPIO_BANK(7, 151, 31), /* gpio 151-181 */
>> #elif defined(CONFIG_ARCH_QSD8X50)
>> MSM_GPIO_BANK(0, 0, 16), /* gpio 0-15 */
>> MSM_GPIO_BANK(1, 16, 27), /* gpio 16-42 */
>> MSM_GPIO_BANK(2, 43, 25), /* gpio 43-67 */
>> MSM_GPIO_BANK(3, 68, 27), /* gpio 68-94 */
>> MSM_GPIO_BANK(4, 95, 9), /* gpio 95-103 */
>> MSM_GPIO_BANK(5, 104, 18), /* gpio 104-121 */
>> MSM_GPIO_BANK(6, 122, 31), /* gpio 122-152 */
>> MSM_GPIO_BANK(7, 153, 12), /* gpio 153-164 */
>> #else
>> MSM_GPIO_BANK(0, 0, 16), /* gpio 0-15 */
>> MSM_GPIO_BANK(1, 16, 27), /* gpio 16-42 */
>> MSM_GPIO_BANK(2, 43, 25), /* gpio 43-67 */
>> MSM_GPIO_BANK(3, 68, 27), /* gpio 68-94 */
>> MSM_GPIO_BANK(4, 95, 12), /* gpio 95-106 */
>> MSM_GPIO_BANK(5, 107, 15), /* gpio 107-121 */
>> #endif
>> };
Which could also be further reduced to:
struct msm_gpio_chip msm_gpio_chips[] = {
MSM_GPIO_BANK(0, 0, 16), /* gpio 0-15 */
#if defined(CONFIG_ARCH_MSM7X30)
MSM_GPIO_BANK(1, 16, 28), /* gpio 16-43 */
MSM_GPIO_BANK(2, 44, 24), /* gpio 44-67 */
MSM_GPIO_BANK(3, 68, 27), /* gpio 68-94 */
MSM_GPIO_BANK(4, 95, 12), /* gpio 95-106 */
MSM_GPIO_BANK(5, 107, 27), /* gpio 107-133 */
MSM_GPIO_BANK(6, 134, 17), /* gpio 134-150 */
MSM_GPIO_BANK(7, 151, 31), /* gpio 151-181 */
#else
MSM_GPIO_BANK(1, 16, 27), /* gpio 16-42 */
MSM_GPIO_BANK(2, 43, 25), /* gpio 43-67 */
MSM_GPIO_BANK(3, 68, 27), /* gpio 68-94 */
#if defined(CONFIG_ARCH_QSD8X50)
MSM_GPIO_BANK(4, 95, 9), /* gpio 95-103 */
MSM_GPIO_BANK(5, 104, 18), /* gpio 104-121 */
MSM_GPIO_BANK(6, 122, 31), /* gpio 122-152 */
MSM_GPIO_BANK(7, 153, 12), /* gpio 153-164 */
#else
MSM_GPIO_BANK(4, 95, 12), /* gpio 95-106 */
MSM_GPIO_BANK(5, 107, 15), /* gpio 107-121 */
#endif
#endif
~Ryan
--
Bluewater Systems Ltd - ARM Technology Solution Centre
Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
