| Message ID | 20110523171023.4df5af10.ospite@studenti.unina.it (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 23 May 2011 17:10:23 +0200 Antonio Ospite <ospite@studenti.unina.it> wrote: > On Wed, 18 May 2011 12:42:12 -0700 > Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > > > On Wed, May 18, 2011 at 07:23:20PM +0200, Antonio Ospite wrote: > > > [...] > > > 2. Add a proper function with all the needed parameters to abstract > > > the actual var names, this would be something like: > > > mmc_regulator_set_power(mmc, power_mode, vdd, vcc, pdata) > > > and yet checking for pdata->setpower can be tricky as 'setpower' > > > is an arbitrary name. > > > > This would be good. > > A WIP patch, I don't have a lot of time to spend on this, I tried to > handle the driver specific 'setpower' callback with a macro, let me > know if you have a better idea: > > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index bcb793e..dbbe535 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -298,6 +298,32 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc, > } > #endif > > +#define STRUCT_FIELD(s, f) ((s) && (s)->f ? (s)->f : NULL ) > + Any opinion on this macro? See its use below. It is meant to deal with driver specific struct fields, which can have arbitrary names, I though that using some syntactic sugar to deal with those as arguments when calling the function was not that horrible. If that looks acceptable to you too I will submit the mmc_regulator_set_power () patch, otherwise I would ask to consider the simple patch to mmc_spi.c for now. [...] > diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c > index 2e8db20..c9a229f 100644 > --- a/drivers/mmc/host/mmc_spi.c > +++ b/drivers/mmc/host/mmc_spi.c > @@ -1231,21 +1231,18 @@ static inline int mmc_spi_setpower(struct mmc_spi_host *host, > unsigned char power_mode, > unsigned int vdd) > { > + int ret; > + > if (!mmc_spi_canpower(host)) > return -ENOSYS; > > - if (host->vcc) { > - int ret; > > - if (power_mode == MMC_POWER_OFF) > - vdd = 0; > + ret = mmc_regulator_set_power(host->mmc, power_mode, host->vcc, vdd, > + &host->spi->dev, > + STRUCT_FIELD(host->pdata, setpower)); ^^^^^^^^^^^^^ > + if (ret) > + return ret; > > - ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd); > - if (ret) > - return ret; > - } else { > - host->pdata->setpower(&host->spi->dev, vdd); > - } > > if (power_mode == MMC_POWER_UP) > msleep(host->powerup_msecs); Thanks, Antonio
On Mon, May 30, 2011 at 11:07:49AM +0200, Antonio Ospite wrote: > On Mon, 23 May 2011 17:10:23 +0200 > > +#define STRUCT_FIELD(s, f) ((s) && (s)->f ? (s)->f : NULL ) > Any opinion on this macro? See its use below. It is meant to deal with > driver specific struct fields, which can have arbitrary names, I though > that using some syntactic sugar to deal with those as arguments when > calling the function was not that horrible. > If that looks acceptable to you too I will submit the > mmc_regulator_set_power () patch, otherwise I would ask to consider the > simple patch to mmc_spi.c for now. Would it not be simpler just to provide a standard generic struct that people can embed into their pdata? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 30 May 2011 18:14:48 +0800 Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, May 30, 2011 at 11:07:49AM +0200, Antonio Ospite wrote: > > On Mon, 23 May 2011 17:10:23 +0200 > > > > +#define STRUCT_FIELD(s, f) ((s) && (s)->f ? (s)->f : NULL ) > > > Any opinion on this macro? See its use below. It is meant to deal with > > driver specific struct fields, which can have arbitrary names, I though > > that using some syntactic sugar to deal with those as arguments when > > calling the function was not that horrible. > > > If that looks acceptable to you too I will submit the > > mmc_regulator_set_power () patch, otherwise I would ask to consider the > > simple patch to mmc_spi.c for now. > > Would it not be simpler just to provide a standard generic struct that > people can embed into their pdata? > I thought to something like: struct mmc_driver_ops { int (*setpower)(struct device *, unsigned int); }; struct pxamci_platform_data { [...] struct mmc_driver_ops *ops; }; static inline int mmc_regulator_set_power(struct mmc_host *mmc, unsigned char power_mode, struct regulator *supply, unsigned short vdd_bit, struct device *device, struct mmc_driver_ops ops) { [...] if (ops->setpower) ops->setpower(device, vdd_bit); [...] } static inline int pxamci_set_power(struct pxamci_host *host, unsigned char power_mode, unsigned int vdd) { [...] ret = mmc_regulator_set_power(host->mmc, power_mode, host->vcc, vdd, mmc_dev(host->mmc), host->pdata->ops); [...] } It is cleaner and more uniform indeed, but it does not solve the problem I am seeing, the issue is still there when pdata is NULL, we are at the same point as before (the current code checks for (pdata && pdata->field)), and we cannot pass pdata directly as a function argument (can we?) as it is driver specific as well. I would be glad to discover than I am still missing something :) Thanks, Antonio
On Mon, May 30, 2011 at 12:57:57PM +0200, Antonio Ospite wrote: > It is cleaner and more uniform indeed, but it does not solve the problem > I am seeing, the issue is still there when pdata is NULL, we are at the > same point as before (the current code checks for (pdata && > pdata->field)), and we cannot pass pdata directly as a function argument > (can we?) as it is driver specific as well. You can do a nasty type punning trick if the generic pdata is the first member of the platform data, otherwise the easiest thing is usually to provide a defualt pdata if the pdata is NULL. I'd expect that in a lot of cases the standard platform data would be the only platform data so for many drivers they wouldn't need to worry about extra data but perhaps MMC isn't like that, I've never really looked at the code. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index bcb793e..dbbe535 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -298,6 +298,32 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc, } #endif +#define STRUCT_FIELD(s, f) ((s) && (s)->f ? (s)->f : NULL ) + +static inline int mmc_regulator_set_power(struct mmc_host *mmc, + unsigned char power_mode, + struct regulator *supply, + unsigned short vdd_bit, + struct device *device, + void (*setpower_cb)(struct device *, unsigned int)) +{ + if (supply) { + int ret; + + if (power_mode == MMC_POWER_OFF) + vdd_bit = 0; + + ret = mmc_regulator_set_ocr(mmc, supply, vdd_bit); + if (ret) + return ret; + } else { + if (setpower_cb) + setpower_cb(device, vdd_bit); + } + + return 0; +} + int mmc_card_awake(struct mmc_host *host); int mmc_card_sleep(struct mmc_host *host); int mmc_card_can_sleep(struct mmc_host *host); diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c index 2e8db20..c9a229f 100644 --- a/drivers/mmc/host/mmc_spi.c +++ b/drivers/mmc/host/mmc_spi.c @@ -1231,21 +1231,18 @@ static inline int mmc_spi_setpower(struct mmc_spi_host *host, unsigned char power_mode, unsigned int vdd) { + int ret; + if (!mmc_spi_canpower(host)) return -ENOSYS; - if (host->vcc) { - int ret; - if (power_mode == MMC_POWER_OFF) - vdd = 0; + ret = mmc_regulator_set_power(host->mmc, power_mode, host->vcc, vdd, + &host->spi->dev, + STRUCT_FIELD(host->pdata, setpower)); + if (ret) + return ret; - ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd); - if (ret) - return ret; - } else { - host->pdata->setpower(&host->spi->dev, vdd); - } if (power_mode == MMC_POWER_UP) msleep(host->powerup_msecs); diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c index fbdb21e..f5d9529 100644 --- a/drivers/mmc/host/pxamci.c +++ b/drivers/mmc/host/pxamci.c @@ -103,29 +103,20 @@ static inline int pxamci_set_power(struct pxamci_host *host, unsigned int vdd) { int on; + int ret; - if (host->vcc) { - int ret; + ret = mmc_regulator_set_power(host->mmc, power_mode, host->vcc, vdd, + mmc_dev(host->mmc), + STRUCT_FIELD(host->pdata, setpower)); + if (ret) + return ret; - if (power_mode == MMC_POWER_UP) { - ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd); - if (ret) - return ret; - } else if (power_mode == MMC_POWER_OFF) { - ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0); - if (ret) - return ret; - } - } if (!host->vcc && host->pdata && gpio_is_valid(host->pdata->gpio_power)) { on = ((1 << vdd) & host->pdata->ocr_mask); gpio_set_value(host->pdata->gpio_power, !!on ^ host->pdata->gpio_power_invert); } - if (!host->vcc && host->pdata && host->pdata->setpower) - host->pdata->setpower(mmc_dev(host->mmc), vdd); - return 0; }