On Sunday 13 July 2014 14:29:31 Mollie Wu wrote:
> +Required properties:
> +- compatible: "fujitsu,f-sdh30"
> +- voltage-ranges : This is a list of pairs. In each pair, two cells
> + are required. First cell specifies minimum slot voltage (mV), second
> + cell specifies maximum slot voltage (mV). In case of supported voltage
> + range is discontinuous, several ranges could be specified as a list.
> +
> +Optional properties:
> +- pwr-mux-gpios: This is one optional gpio for controlling a power mux
> + which switches between two power supplies. 3.3V is selected when gpio
> + is high, and 1.8V is selected when gpio is low. This voltage is used
> + for signal level.
It sounds like what you really want here is a reference to a gpio-regulator
node and, and to put the details of the voltage switching in there.
> diff --git a/drivers/mmc/host/sdhci_f_sdh30.c b/drivers/mmc/host/sdhci_f_sdh30.c
> new file mode 100644
> index 0000000..8d23f2d
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci_f_sdh30.c
> @@ -0,0 +1,380 @@
> +/*
> + * linux/drivers/mmc/host/sdhci_f_sdh30.c
> + *
> + * Copyright (C) 2013 - 2014 Fujitsu Semiconductor, Ltd
> + * Vincent Yang <vincent.yang@xxxxxxxxxxxxxx>
> + * Copyright (C) 2014 Linaro Ltd Andy Green <andy.green@xxxxxxxxxx>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/mmc/sd.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/card.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/suspend.h>
> +
> +#include "sdhci.h"
> +#include "sdhci-pltfm.h"
> +#include "../core/core.h"
Should this be <linux/mmc/core.h>? A device driver should not be looking
at the drivers/mmc/core/core.h.
> +#define DRIVER_NAME "f_sdh30"
This macro doesn't seem to serve any purpose, it would be easier to
read if you open-code this.
> +
> +void sdhci_f_sdh30_soft_voltage_switch(struct sdhci_host *host)
> +{
> + struct f_sdhost_priv *priv = sdhci_priv(host);
> + u32 ctrl = 0;
> +
> + usleep_range(2500, 3000);
> + ctrl = sdhci_readl(host, F_SDH30_IO_CONTROL2);
> + ctrl |= F_SDH30_CRES_O_DN;
> + sdhci_writel(host, ctrl, F_SDH30_IO_CONTROL2);
> + ctrl |= F_SDH30_MSEL_O_1_8;
> + sdhci_writel(host, ctrl, F_SDH30_IO_CONTROL2);
> +
> + if (gpio_is_valid(priv->gpio_select_1v8)) {
> + dev_info(priv->dev, "%s: setting gpio\n", __func__);
> + gpio_direction_output(priv->gpio_select_1v8, 0);
> + }
> +
> + ctrl &= ~F_SDH30_CRES_O_DN;
> + sdhci_writel(host, ctrl, F_SDH30_IO_CONTROL2);
> + usleep_range(2500, 3000);
> +
> + if (priv->vendor_hs200) {
> + dev_info(priv->dev, "%s: setting hs200\n", __func__);
> + ctrl = sdhci_readl(host, F_SDH30_ESD_CONTROL);
> + ctrl |= priv->vendor_hs200;
> + sdhci_writel(host, ctrl, F_SDH30_ESD_CONTROL);
> + }
> +
> + ctrl = sdhci_readl(host, F_SDH30_TUNING_SETTING);
> + ctrl |= F_SDH30_CMD_CHK_DIS;
> + sdhci_writel(host, ctrl, F_SDH30_TUNING_SETTING);
> +}
I think this should really use the regulator API.
If the regular is defined properly, this will work without any extra code.
> +unsigned int sdhci_f_sdh30_get_min_clock(struct sdhci_host *host)
> +{
> + return F_SDH30_MIN_CLOCK;
> +}
> +
> +void sdhci_f_sdh30_reset(struct sdhci_host *host, u8 mask)
> +{
> + struct f_sdhost_priv *priv = sdhci_priv(host);
> +
> + if (gpio_is_valid(priv->gpio_select_1v8))
> + gpio_direction_output(priv->gpio_select_1v8, 1);
> +
> + if (sdhci_readw(host, SDHCI_CLOCK_CONTROL) == 0) {
> + sdhci_writew(host, 0xBC01, SDHCI_CLOCK_CONTROL);
> + mmiowb();
> + }
Can you explain the mmiowb call here?
> +
> + if (!of_property_read_u32(pdev->dev.of_node, "bus-width", &bus_width)) {
> + switch (bus_width) {
> + case 8:
> + dev_info(dev, "Applying 8 bit bus width\n");
> + host->mmc->caps |= MMC_CAP_8_BIT_DATA;
> + break;
> + case 4:
> + dev_info(dev, "Applying 4 bit bus width\n");
> + host->mmc->caps |= MMC_CAP_4_BIT_DATA;
> + break;
> + case 1:
> + default:
> + dev_err(dev, "Invalid bus width: %u\n", bus_width);
> + break;
> + }
> + }
This should probably be done in generic sdhci code somewhere. How about
adding it to sdhci_get_of_property instead_
> + priv->clk_sd4 = clk_get(&pdev->dev, "sd_sd4clk");
> + if (!IS_ERR(priv->clk_sd4)) {
> + ret = clk_prepare_enable(priv->clk_sd4);
> + if (ret < 0) {
> + dev_err(dev, "Failed to enable sd4 clock: %d\n", ret);
> + goto err_clk1;
> + }
> + }
> + priv->clk_b = clk_get(&pdev->dev, "sd_bclk");
> + if (!IS_ERR(priv->clk_b)) {
> + ret = clk_prepare_enable(priv->clk_b);
> + if (ret < 0) {
> + dev_err(dev, "Failed to enable clk_b clock: %d\n", ret);
> + goto err_clk2;
> + }
> + }
Please pick clock names that match what some of the other drivers use.
Ideally some of that should also move to common code.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
