* [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
@ 2015-03-20 21:22 Howard Mitchell
2015-03-22 16:24 ` Mark Brown
2015-03-22 21:25 ` Peter Rosin
0 siblings, 2 replies; 13+ messages in thread
From: Howard Mitchell @ 2015-03-20 21:22 UTC (permalink / raw
To: broonie, peda
Cc: tiwai, lgirdwood, perex, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, galak, devicetree, alsa-devel, linux-kernel,
Howard Mitchell
Currently the PLL Lock output signal is hardcoded to GPIO4. This
makes it seletable in the same way as pll-in and pll-out.
Signed-off-by: Howard Mitchell <hm@hmbedded.co.uk>
---
.../devicetree/bindings/sound/pcm512x.txt | 3 ++
sound/soc/codecs/pcm512x.c | 47 +++++++++++++-------
2 files changed, 33 insertions(+), 17 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/pcm512x.txt b/Documentation/devicetree/bindings/sound/pcm512x.txt
index 3aae3b4..432f186 100644
--- a/Documentation/devicetree/bindings/sound/pcm512x.txt
+++ b/Documentation/devicetree/bindings/sound/pcm512x.txt
@@ -26,6 +26,8 @@ Optional properties:
given pll-in pin and PLL output on the given pll-out pin. An
external connection from the pll-out pin to the SCLK pin is assumed.
+ - pll-lock : gpio pin used to output the PLL lock flag.
+
Examples:
pcm5122: pcm5122@4c {
@@ -49,4 +51,5 @@ Examples:
clocks = <&sck>;
pll-in = <3>;
pll-out = <6>;
+ pll-lock = <4>;
};
diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
index 8472099..a4217d7 100644
--- a/sound/soc/codecs/pcm512x.c
+++ b/sound/soc/codecs/pcm512x.c
@@ -49,6 +49,7 @@ struct pcm512x_priv {
int fmt;
int pll_in;
int pll_out;
+ int pll_lock;
int pll_r;
int pll_j;
int pll_d;
@@ -1296,24 +1297,26 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream,
ret, pcm512x->pll_out);
return ret;
}
+ }
- gpio = PCM512x_G1OE << (4 - 1);
- ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN,
- gpio, gpio);
- if (ret != 0) {
- dev_err(codec->dev, "Failed to enable gpio %d: %d\n",
- 4, ret);
- return ret;
- }
-
- gpio = PCM512x_GPIO_OUTPUT_1 + 4 - 1;
- ret = regmap_update_bits(pcm512x->regmap, gpio,
- PCM512x_GxSL, PCM512x_GxSL_PLLLK);
- if (ret != 0) {
- dev_err(codec->dev,
- "Failed to output pll lock on %d: %d\n",
- ret, 4);
- return ret;
+ if (pcm512x->pll_lock) {
+ gpio = PCM512x_G1OE << (pcm512x->pll_lock - 1);
+ ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN,
+ gpio, gpio);
+ if (ret != 0) {
+ dev_err(codec->dev, "Failed to enable gpio %d: %d\n",
+ pcm512x->pll_lock, ret);
+ return ret;
+ }
+
+ gpio = PCM512x_GPIO_OUTPUT_1 + pcm512x->pll_lock - 1;
+ ret = regmap_update_bits(pcm512x->regmap, gpio,
+ PCM512x_GxSL, PCM512x_GxSL_PLLLK);
+ if (ret != 0) {
+ dev_err(codec->dev,
+ "Failed to output pll lock on %d: %d\n",
+ ret, pcm512x->pll_lock);
+ return ret;
}
}
@@ -1518,6 +1521,16 @@ int pcm512x_probe(struct device *dev, struct regmap *regmap)
ret = -EINVAL;
goto err_clk;
}
+
+ if (of_property_read_u32(np, "pll-lock", &val) >= 0) {
+ if (val > 6) {
+ dev_err(dev, "Invalid pll-lock\n");
+ ret = -EINVAL;
+ goto err_clk;
+ }
+ pcm512x->pll_lock = val;
+ }
+
}
#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
2015-03-20 21:22 [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree Howard Mitchell
@ 2015-03-22 16:24 ` Mark Brown
2015-03-23 9:43 ` Howard Mitchell
2015-03-22 21:25 ` Peter Rosin
1 sibling, 1 reply; 13+ messages in thread
From: Mark Brown @ 2015-03-22 16:24 UTC (permalink / raw
To: Howard Mitchell
Cc: peda, tiwai, lgirdwood, perex, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, galak, devicetree, alsa-devel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 791 bytes --]
On Fri, Mar 20, 2015 at 09:22:43PM +0000, Howard Mitchell wrote:
> + if (pcm512x->pll_lock) {
> + if (of_property_read_u32(np, "pll-lock", &val) >= 0) {
> + if (val > 6) {
> + dev_err(dev, "Invalid pll-lock\n");
> + ret = -EINVAL;
> + goto err_clk;
> + }
> + pcm512x->pll_lock = val;
> + }
This breaks existing boards which rely on GPIO 4 being set as the lock
output. This is very unfortunate since it's a silly thing for the
driver to default to but nontheless we should really continue to support
them - at a guess Peter's board is relying on this, and even if it isn't
someone else's might.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
@ 2015-03-22 21:25 ` Peter Rosin
0 siblings, 0 replies; 13+ messages in thread
From: Peter Rosin @ 2015-03-22 21:25 UTC (permalink / raw
To: Howard Mitchell, broonie@kernel.org
Cc: tiwai@suse.de, lgirdwood@gmail.com, perex@perex.cz,
robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com,
ijc+devicetree@hellion.org.uk, galak@codeaurora.org,
devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
linux-kernel@vger.kernel.org
Howard Mitchell wrote:
> Currently the PLL Lock output signal is hardcoded to GPIO4. This
> makes it seletable in the same way as pll-in and pll-out.
Oops, I never intended the plllock code the hit upstream. I thought
I had removed that testing code and was very surprised to see it, that
was an odd experience. From my point of view it is fine to instead
remove the whole pll-lock thing.
But now the cat is out, so maybe we have to keep a way to output
the pll-lock signal for backwards compatibility?
Appart from the space-indent changes, this looks fine (if we do in fact
need to keep it at all). But I would like to see a new version of the
patch without the whitespace changes before I commit to that.
Cheers,
Peter
> Signed-off-by: Howard Mitchell <hm@hmbedded.co.uk>
> ---
> .../devicetree/bindings/sound/pcm512x.txt | 3 ++
> sound/soc/codecs/pcm512x.c | 47 +++++++++++++-------
> 2 files changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/pcm512x.txt
> b/Documentation/devicetree/bindings/sound/pcm512x.txt
> index 3aae3b4..432f186 100644
> --- a/Documentation/devicetree/bindings/sound/pcm512x.txt
> +++ b/Documentation/devicetree/bindings/sound/pcm512x.txt
> @@ -26,6 +26,8 @@ Optional properties:
> given pll-in pin and PLL output on the given pll-out pin. An
> external connection from the pll-out pin to the SCLK pin is assumed.
>
> + - pll-lock : gpio pin used to output the PLL lock flag.
> +
> Examples:
>
> pcm5122: pcm5122@4c {
> @@ -49,4 +51,5 @@ Examples:
> clocks = <&sck>;
> pll-in = <3>;
> pll-out = <6>;
> + pll-lock = <4>;
> };
> diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
> index 8472099..a4217d7 100644
> --- a/sound/soc/codecs/pcm512x.c
> +++ b/sound/soc/codecs/pcm512x.c
> @@ -49,6 +49,7 @@ struct pcm512x_priv {
> int fmt;
> int pll_in;
> int pll_out;
> + int pll_lock;
> int pll_r;
> int pll_j;
> int pll_d;
> @@ -1296,24 +1297,26 @@ static int pcm512x_hw_params(struct
> snd_pcm_substream *substream,
> ret, pcm512x->pll_out);
> return ret;
> }
> + }
>
> - gpio = PCM512x_G1OE << (4 - 1);
> - ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN,
> - gpio, gpio);
> - if (ret != 0) {
> - dev_err(codec->dev, "Failed to enable gpio %d: %d\n",
> - 4, ret);
> - return ret;
> - }
> -
> - gpio = PCM512x_GPIO_OUTPUT_1 + 4 - 1;
> - ret = regmap_update_bits(pcm512x->regmap, gpio,
> - PCM512x_GxSL, PCM512x_GxSL_PLLLK);
> - if (ret != 0) {
> - dev_err(codec->dev,
> - "Failed to output pll lock on %d: %d\n",
> - ret, 4);
> - return ret;
> + if (pcm512x->pll_lock) {
> + gpio = PCM512x_G1OE << (pcm512x->pll_lock - 1);
> + ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN,
> + gpio, gpio);
> + if (ret != 0) {
> + dev_err(codec->dev, "Failed to enable gpio %d: %d\n",
> + pcm512x->pll_lock, ret);
> + return ret;
> + }
> +
> + gpio = PCM512x_GPIO_OUTPUT_1 + pcm512x->pll_lock - 1;
> + ret = regmap_update_bits(pcm512x->regmap, gpio,
> + PCM512x_GxSL, PCM512x_GxSL_PLLLK);
> + if (ret != 0) {
> + dev_err(codec->dev,
> + "Failed to output pll lock on %d: %d\n",
> + ret, pcm512x->pll_lock);
> + return ret;
> }
> }
>
> @@ -1518,6 +1521,16 @@ int pcm512x_probe(struct device *dev, struct
> regmap *regmap)
> ret = -EINVAL;
> goto err_clk;
> }
> +
> + if (of_property_read_u32(np, "pll-lock", &val) >= 0) {
> + if (val > 6) {
> + dev_err(dev, "Invalid pll-lock\n");
> + ret = -EINVAL;
> + goto err_clk;
> + }
> + pcm512x->pll_lock = val;
> + }
> +
> }
> #endif
>
> --
> 1.7.9.5
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
@ 2015-03-22 21:25 ` Peter Rosin
0 siblings, 0 replies; 13+ messages in thread
From: Peter Rosin @ 2015-03-22 21:25 UTC (permalink / raw
To: Howard Mitchell, broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Cc: tiwai-l3A5Bk7waGM@public.gmane.org,
lgirdwood-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
perex-/Fr2/VpizcU@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
pawel.moll-5wv7dgnIgG8@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org,
galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Howard Mitchell wrote:
> Currently the PLL Lock output signal is hardcoded to GPIO4. This
> makes it seletable in the same way as pll-in and pll-out.
Oops, I never intended the plllock code the hit upstream. I thought
I had removed that testing code and was very surprised to see it, that
was an odd experience. From my point of view it is fine to instead
remove the whole pll-lock thing.
But now the cat is out, so maybe we have to keep a way to output
the pll-lock signal for backwards compatibility?
Appart from the space-indent changes, this looks fine (if we do in fact
need to keep it at all). But I would like to see a new version of the
patch without the whitespace changes before I commit to that.
Cheers,
Peter
> Signed-off-by: Howard Mitchell <hm-n6SlHCM0l+K9FHfhHBbuYA@public.gmane.org>
> ---
> .../devicetree/bindings/sound/pcm512x.txt | 3 ++
> sound/soc/codecs/pcm512x.c | 47 +++++++++++++-------
> 2 files changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/pcm512x.txt
> b/Documentation/devicetree/bindings/sound/pcm512x.txt
> index 3aae3b4..432f186 100644
> --- a/Documentation/devicetree/bindings/sound/pcm512x.txt
> +++ b/Documentation/devicetree/bindings/sound/pcm512x.txt
> @@ -26,6 +26,8 @@ Optional properties:
> given pll-in pin and PLL output on the given pll-out pin. An
> external connection from the pll-out pin to the SCLK pin is assumed.
>
> + - pll-lock : gpio pin used to output the PLL lock flag.
> +
> Examples:
>
> pcm5122: pcm5122@4c {
> @@ -49,4 +51,5 @@ Examples:
> clocks = <&sck>;
> pll-in = <3>;
> pll-out = <6>;
> + pll-lock = <4>;
> };
> diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
> index 8472099..a4217d7 100644
> --- a/sound/soc/codecs/pcm512x.c
> +++ b/sound/soc/codecs/pcm512x.c
> @@ -49,6 +49,7 @@ struct pcm512x_priv {
> int fmt;
> int pll_in;
> int pll_out;
> + int pll_lock;
> int pll_r;
> int pll_j;
> int pll_d;
> @@ -1296,24 +1297,26 @@ static int pcm512x_hw_params(struct
> snd_pcm_substream *substream,
> ret, pcm512x->pll_out);
> return ret;
> }
> + }
>
> - gpio = PCM512x_G1OE << (4 - 1);
> - ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN,
> - gpio, gpio);
> - if (ret != 0) {
> - dev_err(codec->dev, "Failed to enable gpio %d: %d\n",
> - 4, ret);
> - return ret;
> - }
> -
> - gpio = PCM512x_GPIO_OUTPUT_1 + 4 - 1;
> - ret = regmap_update_bits(pcm512x->regmap, gpio,
> - PCM512x_GxSL, PCM512x_GxSL_PLLLK);
> - if (ret != 0) {
> - dev_err(codec->dev,
> - "Failed to output pll lock on %d: %d\n",
> - ret, 4);
> - return ret;
> + if (pcm512x->pll_lock) {
> + gpio = PCM512x_G1OE << (pcm512x->pll_lock - 1);
> + ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN,
> + gpio, gpio);
> + if (ret != 0) {
> + dev_err(codec->dev, "Failed to enable gpio %d: %d\n",
> + pcm512x->pll_lock, ret);
> + return ret;
> + }
> +
> + gpio = PCM512x_GPIO_OUTPUT_1 + pcm512x->pll_lock - 1;
> + ret = regmap_update_bits(pcm512x->regmap, gpio,
> + PCM512x_GxSL, PCM512x_GxSL_PLLLK);
> + if (ret != 0) {
> + dev_err(codec->dev,
> + "Failed to output pll lock on %d: %d\n",
> + ret, pcm512x->pll_lock);
> + return ret;
> }
> }
>
> @@ -1518,6 +1521,16 @@ int pcm512x_probe(struct device *dev, struct
> regmap *regmap)
> ret = -EINVAL;
> goto err_clk;
> }
> +
> + if (of_property_read_u32(np, "pll-lock", &val) >= 0) {
> + if (val > 6) {
> + dev_err(dev, "Invalid pll-lock\n");
> + ret = -EINVAL;
> + goto err_clk;
> + }
> + pcm512x->pll_lock = val;
> + }
> +
> }
> #endif
>
> --
> 1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
@ 2015-03-23 9:43 ` Howard Mitchell
0 siblings, 0 replies; 13+ messages in thread
From: Howard Mitchell @ 2015-03-23 9:43 UTC (permalink / raw
To: Mark Brown
Cc: peda, tiwai, lgirdwood, perex, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, galak, devicetree, alsa-devel, linux-kernel
On 22/03/15 16:24, Mark Brown wrote:
> On Fri, Mar 20, 2015 at 09:22:43PM +0000, Howard Mitchell wrote:
>
>> + if (pcm512x->pll_lock) {
>> + if (of_property_read_u32(np, "pll-lock", &val) >= 0) {
>> + if (val > 6) {
>> + dev_err(dev, "Invalid pll-lock\n");
>> + ret = -EINVAL;
>> + goto err_clk;
>> + }
>> + pcm512x->pll_lock = val;
>> + }
> This breaks existing boards which rely on GPIO 4 being set as the lock
> output. This is very unfortunate since it's a silly thing for the
> driver to default to but nontheless we should really continue to support
> them - at a guess Peter's board is relying on this, and even if it isn't
> someone else's might.
I take your point, but the reason I pushed this patch was that I wanted
to use GPIO4 for pll-out and unfortunately because the pll-lock
configuration is after the pll-out configuration it stomps on it. If I
modify the patch to provide a default for pll-lock I will then be
obliged to specify pll-lock on another GPIO. The pcm5122 has limited IO
so being forced to have a GPIO for pll-lock seems wrong to me. A future
user of the device may well decide to use the GPIOs for other purposes
and therefore not want a pll-lock signal at all. Surely we should allow
for that possibility?
Given that Peter has indicated that he'd be happy with this solution and
that this code hasn't reached a published kernel would it be reasonable
to go ahead with my current patch (happy to clean up the indent issues
that Peter pointed out of course)?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
@ 2015-03-23 9:43 ` Howard Mitchell
0 siblings, 0 replies; 13+ messages in thread
From: Howard Mitchell @ 2015-03-23 9:43 UTC (permalink / raw
To: Mark Brown
Cc: peda-koto5C5qi+TLoDKTGw+V6w, tiwai-l3A5Bk7waGM,
lgirdwood-Re5JQEeQqe8AvxtiuMwx3w, perex-/Fr2/VpizcU,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, pawel.moll-5wv7dgnIgG8,
mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, devicetree-u79uwXL29TY76Z2rM5mHXA,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On 22/03/15 16:24, Mark Brown wrote:
> On Fri, Mar 20, 2015 at 09:22:43PM +0000, Howard Mitchell wrote:
>
>> + if (pcm512x->pll_lock) {
>> + if (of_property_read_u32(np, "pll-lock", &val) >= 0) {
>> + if (val > 6) {
>> + dev_err(dev, "Invalid pll-lock\n");
>> + ret = -EINVAL;
>> + goto err_clk;
>> + }
>> + pcm512x->pll_lock = val;
>> + }
> This breaks existing boards which rely on GPIO 4 being set as the lock
> output. This is very unfortunate since it's a silly thing for the
> driver to default to but nontheless we should really continue to support
> them - at a guess Peter's board is relying on this, and even if it isn't
> someone else's might.
I take your point, but the reason I pushed this patch was that I wanted
to use GPIO4 for pll-out and unfortunately because the pll-lock
configuration is after the pll-out configuration it stomps on it. If I
modify the patch to provide a default for pll-lock I will then be
obliged to specify pll-lock on another GPIO. The pcm5122 has limited IO
so being forced to have a GPIO for pll-lock seems wrong to me. A future
user of the device may well decide to use the GPIOs for other purposes
and therefore not want a pll-lock signal at all. Surely we should allow
for that possibility?
Given that Peter has indicated that he'd be happy with this solution and
that this code hasn't reached a published kernel would it be reasonable
to go ahead with my current patch (happy to clean up the indent issues
that Peter pointed out of course)?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
@ 2015-03-23 11:00 ` Peter Rosin
0 siblings, 0 replies; 13+ messages in thread
From: Peter Rosin @ 2015-03-23 11:00 UTC (permalink / raw
To: Howard Mitchell, Mark Brown
Cc: tiwai@suse.de, lgirdwood@gmail.com, perex@perex.cz,
robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com,
ijc+devicetree@hellion.org.uk, galak@codeaurora.org,
devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
linux-kernel@vger.kernel.org
Howard Mitchell wrote:
> On 22/03/15 16:24, Mark Brown wrote:
> > On Fri, Mar 20, 2015 at 09:22:43PM +0000, Howard Mitchell wrote:
> >
> >> + if (pcm512x->pll_lock) {
> >> + if (of_property_read_u32(np, "pll-lock", &val) >= 0) {
> >> + if (val > 6) {
> >> + dev_err(dev, "Invalid pll-lock\n");
> >> + ret = -EINVAL;
> >> + goto err_clk;
> >> + }
> >> + pcm512x->pll_lock = val;
> >> + }
> > This breaks existing boards which rely on GPIO 4 being set as the lock
> > output. This is very unfortunate since it's a silly thing for the
> > driver to default to but nontheless we should really continue to support
> > them - at a guess Peter's board is relying on this, and even if it isn't
> > someone else's might.
> I take your point, but the reason I pushed this patch was that I wanted
> to use GPIO4 for pll-out and unfortunately because the pll-lock
> configuration is after the pll-out configuration it stomps on it. If I
> modify the patch to provide a default for pll-lock I will then be
> obliged to specify pll-lock on another GPIO. The pcm5122 has limited IO
> so being forced to have a GPIO for pll-lock seems wrong to me. A future
> user of the device may well decide to use the GPIOs for other purposes
> and therefore not want a pll-lock signal at all. Surely we should allow
> for that possibility?
>
> Given that Peter has indicated that he'd be happy with this solution and
> that this code hasn't reached a published kernel would it be reasonable
> to go ahead with my current patch (happy to clean up the indent issues
> that Peter pointed out of course)?
Strongly agreed that we should fix this before it is published (I assumed
that is was included in 3.19, it felt so long ago that Mark merged it...). My
preference would be to remove the pll-lock things entirely though. Assuming
you don't need it for your board of course, but I doubt it from your description.
I used it to make sure I had understood the chip correctly, that's all.
Cheers,
Peter
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
@ 2015-03-23 11:00 ` Peter Rosin
0 siblings, 0 replies; 13+ messages in thread
From: Peter Rosin @ 2015-03-23 11:00 UTC (permalink / raw
To: Howard Mitchell, Mark Brown
Cc: tiwai-l3A5Bk7waGM@public.gmane.org,
lgirdwood-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
perex-/Fr2/VpizcU@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
pawel.moll-5wv7dgnIgG8@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org,
galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Howard Mitchell wrote:
> On 22/03/15 16:24, Mark Brown wrote:
> > On Fri, Mar 20, 2015 at 09:22:43PM +0000, Howard Mitchell wrote:
> >
> >> + if (pcm512x->pll_lock) {
> >> + if (of_property_read_u32(np, "pll-lock", &val) >= 0) {
> >> + if (val > 6) {
> >> + dev_err(dev, "Invalid pll-lock\n");
> >> + ret = -EINVAL;
> >> + goto err_clk;
> >> + }
> >> + pcm512x->pll_lock = val;
> >> + }
> > This breaks existing boards which rely on GPIO 4 being set as the lock
> > output. This is very unfortunate since it's a silly thing for the
> > driver to default to but nontheless we should really continue to support
> > them - at a guess Peter's board is relying on this, and even if it isn't
> > someone else's might.
> I take your point, but the reason I pushed this patch was that I wanted
> to use GPIO4 for pll-out and unfortunately because the pll-lock
> configuration is after the pll-out configuration it stomps on it. If I
> modify the patch to provide a default for pll-lock I will then be
> obliged to specify pll-lock on another GPIO. The pcm5122 has limited IO
> so being forced to have a GPIO for pll-lock seems wrong to me. A future
> user of the device may well decide to use the GPIOs for other purposes
> and therefore not want a pll-lock signal at all. Surely we should allow
> for that possibility?
>
> Given that Peter has indicated that he'd be happy with this solution and
> that this code hasn't reached a published kernel would it be reasonable
> to go ahead with my current patch (happy to clean up the indent issues
> that Peter pointed out of course)?
Strongly agreed that we should fix this before it is published (I assumed
that is was included in 3.19, it felt so long ago that Mark merged it...). My
preference would be to remove the pll-lock things entirely though. Assuming
you don't need it for your board of course, but I doubt it from your description.
I used it to make sure I had understood the chip correctly, that's all.
Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
2015-03-23 11:00 ` Peter Rosin
@ 2015-03-23 11:14 ` Howard Mitchell
-1 siblings, 0 replies; 13+ messages in thread
From: Howard Mitchell @ 2015-03-23 11:14 UTC (permalink / raw
To: Peter Rosin, Mark Brown
Cc: tiwai@suse.de, lgirdwood@gmail.com, perex@perex.cz,
robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com,
ijc+devicetree@hellion.org.uk, galak@codeaurora.org,
devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
linux-kernel@vger.kernel.org
On 23/03/15 11:00, Peter Rosin wrote:
> Howard Mitchell wrote:
>> On 22/03/15 16:24, Mark Brown wrote:
>>> On Fri, Mar 20, 2015 at 09:22:43PM +0000, Howard Mitchell wrote:
>>>
>>>> + if (pcm512x->pll_lock) {
>>>> + if (of_property_read_u32(np, "pll-lock", &val) >= 0) {
>>>> + if (val > 6) {
>>>> + dev_err(dev, "Invalid pll-lock\n");
>>>> + ret = -EINVAL;
>>>> + goto err_clk;
>>>> + }
>>>> + pcm512x->pll_lock = val;
>>>> + }
>>> This breaks existing boards which rely on GPIO 4 being set as the lock
>>> output. This is very unfortunate since it's a silly thing for the
>>> driver to default to but nontheless we should really continue to support
>>> them - at a guess Peter's board is relying on this, and even if it isn't
>>> someone else's might.
>> I take your point, but the reason I pushed this patch was that I wanted
>> to use GPIO4 for pll-out and unfortunately because the pll-lock
>> configuration is after the pll-out configuration it stomps on it. If I
>> modify the patch to provide a default for pll-lock I will then be
>> obliged to specify pll-lock on another GPIO. The pcm5122 has limited IO
>> so being forced to have a GPIO for pll-lock seems wrong to me. A future
>> user of the device may well decide to use the GPIOs for other purposes
>> and therefore not want a pll-lock signal at all. Surely we should allow
>> for that possibility?
>>
>> Given that Peter has indicated that he'd be happy with this solution and
>> that this code hasn't reached a published kernel would it be reasonable
>> to go ahead with my current patch (happy to clean up the indent issues
>> that Peter pointed out of course)?
> Strongly agreed that we should fix this before it is published (I assumed
> that is was included in 3.19, it felt so long ago that Mark merged it...). My
> preference would be to remove the pll-lock things entirely though. Assuming
> you don't need it for your board of course, but I doubt it from your description.
> I used it to make sure I had understood the chip correctly, that's all.
>
> Cheers,
> Peter
>
I'd be happy to remove it entirely. I only used it as you did as a debug
tool. Let's see what Mark thinks.....
- Howard
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
@ 2015-03-23 11:14 ` Howard Mitchell
0 siblings, 0 replies; 13+ messages in thread
From: Howard Mitchell @ 2015-03-23 11:14 UTC (permalink / raw
To: Peter Rosin, Mark Brown
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
alsa-devel@alsa-project.org, pawel.moll@arm.com,
ijc+devicetree@hellion.org.uk, tiwai@suse.de,
linux-kernel@vger.kernel.org, lgirdwood@gmail.com,
robh+dt@kernel.org, galak@codeaurora.org
On 23/03/15 11:00, Peter Rosin wrote:
> Howard Mitchell wrote:
>> On 22/03/15 16:24, Mark Brown wrote:
>>> On Fri, Mar 20, 2015 at 09:22:43PM +0000, Howard Mitchell wrote:
>>>
>>>> + if (pcm512x->pll_lock) {
>>>> + if (of_property_read_u32(np, "pll-lock", &val) >= 0) {
>>>> + if (val > 6) {
>>>> + dev_err(dev, "Invalid pll-lock\n");
>>>> + ret = -EINVAL;
>>>> + goto err_clk;
>>>> + }
>>>> + pcm512x->pll_lock = val;
>>>> + }
>>> This breaks existing boards which rely on GPIO 4 being set as the lock
>>> output. This is very unfortunate since it's a silly thing for the
>>> driver to default to but nontheless we should really continue to support
>>> them - at a guess Peter's board is relying on this, and even if it isn't
>>> someone else's might.
>> I take your point, but the reason I pushed this patch was that I wanted
>> to use GPIO4 for pll-out and unfortunately because the pll-lock
>> configuration is after the pll-out configuration it stomps on it. If I
>> modify the patch to provide a default for pll-lock I will then be
>> obliged to specify pll-lock on another GPIO. The pcm5122 has limited IO
>> so being forced to have a GPIO for pll-lock seems wrong to me. A future
>> user of the device may well decide to use the GPIOs for other purposes
>> and therefore not want a pll-lock signal at all. Surely we should allow
>> for that possibility?
>>
>> Given that Peter has indicated that he'd be happy with this solution and
>> that this code hasn't reached a published kernel would it be reasonable
>> to go ahead with my current patch (happy to clean up the indent issues
>> that Peter pointed out of course)?
> Strongly agreed that we should fix this before it is published (I assumed
> that is was included in 3.19, it felt so long ago that Mark merged it...). My
> preference would be to remove the pll-lock things entirely though. Assuming
> you don't need it for your board of course, but I doubt it from your description.
> I used it to make sure I had understood the chip correctly, that's all.
>
> Cheers,
> Peter
>
I'd be happy to remove it entirely. I only used it as you did as a debug
tool. Let's see what Mark thinks.....
- Howard
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
2015-03-23 11:00 ` Peter Rosin
(?)
(?)
@ 2015-03-23 16:56 ` Mark Brown
2015-03-23 19:15 ` Howard Mitchell
-1 siblings, 1 reply; 13+ messages in thread
From: Mark Brown @ 2015-03-23 16:56 UTC (permalink / raw
To: Peter Rosin
Cc: Howard Mitchell, tiwai@suse.de, lgirdwood@gmail.com,
perex@perex.cz, robh+dt@kernel.org, pawel.moll@arm.com,
mark.rutland@arm.com, ijc+devicetree@hellion.org.uk,
galak@codeaurora.org, devicetree@vger.kernel.org,
alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 812 bytes --]
On Mon, Mar 23, 2015 at 11:00:53AM +0000, Peter Rosin wrote:
> Strongly agreed that we should fix this before it is published (I assumed
> that is was included in 3.19, it felt so long ago that Mark merged it...). My
> preference would be to remove the pll-lock things entirely though. Assuming
> you don't need it for your board of course, but I doubt it from your description.
> I used it to make sure I had understood the chip correctly, that's all.
It didn't make v3.19 so if we're very quick we can get this in as a fix.
Can someone resend a version which splits this into two patches, one
deleting the existing code to configure GPIO 4 and the other adding the
configurability? I can then send the removal as a bug fix to v4.0 so we
don't have any compatibility issues and add the new feature for v4.1.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
@ 2015-03-23 19:15 ` Howard Mitchell
0 siblings, 0 replies; 13+ messages in thread
From: Howard Mitchell @ 2015-03-23 19:15 UTC (permalink / raw
To: Mark Brown, Peter Rosin
Cc: tiwai@suse.de, lgirdwood@gmail.com, perex@perex.cz,
robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com,
ijc+devicetree@hellion.org.uk, galak@codeaurora.org,
devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
linux-kernel@vger.kernel.org
On 23/03/15 16:56, Mark Brown wrote:
> On Mon, Mar 23, 2015 at 11:00:53AM +0000, Peter Rosin wrote:
>
>> Strongly agreed that we should fix this before it is published (I assumed
>> that is was included in 3.19, it felt so long ago that Mark merged it...). My
>> preference would be to remove the pll-lock things entirely though. Assuming
>> you don't need it for your board of course, but I doubt it from your description.
>> I used it to make sure I had understood the chip correctly, that's all.
> It didn't make v3.19 so if we're very quick we can get this in as a fix.
> Can someone resend a version which splits this into two patches, one
> deleting the existing code to configure GPIO 4 and the other adding the
> configurability? I can then send the removal as a bug fix to v4.0 so we
> don't have any compatibility issues and add the new feature for v4.1.
Ok, will do, although it seems that both Peter and I agree that the
configurability is not required, so I'll just submit a patch for the
deletion.
- Howard
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree.
@ 2015-03-23 19:15 ` Howard Mitchell
0 siblings, 0 replies; 13+ messages in thread
From: Howard Mitchell @ 2015-03-23 19:15 UTC (permalink / raw
To: Mark Brown, Peter Rosin
Cc: tiwai-l3A5Bk7waGM@public.gmane.org,
lgirdwood-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
perex-/Fr2/VpizcU@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
pawel.moll-5wv7dgnIgG8@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org,
galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 23/03/15 16:56, Mark Brown wrote:
> On Mon, Mar 23, 2015 at 11:00:53AM +0000, Peter Rosin wrote:
>
>> Strongly agreed that we should fix this before it is published (I assumed
>> that is was included in 3.19, it felt so long ago that Mark merged it...). My
>> preference would be to remove the pll-lock things entirely though. Assuming
>> you don't need it for your board of course, but I doubt it from your description.
>> I used it to make sure I had understood the chip correctly, that's all.
> It didn't make v3.19 so if we're very quick we can get this in as a fix.
> Can someone resend a version which splits this into two patches, one
> deleting the existing code to configure GPIO 4 and the other adding the
> configurability? I can then send the removal as a bug fix to v4.0 so we
> don't have any compatibility issues and add the new feature for v4.1.
Ok, will do, although it seems that both Peter and I agree that the
configurability is not required, so I'll just submit a patch for the
deletion.
- Howard
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-03-23 19:15 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-20 21:22 [PATCH] ASoC:pcm512x: Make PLL lock output selectable via device tree Howard Mitchell
2015-03-22 16:24 ` Mark Brown
2015-03-23 9:43 ` Howard Mitchell
2015-03-23 9:43 ` Howard Mitchell
2015-03-23 11:00 ` Peter Rosin
2015-03-23 11:00 ` Peter Rosin
2015-03-23 11:14 ` Howard Mitchell
2015-03-23 11:14 ` Howard Mitchell
2015-03-23 16:56 ` Mark Brown
2015-03-23 19:15 ` Howard Mitchell
2015-03-23 19:15 ` Howard Mitchell
2015-03-22 21:25 ` Peter Rosin
2015-03-22 21:25 ` Peter Rosin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.