| Message ID | 1343393162-11938-6-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 27, 2012 at 01:45:49PM +0100, Lee Jones wrote: > + if (of_get_property(np, "stericsson,use-pinctrl", NULL)) > + msp->use_pinctrl = true; > + else > + msp->use_pinctrl = false; I don't recall seeing any response to my query about this on the first iteration. Please don't just ignore review comments.
On 29/07/12 21:42, Mark Brown wrote: > On Fri, Jul 27, 2012 at 01:45:49PM +0100, Lee Jones wrote: > >> + if (of_get_property(np, "stericsson,use-pinctrl", NULL)) >> + msp->use_pinctrl = true; >> + else >> + msp->use_pinctrl = false; > > I don't recall seeing any response to my query about this on the first > iteration. Please don't just ignore review comments. It wasn't intentional. I must have missed it before. > This doesn't seem particularly sane... why is this conditional? It's conditional because only MSP1 and MSP3 have pinctrl support.
On Mon, Jul 30, 2012 at 07:53:36AM +0100, Lee Jones wrote: > On 29/07/12 21:42, Mark Brown wrote: > >>+ if (of_get_property(np, "stericsson,use-pinctrl", NULL)) > >This doesn't seem particularly sane... why is this conditional? > It's conditional because only MSP1 and MSP3 have pinctrl support. Why does the driver care - doesn't the pinctrl abstraction and/or bindings handle this sensibly?
On 30/07/12 14:39, Mark Brown wrote: > On Mon, Jul 30, 2012 at 07:53:36AM +0100, Lee Jones wrote: >> On 29/07/12 21:42, Mark Brown wrote: > >>>> + if (of_get_property(np, "stericsson,use-pinctrl", NULL)) > >>> This doesn't seem particularly sane... why is this conditional? > >> It's conditional because only MSP1 and MSP3 have pinctrl support. > > Why does the driver care - doesn't the pinctrl abstraction and/or > bindings handle this sensibly? Not when I tested it. pinctrl_get() came back !IS_ERR() for MSP0, MSP2 & MSP3, then when it went on to pinctrl_lookup_state(), only then did it fail. Would it be more sane to retract the error messages and just let it fail silently? It's either that or have lots of "could not get MSP defstate" clogging up the log.
On Mon, Jul 30, 2012 at 02:57:12PM +0100, Lee Jones wrote: > On 30/07/12 14:39, Mark Brown wrote: > >Why does the driver care - doesn't the pinctrl abstraction and/or > >bindings handle this sensibly? > Not when I tested it. pinctrl_get() came back !IS_ERR() for MSP0, > MSP2 & MSP3, then when it went on to pinctrl_lookup_state(), only > then did it fail. Would it be more sane to retract the error > messages and just let it fail silently? It's either that or have > lots of "could not get MSP defstate" clogging up the log. This sounds to me like we should be ensuring that there's a single fixed state available for these MSPs (representing the fact that the pins are always in the right mode) in the pinctrl bindings. As with several of other changes for this platform it seems like we need to think carefully about the abstraction level we're working at.
diff --git a/sound/soc/ux500/ux500_msp_dai.c b/sound/soc/ux500/ux500_msp_dai.c index 772cb19..0f7dd49 100644 --- a/sound/soc/ux500/ux500_msp_dai.c +++ b/sound/soc/ux500/ux500_msp_dai.c @@ -833,10 +833,16 @@ static int __devexit ux500_msp_drv_remove(struct platform_device *pdev) return 0; } +static const struct of_device_id ux500_msp_i2c_match[] = { + { .compatible = "stericsson,ux500-msp-i2s", }, + {}, +}; + static struct platform_driver msp_i2s_driver = { .driver = { .name = "ux500-msp-i2s", .owner = THIS_MODULE, + .of_match_table = ux500_msp_i2c_match, }, .probe = ux500_msp_drv_probe, .remove = ux500_msp_drv_remove, diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c index 72ad6e8..1ecdec0 100644 --- a/sound/soc/ux500/ux500_msp_i2s.c +++ b/sound/soc/ux500/ux500_msp_i2s.c @@ -18,6 +18,7 @@ #include <linux/pinctrl/consumer.h> #include <linux/delay.h> #include <linux/slab.h> +#include <linux/of.h> #include <mach/hardware.h> #include <mach/msp.h> @@ -685,6 +686,16 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir) } +void ux500_msp_i2s_of_init_msp(struct platform_device *pdev, + struct ux500_msp *msp, + struct device_node *np) +{ + if (of_get_property(np, "stericsson,use-pinctrl", NULL)) + msp->use_pinctrl = true; + else + msp->use_pinctrl = false; +} + int ux500_msp_i2s_init_msp(struct platform_device *pdev, struct ux500_msp **msp_p, struct msp_i2s_platform_data *platform_data) @@ -692,17 +703,33 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev, int ret = 0; struct resource *res = NULL; struct i2s_controller *i2s_cont; + struct device_node *np = pdev->dev.of_node; struct ux500_msp *msp; static int initialised = false; - dev_dbg(&pdev->dev, "%s: Enter (name: %s, id: %d).\n", __func__, - pdev->name, platform_data->id); - *msp_p = devm_kzalloc(&pdev->dev, sizeof(struct ux500_msp), GFP_KERNEL); msp = *msp_p; if (!msp) return -ENOMEM; + if (np) { + if (!platform_data) { + platform_data = devm_kzalloc(&pdev->dev, + sizeof(struct msp_i2s_platform_data), GFP_KERNEL); + if (!platform_data) + ret = -ENOMEM; + } + ux500_msp_i2s_of_init_msp(pdev, msp, np); + } else + if (!platform_data) + ret = -EINVAL; + + if (ret) + goto err_res; + + dev_dbg(&pdev->dev, "%s: Enter (name: %s, id: %d).\n", __func__, + pdev->name, platform_data->id); + msp->id = platform_data->id; msp->dev = &pdev->dev; msp->use_pinctrl = platform_data->use_pinctrl;
Register both parts of the MSP driver from Device Tree so that they are probed when Device Tree is enabled. Also, as there is platform data involved, we ensure that there is allocated memory to place the configuration into and that the correct information is extracted from the DT binary. Signed-off-by: Lee Jones <lee.jones@linaro.org> --- sound/soc/ux500/ux500_msp_dai.c | 6 ++++++ sound/soc/ux500/ux500_msp_i2s.c | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 36 insertions(+), 3 deletions(-)