Re: [PATCH 1/4] [OMAP1] Add MMC board code common to HTC devices

[Cory Maccarrone <darkstar6262@gmail.com>]

On Tue, Jun 1, 2010 at 3:19 AM, Ladislav Michl <Ladislav.Michl@seznam.cz> wrote:
> On Fri, May 28, 2010 at 10:28:04PM -0700, Cory Maccarrone wrote:
>> This change adds the htc-mmc.c and htc-mmc.h files that
>> contain common setup code for many OMAP850-based HTC
>> smartphones.  This code is a port of the code used by
>> the linwizard project to support devices such as the
>> HTC Wizard, HTC Herald, HTC Opal, etc.
>>
>> Signed-off-by: Cory Maccarrone <darkstar6262@gmail.com>
>> ---
>>  arch/arm/mach-omap1/htc-mmc.c |  104 +++++++++++++++++++++++++++++++++++++++++
>>  arch/arm/mach-omap1/htc-mmc.h |   26 ++++++++++
>>  2 files changed, 130 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/arm/mach-omap1/htc-mmc.c
>>  create mode 100644 arch/arm/mach-omap1/htc-mmc.h
>>
>> diff --git a/arch/arm/mach-omap1/htc-mmc.c b/arch/arm/mach-omap1/htc-mmc.c
>> new file mode 100644
>> index 0000000..4fa8bb4
>> --- /dev/null
>> +++ b/arch/arm/mach-omap1/htc-mmc.c
>> @@ -0,0 +1,104 @@
>> +/*
>> + * linux/arch/arm/mach-omap1/htc-mmc.c
>> + *
>> + * Copyright (C) 2007 Instituto Nokia de Tecnologia - INdT
>> + * Author: Felipe Balbi <felipe.lima@indt.org.br>
>> + *
>> + * This code is based on linux/arch/arm/mach-omap2/board-n800-mmc.c, which is:
>> + * Copyright (C) 2006 Nokia Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <plat/mmc.h>
>> +#include <asm/mach-types.h>
>> +
>> +#include <linux/gpio.h>
>> +#include <linux/delay.h>
>> +
>> +#if defined(CONFIG_MMC_OMAP) || defined(CONFIG_MMC_OMAP_MODULE)
>> +
>> +#define OMAP_MMC_REG_SYSC (0xfffb7800 + 0x32)
>> +#define OMAP_MMC_REG_SYSS (0xfffb7800 + 0x34)
>
> Where are these used?
>
>> +static int slot_cover_open;
>> +static struct device *mmc_device;
>> +
>> +static int htc_mmc_set_power(struct device *dev, int slot, int power_on,
>> +                             int vdd)
>> +{
>> +#ifdef CONFIG_MMC_DEBUG
>> +     dev_dbg(dev, "Set slot %d power: %s (vdd %d)\n", slot + 1,
>> +             power_on ? "on" : "off", vdd);
>> +#endif
>> +     if (slot != 0) {
>> +             dev_err(dev, "No such slot %d\n", slot + 1);
>> +             return -ENODEV;
>> +     }
>> +
>> +     return 0;
>> +}
>
> No-op?
>
>> +static int htc_mmc_set_bus_mode(struct device *dev, int slot, int bus_mode)
>> +{
>> +#ifdef CONFIG_MMC_DEBUG
>> +     dev_dbg(dev, "Set slot %d bus_mode %s\n", slot + 1,
>> +             bus_mode == MMC_BUSMODE_OPENDRAIN ? "open-drain" : "push-pull");
>> +#endif
>> +     if (slot != 0) {
>> +             dev_err(dev, "No such slot %d\n", slot + 1);
>> +             return -ENODEV;
>> +     }
>> +
>> +     /* Treated on upper level */
>> +
>> +     return bus_mode;
>> +}
>> +
>> +static int htc_mmc_get_cover_state(struct device *dev, int slot)
>> +{
>> +     BUG_ON(slot != 0);
>> +     return slot_cover_open;
>> +}
>
> Just a complicated way to return zero...
>
>> +static int htc_mmc_late_init(struct device *dev)
>> +{
>> +     mmc_device = dev;
>> +     return 0;
>> +}
>
> What is this good for?
>
>> +static void htc_mmc_cleanup(struct device *dev)
>> +{
>> +}
>> +
>> +static struct omap_mmc_platform_data htc_mmc1_data = {
>> +     .nr_slots                       = 1,
>> +     .switch_slot                    = NULL,
>> +     .init                           = htc_mmc_late_init,
>> +     .cleanup                        = htc_mmc_cleanup,
>> +     .slots[0]       = {
>> +             .set_power              = htc_mmc_set_power,
>> +             .set_bus_mode           = htc_mmc_set_bus_mode,
>> +             .get_ro                 = NULL,
>> +             .get_cover_state        = htc_mmc_get_cover_state,
>> +             .ocr_mask               = MMC_VDD_28_29 | MMC_VDD_30_31 |
>> +                                       MMC_VDD_32_33 | MMC_VDD_33_34,
>> +             .name                   = "mmcblk",
>> +             .nomux                  = 1,
>> +             .wires                  = 4,
>> +             .switch_pin             = -1,
>> +     },
>> +};
>
> To me above seems completely no-op, so it could be simplified this way:
> static struct omap_mmc_platform_data htc_mmc1_data = {
>        .nr_slots                       = 1,
>        .switch_slot                    = NULL,
>        .slots[0]       = {
>                .ocr_mask               = MMC_VDD_28_29 | MMC_VDD_30_31 |
>                                          MMC_VDD_32_33 | MMC_VDD_33_34,
>                .name                   = "mmcblk",
>                .nomux                  = 1,
>                .wires                  = 4,
>                .switch_pin             = -1,
>        },
> };
>
> Now, once this file contains only one structure definition, is it worth
> to let it exist at all? What about adding it to board-htcherald.c instead?
>
>> +
>> +static struct omap_mmc_platform_data *htc_mmc_data[1];
>> +
>> +void __init htc_mmc_init(void)
>> +{
>> +     /* register it */
>> +     htc_mmc_data[0] = &htc_mmc1_data;
>> +     omap1_init_mmc(htc_mmc_data, 1);
>> +}
>> +#endif
>> +
>> diff --git a/arch/arm/mach-omap1/htc-mmc.h b/arch/arm/mach-omap1/htc-mmc.h
>> new file mode 100644
>> index 0000000..480de14
>> --- /dev/null
>> +++ b/arch/arm/mach-omap1/htc-mmc.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * linux/arch/arm/mach-omap1/htc-mmc.h
>> + *
>> + * Copyright (C) 2007 Instituto Nokia de Tecnologia - INdT
>> + * Author: Felipe Balbi <felipe.lima@indt.org.br>
>> + *
>> + * This code is based on linux/arch/arm/mach-omap2/board-n800-mmc.c, which is:
>> + * Copyright (C) 2006 Nokia Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#ifndef __HTC_MMC_H
>> +#define __HTC_MMC_H
>> +
>> +#if defined(CONFIG_MMC_OMAP) || defined(CONFIG_MMC_OMAP_MODULE)
>> +extern void htc_mmc_init(void);
>> +#else
>> +static inline void htc_mmc_init(void)
>> +{
>> +}
>> +#endif
>> +
>> +#endif /* __HTC_MMC_H */
>> +
>> --
>> 1.7.0.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Thanks for the feedback.  I had originally intended this code to be
used by other devices that have yet to be submitted, but looking at
your comments, you're right -- most of that is a no-op.  It can very
easily be moved directly into the board file.

This patch can be disregarded, and I'll submit a revised version of
patch 4 for the herald.

Thanks.
- Cory
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html