On Tue, Jun 1, 2010 at 3:19 AM, Ladislav Michl <Ladislav.Michl@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxx>
>> + *
>> + * 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@xxxxxxxxxxx>
>> + *
>> + * 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@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
