RE: [PATCH 1/4] ASoC: simple-card: Fix device node locks
> @@ -169,22 +164,26 @@ static int asoc_simple_card_parse_of(struct device_node
> *node,
> /* CPU sub-node */
> ret = -EINVAL;
> np = of_get_child_by_name(node, "simple-audio-card,cpu");
> - if (np)
> + if (np) {
> ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
> &priv->cpu_dai,
> &dai_link->cpu_of_node,
> &dai_link->cpu_dai_name);
> + of_node_put(np);
Does the of_node_put(np) is really needed here ?
> + }
> if (ret < 0)
> return ret;
>
> /* CODEC sub-node */
> ret = -EINVAL;
> np = of_get_child_by_name(node, "simple-audio-card,codec");
> - if (np)
> + if (np) {
> ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
> &priv->codec_dai,
> &dai_link->codec_of_node,
> &dai_link->codec_dai_name);
> + of_node_put(np);
The same here...
> + }
> if (ret < 0)
> return ret;
>
Thanks,
--
Best Regards,
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/4] ASoC: simple-card: dynamically allocate the DAI link array
> @@ -20,7 +20,6 @@ struct simple_card_data {
> unsigned int daifmt;
> struct asoc_simple_dai cpu_dai;
> struct asoc_simple_dai codec_dai;
> - struct snd_soc_dai_link snd_link;
> };
>
> static int __asoc_simple_card_dai_init(struct snd_soc_dai *dai,
> @@ -246,7 +245,9 @@ static int asoc_simple_card_probe(struct platform_device
> *pdev)
> struct device *dev = &pdev->dev;
> int ret;
>
> - priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + priv = devm_kzalloc(dev,
> + sizeof(*priv) + sizeof(*dai_link),
This is okey for me.
Well, how about splitting the *priv and *dai_link into two separated
memory blocks? As we can get the dai-link pointer via priv->snd_card.dai_link
in other places.
IMHO, then the code will be much more simplifier and readable.
Just for one suggestion.
Thanks,
--
Best regards,
Xiubo
> + GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
>
> @@ -255,7 +256,7 @@ static int asoc_simple_card_probe(struct platform_device
> *pdev)
>*/
> priv->snd_card.owner = THIS_MODULE;
> priv->snd_card.dev = dev;
> - dai_link = &priv->snd_link;
> + dai_link = (struct snd_soc_dai_link *) (priv + 1);
> priv->snd_card.dai_link = dai_link;
> priv->snd_card.num_links = 1;
>
> --
> 1.9.0
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv9 Resend 0/4] Add Freescale FTM PWM driver
Thierry, Ping ? :) > -Original Message- > From: Xiubo Li [mailto:li.xi...@freescale.com] > Sent: Wednesday, February 19, 2014 4:39 PM > To: thierry.red...@gmail.com; linux-...@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; Xiubo Li-B47053 > Subject: [PATCHv9 Resend 0/4] Add Freescale FTM PWM driver > > Resend this patch series, just adding the missing and new Reviewed-by and > Acked-by infomation of this patch series. > > > Xiubo Li (4): > pwm: Add Freescale FTM PWM driver support > ARM: dts: vf610: Add Freescale FTM PWM node. > ARM: dts: vf610-twr: Enables FTM PWM device. > Documentation: Add device tree bindings for Freescale FTM PWM. > > .../devicetree/bindings/pwm/pwm-fsl-ftm.txt| 34 ++ > arch/arm/boot/dts/vf610-twr.dts| 6 + > arch/arm/boot/dts/vf610.dtsi | 13 + > drivers/pwm/Kconfig| 10 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-fsl-ftm.c | 479 > + > 6 files changed, 543 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt > create mode 100644 drivers/pwm/pwm-fsl-ftm.c > > -- > 1.8.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv3 resend 1/3] ASoC: codec: Simplify ASoC probe code.
> Subject: Re: [PATCHv3 resend 1/3] ASoC: codec: Simplify ASoC probe code. > > On Tue, Mar 11, 2014 at 12:43:20PM +0800, Xiubo Li wrote: > > For some CODEC drivers like who act as the MFDs children are ignored > > by this patch. > > Applied all, thanks. I'd generally recommend about putting things like > "resend" in the subject, it can make the prefix awfully long and hide > the actual subject. Yes, I will follow this. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 3/4] ASoC: simple-card: accept many DAI links
> Subject: [PATCH v2 3/4] ASoC: simple-card: accept many DAI links
>
> Some simple audio cards may have many DAI links.
> This patch extends the simple-card driver for handling such cards.
>
> Signed-off-by: Jean-Francois Moine
> ---
> sound/soc/generic/simple-card.c | 132 ++-
> -
> 1 file changed, 85 insertions(+), 47 deletions(-)
>
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 2710b52..8188c34 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -107,6 +107,9 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
> if (ret < 0)
> return ret;
>
> + if (!dai)
> + return 0;
> +
> /* parse TDM slot */
> ret = snd_soc_of_parse_tdm_slot(np, &dai->slots, &dai->slot_width);
> if (ret)
> @@ -154,7 +157,7 @@ static int asoc_simple_card_parse_of(struct device_node
> *node,
> struct snd_soc_dai_link *dai_link = priv->snd_card.dai_link;
> struct device_node *np;
> char *name;
> - int ret;
> + int first_link, ret;
>
> /* parsing the card name from DT */
> snd_soc_of_parse_card_name(&priv->snd_card, "simple-audio-card,name");
> @@ -179,50 +182,67 @@ static int asoc_simple_card_parse_of(struct device_node
> *node,
> return ret;
> }
>
> - /* CPU sub-node */
> - ret = -EINVAL;
> - np = of_get_child_by_name(node, "simple-audio-card,cpu");
> - if (np) {
> + /* loop on the DAI links */
> + np = NULL;
> + first_link = 1;
> + for (;;) {
> + np = of_get_next_child(node, np);
> + if (!np)
> + break;
> +
> + /* CPU sub-node */
> + if (strcmp(np->name, "simple-audio-card,cpu") != 0) {
> + dev_err(dev, "Bad CPU DAI\n");
> + ret = -EINVAL;
> + goto err;
> + }
> ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
> - &priv->cpu_dai,
> + first_link ? &priv->cpu_dai : NULL,
@Jean-Francois,
I'm not sure why only the first link needs parsing the DAIFMT ?
On my LS1 platform there are two CODECs and two CPU SAI deivces,
SGTL5000 <---> SAI
CS42888 <---> ESAI
Could the many dai links feature support this case?
I my understanding is correct here, this patch will support the
Board, in which board there maybe only one CODEC supporting many
DAIs...
Thanks,
--
Best Regards,
Xiubo
> &dai_link->cpu_of_node,
> &dai_link->cpu_dai_name);
> - of_node_put(np);
> - }
> - if (ret < 0)
> - return ret;
> + if (ret < 0)
> + goto err;
>
> - /* CODEC sub-node */
> - ret = -EINVAL;
> - np = of_get_child_by_name(node, "simple-audio-card,codec");
> - if (np) {
> + /* CODEC sub-node */
> + np = of_get_next_child(node, np);
> + if (strcmp(np->name, "simple-audio-card,codec") != 0) {
> + dev_err(dev, "Bad CODEC DAI\n");
> + ret = -EINVAL;
> + goto err;
> + }
> ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
> - &priv->codec_dai,
> + first_link ? &priv->codec_dai : NULL,
> &dai_link->codec_of_node,
> &dai_link->codec_dai_name);
> - of_node_put(np);
> - }
> - if (ret < 0)
> - return ret;
> + if (ret < 0)
> + goto err;
> +
> + if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
> + ret = -EINVAL;
> + goto err;
> + }
>
> - if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name)
> - return -EINVAL;
> + /* simple-card assumes platform == cpu */
> + dai_link->platform_of_node = dai_link->cpu_of_node;
> +
> + name = devm_kzalloc(dev,
> + strlen(dai_link->cpu_dai_name) +
> + strlen(dai_link->codec_dai_name) + 2,
> + GFP_KERNEL);
> + sprintf(name, "%s-%s", dai_link->cpu_dai_name,
> + dai_link->codec_dai_name);
> + dai_link->name = dai_link->stream_name = name;
> +
> + dai_link++;
> + first_link = 0;
> + }
>
> /* card name is created from CPU/CODEC dai name */
> - name = devm_kzalloc(dev,
> - strlen(dai_link->cpu_dai_name) +
> - strlen(dai_link->codec
RE: [PATCH v3 0/4] ASoC: simple-card: multi DAI links extension
> Subject: [PATCH v3 0/4] ASoC: simple-card: multi DAI links extension > > This patch series extends the simple card driver to handle > many DAI links as this exists in the Cubox audio subsystem. > > -v3 > - remove 'Fix the reference count of device nodes' > which is applied (Mark Brown) > - new patch 'Simplify code' > - dynamically allocate and use properties for all DAI links > (Jyri Sarha and Li Xiubo) This patch series looks good to me. For this patch series: Reviewed-by: Xiubo Li Thanks, -- Best Regards, Xiubo > - v2 > - change subject/comment about device node reference count > (Mark Brown) > - use a null size array instead of an implicit area for the DAI links > (Li Xiubo) > - update the reference count of the device node at end of probe > > Jean-Francois Moine (4): > ASoC: simple-card: Simplify code > ASoC: simple-card: dynamically allocate the DAI link and properties > ASoC: simple-card: Handle many DAI links > ASoC: simple-card: Add DT documentation for multi-DAI links > > .../devicetree/bindings/sound/simple-card.txt | 34 +++- > sound/soc/generic/simple-card.c| 181 +--- > - > 2 files changed, 145 insertions(+), 70 deletions(-) > > -- > 1.9.0 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [RFC][PATCH 1/3] ASoC: core: Move the default regmap I/O setting to snd_soc_register_codec()
> Subject: Re: [alsa-devel] [RFC][PATCH 1/3] ASoC: core: Move the default regmap
> I/O setting to snd_soc_register_codec()
>
> On 03/04/2014 09:25 AM, Xiubo Li wrote:
> > Add the default regmap I/O setting to snd_soc_register_codec() while
> > the CODEC is initialising, which will be called by CODEC driver device
> > probe(), and then we can make set_cache_io() go away entirely from each
> > CODEC ASoC probe.
> >
> > Signed-off-by: Xiubo Li
> > ---
> > include/sound/soc.h | 3 +++
> > sound/soc/soc-core.c | 11 +++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/include/sound/soc.h b/include/sound/soc.h
> > index 4c4d7e1..94bc1c4 100644
> > --- a/include/sound/soc.h
> > +++ b/include/sound/soc.h
> > @@ -749,6 +749,9 @@ struct snd_soc_codec_driver {
> > int (*set_pll)(struct snd_soc_codec *codec, int pll_id, int source,
> > unsigned int freq_in, unsigned int freq_out);
> >
> > + /* codec regmap */
> > + struct regmap *regmap;
> > +
>
> Nope. The driver struct is globally shared between all device instances, the
> regmap struct is device instance specific. The proper way to solve this is
> to have a function like snd_soc_register_codec_with_io() which takes a
> pointer to the regmap struct.
>
@Lars
Yes, this maybe a better choice, and I will think of it.
Thanks very much,
--
Best Regards,
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [PATCH 01/10] ASoC: core: Add snd_soc_dai_set_tdm_slot_xlate().
> Subject: Re: [alsa-devel] [PATCH 01/10] ASoC: core: Add > snd_soc_dai_set_tdm_slot_xlate(). > > On Sat, Mar 01, 2014 at 02:19:44PM +0100, Lars-Peter Clausen wrote: > > > I'm not quite sure I fully understand what this patch is trying to > > solve. It adds a variant snd_soc_dai_set_tdm_slot() that instead of > > taking a rx and tx mask calculates the masks based on the number of > > slots? In that case I don't really see how the xlate in the name > > relates to that. xlate is something you'd typically expect in a > > devicetree context. Maybe one should be called > > snd_soc_dai_set_tdm_slot() and the other > > snd_soc_dai_set_tdm_slot_and_masks()? > > > But another question is do we really need this? I don't see the > > problem that is solved by this patchset. > > My understanding is that the patch set aims to provide a way of using > the TDM features of drivers from DT, providing a standardised format for > expressing the TDM setup in the DT. I've not looked at the actual code > yet though. @Lars, Sorry for late, many mails had been discard by my outlook, including the Last one. @Mark, @Lars, This adds the function of snd_soc_dai_set_tdm_slot_xlate, which is almost One new signature of snd_soc_dai_set_tdm_slot discarding the mask Parameters, which could be generated by itself. And I want to provide one standard method for the drivers who are parsing The TDM information from the DT node. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv9 Resend 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
> > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt > b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt > [...] > > +- clocks : Must contain a clock specifier for each entry in clock-names, > > Nit: A slightly more correct way to say this would be: "Must contain a > phandle and clock specifier...". > Yes, I will fix this. Thanks very much, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv9 Resend 1/4] pwm: Add Freescale FTM PWM driver support
Hi Thierry,
Thanks very much, I will fix them all.
:)
--
Best Regards,
Xiubo
> Sorry for taking so long to get back to you. Things have been quite busy
> lately. A few more comments below, but we're getting there.
>
> On Wed, Feb 19, 2014 at 04:38:54PM +0800, Xiubo Li wrote:
> [...]
> > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> [...]
> > +static unsigned long fsl_pwm_calculate_period(struct fsl_pwm_chip *fpc,
> > + unsigned long period_ns)
> > +{
> > + struct clk *cnt_clk[3];
> > + enum fsl_pwm_clk m0, m1;
> > + unsigned long fix_rate, ext_rate, cycles;
> > +
> > + fpc->counter_clk = fpc->sys_clk;
> > + cycles = fsl_pwm_calculate_period_cycles(fpc, period_ns,
> > + FSL_PWM_CLK_SYS);
> > + if (cycles)
> > + return cycles;
> > +
> > + cnt_clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix");
> > + if (IS_ERR(cnt_clk[FSL_PWM_CLK_FIX]))
> > + return PTR_ERR(cnt_clk[FSL_PWM_CLK_FIX]);
> > +
> > + cnt_clk[FSL_PWM_CLK_EXT] = devm_clk_get(fpc->chip.dev, "ftm_ext");
> > + if (IS_ERR(cnt_clk[FSL_PWM_CLK_EXT]))
> > + return PTR_ERR(cnt_clk[FSL_PWM_CLK_EXT]);
> > +
> > + fpc->counter_clk_en = devm_clk_get(fpc->chip.dev, "ftm_cnt_clk_en");
> > + if (IS_ERR(fpc->counter_clk_en))
> > + return PTR_ERR(fpc->counter_clk_en);
>
> You shouldn't do this. You're obtaining a reference to each of these
> clocks whenever pwm_config() is called. And devres will only clean those
> up after the driver is unbound. Can't you simply keep a reference to
> these within struct fsl_pwm_chip?
>
> > +static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
> > +{
> > + u32 val;
> > + int ret;
> > +
> > + if (fpc->counter_clk_enable++)
>
> This function is always called with the fpc->lock held, so you could
> make this much easier by incrementing the .counter_clk_enable field only
> at the very end of the function. That way...
>
> > + return 0;
> > +
> > + ret = clk_prepare_enable(fpc->counter_clk);
> > + if (ret) {
> > + fpc->counter_clk_enable--;
>
> ... this won't be necessary...
>
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(fpc->counter_clk_en);
> > + if (ret) {
> > + fpc->counter_clk_enable--;
>
> ... and neither will this.
>
> > +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> > + u32 val;
> > + int ret;
> > +
> > + val = readl(fpc->base + FTM_OUTMASK);
> > + val &= ~BIT(pwm->hwpwm);
> > + writel(val, fpc->base + FTM_OUTMASK);
> > +
> > + mutex_lock(&fpc->lock);
>
> I think you want to extend the lock to cover the FTM_OUTMASK register
> access as well because there could be a race between pwm_enable() and
> pwm_disable().
>
> > + ret = fsl_counter_clock_enable(fpc);
> > + mutex_unlock(&fpc->lock);
> > +
> > + return ret;
> > +}
>
> Can this function be moved somewhere else so fsl_counter_clock_enable()
> and fsl_counter_clock_disable() are grouped together?
>
> > +static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc)
> > +{
> > + u32 val;
> > +
> > + if (--fpc->counter_clk_enable)
> > + return;
>
> This is going to break. Consider the case where you call pwm_disable()
> on a PWM device and fpc->counter_clk_enable == 1. In that case, this
> will decrement counter_clk_enable to 0 and proceed with the remainder of
> this function.
>
> Now you call pwm_disable() again. The above will decrement again and
> cause fpc->counter_clk_enable to wrap around to UINT_MAX.
>
> So I think a more correct implementation would be:
>
> /*
>* already disabled, do nothing (perhaps output warning message
>* to catch unbalanced calls? )
>*/
> if (fpc->counter_clk_enable == 0)
> return;
>
> /* there are still users, so can't disable yet */
> if (--fpc->counter_clk_enable > 0)
> return;
>
> /* no users left, disable clock */
>
> > +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> > + u32 val;
> > +
> > + val = readl(fpc->base + FTM_OUTMASK);
> > + val |= BIT(pwm->hwpwm);
> > + writel(val, fpc->base + FTM_OUTMASK);
> > +
> > + mutex_lock(&fpc->lock);
>
> This lock should also include the access to FTM_OUTMASK above.
>
> > +static int fsl_pwm_probe(struct platform_device *pdev)
> > +{
> [...]
> > + fpc->sys_clk = devm_clk_get(&pdev->dev, "ftm_sys");
> > + if (IS_ERR(fpc->sys_clk)) {
> > + dev_err(&pdev->dev,
> > + "failed to get \"ftm_sys\" clock\n");
>
> The above easily fits on a single line, no need for the wrapping.
>
> > + return PTR_ERR(fpc->sys_clk);
> > + }
> > +
> > + ret = clk_prepare_enable(fpc->sys_clk);
> > + if (ret)
> > +
RE: [PATCH 2/3] ASoC: core: Set the default I/O up try regmap.
> Subject: Re: [PATCH 2/3] ASoC: core: Set the default I/O up try regmap. > > On Thu, Feb 27, 2014 at 05:49:52PM +0800, Xiubo Li wrote: > > For most CODEC drivers which the REGMAP is used, the soc_probe_codec() > > will do the stuff work of snd_soc_codec_set_cache_io(), which the CODEC > > drivers' ASoC probe will do too, and almost at the same time. > > Applied, thanks. I did a check of the drivers and it looks like they'll > all be fine with this. Yes. I'll send another patches to applied to use this for another CODEC drivers. And there almost 80 files, Should I send them in one patch or split them into individual patch for each CODEC driver ? Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/3] ASoC: core: Set the default I/O up try regmap.
> Subject: Re: [PATCH 2/3] ASoC: core: Set the default I/O up try regmap. > > On Fri, Feb 28, 2014 at 04:00:38AM +0000, li.xi...@freescale.com wrote: > > > I'll send another patches to applied to use this for another CODEC drivers. > > And there almost 80 files, Should I send them in one patch or split them > into > > individual patch for each CODEC driver ? > > I'd suggest doing one patch that covers the boring drivers where the > first thing they do is call set_cache_io() but split out the others into > one patch per driver since the need more examination. Got it. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [PATCH 2/3] ASoC: core: Set the default I/O up try regmap.
> Subject: Re: [alsa-devel] [PATCH 2/3] ASoC: core: Set the default I/O up try > regmap. > > On 02/28/2014 06:56 AM, li.xi...@freescale.com wrote: > >> Subject: Re: [PATCH 2/3] ASoC: core: Set the default I/O up try regmap. > >> > >> On Fri, Feb 28, 2014 at 04:00:38AM +, li.xi...@freescale.com wrote: > >> > >>> I'll send another patches to applied to use this for another CODEC > >>> drivers. > >>> And there almost 80 files, Should I send them in one patch or split them > >> into > >>> individual patch for each CODEC driver ? > >> > >> I'd suggest doing one patch that covers the boring drivers where the > >> first thing they do is call set_cache_io() but split out the others into > >> one patch per driver since the need more examination. > > > > Got it. > > Btw. be careful, just removing the set_cache_io() call will not work for all > drivers. There are some MFD child devices which use regmap from the parent > device. So dev_get_regmap() will return NULL for those. > @Lars, Do you mean the CODEC drivers like wm5110 and wm8997 ? Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [PATCH 2/3] ASoC: core: Set the default I/O up try regmap.
> >>>> Subject: Re: [PATCH 2/3] ASoC: core: Set the default I/O up try regmap. > >>>> > >>>> On Fri, Feb 28, 2014 at 04:00:38AM +, li.xi...@freescale.com wrote: > >>>> > >>>>> I'll send another patches to applied to use this for another CODEC > drivers. > >>>>> And there almost 80 files, Should I send them in one patch or split them > >>>> into > >>>>> individual patch for each CODEC driver ? > >>>> > >>>> I'd suggest doing one patch that covers the boring drivers where the > >>>> first thing they do is call set_cache_io() but split out the others into > >>>> one patch per driver since the need more examination. > >>> > >>> Got it. > >> > >> Btw. be careful, just removing the set_cache_io() call will not work for > all > >> drivers. There are some MFD child devices which use regmap from the parent > >> device. So dev_get_regmap() will return NULL for those. > >> > > > > @Lars, > > > > Do you mean the CODEC drivers like wm5110 and wm8997 ? > > > > > Yes. > I only found these two CODEC drivers using MFD who would get its parent's regmap. Has I missed some ? Thanks, Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [PATCH 2/3] ASoC: core: Set the default I/O up try regmap.
> > A quick grep reveals: > mc13783.c:codec->control_data = dev_get_regmap(codec->dev->parent, NULL); > si476x.c: codec->control_data = dev_get_regmap(codec->dev->parent, NULL); > wm5102.c: codec->control_data = priv->core.arizona->regmap; > wm5110.c: codec->control_data = priv->core.arizona->regmap; > wm8997.c: codec->control_data = priv->core.arizona->regmap; > > But there might be more. > Yes, I'll check it more carefully... Thanks very much. Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [PATCH 2/3] ASoC: core: Set the default I/O up try regmap.
> > A quick grep reveals: > mc13783.c:codec->control_data = dev_get_regmap(codec->dev->parent, NULL); > si476x.c: codec->control_data = dev_get_regmap(codec->dev->parent, NULL); > wm5102.c: codec->control_data = priv->core.arizona->regmap; > wm5110.c: codec->control_data = priv->core.arizona->regmap; > wm8997.c: codec->control_data = priv->core.arizona->regmap; > > But there might be more. > I have found the following ones, who are using MFD & set_cache_io. 1143 sound/soc/codecs/cq93vc.c <> snd_soc_codec_set_cache_io(codec, 32, 32, SND_SOC_REGMAP); 2618 sound/soc/codecs/mc13783.c <> ret = snd_soc_codec_set_cache_io(codec, 8, 24, SND_SOC_REGMAP); 3 1765 sound/soc/codecs/wm5102.c <> ret = snd_soc_codec_set_cache_io(codec, 32, 16, SND_SOC_REGMAP); 4 1593 sound/soc/codecs/wm5110.c <> ret = snd_soc_codec_set_cache_io(codec, 32, 16, SND_SOC_REGMAP); 5 1510 sound/soc/codecs/wm8350.c <> snd_soc_codec_set_cache_io(codec, 8, 16, SND_SOC_REGMAP); 6 1322 sound/soc/codecs/wm8400.c <> snd_soc_codec_set_cache_io(codec, 8, 16, SND_SOC_REGMAP); 7 4004 sound/soc/codecs/wm8994.c <> snd_soc_codec_set_cache_io(codec, 16, 16, SND_SOC_REGMAP); 8 1058 sound/soc/codecs/wm8997.c <> ret = snd_soc_codec_set_cache_io(codec, 32, 16, SND_SOC_REGMAP); Thanks, -- Best regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [PATCHv2 1/3] ASoC: codec: Simplify ASoC probe code.
> Looks good in general, but try to build these kinds of changes with > CONFIG_COMPILE_TEST=y and CONFIG_SND_SOC_ALL_CODECS=y before sending the > patch. There are a lot of warnings about unused variables caused by this > patch. > @Lars, Oh, yes, these are very low-level errors, I will fix them then. Thanks very much. > sound/soc/codecs/ad193x.c: In function 'ad193x_codec_probe': > sound/soc/codecs/ad193x.c:343:2: warning: 'ret' is used uninitialized in > this function [-Wuninitialized] > sound/soc/codecs/adau1373.c: In function 'adau1373_probe': > sound/soc/codecs/adau1373.c:1379:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/adav80x.c: In function 'adav80x_probe': > sound/soc/codecs/adav80x.c:804:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/ak4535.c: In function 'ak4535_probe': > sound/soc/codecs/ak4535.c:392:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/ak4535.c:391:22: warning: unused variable 'ak4535' > [-Wunused-variable] > sound/soc/codecs/ak4641.c: In function 'ak4641_probe': > sound/soc/codecs/ak4641.c:522:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/ak4642.c: In function 'ak4642_probe': > sound/soc/codecs/ak4642.c:468:6: warning: unused variable 'ret' [-Wunused- > sound/soc/codecs/ak4671.c: In function 'ak4671_probe': > sound/soc/codecs/ak4671.c:620:2: warning: 'ret' is used uninitialized in > this function [-Wuninitialized] > sound/soc/codecs/cs42l52.c: In function 'cs42l52_probe': > sound/soc/codecs/cs42l52.c:1125:2: warning: 'ret' is used uninitialized in > this function [-Wuninitialized] > sound/soc/codecs/cs42l73.c: In function 'cs42l73_probe': > sound/soc/codecs/cs42l73.c:1363:2: warning: 'ret' is used uninitialized in > this function [-Wuninitialized] > sound/soc/codecs/da7210.c: In function 'da7210_probe': > sound/soc/codecs/da7210.c:1074:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/da7213.c: In function 'da7213_probe': > sound/soc/codecs/da7213.c:1396:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/da732x.c: In function 'da732x_probe': > sound/soc/codecs/da732x.c:1522:1: warning: label 'err' defined but not used > [-Wunused-label] > sound/soc/codecs/da9055.c: In function 'da9055_probe': > sound/soc/codecs/da9055.c:1386:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/max9850.c: In function 'max9850_probe': > sound/soc/codecs/max9850.c:315:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/ml26124.c: In function 'ml26124_probe': > sound/soc/codecs/ml26124.c:590:23: warning: unused variable 'priv' > [-Wunused-variable] > sound/soc/codecs/ml26124.c:589:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/rt5631.c: In function 'rt5631_probe': > sound/soc/codecs/rt5631.c:1573:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/rt5640.c: In function 'rt5640_probe': > sound/soc/codecs/rt5640.c:1939:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/ssm2518.c: In function 'ssm2518_probe': > sound/soc/codecs/ssm2518.c:652:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/ssm2518.c:651:18: warning: unused variable 'ssm2518' > [-Wunused-variable] > sound/soc/codecs/sta32x.c: In function 'sta32x_probe': > sound/soc/codecs/sta32x.c:940:1: warning: label 'err' defined but not used > [-Wunused-label] > sound/soc/codecs/sta529.c: In function 'sta529_probe': > sound/soc/codecs/sta529.c:326:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/sta529.c:325:17: warning: unused variable 'sta529' > [-Wunused-variable] > sound/soc/codecs/tlv320aic23.c: In function 'tlv320aic23_probe': > sound/soc/codecs/tlv320aic23.c:562:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/wm8510.c: In function 'wm8510_probe': > sound/soc/codecs/wm8510.c:599:2: warning: 'ret' is used uninitialized in > this function [-Wuninitialized] > sound/soc/codecs/wm8523.c: In function 'wm8523_probe': > sound/soc/codecs/wm8523.c:395:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/wm8728.c: In function 'wm8728_probe': > sound/soc/codecs/wm8728.c:236:2: warning: 'ret' is used uninitialized in > this function [-Wuninitialized] > sound/soc/codecs/wm8900.c: In function 'wm8900_probe': > sound/soc/codecs/wm8900.c:1181:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/wm8903.c: In function 'wm8903_probe': > sound/soc/codecs/wm8903.c:1907:2: warning: 'ret' is used uninitialized in > this function [-Wuninitialized] > sound/soc/codecs/wm8904.c: In function 'wm8904_probe': > sound/soc/codecs/wm8904.c:2051:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/wm8961.c: In function 'wm8961_probe': > sound/soc/codecs/wm8961.c:839:6: warning: unused variable 'ret' > [-Wunused-variable] > sound/soc/codecs/wm8978.c: In fun
RE: [PATCHv2 1/3] ASoC: codec: Simplify ASoC probe code.
> > "Just removing the set_cache_io() call will not work for all
> > drivers. There are some MFD child devices which use regmap from the parent
> > device. So dev_get_regmap() will return NULL for those."
>
> This is the sort of thing that I was referring to when talking about
> doing the non-boring drivers separately. As well as the warnings Lars
> mentioned there's a bisection issue here:
>
> > - codec->control_data = da7213->regmap;
> > - ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_REGMAP);
> > - if (ret < 0) {
> > - dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
> > - return ret;
> > - }
> > -
> > /* Default to using ALC auto offset calibration mode. */
> > snd_soc_update_bits(codec, DA7213_ALC_CTRL1,
> > DA7213_ALC_CALIB_MODE_MAN, 0);
>
> Unless the core sets up the I/O before calling probe() the above is
> going to mean that the snd_soc_update_bits() call fails since the I/O
> operations won't have been set up. There is a defualt call to set a
> regmap up but it's only done after the probe.
Yes, like the SGTL5000 CODEC driver, which needs to read the chip information
in main probe. And then it must use the regmap core APIs directly.
And I have do another research early, all the CODEC drivers who are calling
set_cache_io() in ASoC probes, and before that they all are using the regmap
core
APIs instead of ASoC ones if needed.
I think we should move the set_cache_io() calling more earlier as we discussed
before, but need much more research and testing. Splitting them into another
separate patch later will be much better and easier to be reviewed.
Thanks,
--
Best Regards,
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv3 1/3] ASoC: codec: Simplify ASoC probe code.
> > static int ak4535_probe(struct snd_soc_codec *codec)
> > {
> > - struct ak4535_priv *ak4535 = snd_soc_codec_get_drvdata(codec);
> > - int ret;
> > -
> > - codec->control_data = ak4535->regmap;
> > - ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_REGMAP);
> > - if (ret < 0) {
> > - dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
> > - return ret;
> > - }
> > /* power on device */
> > ak4535_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
>
> Are you sure that the set_bias_level() call doesn't do anything with
> I/O? Can I suggest sending a patch that just does the drivers that only
> do set_cache_io() and nothing else in probe - that would be really quick
> and simple to fix.
>
> > - ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_REGMAP);
> > - if (ret < 0) {
> > - dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
> > - return ret;
> > - }
> > -
> > /* Default to using ALC auto offset calibration mode. */
> > snd_soc_update_bits(codec, DA7213_ALC_CTRL1,
> > DA7213_ALC_CALIB_MODE_MAN, 0);
>
> This one will fail.
Sorry, I'm not very understand why will this fail ? Before the ASoC probe,
the ASoC core will set the I/O.
:)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv10 0/4] Add Freescale FTM PWM support
> Subject: Re: [PATCHv10 0/4] Add Freescale FTM PWM support > > On Thu, Feb 27, 2014 at 05:39:48PM +0800, Xiubo Li wrote: > > Changed in V10: > > Fix some bugs and adjust the code from Thierry's comments. > > > > > > Xiubo Li (4): > > pwm: Add Freescale FTM PWM driver support > > ARM: dts: vf610: Add Freescale FTM PWM node. > > ARM: dts: vf610-twr: Enables FTM PWM device. > > Documentation: Add device tree bindings for Freescale FTM PWM. > > > > .../devicetree/bindings/pwm/pwm-fsl-ftm.txt| 35 ++ > > arch/arm/boot/dts/vf610-twr.dts| 6 + > > arch/arm/boot/dts/vf610.dtsi | 13 + > > drivers/pwm/Kconfig| 10 + > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-fsl-ftm.c | 496 > + > > 6 files changed, 561 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt > > create mode 100644 drivers/pwm/pwm-fsl-ftm.c > > I've applied patches 1 and 4 to the PWM tree, with some minor cosmetic > fixups that I thought didn't warrant these patches to go through yet > another revision. =) > > Thanks, > Thierry @Thierry, Great, Thanks very much, :) Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] ASoC: fsl: Make Freescale SAI/ESAI/SPDIF to be visible in Kconfig
Hi Mark, Has this patch been missing? I couldn't found it anywhere in the next branch. :) Thanks very much, -- Best Regards, Xiubo > -Original Message- > From: Nicolin Chen [mailto:guangyu.c...@freescale.com] > Sent: Thursday, February 20, 2014 11:08 AM > To: Mark Brown > Cc: Xiubo Li-B47053; lgirdw...@gmail.com; shawn@linaro.org; > pe...@perex.cz; > ti...@suse.de; Estevam Fabio-R49496; alsa-de...@alsa-project.org; linux- > ker...@vger.kernel.org > Subject: Re: [PATCH 2/2] ASoC: fsl: Make Freescale SAI/ESAI/SPDIF to be > visible in Kconfig > > On Thu, Feb 20, 2014 at 11:44:32AM +0900, Mark Brown wrote: > > On Thu, Feb 20, 2014 at 02:06:20AM +, li.xi...@freescale.com wrote: > > > > > I'm not very sure of this patch, maybe should we add one menu > > > in Kconfig for all visible CPU DAIs firstly like for code drivers? > > > > Allowing them to be individually selected is definitely useful if people > > are trying to minimise their kernel size and/or build time. However > > none of the other Freescale people have commented on this patch (which > > I'd have expected) so I was giving them time and IIRC it needs a rebase > > against current code. > > Last month I was revising a new CODEC driver for ESAI and meanwhile abort > to plan to try simple card for this combination but being suspended due to > some tough internal issues. So I think it should be a good idea for us to > move towards simple card starting from this patch. > > Surely, Acked. > Nicolin Chen > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] ASoC: fsl-sai: convert to use regmap API for Freeacale SAI
> Subject: Re: [PATCH v2] ASoC: fsl-sai: convert to use regmap API for Freeacale > SAI > > On Sat, Feb 08, 2014 at 02:38:28PM +0800, Xiubo Li wrote: > > Signed-off-by: Xiubo Li > > Applied, thanks but I think this needs a select of REGMAP_MMIO in the > Kconfig to ensure that regmap-mmio is actually there (it might well be > OK due to implicit dependencies at the minute but should be fixed > properly). Yes, I will send another patch to fix it. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/2] ASoC: fsl-spdif: big-endian support
> > + spdif_priv->big_endian = of_property_read_bool(np, "big-endian"); > > + if (spdif_priv->big_endian) > > + fsl_spdif_regmap_config.val_format_endian = REGMAP_ENDIAN_BIG; > > Why not just: > if (of_property_read_bool(np, "big-endian")) > fsl_spdif_regmap_config.val_format_endian = REGMAP_ENDIAN_BIG; > I just think maybe other places of the driver maybe use this. Yes, it could be removed just for now. See the next version please. Thanks very much, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] ASoC: fsl-esai: big-endian support
> > + - big-endian : If this property is absent, the native endian mode will > > +be in use as default, or the big endian mode will be in use for all the > > +device registers. > > > + > > + > > Single blank line should be enough here :) > This will be removed. > > @@ -687,6 +688,10 @@ static int fsl_esai_probe(struct platform_device *pdev) > > esai_priv->pdev = pdev; > > strcpy(esai_priv->name, np->name); > > > > + esai_priv->big_endian = of_property_read_bool(np, "big-endian"); > > + if (esai_priv->big_endian) > > + fsl_esai_regmap_config.val_format_endian = REGMAP_ENDIAN_BIG; > > + > > Same comments here. And please wait for Shawn's reply at the other patch > before you revise this part to V2. > Okey, Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/2] ASoC: fsl-spdif: big-endian support
> > + - big-endian : If this property is absent, the native endian mode will > > + be in use as default, or the big endian mode will be in use for all the > > + device registers. > > + > > @Shawn > Does DT have an existing approach to determine if the current SoC this IP > uses is BE or LE? I am thinking the scenario that if all drivers support > BE/LE while the SoC is big-endian, all the nodes in the DT would have to > include a duplicated property (big-endian). > @Nicolin, @Shawn One special scenario likes the LS1 platform, the CPU and a few devices are in LE mode and other devices will be in BE mode. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [PATCH v2 1/3] ASoC: binding: add tdm-slot.txt
> > The current internal API for TDM is very poor, I don't think we want > > to expose that 1 to 1 to the devicetree. Since this means we'd have > > to support that forever. The first thing is that the semantics of > > snd_soc_dai_set_tdm_slot() are very unclear. E.g. some drivers use a > > zero bit for a active slot, some drivers use a 1 bit for a active > > Yes, and if we do end up using masks we need to nail down what's going > on in the DT. > Yes, certainly. > > slot. The second thing is that we are not able to specify which > > channel should be mapped to which slot. You can merely specify > > from/to which slots the CODEC should read/write and then it is up to > > the driver to guess which channel should go to which slot. In my > > opinion a binding that allows to specify a explicit mapping of which > > channel goes to which slot would be much better. > > It'd certainly be good to be able to do that, though having a default > would make life easier. > @Lars, @Mark, Yes, then will that means that we can just end up parsing masks from DT and the masks will be generated by the driver specified dai->driver->ops->of_xlate_tdm_slot_mask(slots, &tx_mask, &rx_mask) callback or one default snd_soc_of_xlate_tdm_slot_mask(slots, &tx_mask, &rx_mask), and the 'slots' is the number of the slot parsed from the DT node ? Or other better ways ? Thanks, -- Best Regards, Xiubo > > Also those are four different settings. In my opinion they should > > not be expressed in one property, but rather in four. E.g. > > specifying a tx_mask for a rx only device does not make much sense. > > That makes sense. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/4] ARM: dts: Enable SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.
> > @@ -127,9 +127,27 @@
> > VF610_PAD_PTB5__UART1_RX0x21a1
> > >;
> > };
> > +
> > + pinctrl_sai2: sai2grp {
>
> To sort it alphabetically, the entry should be added before
> pinctrl_uart1.
>
> > + fsl,pins = <
> > + VF610_PAD_PTA16__SAI2_TX_BCLK 0x02ed
> > + VF610_PAD_PTA18__SAI2_TX_DATA 0x02ee
> > + VF610_PAD_PTA19__SAI2_TX_SYNC 0x02ed
> > + VF610_PAD_PTA21__SAI2_RX_BCLK 0x02ed
> > + VF610_PAD_PTA22__SAI2_RX_DATA 0x02ed
> > + VF610_PAD_PTA23__SAI2_RX_SYNC 0x02ed
> > + VF610_PAD_PTB18__EXT_AUDIO_MCLK 0x02ed
>
> Use tabs instead of spaces.
>
> Shawn
>
Yes, I'll fix these two issues above.
Thanks,
--
Best Regards,
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4] ARM: dts: Add Freescale SAI ALSA SoC Digital Audio Interface node for VF610.
> > 'ARM: dts: vf610: ...' for patch prefix. > > Shawn Please see the next version. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/4] ARM: dts: Enable SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.
> The patch subject can be a little short like > > ARM: dts: vf610-twr: Enable SAI ALSA SoC DAI device > Yes, that looks better. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv9 1/4] pwm: Add Freescale FTM PWM driver support
> Subject: RE: [PATCHv9 1/4] pwm: Add Freescale FTM PWM driver support
>
> > Subject: [PATCHv9 1/4] pwm: Add Freescale FTM PWM driver support
> >
> > The FTM PWM device can be found on Vybrid VF610 Tower and Layerscape LS-1
> > SoCs.
> >
> > Signed-off-by: Xiubo Li
> > ---
>
> For this patch series,
> Reviewed-by: Yuan Yao
>
>
Thanks for your reply very much.
And the following information had been missing and should be added too:
Signed-off-by: Alison Wang
Signed-off-by: Jingchang Lu
Reviewed-by: Sascha Hauer
Thanks,
--
Best Regards,
Xiubo
> Thanks.
>
>
> >
> >
> > Hi Thierry,
> >
> > For this version I just removed the big-endian mode support, and will add
> > it in later separate patches.
> >
> >
> > Changes in v9:
> > - Remove the big-endian mode support.
> >
> > Changes in v7 ~ v8:
> > - Mainly about big-endian mode support.
> >
> > [snip] v1~v6
> >
> >
> >
> > drivers/pwm/Kconfig | 10 +
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-fsl-ftm.c | 479
> > ++
> > 3 files changed, 490 insertions(+)
> > create mode 100644 drivers/pwm/pwm-fsl-ftm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 6a2a1e0a..9a4c641 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -80,6 +80,16 @@ config PWM_EP93XX
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-ep93xx.
> >
> > +config PWM_FSL_FTM
> > + tristate "Freescale FlexTimer Module (FTM) PWM support"
> > + depends on OF
> > + help
> > + Generic FTM PWM framework driver for Freescale VF610 and
> > + Layerscape LS-1 SoCs.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-fsl-ftm.
> > +
> > config PWM_IMX
> > tristate "i.MX PWM support"
> > depends on ARCH_MXC
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > 1b99cfb..c22905c 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o
> > obj-$(CONFIG_PWM_ATMEL_TCB)+= pwm-atmel-tcb.o
> > obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o
> > obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o
> > +obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o
> > obj-$(CONFIG_PWM_IMX) += pwm-imx.o
> > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> > obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
> > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c new
> > file mode 100644 index 000..60467b3
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-fsl-ftm.c
> > @@ -0,0 +1,479 @@
> > +/*
> > + * Freescale FlexTimer Module (FTM) PWM Driver
> > + *
> > + * Copyright 2012-2013 Freescale Semiconductor, Inc.
> > + *
> > + * 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; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define FTM_SC 0x00
> > +#define FTM_SC_CLK_MASK0x3
> > +#define FTM_SC_CLK_SHIFT 3
> > +#define FTM_SC_CLK(c) (((c) + 1) << FTM_SC_CLK_SHIFT)
> > +#define FTM_SC_PS_MASK 0x7
> > +#define FTM_SC_PS_SHIFT0
> > +
> > +#define FTM_CNT0x04
> > +#define FTM_MOD0x08
> > +
> > +#define FTM_CSC_BASE 0x0C
> > +#define FTM_CSC_MSBBIT(5)
> > +#define FTM_CSC_MSABIT(4)
> > +#define FTM_CSC_ELSB BIT(3)
> > +#define FTM_CSC_ELSA BIT(2)
> > +#define FTM_CSC(_channel) (FTM_CSC_BASE + ((_channel) * 8))
> > +
> > +#define FTM_CV_BASE0x10
> > +#define FTM_CV(_channel) (FTM_CV_BASE + ((_channel) * 8))
> > +
> > +#define FTM_CNTIN 0x4C
> > +#define FTM_STATUS 0x50
> > +
> > +#define FTM_MODE 0x54
> > +#define FTM_MODE_FTMEN BIT(0)
> > +#define FTM_MODE_INIT BIT(2)
> > +#define FTM_MODE_PWMSYNC BIT(3)
> > +
> > +#define FTM_SYNC 0x58
> > +#define FTM_OUTINIT0x5C
> > +#define FTM_OUTMASK0x60
> > +#define FTM_COMBINE0x64
> > +#define FTM_DEADTIME 0x68
> > +#define FTM_EXTTRIG0x6C
> > +#define FTM_POL0x70
> > +#define FTM_FMS0x74
> > +#define FTM_FILTER 0x78
> > +#define FTM_FLTCTRL0x7C
> > +#define FTM_QDCTRL 0x80
> > +#define FTM_CONF 0x84
> > +#define FTM_FLTPOL 0x88
> > +#define FTM_SYNCONF0x8C
> > +#define FTM_INVCTRL0x90
> > +#define FTM_SWOCTRL0x94
> > +#define FTM_PWMLOAD0x98
> > +
> > +enum fsl_pwm_clk {
> > + FSL_PWM_CLK_SYS,
> > + FSL_PWM_CLK_FIX,
> > + FSL_PWM_CLK_EXT,
> > +};
> > +
> > +struct fsl_pwm_chip {
> > + struct pwm_chip chip;
> > +
> > + struct mutex lock;
> > +
> > + struct
RE: [PATCH 2/2] ASoC: fsl: Make Freescale SAI/ESAI/SPDIF to be visible in Kconfig
Hi Mark, I'm not very sure of this patch, maybe should we add one menu in Kconfig for all visible CPU DAIs firstly like for code drivers? Thanks, -- Best Regards, Xiubo > Subject: [PATCH 2/2] ASoC: fsl: Make Freescale SAI/ESAI/SPDIF to be visible in > Kconfig > > For simple card since the whole idea is to support any CODEC and > any CPU DAI with the same driver, we should probably just make sure > that all the individual CODEC and CPU DAI drivers can be enabled in > Kconfig. That way we don't have to have specific Kconfig entries for > boards and loose some of the benefit of the generic card. > > This will make the Freescale SAI/ESAI/SPDIF device be visible in > Kconfig. > > Signed-off-by: Xiubo Li > Cc: Nicolin Chen > Cc: Mark Brown > --- > sound/soc/fsl/Kconfig | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig > index d0914c0..f397144 100644 > --- a/sound/soc/fsl/Kconfig > +++ b/sound/soc/fsl/Kconfig > @@ -1,5 +1,5 @@ > config SND_SOC_FSL_SAI > - tristate > + tristate "ALSA SoC support for the Freescale SAI device" > select REGMAP_MMIO > select SND_SOC_GENERIC_DMAENGINE_PCM > > @@ -7,11 +7,11 @@ config SND_SOC_FSL_SSI > tristate > > config SND_SOC_FSL_SPDIF > - tristate > + tristate "ALSA SoC support for the Freescale SPDIF device" > select REGMAP_MMIO > > config SND_SOC_FSL_ESAI > - tristate > + tristate "ALSA SoC support for the Freescale ESAI device" > select REGMAP_MMIO > > config SND_SOC_FSL_UTILS > -- > 1.8.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH Resend] regmap: fix _regmap_update_bits()
> Subject: Re: [PATCH Resend] regmap: fix _regmap_update_bits() > > On Thu, Feb 20, 2014 at 08:50:10AM +0800, Xiubo Li wrote: > > > This resend version just rewrites the commit comment. > > I've applied this but I updated the commit log further - part of the > reason this didn't get applied first time around was that it says it's a > fix but it's actually a coding style change. Yes, it's. Thanks very much for your updating. -- Best Regards, Xiubo > The other part of the > reason was that at first glance I wasn't sure if it was actually an > improvement so wanted to think about it. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] ASoC: fsl: Make Freescale SAI/ESAI/SPDIF to be visible in Kconfig
> Subject: Re: [PATCH 2/2] ASoC: fsl: Make Freescale SAI/ESAI/SPDIF to be > visible in Kconfig > > On Thu, Feb 20, 2014 at 02:06:20AM +0000, li.xi...@freescale.com wrote: > > > I'm not very sure of this patch, maybe should we add one menu > > in Kconfig for all visible CPU DAIs firstly like for code drivers? > > Allowing them to be individually selected is definitely useful if people > are trying to minimise their kernel size and/or build time. However > none of the other Freescale people have commented on this patch (which > I'd have expected) so I was giving them time and IIRC it needs a rebase > against current code. Yes, Surely. As we can see that there will be many CPU DAI drivers to be visible in the future, and they will get a bit noisy too. IMHO, put the CPU DAI drivers in a menu is a good choice for other separate patches. Thanks very much, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 6/8] ASoC: simple-card: add tdm slot supports
Hi David,
Firstly thanks for your comment.
> > +asoc_simple_card_of_parse_tdm_slot(struct device_node *np,
> > + struct device *dev,
> > + struct asoc_simple_dai *dai,
> > + const char *propname)
> > +{
> > + struct asoc_simple_tdm_slot *tdm;
> > + u32 out_value[4];
> > + int ret;
> > +
> > + if (!of_property_read_bool(np, propname))
> > + return 0;
> > +
> > + tdm = devm_kzalloc(dev, sizeof(*tdm), GFP_KERNEL);
> > + if (!tdm)
> > + return -ENOMEM;
> > +
> > + ret = of_property_read_u32_array(np, propname, out_value, 4);
> > + if (ret)
> > + return ret;
>
> Looks like a memory leak?
>
Using devm_ and this will be called by _probe().
Please see the "Documentation/driver-model/devres.txt" for detail about
devres.
> > +
> > + tdm->tx_mask = out_value[0];
> > + tdm->rx_mask = out_value[1];
> > + tdm->slots = out_value[2];
> > + tdm->slot_width = out_value[3];
> > +
> > + dai->tdm = tdm;
> > +
> > + return 0;
> > +}
> > +
Thanks,
Best Regards,
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH Resend 2/8] ASoC: simple-card: fix __asoc_simple_card_dai_init
> > diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-
> card.c
> > index 6443c87..3b8c9a2 100644
> > --- a/sound/soc/generic/simple-card.c
> > +++ b/sound/soc/generic/simple-card.c
> > @@ -31,16 +31,21 @@ static int __asoc_simple_card_dai_init(struct
> snd_soc_dai *dai,
> >
> > daifmt |= set->fmt;
> >
> > - if (daifmt)
> > + if (daifmt) {
> > ret = snd_soc_dai_set_fmt(dai, daifmt);
> > -
> > - if (ret == -ENOTSUPP) {
> > - dev_dbg(dai->dev, "ASoC: set_fmt is not supported\n");
> > - ret = 0;
> > + if (ret && ret != -ENOTSUPP) {
> > + dev_err(dai->dev, "simple-card: set_fmt error\n");
> > + return ret;
> > + }
> > }
> >
> > - if (!ret && set->sysclk)
> > + if (set->sysclk) {
> > ret = snd_soc_dai_set_sysclk(dai, 0, set->sysclk, 0);
> > + if (ret && ret != -ENOTSUPP) {
> > + dev_err(dai->dev, "simple-card: set_sysclk error\n");
> > + return ret;
> > + }
> > + }
> >
> > return ret;
>
> Sorry: you must return 0 here
>
Yes, the ret maybe -ENOTSUPP.
I will revise it.
Thanks,
Best Regards,
Xiubo
RE: [alsa-devel] [PATCH] ASoC: fsl-sai: convert to use regmap API for Freeacale SAI
> > static int fsl_sai_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
> > int clk_id, unsigned int freq, int dir)
> > {
> > - struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> > int ret;
> >
> > if (dir == SND_SOC_CLOCK_IN)
> > return 0;
> >
> > - ret = clk_prepare_enable(sai->clk);
> > - if (ret)
> > - return ret;
>
> The clock related change seems unrelated to the usage of regmap, right?
>
The clock operations in the driver is only related to the module clock, and has
move to the regmap core and regmap core will to the same clock operations at
proper time, since we are using:
+ sai->regmap = devm_regmap_init_mmio_clk(&pdev->dev,
+ "sai", base, &fsl_sai_regmap_config);
Thanks,
Best Regards,
Xiubo
RE: [PATCH] ASoC: simple-card: fix simple card widgets routing property name usage
Hi,
I'd like to know the status about this patch.
Thanks very much.
Best Regards,
Xiubo
> Subject: [PATCH] ASoC: simple-card: fix simple card widgets routing property
> name usage
>
> Fix the usage of simple card widgets routing property, and make it the
> same with simple card routing property name.
>
> Signed-off-by: Xiubo Li
> ---
> Documentation/devicetree/bindings/sound/simple-card.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt
> b/Documentation/devicetree/bindings/sound/simple-card.txt
> index e9e20ec..19c84df 100644
> --- a/Documentation/devicetree/bindings/sound/simple-card.txt
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -43,7 +43,7 @@ Example:
> sound {
> compatible = "simple-audio-card";
> simple-audio-card,format = "left_j";
> - simple-audio-routing =
> + simple-audio-card,routing =
> "MIC_IN", "Mic Jack",
> "Headphone Jack", "HP_OUT",
> "Ext Spk", "LINE_OUT";
> --
> 1.8.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH Resend 7/8] ASoC: add snd_soc_of_parse_audio_simple_widgets for DeviceTree
> > +static struct snd_soc_dapm_widget simple_widgets[] = {
> > + SND_SOC_DAPM_MIC("Mic", NULL),
> > + SND_SOC_DAPM_LINE("Line", NULL),
> > + SND_SOC_DAPM_HP("Hp", NULL),
> > + SND_SOC_DAPM_SPK("Spk", NULL),
> > +};
>
This is the templates for "Microphone XXX", "Line XXX", "Headphone XXX"
and "Spk XXX" .
> Does this mean we're restricted to a particular set of names? That
> seems sad and won't work if there's a desire for more than one of a
> given widget - the main use case I can see is multiple microphones with
> separate microphone biases. How about having some templates that we
> copy and then replace the name with the one the user supplied?
>
This code has already supplied this I think.
Using this API, the only limitation is that, for example if the user has
multiple Microphones, it should set the name in DT file like:
"Mic XXX", "Mic YYY", "Mic ZZZ"...
And these widgets will use the template of : SND_SOC_DAPM_MIC("Mic", NULL),
And the widgets' names will be replaced with "Mic XXX", "Mic YYY",
"Mic ZZZ"... from the user's DT node.
Or your suggestion is ：
The templates is:
+static struct snd_soc_dapm_widget simple_widgets[] = {
+ SND_SOC_DAPM_MIC("Mic", NULL),
+ SND_SOC_DAPM_LINE("Line", NULL),
+ SND_SOC_DAPM_HP("Hp", NULL),
+ SND_SOC_DAPM_SPK("Spk", NULL),
+};
And in the DT node:
simple-off-codec-widgets =
/* template-name user-supplied-name */
"Mic", "Microphone XXX",
"Mic", "UUU Microphone YYY",
"Hp", "TTT Headphone XXX",
"Hp", "Headphone YYY";
And then just replace the template's name with the user supplied name?
Thanks,
Best Regards,
Xiubo
RE: [PATCH Resend 3/8] ASoC: simple-card: simplify the daifmt code
> Subject: Re: [PATCH Resend 3/8] ASoC: simple-card: simplify the daifmt code > > On Thu, Jan 23, 2014 at 01:02:45PM +0800, Xiubo Li wrote: > > In the asoc_simple_card_parse_of() will parse the device node's CPU/CODEC > > DAI commone fmts, and then in asoc_simple_card_sub_parse_of() will parse > > the CPU/CODEC DAI's sub-node fmts, so we can combine the info->daifmt and > > info->set.fmt in asoc_simple_card_sub_parse_of() not while just before > > _set_fmt(). > > This doesn't apply against the current topic/simple, can you please > check and resend? Yes, it's caused by the dependency of PATCH 2/8, and the PATCH 2/8 needs to be revised. I will resend them, which hasn't been applied, and also adding one binding patch. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [PATCH] ASoC: simple-card: fix simple card widgets routing property name usage
> Subject: Re: [alsa-devel] [PATCH] ASoC: simple-card: fix simple card widgets > routing property name usage > > On Thu, Jan 23, 2014 at 11:00:16AM +0000, li.xi...@freescale.com wrote: > > Hi, > > > > I'd like to know the status about this patch. > > Please send upstream mail to kernel.org if you want it reading - almost > all upstream mail sent to my Linaro address is a duplicate and doesn't > get read there. Get it. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/8] ASoC: fsl: Add VF610 soc audio card Kconfig
Hi Mark, > > +config SND_SOC_VF610_SGTL5000 > > + tristate "SoC Audio support for VF610 boards with SGTL5000" > > + depends on OF && I2C > > + select SND_SOC_FSL_SAI > > + select SND_SOC_SGTL5000 > > + select SND_SIMPLE_CARD > > ...for simple card since the whole idea is to support any CODEC with the > same driver we should probably just make sure that all the individual > drivers can be enabled in Kconfig, that way we don't have to have > specific Kconfig entries for boards and loose some of the benefit of the > generic card. I sent a patch earlier exposing the OF supporting CODEC > drivers, one for at least some of the Freescale CPU drivers was sent to > the list recently too but there were some review comments. I hadn't found the patches, could you tell me the titles of them please? Thanks very much, -- * Happy New Year * Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] regmap: mmio: Fix the bug of 'offset' value parsing.
> Subject: Re: [PATCH] regmap: mmio: Fix the bug of 'offset' value parsing. > > On Mon, Mar 31, 2014 at 02:33:31PM +0800, Xiubo Li wrote: > > 'offset = *(u32 *)reg;', this will be okey for 32/64-bit register, but > > for 8/16-bit register, the 'offset' value will overflow. > > This doesn't apply against regmap-v3.15, please check and resend. > > > The core trace: > > === > > Unable to handle kernel paging request at virtual address ffbd > > pgd = 871e8000 > > [ffbd] *pgd= > > Internal error: Oops: 805 [#1] ARM > > Modules linked in: > > CPU: 0 PID: 1004 Comm: sh Not tainted 3.14.0-rc6+ #990 > > task: 871baa40 ti: 8709 task.ti: 8709 > > Don't paste entire backtraces into commit messages, they're enormous and > most of the detail isn't useful making it easy to miss relevant > information - either pick relevant sections or just describe the > failure. Yes, I have resent this patch and have fixed the issue that you pointed out. Thanks :) -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv2 2/2] watchdog: imx2_wdt: Add big-endian support
> Subject: Re: [PATCHv2 2/2] watchdog: imx2_wdt: Add big-endian support > > On Tue, Apr 01, 2014 at 02:24:12AM +0000, li.xi...@freescale.com wrote: > > > Yes, I do think so too and there has already regmap-mmio version patch > > Series about the IMX2 Watchdog driver, but those regmap patches for > > supporting the 16-bit values are in Mark's tree and the IMX2 Watchdog > > will depend on them. > > > How to deal with this situation ? > > Well, anything that's already applied went to Linus already so is in > -rc1. Anything else we can either do a cross tree merge or I can apply > the driver. That's great. Thanks very much, BRs Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] regmap: add DT endianness binding support.
Please discard this patch series: There has some problems about the commit message, I will resend this patch series. > Subject: [PATCH 2/2] regmap: add DT endianness binding support. > > For many drivers which will support rich endianness of CPU<-->Dev > need define DT properties by itself without the binding support. > > The value endianness using regmap-mmio, for example: > IndexCPU Device Endianess flag for DT property > > 1LELE - > 2LEBE 'little-endian' Should be 'big-endian' here. > 3BEBE - > 4BELE 'big-endian' > Should be 'little-endian' here. > > > Here add DT endianness binding support will define two string > properties of the register and value endiannesses: > > 'reg-endian' and 'val-endian'. > > And the value of them will be: > 'LE' : REGMAP_ENDIAN_LITTLE > 'BE' : REGMAP_ENDIAN_BIG > 'NT' : REGMAP_ENDIAN_NATIVE > Absent : REGMAP_ENDIAN_DEFAULT > > The value endianness using regmap-mmio, for example: > IndexCPU Device Endianess flag for DT property > > 1LELE 'NT' or absent > 2LEBE 'LE' Should be 'BE' here. > 3BEBE 'NT' or absent > 4BELE 'BE' > Should 'LE' here. > Please see the following documetation for detail usage: > Documentation/devicetree/bindings/regmap/regmap-endianness.txt > Thanks, BRs Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv3 2/2] watchdog: imx2_wdt: convert to use regmap API.
> Subject: Re: [PATCHv3 2/2] watchdog: imx2_wdt: convert to use regmap API.
>
> On Tue, Apr 01, 2014 at 12:46:38PM +0800, Xiubo Li wrote:
> > Signed-off-by: Xiubo Li
> > Cc: Guenter Roeck
>
> We should probably have some commit log to explain why we need to move
> to use regmap API.
>
Well, yes, I will send V4 series to do the explaination.
Thanks :)
BRs
Xiubo
> Shawn
>
> > ---
> > drivers/watchdog/imx2_wdt.c | 50 +-
> ---
> > 1 file changed, 32 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> > index 1795922..76fa724 100644
> > --- a/drivers/watchdog/imx2_wdt.c
> > +++ b/drivers/watchdog/imx2_wdt.c
> > @@ -31,6 +31,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -61,7 +62,7 @@
> >
> > static struct {
> > struct clk *clk;
> > - void __iomem *base;
> > + struct regmap *regmap;
> > unsigned timeout;
> > unsigned long status;
> > struct timer_list timer;/* Pings the watchdog when closed */
> > @@ -87,7 +88,9 @@ static const struct watchdog_info imx2_wdt_info = {
> >
> > static inline void imx2_wdt_setup(void)
> > {
> > - u16 val = __raw_readw(imx2_wdt.base + IMX2_WDT_WCR);
> > + u32 val;
> > +
> > + regmap_read(imx2_wdt.regmap, IMX2_WDT_WCR, &val);
> >
> > /* Suspend timer in low power mode, write once-only */
> > val |= IMX2_WDT_WCR_WDZST;
> > @@ -100,17 +103,17 @@ static inline void imx2_wdt_setup(void)
> > /* Set the watchdog's Time-Out value */
> > val |= WDOG_SEC_TO_COUNT(imx2_wdt.timeout);
> >
> > - __raw_writew(val, imx2_wdt.base + IMX2_WDT_WCR);
> > + regmap_write(imx2_wdt.regmap, IMX2_WDT_WCR, val);
> >
> > /* enable the watchdog */
> > val |= IMX2_WDT_WCR_WDE;
> > - __raw_writew(val, imx2_wdt.base + IMX2_WDT_WCR);
> > + regmap_write(imx2_wdt.regmap, IMX2_WDT_WCR, val);
> > }
> >
> > static inline void imx2_wdt_ping(void)
> > {
> > - __raw_writew(IMX2_WDT_SEQ1, imx2_wdt.base + IMX2_WDT_WSR);
> > - __raw_writew(IMX2_WDT_SEQ2, imx2_wdt.base + IMX2_WDT_WSR);
> > + regmap_write(imx2_wdt.regmap, IMX2_WDT_WSR, IMX2_WDT_SEQ1);
> > + regmap_write(imx2_wdt.regmap, IMX2_WDT_WSR, IMX2_WDT_SEQ2);
> > }
> >
> > static void imx2_wdt_timer_ping(unsigned long arg)
> > @@ -143,12 +146,8 @@ static void imx2_wdt_stop(void)
> >
> > static void imx2_wdt_set_timeout(int new_timeout)
> > {
> > - u16 val = __raw_readw(imx2_wdt.base + IMX2_WDT_WCR);
> > -
> > - /* set the new timeout value in the WSR */
> > - val &= ~IMX2_WDT_WCR_WT;
> > - val |= WDOG_SEC_TO_COUNT(new_timeout);
> > - __raw_writew(val, imx2_wdt.base + IMX2_WDT_WCR);
> > + regmap_update_bits(imx2_wdt.regmap, IMX2_WDT_WCR, IMX2_WDT_WCR_WT,
> > + WDOG_SEC_TO_COUNT(new_timeout));
> > }
> >
> > static int imx2_wdt_open(struct inode *inode, struct file *file)
> > @@ -181,7 +180,7 @@ static long imx2_wdt_ioctl(struct file *file, unsigned
> int cmd,
> > void __user *argp = (void __user *)arg;
> > int __user *p = argp;
> > int new_value;
> > - u16 val;
> > + u32 val;
> >
> > switch (cmd) {
> > case WDIOC_GETSUPPORT:
> > @@ -192,7 +191,7 @@ static long imx2_wdt_ioctl(struct file *file, unsigned
> int cmd,
> > return put_user(0, p);
> >
> > case WDIOC_GETBOOTSTATUS:
> > - val = __raw_readw(imx2_wdt.base + IMX2_WDT_WRSR);
> > + regmap_read(imx2_wdt.regmap, IMX2_WDT_WRSR, &val);
> > new_value = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;
> > return put_user(new_value, p);
> >
> > @@ -255,15 +254,30 @@ static struct miscdevice imx2_wdt_miscdev = {
> > .fops = &imx2_wdt_fops,
> > };
> >
> > +static struct regmap_config imx2_wdt_regmap_config = {
> > + .reg_bits = 16,
> > + .reg_stride = 2,
> > + .val_bits = 16,
> > + .max_register = 0x8,
> > +};
> > +
> > static int __init imx2_wdt_probe(struct platform_device *pdev)
> > {
> > - int ret;
> > struct resource *res;
> > + void __iomem *base;
> > + int ret;
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > - imx2_wdt.base = devm_ioremap_resource(&pdev->dev, res);
> > - if (IS_ERR(imx2_wdt.base))
> > - return PTR_ERR(imx2_wdt.base);
> > + base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(base))
> > + return PTR_ERR(base);
> > +
> > + imx2_wdt.regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
> > + &imx2_wdt_regmap_config);
> > + if (IS_ERR(imx2_wdt.regmap)) {
> > + dev_err(&pdev->dev, "regmap init failed\n");
> > + return PTR_ERR(imx2_wdt.regmap);
> > + }
> >
> > imx2_wdt.clk = devm_clk_get(&pdev->dev, NULL);
> > if (IS_ERR(imx2_wdt.clk)) {
> > --
> > 1.8.4
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo
RE: [PATCHv2 3/3] regmap: add DT endianness binding support.
> Subject: Re: [PATCHv2 3/3] regmap: add DT endianness binding support. > > On Wed, Apr 02, 2014 at 06:09:09PM +0800, Xiubo Li wrote: > > > + ret = of_regmap_get_endian(dev, bus, config, "reg_endian", ®_endian); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + ret = of_regmap_get_endian(dev, bus, config, "val_endian", &val_endian); > > + if (ret) > > + return ERR_PTR(ret); > > Actually, if we're going to be doing this for all devices then we > probably need to namespace the properties too. Not sure what to do for > a prefix though. One for the DT folks. How about using one prefix string of each regmap bus? Such as: Prefix 'regmap-mmio' for regmap-mmio, Prefix 'regmap-i2c' for regmap-i2c, Precix 'regmap-spi' for regmap-spi ... Or just using the same prefix 'regmap' for all of them ? Thanks, BRs Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv2 2/3] regmap: Add the DT binding documentation for endianness
> Subject: Re: [PATCHv2 2/3] regmap: Add the DT binding documentation for
> endianness
>
> On Wed, Apr 02, 2014 at 06:09:08PM +0800, Xiubo Li wrote:
>
> > +sai2: sai@40031000 {
> > + compatible = "fsl,vf610-sai";
> > + reg = <0x40031000 0x1000>;
> > + ...
> > + val-endian = 'LE';
> > +};
>
> This is mostly OK as a binding (though it should be CCed to the DT list
> and maintiners as all DT bindings should) except using upper case
> doesn't really seem idiomatic for DT - lower case is more normal -
I will your advices.
> and I
> don't think we can make these properties mandatory in themselves.
> Individual bindings would need to make them mandatory. It's also odd to
> have a mandatory property which may be absent!
Well, yes, It is.
The absent one is just to compatible with the old drivers.
>
> It'd probably be better if the binding defined what the default
> endianess was too, or just didn't say what happens in cases where
> nothing is specified, the latter seems better.
I will think it over carefully.
> Generally just not
> mentioning regmap is better for a binding definition, the binding should
> be usable by all OSs and not just Linux.
How about move the endianness OF parsing to the driver/of/ ?
Is this will be better ?
Thanks,
BRs
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv4 2/2] watchdog: imx2_wdt: convert to use regmap API.
> > + base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(base)) > > + return PTR_ERR(base); > > + > > + imx2_wdt.regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base, > > + &imx2_wdt_regmap_config); > > This patch causes the following buiild failure: > > Building arm:imx_v4_v5_defconfig ... failed > -- > Error log: > drivers/built-in.o: In function `imx2_wdt_probe': > clk-composite.c:(.init.text+0x82c4): undefined reference to > `devm_regmap_init_mmio_clk' > make: *** [vmlinux] Error 1 > -- > > This is on top of > > b33ce44 Merge branch 'for-3.15/drivers' of git://git.kernel.dk/linux-block > > Does it have build dependencies on code which is (or was) not yet available > in mainline ? > This is caused by not compiling the regmap core. Could you add the following patch to have a try ? == diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 79d2589..3e55fa9 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -369,6 +369,7 @@ config MAX63XX_WATCHDOG config IMX2_WDT tristate "IMX2+ Watchdog" depends on ARCH_MXC + select REGMAP_MMIO help This is the driver for the hardware watchdog on the Freescale IMX2 and later processors. -- If it's okey, I'll sent the v5 series to fix this. Thanks very much, BRs Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv4 2/2] watchdog: imx2_wdt: convert to use regmap API.
> > This is caused by not compiling the regmap core. > > > > Could you add the following patch to have a try ? > > == > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > > index 79d2589..3e55fa9 100644 > > --- a/drivers/watchdog/Kconfig > > +++ b/drivers/watchdog/Kconfig > > @@ -369,6 +369,7 @@ config MAX63XX_WATCHDOG > > config IMX2_WDT > > tristate "IMX2+ Watchdog" > > depends on ARCH_MXC > > + select REGMAP_MMIO > > help > >This is the driver for the hardware watchdog > >on the Freescale IMX2 and later processors. > > -- > > > > If it's okey, I'll sent the v5 series to fix this. > > > Yes, that fixes the problem. > I'll send the v5 series to fix this. Thanks very much. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] ASoC: fsl_sai: Fix Bit Clock Polarity configurations
> Subject: [PATCH] ASoC: fsl_sai: Fix Bit Clock Polarity configurations > > The BCP bit in TCR4/RCR4 register rules as followings: > 0 Bit clock is active high with drive outputs on rising edge > and sample inputs on falling edge. > 1 Bit clock is active low with drive outputs on falling edge > and sample inputs on rising edge. > > For all formats currently supported in the fsl_sai driver, they're exactly > sending data on the falling edge and sampling on the rising edge. > > However, the driver clears this BCP bit for all of them which results click > noise when working with SGTL5000 and big noise with WM8962. > > Thus this patch corrects the BCP settings for all the formats here to fix > the nosie issue. > > Signed-off-by: Nicolin Chen > --- Good catch. Acked-by: Xiubo Li Thanks, -- BRs, Xiubo > sound/soc/fsl/fsl_sai.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c > index 99051c7..9bbebea 100644 > --- a/sound/soc/fsl/fsl_sai.c > +++ b/sound/soc/fsl/fsl_sai.c > @@ -180,7 +180,7 @@ static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai > *cpu_dai, >* that is, together with the last bit of the previous >* data word. >*/ > - val_cr2 &= ~FSL_SAI_CR2_BCP; > + val_cr2 |= FSL_SAI_CR2_BCP; > val_cr4 |= FSL_SAI_CR4_FSE | FSL_SAI_CR4_FSP; > break; > case SND_SOC_DAIFMT_LEFT_J: > @@ -188,7 +188,7 @@ static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai > *cpu_dai, >* Frame high, one word length for frame sync, >* frame sync asserts with the first bit of the frame. >*/ > - val_cr2 &= ~FSL_SAI_CR2_BCP; > + val_cr2 |= FSL_SAI_CR2_BCP; > val_cr4 &= ~(FSL_SAI_CR4_FSE | FSL_SAI_CR4_FSP); > break; > case SND_SOC_DAIFMT_DSP_A: > @@ -198,7 +198,7 @@ static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai > *cpu_dai, >* that is, together with the last bit of the previous >* data word. >*/ > - val_cr2 &= ~FSL_SAI_CR2_BCP; > + val_cr2 |= FSL_SAI_CR2_BCP; > val_cr4 &= ~FSL_SAI_CR4_FSP; > val_cr4 |= FSL_SAI_CR4_FSE; > sai->is_dsp_mode = true; > @@ -208,7 +208,7 @@ static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai > *cpu_dai, >* Frame high, one bit for frame sync, >* frame sync asserts with the first bit of the frame. >*/ > - val_cr2 &= ~FSL_SAI_CR2_BCP; > + val_cr2 |= FSL_SAI_CR2_BCP; > val_cr4 &= ~(FSL_SAI_CR4_FSE | FSL_SAI_CR4_FSP); > sai->is_dsp_mode = true; > break; > -- > 1.8.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] ASoC: fsl_sai: Fix Bit Clock Polarity configurations
> > Is that possible for you to test those two clock patches for fsl_sai? > > I think most of us are waiting for your reply to it. And I'd really > like to move on to append clock dividing code into the driver so both > of vybrid and imx can easily enable the DAI master mode. > Certainly, I will test them later. Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/2] ASoC: fsl_sai: Add clock control for SAI
Hi,
I have test this series on my Vybrid-TWR board and it works happily.
[...]
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index 3847d2a..2d749df 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -428,5 +428,18 @@ static int fsl_sai_startup(struct snd_pcm_substream
> *substream,
> {
> struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> - u32 reg;
> + struct device *dev = &sai->pdev->dev;
> + u32 reg, ret;
> +
I'd prefer:
+ int ret;
> + ret = clk_prepare_enable(sai->ipg_clk);
> + if (ret) {
> + dev_err(dev, "failed to prepare and enable ipg clock\n");
> + return ret;
> + }
> +
[...]
> @@ -609,5 +630,5 @@ static int fsl_sai_probe(struct platform_device *pdev)
>
> sai->regmap = devm_regmap_init_mmio_clk(&pdev->dev,
> - "sai", base, &fsl_sai_regmap_config);
> + "ipg", base, &fsl_sai_regmap_config);
> if (IS_ERR(sai->regmap)) {
> dev_err(&pdev->dev, "regmap init failed\n");
> @@ -615,4 +636,16 @@ static int fsl_sai_probe(struct platform_device *pdev)
> }
>
> + sai->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> + if (IS_ERR(sai->ipg_clk)) {
> + dev_err(&pdev->dev, "failed to get ipg clock\n");
> + return PTR_ERR(sai->ipg_clk);
> + }
> +
Since the 'ipg' clock is just intend to be used for registers accessing and
We are using the regmap_init_mmio_clk(), so we can just drop it here and
Let the regmap APIs to do the clock options properly.
Otherwise it look good to me.
After this:
Acked-by: Xiubo Li
Thanks,
Brs,
Xiubo
> + sai->sai_clk = devm_clk_get(&pdev->dev, "sai");
> + if (IS_ERR(sai->sai_clk)) {
> + dev_err(&pdev->dev, "failed to get sai clock\n");
> + return PTR_ERR(sai->sai_clk);
> + }
> +
> irq = platform_get_irq(pdev, 0);
> if (irq < 0) {
> diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
> index 677670d..cbaf114 100644
> --- a/sound/soc/fsl/fsl_sai.h
> +++ b/sound/soc/fsl/fsl_sai.h
> @@ -127,4 +127,6 @@ struct fsl_sai {
> struct platform_device *pdev;
> struct regmap *regmap;
> + struct clk *ipg_clk;
> + struct clk *sai_clk;
>
> bool big_endian_regs;
> --
> 1.8.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/2] ASoC: fsl_sai: Add clock control for SAI
> Subject: Re: [PATCH 1/2] ASoC: fsl_sai: Add clock control for SAI
>
> Hi Xiubo,
>
> On Fri, Apr 04, 2014 at 05:24:39PM +0800, Xiubo Li-B47053 wrote:
> > Hi,
> >
> > I have test this series on my Vybrid-TWR board and it works happily.
>
> You just checked the wrong version. I've sent a mail to let people disregard
> this version and a newer v2.
>
Sorry, I didn't receive these, I will search it in the web page.
> >
> > [...]
> > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > > index 3847d2a..2d749df 100644
> > > --- a/sound/soc/fsl/fsl_sai.c
> > > +++ b/sound/soc/fsl/fsl_sai.c
> > > @@ -428,5 +428,18 @@ static int fsl_sai_startup(struct snd_pcm_substream
> > > *substream,
> > > {
> > > struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> > > - u32 reg;
> > > + struct device *dev = &sai->pdev->dev;
> > > + u32 reg, ret;
> > > +
> >
> > I'd prefer:
> > + int ret;
>
> Just like last time I said, it would be converted to 'int' any way. There's
> no much difference between them.
>
Yes, it is...
I have ever encounter something like this in the feature maybe someone will
do the following check:
If (ret < 0)
...
Just one potential problem and one suggestion.
Thanks,
> >
> > > + ret = clk_prepare_enable(sai->ipg_clk);
> > > + if (ret) {
> > > + dev_err(dev, "failed to prepare and enable ipg clock\n");
> > > + return ret;
> > > + }
> > > +
> >
> > [...]
> >
> > > @@ -609,5 +630,5 @@ static int fsl_sai_probe(struct platform_device *pdev)
> > >
> > > sai->regmap = devm_regmap_init_mmio_clk(&pdev->dev,
> > > - "sai", base, &fsl_sai_regmap_config);
> > > + "ipg", base, &fsl_sai_regmap_config);
> > > if (IS_ERR(sai->regmap)) {
> > > dev_err(&pdev->dev, "regmap init failed\n");
> > > @@ -615,4 +636,16 @@ static int fsl_sai_probe(struct platform_device
> > > *pdev)
> > > }
> > >
> > > + sai->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> > > + if (IS_ERR(sai->ipg_clk)) {
> > > + dev_err(&pdev->dev, "failed to get ipg clock\n");
> > > + return PTR_ERR(sai->ipg_clk);
> > > + }
> > > +
> >
> > Since the 'ipg' clock is just intend to be used for registers accessing and
> > We are using the regmap_init_mmio_clk(), so we can just drop it here and
> > Let the regmap APIs to do the clock options properly.
>
> This 'ipg' clock, which I renamed in v2 to 'bus clock', is not just used
> in the way you said but also able to drive bit clock as your own code in
> the fsl_sai_set_dai_sysclk_tr(), and as reference manual describes:
>
> 52.3.1.3 Bus clock
> The bus clock is used by the control and configuration registers and to
> generate synchronous interrupts and DMA requests.
>
> Thanks,
> Nicolin
>
> >
> > Otherwise it look good to me.
> >
> > After this:
> > Acked-by: Xiubo Li
> >
> >
> >
> > Thanks,
> >
> > Brs,
> > Xiubo
> >
> >
> >
> >
> > > + sai->sai_clk = devm_clk_get(&pdev->dev, "sai");
> > > + if (IS_ERR(sai->sai_clk)) {
> > > + dev_err(&pdev->dev, "failed to get sai clock\n");
> > > + return PTR_ERR(sai->sai_clk);
> > > + }
> > > +
> > > irq = platform_get_irq(pdev, 0);
> > > if (irq < 0) {
> > > diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
> > > index 677670d..cbaf114 100644
> > > --- a/sound/soc/fsl/fsl_sai.h
> > > +++ b/sound/soc/fsl/fsl_sai.h
> > > @@ -127,4 +127,6 @@ struct fsl_sai {
> > > struct platform_device *pdev;
> > > struct regmap *regmap;
> > > + struct clk *ipg_clk;
> > > + struct clk *sai_clk;
> > >
> > > bool big_endian_regs;
> > > --
> > > 1.8.4
> > >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 2/2] ARM: dts: Append clock bindings for sai2 on VF610 platform
> Subject: Re: [PATCH v2 2/2] ARM: dts: Append clock bindings for sai2 on VF610 > platform > > Hi Shawn, > >Thanks for the comments, but... > > On Wed, Apr 02, 2014 at 09:03:04PM +0800, Shawn Guo wrote: > > On Wed, Apr 02, 2014 at 06:10:20PM +0800, Nicolin Chen wrote: > > > Since we added fours clock to the DT binding, we should update the current > > > SAI dts/dtsi so as not to break their functions. > > > > > > Signed-off-by: Nicolin Chen > > > --- > > > arch/arm/boot/dts/vf610.dtsi | 6 -- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi > > > index d31ce1b..9fd0007 100644 > > > --- a/arch/arm/boot/dts/vf610.dtsi > > > +++ b/arch/arm/boot/dts/vf610.dtsi > > > @@ -139,8 +139,10 @@ > > > compatible = "fsl,vf610-sai"; > > > reg = <0x40031000 0x1000>; > > > interrupts = <0 86 0x04>; > > > - clocks = <&clks VF610_CLK_SAI2>; > > > - clock-names = "sai"; > > > + clocks = <&clks VF610_CLK_SAI2>, > > > +<&clks VF610_CLK_SAI2>, > > > +<&clks 0>, <&clks 0>; > > > > So it seems that SAI on vf610 does work with only one clock. So the > > driver change will break old DTB for vf610? If that's case, we will > > have to need a new compatible for cases where 4 clocks are needed. > > According to Vybrid's RM Chapter 9.11.12 SAI clocking, the SoC actually > connects SAI with two clocks: SAI_CLK and Platform Bus Clock. So the DT > binding here still needs to be corrected even if ignoring driver change. > > Besides, I've checked both SAI on imx and vf610 and found that they are > seemly identical, especially for the clock part -- "The transmitter and > receiver can independently select between the bus clock and up to three > audio master clocks to generate the bit clock." And the driver that was > designed for vf610 already contains the code to switch the clock between > Bus Clock and Three MCLKs. What I want to say is, even if SAI on vf610 > does work with only one clock, it still doesn't have the full function > on vf610 -- driving clock from Platform Bus Clock unless we make this > improvement to the DT binding. > > So I think it's fair to complete the code here for both platforms, even > though we might take the risk of merging conflict. And I understand > your point to avoid function break on those platform both of us aren't > convenient to test. But I've already involved Xiubo in the list. And > we can wait for his test result. > This has been test on my Vybird-TWR board and it's fine. Thanks, Brs Xiubo > Hope you can understand the circumstance, > Nicolin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] pwm: core: Use devm_kzalloc instead of kzalloc
> > drivers/pwm/core.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 2ca9504..74c9f9a 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -80,7 +80,6 @@ static void free_pwms(struct pwm_chip *chip)
> >
> > bitmap_clear(allocated_pwms, chip->base, chip->npwm);
> >
> > - kfree(chip->pwms);
> > chip->pwms = NULL;
> > }
> >
> > @@ -245,7 +244,9 @@ int pwmchip_add(struct pwm_chip *chip)
> > if (ret < 0)
> > goto out;
> >
> > - chip->pwms = kzalloc(chip->npwm * sizeof(*pwm), GFP_KERNEL);
> > + chip->pwms = devm_kzalloc(chip->dev,
> > + chip->npwm * sizeof(*pwm),
> > + GFP_KERNEL);
> > if (!chip->pwms) {
> > ret = -ENOMEM;
> > goto out;
>
> Is it guaranteed that pwmchip_add()/free_pwms() will only be called in
> probe() and remove() paths? It is probably safe assumption, but maybe it
> should be mentioned in comments now that we definitely have this
> restricion.
>
Yes, for now they are.
Thanks.
--
Best Regards,
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv9 2/4] ARM: dts: Add Freescale FTM PWM node for VF610.
> Has the FTM PWM device tree bindings been accepted/merged? Please only > send me DTS changes after that. Also when you send me the DTS changes, > please base on imx/dt or for-next branch of my git tree below. > > git://git.linaro.org/people/shawnguo/linux-2.6.git > Yes, I will resend these dt patches after the FTM driver has been merged bases on this tree. Thanks, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4] ASoC: simple-card: add simple audio card widgets support
> > For many audio cards there need to add some off-CODEC widgets, and this
> > is hard to be supported by dts only, because maybe some widgets need one
> > or more call backs, such as wevent.
>
> The big question here would be why these widgets need to be added
>
For instance, from the SGTL5000 codec Spec:
The analog input block contains a stereo line input and a microphone input
with mic bias. Either input can be routed to the ADC. The line input can also
be configured to bypass the CODEC and be routed directly to the headphone
output.
And in the SGTL5000 codec driver there is one "Mic Bias" widget, and this
should be routed to one off-CODEC widget, like "Mic Jack"(the microphone input).
> the
> default thing is that all inputs and outputs are enabled so it can't be
> that and as you say callbacks are going to need to be added.
>
Some drivers may need like this:
SND_SOC_DAPM_PGA_E("Output Amp", SND_SOC_NOPM, 0, 0, NULL, 0,
XXX_output_amp_event, SND_SOC_DAPM_PRE_PMU |
SND_SOC_DAPM_POST_PMD),
?
> If this is used then it probably makes sense for the card to set the
> fully routed flag since one possible use is to disable unconnected pins
> on the CODEC.
>
> > +- simple-sudio-card,widgets: The name of the audio card
> > off-CODEC
> widgets.
>
> simple-audio-card. This also needs more documentation about what is
> supposed to be filled in here.
>
Yes, if needed.
> > +void
> > +asoc_simple_card_widgets_unregister(struct asoc_simple_card_widgets *comp)
>
> We always use snd_soc.
>
> One interesting thing here is that this is done separately to the card
> itself. Is there any actual dependency on the simple card, could this
> be made into a generic library that any machine driver could use? I can
> see other machines being able to make use of this.
No, has no dependency on the simple card.
Thanks,
--
Best Regards,
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 3/4] ASoC: fsl: Add VF610 simple audio card widgets driver.
> > This is the SGTL5000 codec based off-CODEC widgets supports. > > > > Signed-off-by: Xiubo Li > > --- > > sound/soc/fsl/Kconfig | 25 ++ > > sound/soc/fsl/Makefile| 3 +++ > > sound/soc/fsl/snd-soc-simple-card-vf610.c | 44 > +++ > > If we're needing to add explict code for a simple audio card that seems > to defeat the point of using the simple audio card - we may as well just > have an explicit machine driver. > IMHO, the simple audio card driver could fulfil most of machine driver's work. And will make some machine driver simpler and eaiser to code. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv8 RFC] pwm: Add Freescale FTM PWM driver support
> > +static unsigned long fsl_pwm_calculate_period_cycles(struct fsl_pwm_chip
> *fpc,
> > +unsigned long period_ns,
> > +enum fsl_pwm_clk index)
> > +{
> > + bool bg = fpc->big_endian;
> > + int ret;
> > +
> > + fpc->counter_clk_select = FTM_SC_CLK(bg, index);
>
> Yes, this is the spirit of what I was suggesting. The code is much less
> efficient/bigger on the Vybrid with this run-time detection; but this is
> more efficient/smaller than previous versions. I think that 'bg' can be
> a compiler '#define' base on the configured SOC-systems. Ie, if the
> kernel config only has 'Vybrid' or only 'LayerScape', then 'bg' can be a
> hard coded value. The compiler will produce much better code in these
> cases.
>
> Also, maybe 'distro' people may want to make a 'hand-held' (Debian) or a
> 'router' (OpenWRT) distribution and they would only pick either 'Vybrid'
> or 'LayerScape'. However, if someone wants an 'every ARM under the
> sun', then the code still works. So, I think that the code is better
> setup for a subsequent patch set like this (or at least just a good).
>
> Especially, the stuff on the I/O swapping in the 'readl()' and
> 'writel()' is no longer needed; I think you can use the same function
> for both SOCs.
>
> > +#define __FTM_SWAP32(v) ((u32)(\
> > + (((u32)(v) & (u32)0x00ffUL) << 24) |\
> > + (((u32)(v) & (u32)0xff00UL) << 8) |\
> > + (((u32)(v) & (u32)0x00ffUL) >> 8) |\
> > + (((u32)(v) & (u32)0xff00UL) >> 24)))
> > +#define FTM_SWAP32(b, v) (b ? __FTM_SWAP32(v) : v)
>
> I think that there are macros that you could use here. For instance,
> '#include ' (powerpc and arm) has some assembler macros
> that are quite fast for swapping. If the kernel config has ARCH >= 6
> for ARM, then the very fast 'rev' instruction is used. If not, then a
> generic version is used as you have coded. The PowerPC (another
> possible future ARCH for QorIQ/Layerscape SOC?) always has inline
> assembler macros.
>
> So,
>
> + #include
> ...
> + #define FTM_SWAP32(b, v)(b ? __swab32(v) : v)
>
> might be better.
>
Yes.
I have removed the big-endian support temporarily, and will send followed
patches about this.
Thanks,
--
Best Regards,
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [PATCH] ASoC: simple-card: fix one bug to writing to the platform data
Hi Jean-Francois, > > If the original cinfo is not used anymore, the use of its structure to > handle the card information is not a good idea: > > - almost all cinfo information are in the struct snd_soc_card, > > - this cinfo structure cannot be extended to handle many DAI links, > > - it contains simple-card information which are of no use for the > platform caller. > > So, I'd rather have seen: > > - the removal of 'snd_link' and 'snd_card' from the platform interface > (struct asoc_simple_card_info), > > - the definition of a local struct simple_card_data containing the > struct snd_soc_card and a pointer to an array of fmt/sysclk values > (one per DAI link). > I have sent one patch to fix some of these and mainly fixed the bug from Mark's comments in another early email. Thanks, -- Best Regards, Xiubo
RE: [PATCH] ASoC: fsl-sai: Add device tree nodes and its availiable check
> > + if (!np || !of_device_is_available(np)) > > + return -ENODEV; > > + > > I would expect the of_device_is_available() check to be done by the > driver core rather than by individual drivers - every single driver > should have that check. Is this not happening? The check for np is OK > though. Yes, I hasn't found the core driver does that for now... And IMO this is needed since the SAI driver is only base dts. And maybe the device is not presently operational, but it might become operational in the future (for example, something is not plugged in, or switched off). Or A serious error was detected in the device, and it is unlikely to become operational without repair... And for now for the individual drivers, before calling the of_device_is_available the '!np' check is need, because: While in __of_device_is_available: > status = of_get_property(device, "status", &statlen); > if (status == NULL) > return 1; The status value returned from 'of_get_property()' will be NULL in two cases: Firstly: the 'device' value (device node) is NULL. Secondly: the 'status' property is actaully not exist. If the device node is NULL, the __of_device_is_available will return true too, that will mean the absent state of the 'status' property. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [PATCH] ASoC: simple-card: fix one bug to writing to the platform data
Hi Mark, Jean-Francios
> If the original cinfo is not used anymore, the use of its structure to
> handle the card information is not a good idea:
>
> - almost all cinfo information are in the struct snd_soc_card,
>
> - this cinfo structure cannot be extended to handle many DAI links,
>
> - it contains simple-card information which are of no use for the
> platform caller.
>
> So, I'd rather have seen:
>
> - the removal of 'snd_link' and 'snd_card' from the platform interface
> (struct asoc_simple_card_info),
>
> - the definition of a local struct simple_card_data containing the
> struct snd_soc_card and a pointer to an array of fmt/sysclk values
> (one per DAI link).
>
@Jean-Francios, apart from this separate issue we're discussing, do you have
any comment on this patch itself?
@Mark, Since what Jean-Francios is concerned by is another issue apart from
this patch itself and being discussed, can you apply the patch?
Thanks,
--
Best Regards,
Xiubo
N�r��yb�X��ǧv�^�)Þº{.n�+{zX����ܨ}ï¿½ï¿½ï¿½Æ z�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jÇ«y�m��@A�a���
0��h���i
RE: [alsa-devel] [RFC][PATCH] ASoC: simple-card: Add asoc_simple_card_data for the simple card driver data
> > Whether the dt is used or not, almost all the simple card information > > for the DAI link and sound card are initialized in the simple card driver. > > > > And for the platform caller, the snd_link and snd_card are of no use, so > > move them from struct asoc_simple_card_info to struct asoc_simple_card_data. > > > > And now only one DAI link is supported for simple card. > > > > Suggested-by: Jean-Francois Moine > > Signed-off-by: Xiubo Li > > --- > > This patch seems have many this kind of lines > - info->xxx > + sdata->info->xxx > > But, how about add this line to each function ? > > struct asoc_simple_card_info *info = &sdata->info; > > Patch will be more readable > Yes, that's looks perfect. See the next version please. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv2 5/6] ASoC: fsl: Add VF610 simple audio card widgets driver.
Hi Mark, I have sent another patch, if that patch is ok, please ignore this patch series. The new patch :" ASoC: add snd_soc_of_parse_audio_simple_widgets for DeviceTree" Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [RFC][PATCH] ASoC: simple-card: Add asoc_simple_card_data for the simple card driver data
Hi Jean-Francios, I do think split the muti-issues into deferent patches will be easier to upstream code, and the risk will be lower. So this patch is just the first step. Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support
> > + struct fsl_pwm_chip *fpc;
> > +
> > + fpc = to_fsl_chip(chip);
> > +
> > + period_cycles = fsl_rate_to_cycles(fpc, period_ns);
> > + if (period_cycles > 0x) {
> > + dev_err(chip->dev, "required PWM period cycles(%lu) overflow "
> > + "16-bits counter!\n", period_cycles);
> > + return -EINVAL;
> > + }
> > +
> > + duty_cycles = fsl_rate_to_cycles(fpc, duty_ns);
> > + if (duty_cycles >= 0x) {
> > + dev_err(chip->dev, "required PWM duty cycles(%lu) overflow "
> > + "16-bits counter!\n", duty_cycles);
> > + return -EINVAL;
> > + }
>
> I'm not sure the error messages are all that useful. A -EINVAL error code
> should make it pretty clear what the problem is.
>
Yes, it is.
But for the pwm leds, there hasn't any check about the return values, if
there is something wrong, we cannot get any information about this.
So I think this error messages are useful for this case.
--
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] ASoC: SGTL5000: Fix kernel failed while trying to get optional VDDD supply.
> > 2, If the regulator dt node is exist but the optional VDDD is absent (i.e. > > The external VDDD is not used), a -EPROBE_DEFER will be returned, if > > just return the -EPROBE_DEFER to the probe(and then the probe deferral > > mechanism will do the probe again later, is that right ?), and then > > the > > regulator_get_optional() will be called later again, and the > > -EPROBE_DEFER will be returned again too, and now how should I handle > > -EPROBE_DEFER error twice ? Or should there be a counter about this ? > > That to say when the -EPROBE_DEFER error is the second time returned > > from regulator_get_optional() can we ensure that the optional VDDD is really > not in use. > > The driver should just defer when it's told to defer, I don't understand why > it > would want to count anything? > It's just one idea for the special handling of regulator_get_optional() in this case. -- Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/2] ASoC: fsl_sai: Two bug fixes for fsl_sai driver
> -Original Message- > From: Nicolin Chen [mailto:nicoleots...@gmail.com] > Sent: Thursday, July 17, 2014 9:22 PM > To: broo...@kernel.org > Cc: linux-kernel@vger.kernel.org; linuxppc-...@lists.ozlabs.org; alsa- > de...@alsa-project.org; ti...@tabi.org; Xiubo Li-B47053; Wang Shengjiu-B02247; > Chen Guangyu-B42378 > Subject: [PATCH 0/2] ASoC: fsl_sai: Two bug fixes for fsl_sai driver > > Nicolin Chen (2): > ASoC: fsl_sai: Reset FIFOs after disabling TE/RE > ASoC: fsl_sai: Fix incorrect register writing in fsl_sai_isr() > Yes, they are all looks good to me. Signed-off-by: Xiubo Li Thanks, BRs Xiubo > sound/soc/fsl/fsl_sai.c | 9 ++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > -- > 1.8.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] clocksource: fix clocksource_mmio_readX_down
> > diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
> > index c0e2512..f17a0d1 100644
> > --- a/drivers/clocksource/mmio.c
> > +++ b/drivers/clocksource/mmio.c
> > @@ -27,7 +27,7 @@ cycle_t clocksource_mmio_readl_up(struct clocksource *c)
> >
> > cycle_t clocksource_mmio_readl_down(struct clocksource *c)
> > {
> > - return ~readl_relaxed(to_mmio_clksrc(c)->reg);
> > + return ~readl_relaxed(to_mmio_clksrc(c)->reg) & c->mask;
> > }
> >
> > cycle_t clocksource_mmio_readw_up(struct clocksource *c)
> > @@ -37,7 +37,7 @@ cycle_t clocksource_mmio_readw_up(struct clocksource *c)
> >
> > cycle_t clocksource_mmio_readw_down(struct clocksource *c)
> > {
> > - return ~(unsigned)readw_relaxed(to_mmio_clksrc(c)->reg);
> > + return ~(unsigned)readw_relaxed(to_mmio_clksrc(c)->reg) & c->mask;
> > }
> >
> > /**
>
>
> Hi,
>
> I realize there is some type confusion here:
>
> cycle_t -> u64
> readl_relaxed -> u32
> readw_relaxed -> u16
>
> and clocksource_mmio_readw_down returns a cast to unsigned (u32)
>
> This patch makes sense but it obfuscate more the types in these
> functions. Worth to clarify the functions first ?
>
Yes, though the short type could be converted to the longer type automatically,
It's better and worth to clarify it firstly.
I'll fix this, please see the next version.
Thanks,
BRs
Xiubo
>
> --
> Facebook |
> Twitter |
>
RE: [PATCHv2 2/3] regmap: Add the DT binding documentation for endianness
> -Original Message- > From: Mark Brown [mailto:broo...@kernel.org] > Sent: Tuesday, April 15, 2014 5:10 AM > To: Xiubo Li-B47053 > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCHv2 2/3] regmap: Add the DT binding documentation for > endianness > > On Thu, Apr 03, 2014 at 07:04:00AM +0000, li.xi...@freescale.com wrote: > > > > Generally just not > > > mentioning regmap is better for a binding definition, the binding should > > > be usable by all OSs and not just Linux. > > > How about move the endianness OF parsing to the driver/of/ ? > > Is this will be better ? > > Where the code is is sensible enough, it's an issue about how the > binding documentation was written rather than about the code. Sorry for late. Well, I will try to enhance this. Thanks, BRs Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
Hi Pankaj,
One more question:
For example:
regmap_read()
> _regmap_read()
> 2112 #ifdef LOG_DEVICE
> 2113 if (strcmp(dev_name(map->dev), LOG_DEVICE)
== 0)
> 2114 dev_info(map->dev, "%x => %x\n",
reg, *val);
> 2115 #endif
In dev_name(map->dev) will also encounter the same crash core trace like in
regmap_get_val_endian(dev, ...).
Could there be another method, such as using 'dummy dev' instead of 'NULL dev' ?
Just one suggestion.
BRs
Xiubo
> -Original Message-
> From: Pankaj Dubey [mailto:pankaj.du...@samsung.com]
> Sent: Thursday, September 18, 2014 2:03 PM
> To: Dong Aisheng-B29396
> Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-...@vger.kernel.org;
> linux-kernel@vger.kernel.org; kgene@samsung.com; li...@arm.linux.org.uk;
> a...@arndb.de; naus...@samsung.com; tomasz.f...@gmail.com; jo...@samsung.com;
> thomas...@samsung.com; vikas.saj...@samsung.com; chow@samsung.com;
> lee.jo...@linaro.org; 'Boris BREZILLON'; Xiubo Li-B47053; 'Geert
> Uytterhoeven';
> 'Stephen Warren'
> Subject: RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform
> devices
>
> Hi,
>
> Adding CC to Xiubo Li, Geert Uytterhoeven and Stephen Warren.
>
> On Thursday, September 18, 2014, Dong Aisheng wrote,
> > On Wed, Sep 17, 2014 at 04:50:50PM +0530, Pankaj Dubey wrote:
> > > Hi,
> > >
> > > On Wednesday, September 17, 2014, Dong Aisheng Wrote,
> > > > >
> > > > > + regmap = regmap_init_mmio(NULL, base,
> &syscon_regmap_config);
> > > >
> > > > Does a NULL device pointer work?
> > >
> > > Yes, it is safe, at least we are able to test on Exynos based SoC.
> > > I have tested it with kgene/for-next kernel on Exynos3250.
> > > Also it has been tested on Exynos5250 based Snow board with 3.17-rc5
> > > based kernel by Vivek Gautam.
> > >
> > > Patch V2 also has been tested by "Borris Brezillon" on AT91 platform.
> > >
> > >
> >
> > The kernel i tested was next-20140915 of linux-next.
> >
> > please see regmap_get_val_endian called in regmap_init function.
> > static enum regmap_endian regmap_get_val_endian(struct device *dev,
> > const struct regmap_bus *bus,
> > const struct regmap_config
> *config) {
> > struct device_node *np = dev->of_node;
> > enum regmap_endian endian;
> > ...
> > }
> > It will crash at the first line of dev->of_node if dev is NULL.
> >
> > Can you check if you're using the same code as mine?
>
> No, it's not same.
> My bad that I was not using linux-next for testing this patch.
> We tested on kgene/for-next where these changes still have not come.
> Just now I checked linux-next and found that it will crash at first line of
> "regmap_get_val_endian" as there is no check for NULL on dev.
>
> I checked git history of regmap.c file and found recently this file has been
> modified
> for adding DT endianness binding support. Following are set of patches gone
> for this
>
> cf673fb regmap: Split regmap_get_endian() in two functions
> 5844a8b regmap: Fix handling of volatile registers for format_write() chips
> 45e1a27 regmap: of_regmap_get_endian() cleanup
> ba1b53f regmap: Fix DT endianess parsing logic
> d647c19 regmap: add DT endianness binding support.
>
> I think there should have been a check for NULL on "dev" in
> "regmap_get_val_endian", so that if dev pointer exist then only it makes
> sense to get
> endianness property from DT.
>
> I will suggest following fix in regmap.c for this. With following fix I
> tested it and it works well
> on linux-next also. So if you can confirm following fix is working for you
> then I can post this
> patch.
>
>
> Subject: [PATCH] regmap: fix NULL pointer dereference in
> regmap_get_val_endian
>
> Recent commits for getting reg endianess causing NULL pointer
> dereference if dev is passed NULL in regmap_init_mmio. This patch
> fixes this issue, and allows to parse reg endianess only if dev
> and dev->of_node exist.
>
> Signed-off-by: Pankaj Dubey
> ---
> drivers/base/regmap/regmap.c | 23 ++-
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index f2281af..455a877 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -477,7 +477,7 @@ static enum regmap_endian regmap_get_val_endian(struct
> device *dev,
> const struct regmap_bus *bus,
> const struct regmap_config *config)
> {
> - struct device_node *np = dev->of_node;
> + struct device_node *np;
> enum regmap_endian endian;
>
> /* Retrieve the endianness specification from the regmap config */
> @@ -487,15 +487,20 @@ static enum regmap_endian regmap_get_val_endian(struct
> device *dev,
> if (endian !=
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
Hi,
[...]
> > please see regmap_get_val_endian called in regmap_init function.
> > static enum regmap_endian regmap_get_val_endian(struct device *dev,
> > const struct regmap_bus *bus,
> > const struct regmap_config
> *config) {
> > struct device_node *np = dev->of_node;
> > enum regmap_endian endian;
> > ...
> > }
> > It will crash at the first line of dev->of_node if dev is NULL.
> >
> > Can you check if you're using the same code as mine?
>
> No, it's not same.
> My bad that I was not using linux-next for testing this patch.
> We tested on kgene/for-next where these changes still have not come.
> Just now I checked linux-next and found that it will crash at first line of
> "regmap_get_val_endian" as there is no check for NULL on dev.
>
> I checked git history of regmap.c file and found recently this file has been
> modified
> for adding DT endianness binding support. Following are set of patches gone
> for this
>
> cf673fb regmap: Split regmap_get_endian() in two functions
> 5844a8b regmap: Fix handling of volatile registers for format_write() chips
> 45e1a27 regmap: of_regmap_get_endian() cleanup
> ba1b53f regmap: Fix DT endianess parsing logic
> d647c19 regmap: add DT endianness binding support.
>
> I think there should have been a check for NULL on "dev" in
> "regmap_get_val_endian", so that if dev pointer exist then only it makes
> sense to get
> endianness property from DT.
>
> I will suggest following fix in regmap.c for this. With following fix I
> tested it and it works well
> on linux-next also. So if you can confirm following fix is working for you
> then I can post this
> patch.
>
>
> Subject: [PATCH] regmap: fix NULL pointer dereference in
> regmap_get_val_endian
>
> Recent commits for getting reg endianess causing NULL pointer
> dereference if dev is passed NULL in regmap_init_mmio. This patch
> fixes this issue, and allows to parse reg endianess only if dev
> and dev->of_node exist.
>
Another question:
Then the regmap core cannot deal with the following issue:
Like the patch adding the endianness support for syscon:
[PATCH] mfd: syscon: binding: Add syscon endianness support.
So here just register one dummy syscon device is a good choice I do
think.
:)
Thanks,
BRs
Xiubo
> Signed-off-by: Pankaj Dubey
> ---
> drivers/base/regmap/regmap.c | 23 ++-
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index f2281af..455a877 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -477,7 +477,7 @@ static enum regmap_endian regmap_get_val_endian(struct
> device *dev,
> const struct regmap_bus *bus,
> const struct regmap_config *config)
> {
> - struct device_node *np = dev->of_node;
> + struct device_node *np;
> enum regmap_endian endian;
>
> /* Retrieve the endianness specification from the regmap config */
> @@ -487,15 +487,20 @@ static enum regmap_endian regmap_get_val_endian(struct
> device *dev,
> if (endian != REGMAP_ENDIAN_DEFAULT)
> return endian;
>
> - /* Parse the device's DT node for an endianness specification */
> - if (of_property_read_bool(np, "big-endian"))
> - endian = REGMAP_ENDIAN_BIG;
> - else if (of_property_read_bool(np, "little-endian"))
> - endian = REGMAP_ENDIAN_LITTLE;
> + /* If the dev and dev->of_node exist try to get endianness from DT
> */
> + if (dev && dev->of_node) {
> + np = dev->of_node;
>
> - /* If the endianness was specified in DT, use that */
> - if (endian != REGMAP_ENDIAN_DEFAULT)
> - return endian;
> + /* Parse the device's DT node for an endianness
> specification */
> + if (of_property_read_bool(np, "big-endian"))
> + endian = REGMAP_ENDIAN_BIG;
> + else if (of_property_read_bool(np, "little-endian"))
> + endian = REGMAP_ENDIAN_LITTLE;
> +
> + /* If the endianness was specified in DT, use that */
> + if (endian != REGMAP_ENDIAN_DEFAULT)
> + return endian;
> + }
>
> /* Retrieve the endianness specification from the bus config */
> if (bus && bus->val_format_endian_default)
> --
>
> Thanks,
> Pankaj Dubey
>
> > Regards
> > Dong Aisheng
> >
> > >
> > > Thanks,
> > > Pankaj Dubey
> > >
> > > > Regards
> > > > Dong Aisheng
> > > >
> > > > >
> > > > >
> > > > > ___
> > > > > linux-arm-kernel mailing list
> > > > > linux-arm-ker...@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > >
--
To unsubscribe from this list: send the line "unsubscribe linux-kerne
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
[...]
> > I think there should have been a check for NULL on "dev" in
> > "regmap_get_val_endian", so that if dev pointer exist then only it makes
> > sense to get
> > endianness property from DT.
> >
> > I will suggest following fix in regmap.c for this. With following fix I
> > tested it and it works well
> > on linux-next also. So if you can confirm following fix is working for you
> > then I can post this
> > patch.
> >
>
> I tested the patch work.
> But as Xiubo pointed in another mail, it may still cause other issues.
> Looking at regmap.c, there're still some other places using the device
> pointer,
> e.g. dev_xxx debug information and some tracepoints also take device pointer
> as parameter(not sure if it will break if dev is NULL).
> Another thing is that if dev is NULL, we may not be able to use regmap debugfs
> feature which seems also not as our expected.
>
> Maybe we could consider create device structure for each syscon compatible
> device in syscon driver in of_syscon_register in first time which seems to
> be reasonable.
>
> Regards
> Dong Aisheng
>
> >
> > Subject: [PATCH] regmap: fix NULL pointer dereference in
> > regmap_get_val_endian
> >
> > Recent commits for getting reg endianess causing NULL pointer
> > dereference if dev is passed NULL in regmap_init_mmio. This patch
> > fixes this issue, and allows to parse reg endianess only if dev
> > and dev->of_node exist.
> >
> > Signed-off-by: Pankaj Dubey
> > ---
> > drivers/base/regmap/regmap.c | 23 ++-
> > 1 file changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> > index f2281af..455a877 100644
> > --- a/drivers/base/regmap/regmap.c
> > +++ b/drivers/base/regmap/regmap.c
> > @@ -477,7 +477,7 @@ static enum regmap_endian regmap_get_val_endian(struct
> > device *dev,
> > const struct regmap_bus *bus,
> > const struct regmap_config *config)
> > {
> > - struct device_node *np = dev->of_node;
> > + struct device_node *np;
And the 'np' must be NULL as default.
Isn't it ?
Thanks,
BRs
Xiubo
> > enum regmap_endian endian;
> >
> > /* Retrieve the endianness specification from the regmap config */
> > @@ -487,15 +487,20 @@ static enum regmap_endian regmap_get_val_endian(struct
> > device *dev,
> > if (endian != REGMAP_ENDIAN_DEFAULT)
> > return endian;
> >
> > - /* Parse the device's DT node for an endianness specification */
> > - if (of_property_read_bool(np, "big-endian"))
> > - endian = REGMAP_ENDIAN_BIG;
> > - else if (of_property_read_bool(np, "little-endian"))
> > - endian = REGMAP_ENDIAN_LITTLE;
> > + /* If the dev and dev->of_node exist try to get endianness from DT
> > */
> > + if (dev && dev->of_node) {
> > + np = dev->of_node;
> >
> > - /* If the endianness was specified in DT, use that */
> > - if (endian != REGMAP_ENDIAN_DEFAULT)
> > - return endian;
> > + /* Parse the device's DT node for an endianness
> > specification */
> > + if (of_property_read_bool(np, "big-endian"))
> > + endian = REGMAP_ENDIAN_BIG;
> > + else if (of_property_read_bool(np, "little-endian"))
> > + endian = REGMAP_ENDIAN_LITTLE;
> > +
> > + /* If the endianness was specified in DT, use that */
> > + if (endian != REGMAP_ENDIAN_DEFAULT)
> > + return endian;
> > + }
> >
> > /* Retrieve the endianness specification from the bus config */
> > if (bus && bus->val_format_endian_default)
> > --
> >
> > Thanks,
> > Pankaj Dubey
> >
> > > Regards
> > > Dong Aisheng
> > >
> > > >
> > > > Thanks,
> > > > Pankaj Dubey
> > > >
> > > > > Regards
> > > > > Dong Aisheng
> > > > >
> > > > > >
> > > > > >
> > > > > > ___
> > > > > > linux-arm-kernel mailing list
> > > > > > linux-arm-ker...@lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
> > Thanks for testing. In that case I will post this change, as I feel this > > should be > > fixed irrespective of my syscon patch. > > > > > But as Xiubo pointed in another mail, it may still cause other issues. > > > Looking at regmap.c, there're still some other places using the device > > pointer, e.g. > > > dev_xxx debug information and some tracepoints also take device pointer as > > > parameter(not sure if it will break if dev is NULL). > > > Another thing is that if dev is NULL, we may not be able to use regmap > > debugfs > > > feature which seems also not as our expected. > > > > > > > I would have preferred to check dev for NULL, as it's only at two places and > > we could > > still have debug prints for NULL dev, as normal pr_info instead of dev_info. > > > > But Xiubo also pointed out that his patch [1] which updates syscon binding > > information > > will be useless if we pass NULL dev in regmap_init_mmio, which he posted > > today, > > and it requires dev pointer in "regmap_get_val_endian" function to read DT > > property. > > > > [1]: [PATCH] mfd: syscon: binding: Add syscon endianness support > > https://lkml.org/lkml/2014/9/18/67 > > > > So instead of adding dummy device or creating device structure, I would > > prefer to get actual > > device pointer corresponding to "np" passed in of_syscon_register function > > as shown below: > > > > I wonder this may not work at early stage before the devices are populated > from device tree. > My initial understanding that one important thing for your patch is > to address this issue, isn't it? > Many people have asked for this feature before. > My boot log: ... of_platform_bus_create() - skipping /chosen, no compatible prop of_platform_bus_create() - skipping /aliases, no compatible prop of_platform_bus_create() - skipping /memory, no compatible prop of_platform_bus_create() - skipping /cpus, no compatible prop create child: /soc/smmu@120 create child: /soc/smmu@128 create child: /soc/smmu@130 create child: /soc/smmu@138 create child: /soc/interrupt-controller@140 create child: /soc/tzasc@150 of_platform_bus_create() - skipping /soc/tzasc@150, no compatible prop create child: /soc/ifc@153 create child: /soc/ifc@153/nor@0,0 create child: /soc/ifc@153/board-control@2,0 create child: /soc/dcfg@1ee syscon 1ee.dcfg: regmap [mem 0x01ee-0x01ee] registered create child: /soc/quadspi@155 create child: /soc/esdhc@156 create child: /soc/scfg@157 create child: /soc/crypto@170 create child: /soc/sfp@1e8 of_platform_bus_create() - skipping /soc/sfp@1e8, no compatible prop create child: /soc/snvs@1e9 of_platform_bus_create() - skipping /soc/snvs@1e9, no compatible prop create child: /soc/serdes1@1ea of_platform_bus_create() - skipping /soc/serdes1@1ea, no compatible prop create child: /soc/clocking@1ee1000 create child: /soc/rcpm@1ee2000 create child: /soc/dspi@210 create child: /soc/dspi@211 create child: /soc/i2c@218 create child: /soc/i2c@219 create child: /soc/i2c@21a create child: /soc/serial@21c0500 create child: /soc/serial@21c0600 create child: /soc/serial@21d0500 create child: /soc/serial@21d0600 create child: /soc/gpio@230 create child: /soc/gpio@231 create child: /soc/gpio@232 create child: /soc/gpio@233 create child: /soc/uqe@240 create child: /soc/uqe@240/qeic@80 create child: /soc/uqe@240/ucc@2000 irq: no irq domain found for /soc/uqe@240/qeic@80 ! create child: /soc/uqe@240/ucc@2200 irq: no irq domain found for /soc/uqe@240/qeic@80 ! create child: /soc/uqe@240/muram@1 create child: /soc/uqe@240/si@700 create child: /soc/uqe@240/siram@1000 create child: /soc/serial@295 create child: /soc/serial@296 create child: /soc/serial@297 create child: /soc/serial@298 create child: /soc/serial@299 create child: /soc/serial@29a create child: /soc/ftm0_1@29d create child: /soc/ftm@29f of_platform_bus_create() - skipping /soc/ftm@29f, no compatible prop create child: /soc/ftm@2a0 create child: /soc/ftm@2a1 of_platform_bus_create() - skipping /soc/ftm@2a1, no compatible prop create child: /soc/ftm@2a2 of_platform_bus_create() - skipping /soc/ftm@2a2, no compatible prop create child: /soc/ftm@2a3 create child: /soc/ftm@2a4 create child: /soc/wdog@2ad create child: /soc/sai@2b6 create child: /soc/edma@2c0 create child: /soc/dcu@2ce create child: /soc/mdio@2d24000 create child: /soc/ptp_clock@2d10e00 create child: /soc/ethernet@2d1 create child: /soc/ethernet@2d5 create child: /soc/ethernet@2d9 create child: /soc/usb@860 create child: /soc/usb3@310 create child: /soc/can@2a7 create child: /
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
[...]
> >create child: /dcsr@2000/dcsr-atbrepl@3a8000
> >create child: /dcsr@2000/dcsr-tsgen-ctrl@3a9000
> >create child: /dcsr@2000/dcsr-tsgen-read@3aa000
> >create child: /regulators/regulator@0
> >...
> >
> > As default the Linux will create all the platform device for each DT node,
> which
> > Can be found from "drivers/of/platform.c".
> >
> > So we can get the pdev node using the specified DT node, and feel safe to
> use
> > it as Pankaj's patch does.
> >
>
> I mean before the devices are populated from device tree.
> For example, we usually call of_platform_populate in .init_machine.
> Before it, we may not be able to get it's device, isn't it?
>
Yes, right.
For this case, we'd better create the pdev or dev manually for the first time
We use it, right ?
Thanks,
BRs
Xiubo
> Regards
> Dong Aisheng
>
> > And also we must make sure that the 'syscon' DT nodes has the compatible
> prop.
> >
> > Thanks,
> >
> > BRs
> > Xiubo
> >
> >
> > > Regards
> > > Dong Aisheng
> > >
> > > >
> > > > static struct syscon *of_syscon_register(struct device_node *np)
> > > > {
> > > > + struct platform_device *pdev;
> > > > struct syscon *syscon;
> > > > struct regmap *regmap;
> > > > void __iomem *base;
> > > > @@ -142,7 +144,11 @@ static struct syscon *of_syscon_register(struct
> > > > device_node *np)
> > > > if (!base)
> > > > return ERR_PTR(-ENOMEM);
> > > >
> > > > - regmap = regmap_init_mmio(NULL, base, &syscon_regmap_config);
> > > > + pdev = of_find_device_by_node(np);
> > > > + if (!(&pdev->dev))
> > > > + return ERR_PTR(-ENODEV);
> > > > +
> > > > + regmap = regmap_init_mmio(&pdev->dev, base,
> &syscon_regmap_config);
> > > > if (IS_ERR(regmap)) {
> > > > pr_err("regmap init failed\n");
> > > > return ERR_CAST(regmap);
> > > > ---
> > > >
> > > > I have tested this in linux-next and it works well. In this way there
> won't
> > > > be any issues of
> > > > dereferencing NULL pointer in regmap.c and at the same time, if DT has
> > > > {big,little}-endian
> > > > optional property in syscon device node, it will be taken care.
> > > >
> > > > So I would wait for Arnd's opinion about above mentioned changes and
> then
> > > > post a new
> > > > change after addressing Arnd's minor comment along with this fix in next
> > > > revision.
> > > >
> > > >
> > > > Thanks,
> > > > Pankaj Dubey
> > > > > Maybe we could consider create device structure for each syscon
> compatible
> > > > device in
> > > > > syscon driver in of_syscon_register in first time which seems to be
> > > > reasonable.
> > > > >
> > > > > Regards
> > > > > Dong Aisheng
> > > > >
> > > > > >
> > > > > > Subject: [PATCH] regmap: fix NULL pointer dereference in
> > > > > > regmap_get_val_endian
> > > > > >
> > > > > > Recent commits for getting reg endianess causing NULL pointer
> > > > > > dereference if dev is passed NULL in regmap_init_mmio. This patch
> > > > > > fixes this issue, and allows to parse reg endianess only if dev and
> > > > > > dev->of_node exist.
> > > > > >
> > > > > > Signed-off-by: Pankaj Dubey
> > > > > > ---
> > > > > > drivers/base/regmap/regmap.c | 23 ++-
> > > > > > 1 file changed, 14 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/base/regmap/regmap.c
> > > > > > b/drivers/base/regmap/regmap.c index f2281af..455a877 100644
> > > > > > --- a/drivers/base/regmap/regmap.c
> > > > > > +++ b/drivers/base/regmap/regmap.c
> > > > > > @@ -477,7 +477,7 @@ static enum regmap_endian
> > > > > > regmap_get_val_endian(struct device *dev,
> > > > > > const struct regmap_bus *bus,
> > > > > > const struct regmap_config
> > > > > > *config)
> > > > {
> > > > > > - struct device_node *np = dev->of_node;
> > > > > > + struct device_node *np;
> > > > > > enum regmap_endian endian;
> > > > > >
> > > > > > /* Retrieve the endianness specification from the regmap config
> > > */
> > > > > > @@ -487,15 +487,20 @@ static enum regmap_endian
> > > > > > regmap_get_val_endian(struct device *dev,
> > > > > > if (endian != REGMAP_ENDIAN_DEFAULT)
> > > > > > return endian;
> > > > > >
> > > > > > - /* Parse the device's DT node for an endianness specification */
> > > > > > - if (of_property_read_bool(np, "big-endian"))
> > > > > > - endian = REGMAP_ENDIAN_BIG;
> > > > > > - else if (of_property_read_bool(np, "little-endian"))
> > > > > > - endian = REGMAP_ENDIAN_LITTLE;
> > > > > > + /* If the dev and dev->of_node exist try to get endianness from
> > > DT
> > > > > > */
> > > > > > + if (dev && dev->of_node) {
> > > > > > + np = dev->of_node;
> > > > > >
> > > > > > - /* If the endianness was specified in DT, use that */
> > > > > > - if (
RE: [PATCH] ARM: ls1021a: add gating clocks to IP blocks.
Hi,
> Subject: Re: [PATCH] ARM: ls1021a: add gating clocks to IP blocks.
>
> On Fri, Sep 19, 2014 at 11:37:27AM +0100, Xiubo Li wrote:
> > A given application may not use all the peripherals on the device.
> > In this case, it may be desirable to disable unused peripherals.
> > DCFG provides a mechanism for gating clocks to IP blocks that are
> > not used when running an application.
> >
> > Signed-off-by: Xiubo Li
> > ---
> > arch/arm/mach-imx/Makefile| 2 +
> > arch/arm/mach-imx/clk-ls1021a.c | 124
> ++
> > arch/arm/mach-imx/clk.h | 21 +
> > include/dt-bindings/clock/ls1021a-clock.h | 54 +
> > 4 files changed, 201 insertions(+)
> > create mode 100644 arch/arm/mach-imx/clk-ls1021a.c
> > create mode 100644 include/dt-bindings/clock/ls1021a-clock.h
>
> This looks to be missing an addition to Documentation/devicetree. Please
> put together a document.
>
Okay, I will do it.
Thanks,
BRs
Xiubo
> Mark.
>
> >
> > diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
> > index 6e4fcd8..f6a1544 100644
> > --- a/arch/arm/mach-imx/Makefile
> > +++ b/arch/arm/mach-imx/Makefile
> > @@ -110,4 +110,6 @@ obj-$(CONFIG_SOC_IMX53) += mach-imx53.o
> >
> > obj-$(CONFIG_SOC_VF610) += clk-vf610.o mach-vf610.o
> >
> > +obj-$(CONFIG_SOC_LS1021A) += clk-ls1021a.o
> > +
> > obj-y += devices/
> > diff --git a/arch/arm/mach-imx/clk-ls1021a.c b/arch/arm/mach-imx/clk-
> ls1021a.c
> > new file mode 100644
> > index 000..680b616
> > --- /dev/null
> > +++ b/arch/arm/mach-imx/clk-ls1021a.c
> > @@ -0,0 +1,124 @@
> > +/*
> > + * Copyright 2014 Freescale Semiconductor, Inc.
> > + *
> > + * 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; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "clk.h"
> > +
> > +static struct clk *clk[LS1021A_CLK_END];
> > +static struct clk_onecell_data clk_data;
> > +
> > +static void __init ls1021a_clocks_init(struct device_node *np)
> > +{
> > + void __iomem *dcfg_base;
> > +
> > +#define DCFG_CCSR_DEVDISR1 (dcfg_base + 0x70)
> > +#define DCFG_CCSR_DEVDISR2 (dcfg_base + 0x74)
> > +#define DCFG_CCSR_DEVDISR3 (dcfg_base + 0x78)
> > +#define DCFG_CCSR_DEVDISR4 (dcfg_base + 0x7c)
> > +#define DCFG_CCSR_DEVDISR5 (dcfg_base + 0x80)
> > +
> > + dcfg_base = of_iomap(np, 0);
> > +
> > + BUG_ON(!dcfg_base);
> > +
> > + clk[LS1021A_CLK_PBL_EN] = ls1021a_clk_gate("pbl_en", "dummy",
> > + DCFG_CCSR_DEVDISR1, 0,
> > true);
> > + clk[LS1021A_CLK_ESDHC_EN] = ls1021a_clk_gate("esdhc_en", "dummy",
> > + DCFG_CCSR_DEVDISR1, 2,
> > true);
> > + clk[LS1021A_CLK_DMA1_EN] = ls1021a_clk_gate("dma1_en", "dummy",
> > + DCFG_CCSR_DEVDISR1, 8,
> > true);
> > + clk[LS1021A_CLK_DMA2_EN] = ls1021a_clk_gate("dma2_en", "dummy",
> > + DCFG_CCSR_DEVDISR1, 9,
> > true);
> > + clk[LS1021A_CLK_USB3_PHY_EN] = ls1021a_clk_gate("usb3_phy_en",
> "dummy",
> > + DCFG_CCSR_DEVDISR1, 12,
> true);
> > + clk[LS1021A_CLK_USB2_EN] = ls1021a_clk_gate("usb2_en", "dummy",
> > + DCFG_CCSR_DEVDISR1, 13,
> true);
> > + clk[LS1021A_CLK_SATA_EN] = ls1021a_clk_gate("sata_en", "dummy",
> > + DCFG_CCSR_DEVDISR1, 16,
> true);
> > + clk[LS1021A_CLK_USB3_EN] = ls1021a_clk_gate("usb3_en", "dummy",
> > + DCFG_CCSR_DEVDISR1, 17,
> true);
> > + clk[LS1021A_CLK_SEC_EN] = ls1021a_clk_gate("sec_en", "dummy",
> > + DCFG_CCSR_DEVDISR1, 22,
> true);
> > + clk[LS1021A_CLK_2D_ACE_EN] = ls1021a_clk_gate("2d_ace_en", "dummy",
> > + DCFG_CCSR_DEVDISR1, 30,
> true);
> > + clk[LS1021A_CLK_QE_EN] = ls1021a_clk_gate("qe_en", "dummy",
> > + DCFG_CCSR_DEVDISR1, 31,
> true);
> > +
> > + clk[LS1021A_CLK_ETSEC1_EN] = ls1021a_clk_gate("etsec1_en", "dummy",
> > + DCFG_CCSR_DEVDISR2, 0,
> > true);
> > + clk[LS1021A_CLK_ETSEC2_EN] = ls1021a_clk_gate("etsec2_en", "dummy",
> > + DCFG_CCSR_DEVDISR2, 1,
> > true);
> > + clk[LS1021A_CLK_ETSEC3_EN] = ls1021a_clk_gate("etsec3_en", "dummy",
> > + DCFG_CCSR_DEVDISR2, 2,
> > true);
> > +
> > + clk[LS1021A_CL
RE: Problems with simple-card
Hi, > I did a mistake in the v1 of my 'ASoC: simple-card: simplify code': I > did not initialize the pointer to the asoc_simple_card_dai_init() > function when DT. Then, I fixed that, and the simple card does not work > for me. > > First, without any 'format' in the DT, I get a fmt for each CPU / CODEC > DAI: SND_SOC_DAIFMT_CBS_CFS is always set. Well, some code is executed > for nothing, but this is not critical. > This is the old issue that I have raised before, which is one limitation of snd_soc_of_parse_daifmt(). As descript above, if without any fmt in the DT node, and just using snd_soc_of_parse_daifmt() for each CPU/CODEC DAI fmt the SND_SOC_DAIFMT_CBS_CFS (none zero) is always set. @Mark and Morimoto-san, there has two ways to deal with this in my mind: Fist, in the snd_soc_of_parse_daifmt(), modifying the boolen : "[prefix]bitclock-master" boolen : "[prefix]frame-master" ==> [prefix]master = "XXX" and "XXX" is : "cbm-cfm" --> SND_SOC_DAIFMT_CBM_CFM "cbs-cfm" --> SND_SOC_DAIFMT_CBS_CFM "cbm-cfs" --> SND_SOC_DAIFMT_CBM_CFS "cbs-cfs" --> SND_SOC_DAIFMT_CBS_CFS And the default value will be zero... If this method is applied, and there is no need any masks for snd_soc_of_parse_daifmt(), like in simple card driver: /* get CPU/CODEC common format via simple-audio-card,format */ info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,") & (SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK); Second, if there hasn't any DAI fmts in DT node and at the same time the CPU and CODEC DAI devices don't need to care about of them, and set_dai() pointer could be set to be NULL, and then snd_soc_dai_set_fmt() just returned -ENOTSUPP. Thanks,
RE: [PATCH] ASoC: simple-card: Add snd_card's name parsing from DT node support
Hi David, > > sound/soc/generic/simple-card | 0 > > sound/soc/generic/simple-card. | 0 > > sound/soc/generic/simple-card.c | 7 ++- > > 3 files changed, 6 insertions(+), 1 deletion(-) > > create mode 100644 sound/soc/generic/simple-card > > create mode 100644 sound/soc/generic/simple-card. > > Probably not the intention to create these empty files. Yes, I have resent this patch. Thanks very much, Best Regards,-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [PATCH] ASoC: core: Use devm_kzalloc() instead kzalloc()
Hi Mar, Lars > > I don't like this. I don't think it is a good design pattern to call > > devm function from within (especial non-devm) library functions. It > > creates an asymmetric API. The memory is allocated when > > snd_dmaengine_pcm_register() is called, but it is not freed when > > snd_dmaengine_pcm_unregister() is called. This goes against the > > principle of least surprise. > > Yes, I tend to agree - unless we only support managed registration the > API shouldn't do managed things internally. Got it. Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] of: fix of_update_property()
> Subject: Re: [PATCH] of: fix of_update_property()
>
> On Thu, Jan 16, 2014 at 10:46 PM, Xiubo Li wrote:
> > The of_update_property() is intent to update a property in a node
>
> s/intent/indended/
>
> > and if the property does not exist, will add it to the node.
> >
> > The second search of the property is possibly won't be found, that
> > maybe removed by other thread just before the second search begain,
> > if so just retry it.
>
> How did you find this problem? Actual use or some artificial stress test?
>
Some artificial stress test at home.
> > Signed-off-by: Xiubo Li
> > ---
> > drivers/of/base.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index f807d0e..d0c53bc 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1572,6 +1572,7 @@ int of_update_property(struct device_node *np, struct
> property *newprop)
> > if (!newprop->name)
> > return -EINVAL;
> >
> > +retry:
> > oldprop = of_find_property(np, newprop->name, NULL);
> > if (!oldprop)
> > return of_add_property(np, newprop);
>
> Isn't there also a race that if you do 2 updates for a non-existent
> property and both threads try to add the property, the first one will
> succeed and the 2nd will fail. The 2nd one needs to retry as well.
>
Well, yes, that will happen.
Maybe we could add one __of_add_property() without any locks, like
__of_find_property(). And then in of_update_prperty() move the searching
and adding operations to between lock and unlock, like:
raw_spin_lock_irqsave();
oldprop = __of_find_property();
if (!oldprop) {
rc = __of_add_property(np, newprop);
...
}
...
replace the node...
...
raw_spin_unlock_irqrestore();
> Also, couldn't the node itself be removed while trying to do the update?
>
For this is between the lock operations. I think this doesn't matter here.
> There seem to be multiple problems with this code, but doing multiple
> simultaneous, conflicting updates seems like an unlikely case.
>
Yes, but this will happen in theory.
Thanks,
Best Regards,
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] ARM: ls1021a: add gating clocks to IP blocks.
Hi,
[...]
> > +++ b/arch/arm/mach-imx/clk-ls1021a.c
> > @@ -0,0 +1,124 @@
> > +/*
> > + * Copyright 2014 Freescale Semiconductor, Inc.
> > + *
> > + * 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; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> This empty line can be removed.
>
Okay.
[...]
> > +static void __init ls1021a_clocks_init(struct device_node *np)
> > +{
> > + void __iomem *dcfg_base;
> > +
> > +#define DCFG_CCSR_DEVDISR1 (dcfg_base + 0x70)
> > +#define DCFG_CCSR_DEVDISR2 (dcfg_base + 0x74)
> > +#define DCFG_CCSR_DEVDISR3 (dcfg_base + 0x78)
> > +#define DCFG_CCSR_DEVDISR4 (dcfg_base + 0x7c)
> > +#define DCFG_CCSR_DEVDISR5 (dcfg_base + 0x80)
> > +
> > + dcfg_base = of_iomap(np, 0);
> > +
> > + BUG_ON(!dcfg_base);
> Is this worth a BUG?
Yes, I do think so.
There is one story about my platform:
We are using the imx2_wdt watchdog device, which cannot stop or suspend
once it has started.
And our platform will also support the Power Management, if the gating
Clock initialized failed, so when the system enters sleep mode, we cannot
Stop the imx2_wdt IP block, so it will reset the board after 180 seconds at
most.
So using this gating clock, the watchdog IP block's clock could be disabled
When the system enter sleep mode.
So I think BUG_ON here is make sense.
Doesn't it ?
> Or is it enough to do
> if (!dcfg_base) {
> pr_warn("failed to map fsl,ls1021a-gate device\n");
> return
> }
>
> > +
> > + clk[LS1021A_CLK_PBL_EN] = ls1021a_clk_gate("pbl_en", "dummy",
> > + DCFG_CCSR_DEVDISR1, 0, true);
> If the machine's device tree contains two (or more) nodes that are
> compatible to "fsl,ls1021a-gate", you overwrite your static clk array. Is
> it worth to add another check here?:
>
On LS1021A SoC, I can make sure that there will only be one gate node. But for
More compatibly using one dynamic clk array will be better.
> if (clk[LS1021A_CLK_PBL_EN]) {
> pr_warn("only a single instance of fsl,ls1021a-gate
> supported\n");
> return;
> }
>
> (Is it a valid case that ls1021a_clk_gate returns NULL? In this case the
> condition above is wrong.)
Yes, maybe.
I'll have a check of these.
[...]
> > + clk[LS1021A_CLK_FLEXCAN1_EN] = ls1021a_clk_gate("flexcan1_en", "dummy",
> > + DCFG_CCSR_DEVDISR5, 12, true);
> > + clk[LS1021A_CLK_FLEXCAN234_EN] = ls1021a_clk_gate("flexcan234_en",
> > + "dummy", DCFG_CCSR_DEVDISR5, 13, true);
> > + /* For flextiemr 2/3/4/5/6/7/8 */
> s/tiemr/timer/
>
Yes.
[...]
> > + clk[LS1021A_CLK_FLEXTIMER1_EN] = ls1021a_clk_gate("flextimer1_en",
> > + "dummy", DCFG_CCSR_DEVDISR5, 31, true);
> > +
> > + /* Add the clocks to provider list */
> > + clk_data.clks = clk;
> > + clk_data.clk_num = ARRAY_SIZE(clk);
> These two could be initialised statically.
>
Yes, I will use LS1021A_CLK_END instead of ARRAY_SIZE(clk).
> > + of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> > +}
> > +CLK_OF_DECLARE(ls1021a, "fsl,ls1021a-gate", ls1021a_clocks_init);
> > diff --git a/arch/arm/mach-imx/clk.h b/arch/arm/mach-imx/clk.h
> > index 4cdf8b6..dd9e369 100644
> > --- a/arch/arm/mach-imx/clk.h
> > +++ b/arch/arm/mach-imx/clk.h
> > @@ -107,6 +107,27 @@ static inline struct clk *imx_clk_gate_dis(const char
> *name, const char *parent,
> > shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock);
> > }
> >
> > +/* The DCFG registers are in big endian mode on LS1021A SoC */
> > +static inline u8 ls1021a_clk_calculate_shift(u8 shift)
> > +{
> > + int m, n;
> > +
> > + m = shift / 8;
> > + n = shift % 8;
> > +
> > + return (3 - m) * 8 + n;
> > +}
> > +
> > +static inline struct clk *ls1021a_clk_gate(const char *name, const char
> *parent,
> > + void __iomem *reg, u8 shift, bool big_endian)
> > +{
> > + if (big_endian)
> This is always true up to now. Does this change in the future?
Yes.
> If not I
> suggest to drop the parameter. Even if this might change, I would
> prefer:
>
> clk[LS1021A_CLK_FLEXTIMER1_EN] =
> ls1021a_clk_gate("flextimer1_en",
>"dummy", DCFG_CCSR_DEVDISR5,
>ls1021a_clk_shift_be(31));
>
> which is moreover more likely to be optimizable by the compiler.
> But maybe that's only me.
>
Yes, this looks good to me too.
Thanks very much for your comments.
BRs
Xiubo
> > + shift = ls1021a_clk_calculate_shift(shift);
> > +
> > + return clk_register_gate(NULL, name, parent, CLK_SET_RATE_PARENT, reg,
> > + shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock);
> > +}
> > +
> > static inline struct
RE: [PATCH v5] mfd: syscon: Decouple syscon interface from platform devices
Hi,
[...]
> +static struct regmap_config syscon_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> };
>
> -static int syscon_match_node(struct device *dev, void *data)
> +static struct syscon *of_syscon_register(struct device_node *np)
> {
> - struct device_node *dn = data;
> + struct platform_device *pdev = NULL;
struct platform_device *pdev; ?
Thanks,
BRs
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] watchdog: imx2_wdt: Add power management support.
Hi,
[...]
> > +#ifdef CONFIG_PM_SLEEP
> > +/* Disable watchdog if it is active or non-active but still running */
> > +static int imx2_wdt_suspend(struct device *dev)
> > +{
> > + struct watchdog_device *wdog = dev_get_drvdata(dev);
> > + struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
> > +
> > + imx2_wdt_set_timeout(wdog, IMX2_WDT_MAX_TIME);
> > + imx2_wdt_ping(wdog);
>
> I am a bit concerned about this one. Why ping the watchdog if it is
> not running ? Shouldn't this only be done if the watchdog is running ?
>
Since the IMX2 watchdog will be always alive once started until power down
it.
Even not running, these code doesn't make any sense actually, and When probe,
the clock has been enabled, so we can feel safe of doing this.
> > +
> > + /* Watchdog has been stopped but IP block is still running */
> > + if (!watchdog_active(wdog) && imx2_wdt_is_running(wdev))
> > + del_timer_sync(&wdev->timer);
> > +
> > + /* If PM will be supported using imx2_wdt, please
> > +* make sure that the wdev->clk could disable the
> > +* watchdog's counter input clock source or can mask
> > +* watchdog's reset request to the core.
> > +*/
>
> As far as I know, the watchdog subsystem uses standard multi-line comments.
>
Okay, I will revise this.
> I am not sure if such a comment in the code will help. People don't usually
> read code to find out why the watchdog resets the board in suspend even
> if it should not.
>
> Can you detect this condition programmatically (presumably that the clock
> was stopped if I understand correctly) to ensure that it is met ?
>
IMO, all the SoCs using this watchdog didn't support PM for now through the
watchdog reference manual document and the current driver, because once started,
we couldn't stop the counter decreasing down to zero without disabling the
counter
clock.
What we actually care about is disabling the input clock source for counter, if
We cannot disable this, the watchdog will reset the core when sleeping. Or if we
can gate the reset signal to the cores is also one good choice.
But these cannot be programmatically to be ensured using one common way for all
SoCs.
> > + clk_disable_unprepare(wdev->clk);
> > +
> > + return 0;
> > +}
> > +
> > +/* Enable watchdog and configure it if necessary */
> > +static int imx2_wdt_resume(struct device *dev)
> > +{
> > + struct watchdog_device *wdog = dev_get_drvdata(dev);
> > + struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
> > +
> > + clk_prepare_enable(wdev->clk);
> > +
> > + if (watchdog_active(wdog) && !imx2_wdt_is_running(wdev)) {
> > + /* If watchdog is still used by consumers and
> > +* resumes from deep sleep state, need to
> > +* restart the watchdog again.
> > +*/
> > + imx2_wdt_setup(wdog);
> > + imx2_wdt_set_timeout(wdog, wdog->timeout);
> > + imx2_wdt_ping(wdog);
> > + } else if (imx2_wdt_is_running(wdev)) {
> > + imx2_wdt_set_timeout(wdog, wdog->timeout);
> > + imx2_wdt_ping(wdog);
> > + /* If watchdog has started --> stopped by the
> > +* consumers and resumes from non-deep sleep state,
> > +* then start the timer again.
> > +*/
> > + if (!watchdog_active(wdog))
> > + mod_timer(&wdev->timer,
> > + jiffies + wdog->timeout * HZ / 2);
> > + } else {
> > + /* If watchdog has been started --> stopped by
> > +* the consumers and resumes from deep sleep state,
> > +* will do nothing. The watchdog will be restarted
> > +* by consumers next time to be used.
> > +*/
>
> I really dislike the notion of an else case just for a comment.
> It should be obvious that nothing needs to be done here if the watchdog
> is not running.
>
Yes, I will remove this.
Thanks,
BRs
Xiubo
> > + }
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(imx2_wdt_pm_ops, imx2_wdt_suspend,
> > +imx2_wdt_resume);
> > +
> > static const struct of_device_id imx2_wdt_dt_ids[] = {
> > { .compatible = "fsl,imx21-wdt", },
> > { /* sentinel */ }
> > @@ -307,6 +372,7 @@ static struct platform_driver imx2_wdt_driver = {
> > .driver = {
> > .name = DRIVER_NAME,
> > .owner = THIS_MODULE,
> > + .pm = &imx2_wdt_pm_ops,
> > .of_match_table = imx2_wdt_dt_ids,
> > },
> > };
> > --
> > 2.1.0.27.g96db324
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] fbdev: fsl-sii902x: HDMI support for Freescale SoCs
Hi, [...] > >>> 4 files changed, 551 insertions(+) > >>> create mode 100644 Documentation/devicetree/bindings/video/fsl- > sii902x.txt > >>> create mode 100644 drivers/video/fbdev/fsl-sii902x.c > >> > >> I don't know how you picked the names of the people you sent this patch > >> to, but looks to me that most of them are probably not interested in > >> this patch. > >> > > > > I just using the get_maintainer.pl script. > > Yes, and that's good, but you have to use your judgment also. > get_maintainer.pl gives you names of people who have at some point > touched the files or directories you are changing. Usually only the > first names returned by get_maintainer.pl are relevant. > Okay :) > >> Anyway, a few quick comments on the patch: > >> > >> - You should probably use regmap instead of direct i2c calls. > >> Interestingly, you define regmap variable, but you never use it. > > > > Yes, this it's my another version in my local machine using regmap which > > need much more test. > > > >> - Use defines for register offsets, instead of magic numbers. > > > > This will be fixed together with regmap using. > > Well, it's better to send the patch only when you have tested and > cleaned up your driver. > > >> - You should not use static variables. They prevent having multiple > >> instances of the device. > >> > > > > Okay. > > > >> So the SiI902x chip is on the SoC, not on the board? And it's a plain > >> standard SiI902x in other aspects? > >> > > > > No, it's on board. > > > > And it will be used for i.MX and LS1+ platforms. > > Ok. In that case, I would suggest you to move to the DRM framework. The > fbdev framework is not suited to adding external encoders. The end > result with fbdev will be a SoC/board specific hack driver. > > That said, we already have such drivers for fbdev, so I'm not 100% > against adding a new one. But I'm very very seriously recommending > moving to DRM. > > And if this driver is added to fbdev, I think the device tree bindings > should use the video ports/endpoints to describe the connections. > I will have a try to use the DRM framework. Thanks, BRs Xiubo > Tomi > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] ARM: ls1021a: add gating clocks to IP blocks.
Hi,
[...]
> > +Required properties:
> > +- compatible: Should be "fsl,ls1021a-gate"
> > +- reg: Address and length of the register set
> > +- #clock-cells:Should be <1>
> > +
> > +Optional property:
> > +- big-endian: If present the gate clock base registers are
> > + implemented in big endian mode, otherwise in
> > + little mode.
>
> Do you have a real world example that has the block work in little
> endian? Otherwise, I suggest we make big-endian the default and save
> the property for now.
>
On ls1 it is big-endian, not sure will be little-endian in another version.
Maybe add this if need in the future is better ? if so, I will remove this.
[...]
> > +
> > + BUG_ON(!clk_data || !clks);
> > +
> > + dcfg_base = of_iomap(np, 0);
> > +
> > + BUG_ON(!dcfg_base);
> > +
> > + if (of_property_read_bool(np, "big-endian"))
> > + ls1021a_clk_shift = ls1021a_clk_shift_be;
>
> Suggest to make this default and save the property for now.
>
Okay. I will have a look at this.
> > +
> > + clks[LS1021A_CLK_PBL_EN] = ls1021a_clk_gate("pbl_en", "dummy",
>
> So the parent of all these gate clocks are "dummy". Does it mean that
> the clock tree on LS1021A consists of only gate clocks? No dividers
> or multiplexers?
>
Yes, only gate clocks.
> > + DCFG_CCSR_DEVDISR1,
> > + ls1021a_clk_shift(0));
>
> I think we can ignore the 80 columns warning and put these on one line
> to make the code a bit easier to read.
>
Okay, as you have agreed, I will put these on one line.
[...]
> > + clks[LS1021A_CLK_I2C1_EN] = ls1021a_clk_gate("i2c1_en", "dummy",
> > + DCFG_CCSR_DEVDISR5,
> > + ls1021a_clk_shift(29));
> > + clks[LS1021A_CLK_LPUART1_EN] = ls1021a_clk_gate("lpuart1_en", "dummy",
> > + DCFG_CCSR_DEVDISR5,
> > + ls1021a_clk_shift(30));
> > + clks[LS1021A_CLK_FLEXTIMER1_EN] = ls1021a_clk_gate("flextimer1_en",
> > + "dummy", DCFG_CCSR_DEVDISR5,
> > + ls1021a_clk_shift(31));
>
> So "dummy" and ls1021a_clk_shift() are two common things for every
> single call of ls1021a_clk_gate(). Can we handle them in
> ls1021a_clk_gate() wrapper, so that we can make these calls a bit
> shorter?
>
There one gate has one parent.
If the 'big-endian' will as default, I think ls1021a_clk_gate() could be handled
In ls1021a_clk_gate() wrapper to simplify the code.
> > +
> > + for (i = 0; i < LS1021A_CLK_END; i++) {
> > + if (IS_ERR(clks[i])) {
> > + pr_err("LS1021A clk %d: register failed with %ld\n",
> > + i, PTR_ERR(clks[i]));
> > + BUG();
> > + }
> > + }
> > +
> > + /* Add the clocks to provider list */
> > + clk_data->clks = clks;
> > + clk_data->clk_num = LS1021A_CLK_END;
> > + of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
> > +}
> > +CLK_OF_DECLARE(ls1021a, "fsl,ls1021a-gate", ls1021a_clocks_init);
> > diff --git a/arch/arm/mach-imx/clk.h b/arch/arm/mach-imx/clk.h
> > index 4cdf8b6..cfbae8c 100644
> > --- a/arch/arm/mach-imx/clk.h
> > +++ b/arch/arm/mach-imx/clk.h
> > @@ -107,6 +107,14 @@ static inline struct clk *imx_clk_gate_dis(const char
> *name, const char *parent,
> > shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock);
> > }
> >
> > +static inline struct clk *ls1021a_clk_gate(const char *name, const char
> *parent,
> > + void __iomem *reg, u8 shift)
> > +{
> > + return clk_register_gate(NULL, name, parent, CLK_SET_RATE_PARENT |
>
> As the parent of the clocks registered by this function is "dummy" from
> what I see, what is the point of setting flag CLK_SET_RATE_PARENT then?
>
> > + CLK_IGNORE_UNUSED, reg, shift,
>
> Why flag CLK_IGNORE_UNUSED?
>
As the SoC reference manual all the IP blocks will be enabled when powered up.
But the Linux OS will disable all the IP blocks' clocks as default without this.
For now, this gate driver will only used for PM support of some IP blocks. For
the
Others needed to be enabled as default.
I'm thinking how about just adding the gates needed, for the others add them
later
When they are needed in the future ?
Thanks,
BRs
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] ARM: ls1021a: add gating clocks to IP blocks.
> > > As the parent of the clocks registered by this function is "dummy" from > > > what I see, what is the point of setting flag CLK_SET_RATE_PARENT then? > > > > > > > + CLK_IGNORE_UNUSED, reg, shift, > > > > > > Why flag CLK_IGNORE_UNUSED? > > > > > > > As the SoC reference manual all the IP blocks will be enabled when powered > up. > > But the Linux OS will disable all the IP blocks' clocks as default without > this. > > > > For now, this gate driver will only used for PM support of some IP blocks. > For the > > Others needed to be enabled as default. > > Ok, got it. I'm fine with the flag then, and we can remove it after all > those client devices manage their clocks well. > Okay. Thanks, BRs Xiubo > Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] ARM: Kconfig: Open force maximum zone order for all SoCs
Hi Russell, I'd like to know the status of this patch. Our Platforms depend on it. Thanks, BRs Xiubo > -Original Message- > From: Xiubo Li [mailto:li.xi...@freescale.com] > Sent: Monday, September 01, 2014 12:42 PM > To: li...@arm.linux.org.uk; linux-arm-ker...@lists.infradead.org > Cc: linux-kernel@vger.kernel.org; Xiubo Li-B47053 > Subject: [PATCH] ARM: Kconfig: Open force maximum zone order for all SoCs > > Signed-off-by: Xiubo Li > --- > arch/arm/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index c49a775..bf8445c 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -1687,7 +1687,7 @@ config ARCH_WANT_GENERAL_HUGETLB > source "mm/Kconfig" > > config FORCE_MAX_ZONEORDER > - int "Maximum zone order" if ARCH_SHMOBILE_LEGACY > + int "Maximum zone order" > range 11 64 if ARCH_SHMOBILE_LEGACY > default "12" if SOC_AM33XX > default "9" if SA || ARCH_EFM32 > -- > 1.8.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] ARM: Kconfig: Open force maximum zone order for all SoCs
[...] > On Fri, Sep 26, 2014 at 05:30:40AM +0000, li.xi...@freescale.com wrote: > > Hi Russell, > > > > I'd like to know the status of this patch. > > > > Our Platforms depend on it. > > I'm not applying it. This option should not be exposed for general > use. The usage pattern is clear from the existing users - propose > an alternative default value for your platform rather than making it > a user visible tweakable option. > Okay. Something like : ++ diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 1ad6fb6..5729a2c 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1802,7 +1802,7 @@ source "mm/Kconfig" config FORCE_MAX_ZONEORDER int "Maximum zone order" if ARCH_SHMOBILE range 11 64 if ARCH_SHMOBILE - default "12" if SOC_AM33XX + default "12" if SOC_AM33XX || SOC_LS1021A default "9" if SA default "11" help -- Is okay ? Thanks, BRs Xiubo > -- > FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up > according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] ARM: Kconfig: Open force maximum zone order for all SoCs
> Subject: Re: [PATCH] ARM: Kconfig: Open force maximum zone order for all SoCs > > On Fri, Sep 26, 2014 at 08:39:04AM +0000, li.xi...@freescale.com wrote: > > [...] > > > On Fri, Sep 26, 2014 at 05:30:40AM +, li.xi...@freescale.com wrote: > > > > Hi Russell, > > > > > > > > I'd like to know the status of this patch. > > > > > > > > Our Platforms depend on it. > > > > > > I'm not applying it. This option should not be exposed for general > > > use. The usage pattern is clear from the existing users - propose > > > an alternative default value for your platform rather than making it > > > a user visible tweakable option. > > > > > > > Okay. > > > > Something like : > > ++ > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > index 1ad6fb6..5729a2c 100644 > > --- a/arch/arm/Kconfig > > +++ b/arch/arm/Kconfig > > @@ -1802,7 +1802,7 @@ source "mm/Kconfig" > > config FORCE_MAX_ZONEORDER > > int "Maximum zone order" if ARCH_SHMOBILE > > range 11 64 if ARCH_SHMOBILE > > - default "12" if SOC_AM33XX > > + default "12" if SOC_AM33XX || SOC_LS1021A > > default "9" if SA > > default "11" > > help > > -- > > > > Is okay ? > > It's much better than exposing the option, but I'd like this patch to > appear as a properly submitted change (including a change log which > provides the reason why this is necessary.) > Okay, I will. Thanks, BRs Xiubo > Thanks. > > -- > FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up > according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] clocksource: Add BE/LE APIs support for clocksource counter reading.
> Cc: daniel.lezc...@linaro.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] clocksource: Add BE/LE APIs support for clocksource
> counter reading.
>
> On Wed, 10 Sep 2014, Xiubo Li wrote:
>
> So you add BE/LE APIs according to the subject line, but you fail
> again to tell WHY. If that would be the only issue
>
> > Signed-off-by: Xiubo Li
> > ---
> > drivers/clocksource/mmio.c | 44
>
> > 1 file changed, 44 insertions(+)
> >
> > diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
> > index 1593ade..ddc5214 100644
> > --- a/drivers/clocksource/mmio.c
> > +++ b/drivers/clocksource/mmio.c
>
> So you think that adding this to mmio.c is the solution to the
> problem?
>
It's just one solution of this, and will be a little faster than the old one
When reading the counter value.
> Ever heard about function prototypes, header files and such?
>
Yes, this will be fixed later if this is acceptable.
> > @@ -20,21 +20,65 @@ static inline struct clocksource_mmio
> *to_mmio_clksrc(struct clocksource *c)
> > return container_of(c, struct clocksource_mmio, clksrc);
> > }
> >
> > +cycle_t clocksource_mmio_readl_up_be(struct clocksource *c)
> > +{
> > + return (cycle_t)be32_to_cpu(readl_relaxed(to_mmio_clksrc(c)->reg));
> > +}
> > +
> > +cycle_t clocksource_mmio_readl_up_le(struct clocksource *c)
> > +{
> > + return (cycle_t)le32_to_cpu(readl_relaxed(to_mmio_clksrc(c)->reg));
> > +}
> > +
> > cycle_t clocksource_mmio_readl_up(struct clocksource *c)
> > {
> > return (cycle_t)readl_relaxed(to_mmio_clksrc(c)->reg);
> > }
>
> So now we have three global functions which provide access to the mmio
> clocksource. And one of them is always redundant for a given compile
> time endianess. You can't be serious about this...
>
Yes, any good suggestion about this ?
I found whatever the method it is about the endianness will always be many
Different voices from different people and found it hard to taste all of them.
So firstly I just using the driver's local APIs.
Thanks,
BRs
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] clocksource: Add BE APIs support for clocksource counter reading.
Hi,
> Subject: Re: [PATCH v2] clocksource: Add BE APIs support for clocksource
> counter reading.
>
> On Fri, 26 Sep 2014, Xiubo Li wrote:
> > For now I just added _be() support using ioread{16,32}be.
> > And i have a check of the clocksource drivers, didn't find anyone
> > using LE mode on one BE SoC, so _le() APIs is not needed.
>
> Nonsense. The existing clocksource_mmio accessor function are
> providing LE access independent of the CPU endianess. So we don't need
> an _le() API simply because we have it already.
>
> > cycle_t clocksource_mmio_readl_up(struct clocksource *c)
> > {
> > - return (cycle_t)readl_relaxed(to_mmio_clksrc(c)->reg);
> > + return (cycle_t)ioread32(to_mmio_clksrc(c)->reg);
>
> And how exactly is this change related to adding BE support?
>
Actually not very much, since the _be() APIs are using ioread{16,32}be(),
so I think using ioread{16,32}() will be less odd to having two different
accessors here.
Wouldn't this be more unified somehow ?
Thanks very much,
BRs
Xiubo
> Thanks,
>
> tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] regmap: fix possible ZERO_SIZE_PTR pointer dereferencing error.
Hi Mark,
> Subject: [PATCH] regmap: fix possible ZERO_SIZE_PTR pointer dereferencing
> error.
>
> Since we cannot make sure the 'val_count' will always be none zero
> here, and then if it equals to zero, the kmemdup() will return
> ZERO_SIZE_PTR, which equals to ((void *)16).
>
> So this patch fix this with just doing the zero check before calling
> kmemdup().
>
> Signed-off-by: Xiubo Li
> ---
> drivers/base/regmap/regmap.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 455a877..3d93e38 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -1716,6 +1716,9 @@ out:
> } else {
> void *wval;
>
> + if (!val_count)
> + return -EINVAL;
> +
Should it return zero as success or just return -EINVAL for error here ?
If it allow zero of val_count in regmap_bulk_write(..., val_count) could do
Nothing and just return zero as success at the beginning of it.
I will respin this patch if return zero is better ...
Thanks,
BRs
Xiubo
> wval = kmemdup(val, val_count * val_bytes, GFP_KERNEL);
> if (!wval) {
> dev_err(map->dev, "Error in memory allocation\n");
> --
> 2.1.0.27.g96db324
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] clocksource: Add BE APIs support for clocksource counter reading.
Hi,
> Subject: RE: [PATCH v2] clocksource: Add BE APIs support for clocksource
> counter reading.
>
> On Sun, 28 Sep 2014, li.xi...@freescale.com wrote:
> > > Subject: Re: [PATCH v2] clocksource: Add BE APIs support for clocksource
> > > counter reading.
> > >
> > > On Fri, 26 Sep 2014, Xiubo Li wrote:
> > > > For now I just added _be() support using ioread{16,32}be.
> > > > And i have a check of the clocksource drivers, didn't find anyone
> > > > using LE mode on one BE SoC, so _le() APIs is not needed.
> > >
> > > Nonsense. The existing clocksource_mmio accessor function are
> > > providing LE access independent of the CPU endianess. So we don't need
> > > an _le() API simply because we have it already.
> > >
> > > > cycle_t clocksource_mmio_readl_up(struct clocksource *c)
> > > > {
> > > > - return (cycle_t)readl_relaxed(to_mmio_clksrc(c)->reg);
> > > > + return (cycle_t)ioread32(to_mmio_clksrc(c)->reg);
> > >
> > > And how exactly is this change related to adding BE support?
> > >
> >
> > Actually not very much, since the _be() APIs are using ioread{16,32}be(),
> > so I think using ioread{16,32}() will be less odd to having two different
> > accessors here.
> >
> > Wouldn't this be more unified somehow ?
>
> Changing existing code wants to be a separate patch with a proper
> changelog and a proper argument WHY it needs to be changed in the
> first place.
>
> So please provide that separate patch first with a VERY REASONABLE
> explanation in the changelog WHY the existing readl_relaxed() should
> be replaced by ioread32().
>
Okay, I will follow your advice.
Thanks,
BRs
Xiubo
> Thanks,
>
> tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] regmap: fix possible ZERO_SIZE_PTR pointer dereferencing error.
Hi, > > > diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c > > index 455a877..3d93e38 100644 > > --- a/drivers/base/regmap/regmap.c > > +++ b/drivers/base/regmap/regmap.c > > @@ -1716,6 +1716,9 @@ out: > > Whatever you're using to generate the patches isn't annotating with the > function being changed like git normally does which isn't great for > working out what the best return value should be. Yes, I will add more detail info about the return value in future of these kind patches. Thanks, BRs Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] fbdev: fsl-sii902x: HDMI support for Freescale SoCs
Hi Tomi, Thanks very much for your comments. > Subject: Re: [PATCH] fbdev: fsl-sii902x: HDMI support for Freescale SoCs > > Hi, > > On 05/09/14 07:48, Xiubo Li wrote: > > Some Freescale SoCs, there has an DVI/HDMI controller and a PHY, > > attached to one of their display controller unit's LCDC interfaces. > > This patch adds a preliminary static support for such controllers. > > > > This will support for many modes and a dynamic switching between > > them. > > > > Signed-off-by: Xiubo Li > > --- > > .../devicetree/bindings/video/fsl-sii902x.txt | 17 + > > drivers/video/fbdev/Kconfig| 7 + > > drivers/video/fbdev/Makefile | 1 + > > drivers/video/fbdev/fsl-sii902x.c | 526 > + > > 4 files changed, 551 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/video/fsl-sii902x.txt > > create mode 100644 drivers/video/fbdev/fsl-sii902x.c > > I don't know how you picked the names of the people you sent this patch > to, but looks to me that most of them are probably not interested in > this patch. > I just using the get_maintainer.pl script. > Anyway, a few quick comments on the patch: > > - You should probably use regmap instead of direct i2c calls. > Interestingly, you define regmap variable, but you never use it. Yes, this it's my another version in my local machine using regmap which need much more test. > - Use defines for register offsets, instead of magic numbers. This will be fixed together with regmap using. > - You should not use static variables. They prevent having multiple > instances of the device. > Okay. > So the SiI902x chip is on the SoC, not on the board? And it's a plain > standard SiI902x in other aspects? > No, it's on board. And it will be used for i.MX and LS1+ platforms. Thanks, BRs Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] drm/fb-helper: Do the mode_set.connectors ZERO_SIZE_PTR check
> Subject: Re: [PATCH] drm/fb-helper: Do the mode_set.connectors ZERO_SIZE_PTR > check > > On Thu, 06 Mar 2014, Xiubo Li wrote: > > Since we cannot make sure the 'max_conn_count' will always be none > > zero from the users, and then if max_conn_count equals to zero, the > > kcalloc() will return ZERO_SIZE_PTR, which equals to ((void *)16). > > > > So this patch fix this via doing the zero pionter check of it. > > Please just add a check for max_conn_count == 0 up front and handle it. > > BR, > Jani. > Yes, that's one better choice. See the next version please. Thanks very much! -- Best Regards, Xiubo > > > > > Signed-off-by: Xiubo Li > > --- > > drivers/gpu/drm/drm_fb_helper.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c > b/drivers/gpu/drm/drm_fb_helper.c > > index 3d13ca6e2..c6680ef 100644 > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -536,7 +536,7 @@ int drm_fb_helper_init(struct drm_device *dev, > > sizeof(struct drm_connector *), > > GFP_KERNEL); > > > > - if (!fb_helper->crtc_info[i].mode_set.connectors) > > + if > > (ZERO_OR_NULL_PTR(fb_helper->crtc_info[i].mode_set.connectors)) > > goto out_free; > > fb_helper->crtc_info[i].mode_set.num_connectors = 0; > > } > > -- > > 1.8.4 > > > > > > ___ > > dri-devel mailing list > > dri-de...@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Jani Nikula, Intel Open Source Technology Center > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv3 1/3] ASoC: codec: Simplify ASoC probe code.
> Subject: Re: [PATCHv3 1/3] ASoC: codec: Simplify ASoC probe code. > > On Mon, Mar 03, 2014 at 07:24:36AM +0000, li.xi...@freescale.com wrote: > > > > > /* Default to using ALC auto offset calibration mode. */ > > > > snd_soc_update_bits(codec, DA7213_ALC_CTRL1, > > > > DA7213_ALC_CALIB_MODE_MAN, 0); > > > > This one will fail. > > > Sorry, I'm not very understand why will this fail ? Before the ASoC probe, > > the ASoC core will set the I/O. > > :) > > OK, that's now been refactored. @Mark, @Lars, Has there any other problems about this patch series? And this I had tested on our Vybrid-Twr board based on SGTL5000 codec and SAI drivers. If not, I can continue with my second patches series about " Remove set_cache_io entirely from ASoC probe". Thanks very much. -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [alsa-devel] [PATCHv3 1/3] ASoC: codec: Simplify ASoC probe code.
> Subject: Re: [alsa-devel] [PATCHv3 1/3] ASoC: codec: Simplify ASoC probe code. > > On 03/10/2014 08:38 AM, Mark Brown wrote: > > On Mon, Mar 10, 2014 at 07:57:38AM +0100, Lars-Peter Clausen wrote: > >> On 03/10/2014 04:51 AM, li.xi...@freescale.com wrote: > > > >>> Has there any other problems about this patch series? And this I had > tested on > >>> our Vybrid-Twr board based on SGTL5000 codec and SAI drivers. If not, I > can > >>> continue with my second patches series about " Remove set_cache_io > entirely from > >>> ASoC probe". > > > >> It looks good to me. > > > > I don't have the patches any more. > > Xiubo, when you resend the patch series can you make refresh your patch 3 > against these two patches. They should preferably go in before we change the > signature. > > http://mailman.alsa-project.org/pipermail/alsa-devel/2014-March/073785.html > http://mailman.alsa-project.org/pipermail/alsa-devel/2014-March/073786.html > @Lars, Yes, certainly. @Mark, @Lars, This two patch above should be applied firstly. And I will resent my patch Series later. Thanks, -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv2 2/3] regmap: Add the DT binding documentation for endianness
> Subject: Re: [PATCHv2 2/3] regmap: Add the DT binding documentation for > endianness > > On Wed, Apr 23, 2014 at 07:46:34AM +0100, Xiubo Li wrote: > > Signed-off-by: Xiubo Li > > --- > > .../bindings/regmap/regmap-endianness.txt | 48 > ++ > > 1 file changed, 48 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/regmap/regmap- > endianness.txt > > > > diff --git a/Documentation/devicetree/bindings/regmap/regmap-endianness.txt > b/Documentation/devicetree/bindings/regmap/regmap-endianness.txt > > new file mode 100644 > > index 000..1d838c5 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/regmap/regmap-endianness.txt > > @@ -0,0 +1,48 @@ > > +Device-Tree bindings for regmap endianness > > As regmap is a Linux internal detail, I don't see why it needs to leak > into bindings. > > > +Required properties: > > +- regmap-reg-endian: register endianness, see ../endianness/endianness.txt > > + for detail. > > +- regmap-val-endian: value endianness, see ../endianness/endianness.txt for > > I'm not familiar with regmap. What is the difference between register > and value endianness? > Sorry for late, I'm very busy these days. The register endianness here is used for the I2C and SPI protocol, which will Send the register address(which maybe 8-bit or 16-bit) before its values. BRs Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/3] FTM PWM adds regmap and endianness support.
> > Xiubo Li (3): > > pwm: ftm-pwm: Clean up the code. > > pwm: ftm-pwm: Convert to direct regmap API usage. > > pwm: ftm-pwm: Add big-endian support > > > > drivers/pwm/pwm-fsl-ftm.c | 96 > > ++- > > 1 file changed, 53 insertions(+), 43 deletions(-) > > This leaves me with only very vague idea of why this is necessary and > why it should be merged. > > Please describe in more detail (in both the cover-letter and each > individual patch) why you want me to apply these patches. > Thanks very much for your reply. Should I resend this patch series? If so, I will add some thing like the following: The FTM PWM driver will be used in our Vybrid, LS1 and LS2+ SoCs, and on Vybrid and LS2 SoCs, the FTM devices are in LE mode, while on LS1 SoCs it in BE mode. So this patch series add endianness support based on the regmap core, which has already support the rich endiannesses for the same device. Thanks, BRs Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
