RE: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support
> > > > +static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc)
> > > > +{
> > > > + int ret;
> > > > + struct of_phandle_args clkspec;
> > > > + struct device_node *np = fpc->chip.dev->of_node;
> > > > +
> > > > + fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm0");
> > > > + if (IS_ERR(fpc->sys_clk)) {
> > > > + ret = PTR_ERR(fpc->sys_clk);
> > > > + dev_err(fpc->chip.dev,
> > > > + "failed to get \"ftm0\" clock %d\n",
> > > > ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + fpc->counter_clk = devm_clk_get(fpc->chip.dev, "ftm0_counter");
> > > > + if (IS_ERR(fpc->counter_clk)) {
> > > > + ret = PTR_ERR(fpc->counter_clk);
> > > > + dev_err(fpc->chip.dev,
> > > > + "failed to get \"ftm0_counter\" clock
> > > > %d\n",
> > > > + ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells",
> 1,
> > > > + );
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + fpc->counter_clk_select = clkspec.args[0];
> > >
> > > This isn't at all pretty. But given that once you have access to a
> > > struct clk there's no way to identify it, I don't know of a better
> > > alternative.
> >
> > Hi Mike,
> >
> > I've seen this crop up a number of times now, to varying degrees of
> > gravity. In this particular case, the driver needs to know the type of a
> > clock because it needs to program this hardware differently depending on
> > which clock feeds the counter. Since there is no way to obtain any kind
> > of identifying information from a struct clk, drivers need to rely on
> > hacks like this and manually reach into the device tree to obtain that
> > information.
>
> Which property of the clock is the consumer concerned with in this case?
>
The "ftm0_counter" clock.
> From a quick look at the driver it looks like there are actually a
> number of different input lines to the device that share the clock-name
> "ftm0_counter", though they are actually separate and each has a
> different divider. Have I got that right?
>
Yes mostly, there are three different input lines wired to the counter
clock, but they share only one divider. And between the three lines and
the divider there is one mux inside of the FTM IP block.
The three clock sources are:
"ftm0",
"ftm0-fix",
"ftm0-ext".
_
|
|
| +++ FTM Module
|
ftm0 ---|-->+ +
|
| + + + ++|
ftm0-fix ---|-->+ MUX +>+ divider +>+ counter +|
| + + + ++|
ftm0-ext ---|-->+ +
|
| +++
|
|_|
> If that's the case, having a unique clock-names value for each of those
> lines would be the solution I'd expect. Then you just have to list the
> one(s) that are wired up, and the driver can figure out the appropriate
> line to use either by requesting by name until it finds a match or
> inspecting the clock-names property.
>
Hi Kumar,
In the list archives for mails of "[RFC][PATCHv5 4/4] Documentation: Add
device tree bindings for Freescale FTM PWM.",we have discussed about this.
Is there any different with your suggestions or ideas?
> Is there some other property of the parent that we care about here?
>
As I know, not yet.
--
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: [PATCH v2] ASoC: SGTL5000: Fix kernel failed while trying to get optional VDDD supply.
> > +static int sgtl5000_external_vddd_used(struct snd_soc_codec *codec) {
> > + struct regulator *consumer;
> > + struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
> > +
> > + consumer = regulator_get_optional(codec->dev,
> > + sgtl5000->supplies[VDDD].supply);
> > + if (IS_ERR(consumer))
> > + return 0;
> > +
> > + regulator_put(consumer);
> > +
> > + return 1;
> > +}
>
> This is broken with respect to deferred probe, deferred probe returns an
> error when an external regulator is in use but not yet registered.
> The driver needs to at least handle -EPROBE_DEFER specially here.
>
Well, as we can see that:
1, If the dev has no regulator dt node, a -ENODEV error will be returned.
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.
That's also to say, the regulator_get_optional() must be called twice then
could we know whether the optional external VDDD is really in use or not by
using one counter here.
Or maybe adding a counter in regulator_get_optional() is better.
And do you have any other suggestions to deal with this ?
> It's also not nice to get the regulator, release it and get it again which
> you end up needing to do because...
>
I have also considered about this. Because if the regulator_bulk_get()
has failed to get the other supplies, the optional one should be released too.
And it can be more easy to deal with.
Yes, this is not very nice and I will revise this.
> > - ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),
> > + if (sgtl5000_external_vddd_used(codec)) {
> > + ret = regulator_bulk_get(codec->dev,
> > + ARRAY_SIZE(sgtl5000->supplies),
> > sgtl5000->supplies);
>
> ...I think my previous comments about this code being poorly structured in
> the existing driver stand. The bulk get should be used for the supplies
> that are always needed and a separate optional get should be used for this
> one.
Yes, it's.
--
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: [PATCH v2] ASoC: SGTL5000: Fix kernel failed while trying to get optional VDDD supply.
+static int sgtl5000_external_vddd_used(struct snd_soc_codec *codec) {
+ struct regulator *consumer;
+ struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
+
+ consumer = regulator_get_optional(codec-dev,
+ sgtl5000-supplies[VDDD].supply);
+ if (IS_ERR(consumer))
+ return 0;
+
+ regulator_put(consumer);
+
+ return 1;
+}
This is broken with respect to deferred probe, deferred probe returns an
error when an external regulator is in use but not yet registered.
The driver needs to at least handle -EPROBE_DEFER specially here.
Well, as we can see that:
1, If the dev has no regulator dt node, a -ENODEV error will be returned.
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.
That's also to say, the regulator_get_optional() must be called twice then
could we know whether the optional external VDDD is really in use or not by
using one counter here.
Or maybe adding a counter in regulator_get_optional() is better.
And do you have any other suggestions to deal with this ?
It's also not nice to get the regulator, release it and get it again which
you end up needing to do because...
I have also considered about this. Because if the regulator_bulk_get()
has failed to get the other supplies, the optional one should be released too.
And it can be more easy to deal with.
Yes, this is not very nice and I will revise this.
- ret = regulator_bulk_get(codec-dev, ARRAY_SIZE(sgtl5000-supplies),
+ if (sgtl5000_external_vddd_used(codec)) {
+ ret = regulator_bulk_get(codec-dev,
+ ARRAY_SIZE(sgtl5000-supplies),
sgtl5000-supplies);
...I think my previous comments about this code being poorly structured in
the existing driver stand. The bulk get should be used for the supplies
that are always needed and a separate optional get should be used for this
one.
Yes, it's.
--
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: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support
+static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc)
+{
+ int ret;
+ struct of_phandle_args clkspec;
+ struct device_node *np = fpc-chip.dev-of_node;
+
+ fpc-sys_clk = devm_clk_get(fpc-chip.dev, ftm0);
+ if (IS_ERR(fpc-sys_clk)) {
+ ret = PTR_ERR(fpc-sys_clk);
+ dev_err(fpc-chip.dev,
+ failed to get \ftm0\ clock %d\n,
ret);
+ return ret;
+ }
+
+ fpc-counter_clk = devm_clk_get(fpc-chip.dev, ftm0_counter);
+ if (IS_ERR(fpc-counter_clk)) {
+ ret = PTR_ERR(fpc-counter_clk);
+ dev_err(fpc-chip.dev,
+ failed to get \ftm0_counter\ clock
%d\n,
+ ret);
+ return ret;
+ }
+
+ ret = of_parse_phandle_with_args(np, clocks, #clock-cells,
1,
+ clkspec);
+ if (ret)
+ return ret;
+
+ fpc-counter_clk_select = clkspec.args[0];
This isn't at all pretty. But given that once you have access to a
struct clk there's no way to identify it, I don't know of a better
alternative.
Hi Mike,
I've seen this crop up a number of times now, to varying degrees of
gravity. In this particular case, the driver needs to know the type of a
clock because it needs to program this hardware differently depending on
which clock feeds the counter. Since there is no way to obtain any kind
of identifying information from a struct clk, drivers need to rely on
hacks like this and manually reach into the device tree to obtain that
information.
Which property of the clock is the consumer concerned with in this case?
The ftm0_counter clock.
From a quick look at the driver it looks like there are actually a
number of different input lines to the device that share the clock-name
ftm0_counter, though they are actually separate and each has a
different divider. Have I got that right?
Yes mostly, there are three different input lines wired to the counter
clock, but they share only one divider. And between the three lines and
the divider there is one mux inside of the FTM IP block.
The three clock sources are:
ftm0,
ftm0-fix,
ftm0-ext.
_
|
|
| +++ FTM Module
|
ftm0 ---|--+ +
|
| + + + ++|
ftm0-fix ---|--+ MUX ++ divider ++ counter +|
| + + + ++|
ftm0-ext ---|--+ +
|
| +++
|
|_|
If that's the case, having a unique clock-names value for each of those
lines would be the solution I'd expect. Then you just have to list the
one(s) that are wired up, and the driver can figure out the appropriate
line to use either by requesting by name until it finds a match or
inspecting the clock-names property.
Hi Kumar,
In the list archives for mails of [RFC][PATCHv5 4/4] Documentation: Add
device tree bindings for Freescale FTM PWM.,we have discussed about this.
Is there any different with your suggestions or ideas?
Is there some other property of the parent that we care about here?
As I know, not yet.
--
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: [PATCHv6 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
> > +"ftm0" (module clock), > > +"ftm0_counter" (counter clock), > > +- clocks : Must contain a clock specifier for each entry in > > +clock-names, > > + See clock/clock-bindings.txt for details of the property values. > > Note that the order is significant here, at least from the way the driver > currently implements this. You can probably make this more generic in the > driver by using of_property_match_string() on the clock-names property to > find the index of "ftm0_counter" and pass that index to the > of_parse_phandle_with_args() function to cope with the situation where the > device tree has the clocks in a different order. > Yes, this's much better. As this is still under discussing, I will improve about this after that. And I will also revise other as you have piontet out. -- 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: [PATCHv6 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
+ftm0 (module clock), +ftm0_counter (counter clock), +- clocks : Must contain a clock specifier for each entry in +clock-names, + See clock/clock-bindings.txt for details of the property values. Note that the order is significant here, at least from the way the driver currently implements this. You can probably make this more generic in the driver by using of_property_match_string() on the clock-names property to find the index of ftm0_counter and pass that index to the of_parse_phandle_with_args() function to cope with the situation where the device tree has the clocks in a different order. Yes, this's much better. As this is still under discussing, I will improve about this after that. And I will also revise other as you have piontet out. -- 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: [PATCHv6 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
> > > +"ftm0" (module clock), > > +"ftm0_counter" (counter clock), > > +- clocks : Must contain a clock specifier for each entry in > > +clock-names, > > + See clock/clock-bindings.txt for details of the property values. > > Note that the order is significant here, at least from the way the driver > currently implements this. You can probably make this more generic in the > driver by using of_property_match_string() on the clock-names property to > find the index of "ftm0_counter" and pass that index to the > of_parse_phandle_with_args() function to cope with the situation where the > device tree has the clocks in a different order. > Yes, that's much better. -- 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
> > > > +#define FTM_CNTIN_VAL 0x00
> > >
> > > Do we really need this?
> > >
> >
> > Maybe not, I think that the initial value maybe modified in the future.
> > And this can be more easy to ajust it.
>
> Why would it need to be modified?
>
Well, for the PWM function modifying it make no sense, so I'll remove this.
> > > > + writel(period_cycles + cntin - 1, fpc->base + FTM_MOD);
> > > > + writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm));
> > >
> > > And these:
> > >
> > > writel(period - 1, fpc->base + FTM_MOD);
> > > writel(duty, fpc->base + FTM_CV(pwm->hwpwm));
> > >
> > > Although now that I think about it, this seems broken. The period is
> > > set in a global register, so I assume it is valid for all channels.
> > > What if you want to use different periods for individual channels?
> > > The way I read this the last one to be configured will win and
> > > change the period to whatever it wants. Other channels won't even
> notice.
> > >
> >
> > That's right. And all the 8 channels share the same period settings.
> >
> > > Is there a way to set the period per channel?
> > >
> >
> > Not yet. Only could we do is to set the duty value individually for
> > each channel. So here is a limitation for the cusumers that all the 8
> channels'
> > period values should be the same.
>
> That will need to be handled some way. Perhaps the easiest would be to
> check whether or not another channel is already running and check that
> indeed the period as requested by the new channel matches that of any
> running channels. If it doesn't match, then pwm_config() should fail.
>
> When you do that you can also optimize this a bit by only setting the
> frequency once. Whenever a new channel is configured it will have the same
> period and therefore the register doesn't need to be reprogrammed.
>
Yes, if it dosen't match it should fail, or nothing to do with the period
register if it's not the first channel to be configured.
> > > > +static void fsl_pwm_disable(struct pwm_chip *chip, struct
> > > > +pwm_device
> > > *pwm)
> > > > +{
> > > > + struct fsl_pwm_chip *fpc;
> > > > +
> > > > + fpc = to_fsl_chip(chip);
> > > > +
> > > > + fsl_counter_clock_disable(fpc);
> > > > +}
> > >
> > > Same here. Since you can't propagate the error, perhaps an error
> > > message would be appropriate here?
> > >
> >
> > Well, in fsl_counter_clock_disable(fpc); only '0' could be returned,
> > maybe let it a void type value to return is better.
> > Just like:
> > static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc)
--
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
+#define FTM_CNTIN_VAL 0x00
Do we really need this?
Maybe not, I think that the initial value maybe modified in the future.
And this can be more easy to ajust it.
Why would it need to be modified?
Well, for the PWM function modifying it make no sense, so I'll remove this.
+ writel(period_cycles + cntin - 1, fpc-base + FTM_MOD);
+ writel(duty_cycles + cntin, fpc-base + FTM_CV(pwm-hwpwm));
And these:
writel(period - 1, fpc-base + FTM_MOD);
writel(duty, fpc-base + FTM_CV(pwm-hwpwm));
Although now that I think about it, this seems broken. The period is
set in a global register, so I assume it is valid for all channels.
What if you want to use different periods for individual channels?
The way I read this the last one to be configured will win and
change the period to whatever it wants. Other channels won't even
notice.
That's right. And all the 8 channels share the same period settings.
Is there a way to set the period per channel?
Not yet. Only could we do is to set the duty value individually for
each channel. So here is a limitation for the cusumers that all the 8
channels'
period values should be the same.
That will need to be handled some way. Perhaps the easiest would be to
check whether or not another channel is already running and check that
indeed the period as requested by the new channel matches that of any
running channels. If it doesn't match, then pwm_config() should fail.
When you do that you can also optimize this a bit by only setting the
frequency once. Whenever a new channel is configured it will have the same
period and therefore the register doesn't need to be reprogrammed.
Yes, if it dosen't match it should fail, or nothing to do with the period
register if it's not the first channel to be configured.
+static void fsl_pwm_disable(struct pwm_chip *chip, struct
+pwm_device
*pwm)
+{
+ struct fsl_pwm_chip *fpc;
+
+ fpc = to_fsl_chip(chip);
+
+ fsl_counter_clock_disable(fpc);
+}
Same here. Since you can't propagate the error, perhaps an error
message would be appropriate here?
Well, in fsl_counter_clock_disable(fpc); only '0' could be returned,
maybe let it a void type value to return is better.
Just like:
static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc)
--
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 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
+ftm0 (module clock), +ftm0_counter (counter clock), +- clocks : Must contain a clock specifier for each entry in +clock-names, + See clock/clock-bindings.txt for details of the property values. Note that the order is significant here, at least from the way the driver currently implements this. You can probably make this more generic in the driver by using of_property_match_string() on the clock-names property to find the index of ftm0_counter and pass that index to the of_parse_phandle_with_args() function to cope with the situation where the device tree has the clocks in a different order. Yes, that's much better. -- 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
> > +#define FTM_CNTIN_VAL 0x00
>
> Do we really need this?
>
Maybe not, I think that the initial value maybe modified in the future.
And this can be more easy to ajust it.
> > + 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, these could be absent.
> > + writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm));
> > +
> > + writel(0xF0, fpc->base + FTM_OUTMASK);
> > + writel(0x0F, fpc->base + FTM_OUTINIT);
>
> The purpose of this eludes me. These seem to be global (not specific to
> channel pwm->hwpwm) registers, so why are they updated whenever a single
> channel is reconfigured?
>
Well, certainly there has one way to update per channel's configuration
about this. I will revise it then.
> > + writel(period_cycles + cntin - 1, fpc->base + FTM_MOD);
> > + writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm));
>
> And these:
>
> writel(period - 1, fpc->base + FTM_MOD);
> writel(duty, fpc->base + FTM_CV(pwm->hwpwm));
>
> Although now that I think about it, this seems broken. The period is set
> in a global register, so I assume it is valid for all channels. What if
> you want to use different periods for individual channels? The way I
> read this the last one to be configured will win and change the period
> to whatever it wants. Other channels won't even notice.
>
That's right. And all the 8 channels share the same period settings.
> Is there a way to set the period per channel?
>
Not yet. Only could we do is to set the duty value individually for each
channel. So here is a limitation for the cusumers that all the 8 channels'
period values should be the same.
> > +static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
> > +{
> > + int ret;
> > + unsigned long reg;
> > +
> > + if (fpc->counter_clk_enable++)
> > + return 0;
>
> Are you sure this is safe? I think you'll need to use either an atomic
> or a mutex to lock this.
>
Maybe a mutex lock is a good choice.
> > + ret = clk_prepare_enable(fpc->counter_clk);
> > + if (ret)
> > + return ret;
>
> In case clk_prepare_enable() fails, the counter_clk_enable will need to
> be decremented in order to track the state correctly, doesn't it?
>
Yes, it should be.
> > +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct fsl_pwm_chip *fpc;
> > +
> > + fpc = to_fsl_chip(chip);
> > +
> > + fsl_counter_clock_enable(fpc);
>
> This can fail. Should the error be propagated?
>
That's better.
> > +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device
> *pwm)
> > +{
> > + struct fsl_pwm_chip *fpc;
> > +
> > + fpc = to_fsl_chip(chip);
> > +
> > + fsl_counter_clock_disable(fpc);
> > +}
>
> Same here. Since you can't propagate the error, perhaps an error message
> would be appropriate here?
>
Well, in fsl_counter_clock_disable(fpc); only '0' could be returned, maybe
let it a void type value to return is better.
Just like:
static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc) {}
> Also for the locking above, perhaps a good solution would be to acquire
> the lock around the calls to fsl_counter_clock_{enable,disable}() so
> that they can safely assume that they are called with the lock held,
> which will make their implementation a lot simpler.
>
> So what you could do is this:
>
> static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device
> *pwm)
> {
> struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> int ret;
>
> mutex_lock(>lock);
> ret = fsl_counter_clock_enable(fpc);
> mutex_unlock(>lock);
>
> return ret;
> }
>
> And analogously for fsl_pwm_disable().
>
I will think about this.
>
> > +static int fsl_pwm_calculate_ps(struct fsl_pwm_chip *fpc)
> > +{
> > + unsigned long long sys_rate, counter_rate, ratio;
> > +
> > + sys_rate = clk_get_rate(fpc->sys_clk);
> > + if (!sys_rate)
> > + return -EINVAL;
> > +
> > + counter_rate = clk_get_rate(fpc->counter_clk);
> > + if (!counter_rate) {
> > + fpc->counter_clk = fpc->sys_clk;
> > + fpc->counter_clk_select = VF610_CLK_FTM0;
> > + dev_warn(fpc->chip.dev,
> > + "the counter source clock is a dummy clock,
RE: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support
Hi Thierry,
Thanks for your detail comments.
> > + switch (fpc->counter_clk_select) {
> > + case VF610_CLK_FTM0:
> > + reg |= FTMSC_CLKSYS;
> > + break;
> > + case VF610_CLK_FTM0_FIX_SEL:
> > + reg |= FTMSC_CLKFIX;
> > + break;
> > + case VF610_CLK_FTM0_EXT_SEL:
> > + reg |= FTMSC_CLKEXT;
> > + break;
> > + default:
> > + break;
> > + }
> > + reg |= fpc->clk_ps;
>
> And another one above this line.
>
> > + writel(reg, fpc->base + FTM_SC);
>
> I think with the proper locking in place what you should do is increment
> counter_clk_enable only here. That makes avoids having to decrement the
> count on error.
>
> Similarly in fsl_counter_clock_disable() you can postpone decrementing
> the count until the very end.
>
As the other mails we have talked about this that there are 8 channels
supported, but they share the same counter clock source. So we need to
make sure that when one channel is calling fsl_counter_clock_disable()
it shouldn't disable the counter clock if any other channel is still
enabled. Similary in fsl_counter clock_enable().
This is why I set counter_clk_enable property here.
--
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: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support
Hi Thierry,
Thanks for your detail comments.
+ switch (fpc-counter_clk_select) {
+ case VF610_CLK_FTM0:
+ reg |= FTMSC_CLKSYS;
+ break;
+ case VF610_CLK_FTM0_FIX_SEL:
+ reg |= FTMSC_CLKFIX;
+ break;
+ case VF610_CLK_FTM0_EXT_SEL:
+ reg |= FTMSC_CLKEXT;
+ break;
+ default:
+ break;
+ }
+ reg |= fpc-clk_ps;
And another one above this line.
+ writel(reg, fpc-base + FTM_SC);
I think with the proper locking in place what you should do is increment
counter_clk_enable only here. That makes avoids having to decrement the
count on error.
Similarly in fsl_counter_clock_disable() you can postpone decrementing
the count until the very end.
As the other mails we have talked about this that there are 8 channels
supported, but they share the same counter clock source. So we need to
make sure that when one channel is calling fsl_counter_clock_disable()
it shouldn't disable the counter clock if any other channel is still
enabled. Similary in fsl_counter clock_enable().
This is why I set counter_clk_enable property here.
--
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: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support
+#define FTM_CNTIN_VAL 0x00
Do we really need this?
Maybe not, I think that the initial value maybe modified in the future.
And this can be more easy to ajust it.
+ 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, these could be absent.
+ writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc-base + FTM_CSC(pwm-hwpwm));
+
+ writel(0xF0, fpc-base + FTM_OUTMASK);
+ writel(0x0F, fpc-base + FTM_OUTINIT);
The purpose of this eludes me. These seem to be global (not specific to
channel pwm-hwpwm) registers, so why are they updated whenever a single
channel is reconfigured?
Well, certainly there has one way to update per channel's configuration
about this. I will revise it then.
+ writel(period_cycles + cntin - 1, fpc-base + FTM_MOD);
+ writel(duty_cycles + cntin, fpc-base + FTM_CV(pwm-hwpwm));
And these:
writel(period - 1, fpc-base + FTM_MOD);
writel(duty, fpc-base + FTM_CV(pwm-hwpwm));
Although now that I think about it, this seems broken. The period is set
in a global register, so I assume it is valid for all channels. What if
you want to use different periods for individual channels? The way I
read this the last one to be configured will win and change the period
to whatever it wants. Other channels won't even notice.
That's right. And all the 8 channels share the same period settings.
Is there a way to set the period per channel?
Not yet. Only could we do is to set the duty value individually for each
channel. So here is a limitation for the cusumers that all the 8 channels'
period values should be the same.
+static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
+{
+ int ret;
+ unsigned long reg;
+
+ if (fpc-counter_clk_enable++)
+ return 0;
Are you sure this is safe? I think you'll need to use either an atomic
or a mutex to lock this.
Maybe a mutex lock is a good choice.
+ ret = clk_prepare_enable(fpc-counter_clk);
+ if (ret)
+ return ret;
In case clk_prepare_enable() fails, the counter_clk_enable will need to
be decremented in order to track the state correctly, doesn't it?
Yes, it should be.
+static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct fsl_pwm_chip *fpc;
+
+ fpc = to_fsl_chip(chip);
+
+ fsl_counter_clock_enable(fpc);
This can fail. Should the error be propagated?
That's better.
+static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device
*pwm)
+{
+ struct fsl_pwm_chip *fpc;
+
+ fpc = to_fsl_chip(chip);
+
+ fsl_counter_clock_disable(fpc);
+}
Same here. Since you can't propagate the error, perhaps an error message
would be appropriate here?
Well, in fsl_counter_clock_disable(fpc); only '0' could be returned, maybe
let it a void type value to return is better.
Just like:
static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc) {}
Also for the locking above, perhaps a good solution would be to acquire
the lock around the calls to fsl_counter_clock_{enable,disable}() so
that they can safely assume that they are called with the lock held,
which will make their implementation a lot simpler.
So what you could do is this:
static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device
*pwm)
{
struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
int ret;
mutex_lock(fpc-lock);
ret = fsl_counter_clock_enable(fpc);
mutex_unlock(fpc-lock);
return ret;
}
And analogously for fsl_pwm_disable().
I will think about this.
+static int fsl_pwm_calculate_ps(struct fsl_pwm_chip *fpc)
+{
+ unsigned long long sys_rate, counter_rate, ratio;
+
+ sys_rate = clk_get_rate(fpc-sys_clk);
+ if (!sys_rate)
+ return -EINVAL;
+
+ counter_rate = clk_get_rate(fpc-counter_clk);
+ if (!counter_rate) {
+ fpc-counter_clk = fpc-sys_clk;
+ fpc-counter_clk_select = VF610_CLK_FTM0;
+ dev_warn(fpc-chip.dev,
+ the counter source clock is a dummy clock,
+ so select the system clock as default!\n);
+ }
+
+ switch (fpc-counter_clk_select) {
+ case VF610_CLK_FTM0_FIX_SEL:
+ ratio = 2 * counter_rate - 1;
+
RE: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
> Subject: Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting > regulator consumers > > On Wed, Nov 27, 2013 at 08:13:03AM +0000, Li Xiubo wrote: > > Please fix your mailer to word wrap within paragraphs, it makes your mail > much more legible. > Yes, I will. > > There is one dependency patch: "regulator: core: Provide a dummy > regulator with full constraints". > > > From the dependency patch, we can see that using regulator_get_optional() > instead can resovle the problem you descripted above. > > > When or will this dependency patch be merged into the -next tree ? > > That patch is already in mainline. > Okey. > > > I'd expect to see a commit description that describes how the driver > > > currently tries to handle this, why it doesn't work and how the > > > patch fixes it. > > > The SGTL5000 requires 2 external power supplies: VDDA and VDDIO. An > > optional third external power supply VDDD may be provided externally > > You need to put your working through of this stuff in the commit message. I did it already, please see the next version. -- 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: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
> > +static int sgtl5000_external_vddd_used(struct snd_soc_codec *codec)
> > +{
> > + struct regulator *consumer;
> > + struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
> > +
> > + consumer = regulator_get(codec->dev, sgtl5000-
> >supplies[VDDD].supply);
> > + if (IS_ERR(consumer)) {
> > + return 0;
> > + }
> > + regulator_put(consumer);
> > +
> > + return 1;
> > +}
>
> This is going to fail to do what you expect if a dummy regulator is
> substituted which it will almost all of the time when the supply is
> genuinely absent. It's also going to interact poorly with probe
> deferral.
>
There is one dependency patch: "regulator: core: Provide a dummy regulator with
full constraints".
>From the dependency patch, we can see that using regulator_get_optional()
>instead can resovle the problem you descripted above.
When or will this dependency patch be merged into the -next tree ?
> I'd expect to see a commit description that describes how the driver
> currently tries to handle this, why it doesn't work and how the patch
> fixes it.
The SGTL5000 requires 2 external power supplies: VDDA and VDDIO. An optional
third external power supply VDDD may be provided externally to achieve lower
power.If an external supply is not used for VDDD, the SGTL5000 driver will
register it's own non-DT regulator device, and then provides the VDDD supply
consumer, and now there will be two register devices exist, non-DT regulator
for VDDD and DT regulator for VDDIO, VDDA.
+++
+ +|3.3V VDDIO
+ +
+ SGTL5000 codec +---X VDDD
+ +
+ +|3.3V VDDA
+++
If an external supply is not used for VDDD, in the DT file, only "VDDA-supply"
and "VDDIO-supply" properties will be presented. This caused the following
kernel failed while trying to get the external VDDD regulator consumer before
trying to register it's own regulator device.
sgtl5000 0-000a: Failed to get supply 'VDDD': -19
While if an external supply is used for VDDD, in the DT file, all the three
"VDDA-supply", "VDDIO-supply" and "VDDD-supply" properties can absent, by just
using the dummy regulator.
IMO, the failed log will confuse most people(especially for those not familiar
with the SGTL5000 driver) that why getting the 'VDDD' supply failed but it
still works? or is there something wrong with it ? ,etc.
Yes, this is a buggy patch, and I will revise it later.
--
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: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
+static int sgtl5000_external_vddd_used(struct snd_soc_codec *codec)
+{
+ struct regulator *consumer;
+ struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
+
+ consumer = regulator_get(codec-dev, sgtl5000-
supplies[VDDD].supply);
+ if (IS_ERR(consumer)) {
+ return 0;
+ }
+ regulator_put(consumer);
+
+ return 1;
+}
This is going to fail to do what you expect if a dummy regulator is
substituted which it will almost all of the time when the supply is
genuinely absent. It's also going to interact poorly with probe
deferral.
There is one dependency patch: regulator: core: Provide a dummy regulator with
full constraints.
From the dependency patch, we can see that using regulator_get_optional()
instead can resovle the problem you descripted above.
When or will this dependency patch be merged into the -next tree ?
I'd expect to see a commit description that describes how the driver
currently tries to handle this, why it doesn't work and how the patch
fixes it.
The SGTL5000 requires 2 external power supplies: VDDA and VDDIO. An optional
third external power supply VDDD may be provided externally to achieve lower
power.If an external supply is not used for VDDD, the SGTL5000 driver will
register it's own non-DT regulator device, and then provides the VDDD supply
consumer, and now there will be two register devices exist, non-DT regulator
for VDDD and DT regulator for VDDIO, VDDA.
+++
+ +|3.3V VDDIO
+ +
+ SGTL5000 codec +---X VDDD
+ +
+ +|3.3V VDDA
+++
If an external supply is not used for VDDD, in the DT file, only VDDA-supply
and VDDIO-supply properties will be presented. This caused the following
kernel failed while trying to get the external VDDD regulator consumer before
trying to register it's own regulator device.
sgtl5000 0-000a: Failed to get supply 'VDDD': -19
While if an external supply is used for VDDD, in the DT file, all the three
VDDA-supply, VDDIO-supply and VDDD-supply properties can absent, by just
using the dummy regulator.
IMO, the failed log will confuse most people(especially for those not familiar
with the SGTL5000 driver) that why getting the 'VDDD' supply failed but it
still works? or is there something wrong with it ? ,etc.
Yes, this is a buggy patch, and I will revise it later.
--
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: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
Subject: Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers On Wed, Nov 27, 2013 at 08:13:03AM +, Li Xiubo wrote: Please fix your mailer to word wrap within paragraphs, it makes your mail much more legible. Yes, I will. There is one dependency patch: regulator: core: Provide a dummy regulator with full constraints. From the dependency patch, we can see that using regulator_get_optional() instead can resovle the problem you descripted above. When or will this dependency patch be merged into the -next tree ? That patch is already in mainline. Okey. I'd expect to see a commit description that describes how the driver currently tries to handle this, why it doesn't work and how the patch fixes it. The SGTL5000 requires 2 external power supplies: VDDA and VDDIO. An optional third external power supply VDDD may be provided externally You need to put your working through of this stuff in the commit message. I did it already, please see the next version. -- 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: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
> > else {
> > ret = sgtl5000_replace_vddd_with_ldo(codec);
> >
> You could fix the coding style issue (braces on both branches of the if
> clause) here too.
>
>
Yes, I will.
Thanks.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
else {
ret = sgtl5000_replace_vddd_with_ldo(codec);
You could fix the coding style issue (braces on both branches of the if
clause) here too.
Yes, I will.
Thanks.
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: [PATCHv6 0/4] Add Freescale FTM PWM driver.
Hi Thierry, Could you help review this series of patches please ? And could it be merged into the PWM next tree? Thanks very much. -- > Subject: [PATCHv6 0/4] Add Freescale FTM PWM driver. > > Hello Thierry, > > The series of patches for this version has been reviewed and acked > already. > To see each patch for detail. > > > -- > -- > > This patch series is the Freescale FTM PWM implementation. And there are > 8 channels most supported by the FTM PWM. This implementation is only > compatible with device tree definition. > > This patch series is based on linux-next and has been tested on Vybrid > VF610 Tower board using device tree. > > > > Changes in v6: > - Nothing. > > Changes in RFCv2 of PATVHv5: > - Remove "fsl,pwm-counter-clk". > > Changes in v5: > - Remove active/idle pinctrl stuff. > > Changes in v4: > - Check for the result and return an error for devm_kzalloc(). > - Move pinmux setting from the SoC file to the board specific file. > - Revise the written mistake of 'ret |= FTMSC_CLKEXT;' --> 'reg |= > FTMSC_CLKEXT;'. > > > Changes in v3: > - Remove "availabe" field. > - Remove "fsl,pwm-avaliable-chs" property. > - ... > > Changes in v2: > - Remove PWM CPWM/EPWM feature and sysfs. > - Remove some redundant code. > - Revise some code for more readable. > - Remove "fsl,pwm-clk-ps", "fsl,pwm-number", "fsl,pwm-channels",etc. > - Add "fsl,pwm-avaliable-chs", "fsl,pwm-counter-clk", etc. > - Support 8 channels default in dtsi file. > - Add counter clock source selection. > - Rename some function name, macro name, etc. > - Use PWM's and OF's existing function interfaces. > - Split clk_unprepare_enable to clk_unprepare and clk_enable,etc. > - ... > > Added in v1: > - Add Freescale FTM PWM driver support. > - Add Freescale FTM PWM node for VF610. > - Enable Enables FTM PWM device for Vybrid VF610 TOWER. > - Add device tree bindings for Freescale. > > > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pwm" in > the body of a message to majord...@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html -- 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 0/4] Add Freescale FTM PWM driver.
Hi Thierry, Could you help review this series of patches please ? And could it be merged into the PWM next tree? Thanks very much. -- Subject: [PATCHv6 0/4] Add Freescale FTM PWM driver. Hello Thierry, The series of patches for this version has been reviewed and acked already. To see each patch for detail. -- -- This patch series is the Freescale FTM PWM implementation. And there are 8 channels most supported by the FTM PWM. This implementation is only compatible with device tree definition. This patch series is based on linux-next and has been tested on Vybrid VF610 Tower board using device tree. Changes in v6: - Nothing. Changes in RFCv2 of PATVHv5: - Remove fsl,pwm-counter-clk. Changes in v5: - Remove active/idle pinctrl stuff. Changes in v4: - Check for the result and return an error for devm_kzalloc(). - Move pinmux setting from the SoC file to the board specific file. - Revise the written mistake of 'ret |= FTMSC_CLKEXT;' -- 'reg |= FTMSC_CLKEXT;'. Changes in v3: - Remove availabe field. - Remove fsl,pwm-avaliable-chs property. - ... Changes in v2: - Remove PWM CPWM/EPWM feature and sysfs. - Remove some redundant code. - Revise some code for more readable. - Remove fsl,pwm-clk-ps, fsl,pwm-number, fsl,pwm-channels,etc. - Add fsl,pwm-avaliable-chs, fsl,pwm-counter-clk, etc. - Support 8 channels default in dtsi file. - Add counter clock source selection. - Rename some function name, macro name, etc. - Use PWM's and OF's existing function interfaces. - Split clk_unprepare_enable to clk_unprepare and clk_enable,etc. - ... Added in v1: - Add Freescale FTM PWM driver support. - Add Freescale FTM PWM node for VF610. - Enable Enables FTM PWM device for Vybrid VF610 TOWER. - Add device tree bindings for Freescale. -- To unsubscribe from this list: send the line unsubscribe linux-pwm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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: [RFCv2][PATCHv5 0/4] Add Freescale FTM PWM driver.
Hi Thierry, The document binding patch of this patch series has been acked. And other patches have been acked before. Should I send a V6 patch series ? Thanks very much. -- > Subject: [RFCv2][PATCHv5 0/4] Add Freescale FTM PWM driver. > > Hello, > > This patch series is the Freescale FTM PWM implementation. And there are > 8 channels most supported by the FTM PWM. This implementation is only > compatible with device tree definition. > > This patch series is based on linux-next and has been tested on Vybrid > VF610 Tower board using device tree. > > > > Changes in RFCv2 of PATVHv5: > - Remove "fsl,pwm-counter-clk"(all the four patches are modified). > > Changes in v5: > - Remove active/idle pinctrl stuff. > > Changes in v4: > - Check for the result and return an error for devm_kzalloc(). > - Move pinmux setting from the SoC file to the board specific file. > - Revise the written mistake of 'ret |= FTMSC_CLKEXT;' --> 'reg |= > FTMSC_CLKEXT;'. > > > Changes in v3: > - Remove "availabe" field. > - Remove "fsl,pwm-avaliable-chs" property. > - ... > > Changes in v2: > - Remove PWM CPWM/EPWM feature and sysfs. > - Remove some redundant code. > - Revise some code for more readable. > - Remove "fsl,pwm-clk-ps", "fsl,pwm-number", "fsl,pwm-channels",etc. > - Add "fsl,pwm-avaliable-chs", "fsl,pwm-counter-clk", etc. > - Support 8 channels default in dtsi file. > - Add counter clock source selection. > - Rename some function name, macro name, etc. > - Use PWM's and OF's existing function interfaces. > - Split clk_unprepare_enable to clk_unprepare and clk_enable,etc. > - ... > > Added in v1: > - Add Freescale FTM PWM driver support. > - Add Freescale FTM PWM node for VF610. > - Enable Enables FTM PWM device for Vybrid VF610 TOWER. > - Add device tree bindings for Freescale. -- 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: [RFCv2][PATCHv5 0/4] Add Freescale FTM PWM driver.
Hi Thierry, The document binding patch of this patch series has been acked. And other patches have been acked before. Should I send a V6 patch series ? Thanks very much. -- Subject: [RFCv2][PATCHv5 0/4] Add Freescale FTM PWM driver. Hello, This patch series is the Freescale FTM PWM implementation. And there are 8 channels most supported by the FTM PWM. This implementation is only compatible with device tree definition. This patch series is based on linux-next and has been tested on Vybrid VF610 Tower board using device tree. Changes in RFCv2 of PATVHv5: - Remove fsl,pwm-counter-clk(all the four patches are modified). Changes in v5: - Remove active/idle pinctrl stuff. Changes in v4: - Check for the result and return an error for devm_kzalloc(). - Move pinmux setting from the SoC file to the board specific file. - Revise the written mistake of 'ret |= FTMSC_CLKEXT;' -- 'reg |= FTMSC_CLKEXT;'. Changes in v3: - Remove availabe field. - Remove fsl,pwm-avaliable-chs property. - ... Changes in v2: - Remove PWM CPWM/EPWM feature and sysfs. - Remove some redundant code. - Revise some code for more readable. - Remove fsl,pwm-clk-ps, fsl,pwm-number, fsl,pwm-channels,etc. - Add fsl,pwm-avaliable-chs, fsl,pwm-counter-clk, etc. - Support 8 channels default in dtsi file. - Add counter clock source selection. - Rename some function name, macro name, etc. - Use PWM's and OF's existing function interfaces. - Split clk_unprepare_enable to clk_unprepare and clk_enable,etc. - ... Added in v1: - Add Freescale FTM PWM driver support. - Add Freescale FTM PWM node for VF610. - Enable Enables FTM PWM device for Vybrid VF610 TOWER. - Add device tree bindings for Freescale. -- 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: [RFCv2][PATCHv5 0/4] Add Freescale FTM PWM driver.
Hi Therry, The binding patch has been acked, how about the other patches ? If they are all okey, should I sent a another official version again ? Thanks. -- Xiubo > Subject: [RFCv2][PATCHv5 0/4] Add Freescale FTM PWM driver. > > Hello, > > This patch series is the Freescale FTM PWM implementation. And there are > 8 channels most supported by the FTM PWM. This implementation is only > compatible with device tree definition. > > This patch series is based on linux-next and has been tested on Vybrid > VF610 Tower board using device tree. > > > > Changes in RFCv2 of PATVHv5: > - Remove "fsl,pwm-counter-clk"(all the four patches are modified). > > Changes in v5: > - Remove active/idle pinctrl stuff. > > Changes in v4: > - Check for the result and return an error for devm_kzalloc(). > - Move pinmux setting from the SoC file to the board specific file. > - Revise the written mistake of 'ret |= FTMSC_CLKEXT;' --> 'reg |= > FTMSC_CLKEXT;'. > > > Changes in v3: > - Remove "availabe" field. > - Remove "fsl,pwm-avaliable-chs" property. > - ... > > Changes in v2: > - Remove PWM CPWM/EPWM feature and sysfs. > - Remove some redundant code. > - Revise some code for more readable. > - Remove "fsl,pwm-clk-ps", "fsl,pwm-number", "fsl,pwm-channels",etc. > - Add "fsl,pwm-avaliable-chs", "fsl,pwm-counter-clk", etc. > - Support 8 channels default in dtsi file. > - Add counter clock source selection. > - Rename some function name, macro name, etc. > - Use PWM's and OF's existing function interfaces. > - Split clk_unprepare_enable to clk_unprepare and clk_enable,etc. > - ... > > Added in v1: > - Add Freescale FTM PWM driver support. > - Add Freescale FTM PWM node for VF610. > - Enable Enables FTM PWM device for Vybrid VF610 TOWER. > - Add device tree bindings for Freescale. > > > > > > > ___ > 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: [RFCv2][PATCHv5 0/4] Add Freescale FTM PWM driver.
Hi Therry, The binding patch has been acked, how about the other patches ? If they are all okey, should I sent a another official version again ? Thanks. -- Xiubo Subject: [RFCv2][PATCHv5 0/4] Add Freescale FTM PWM driver. Hello, This patch series is the Freescale FTM PWM implementation. And there are 8 channels most supported by the FTM PWM. This implementation is only compatible with device tree definition. This patch series is based on linux-next and has been tested on Vybrid VF610 Tower board using device tree. Changes in RFCv2 of PATVHv5: - Remove fsl,pwm-counter-clk(all the four patches are modified). Changes in v5: - Remove active/idle pinctrl stuff. Changes in v4: - Check for the result and return an error for devm_kzalloc(). - Move pinmux setting from the SoC file to the board specific file. - Revise the written mistake of 'ret |= FTMSC_CLKEXT;' -- 'reg |= FTMSC_CLKEXT;'. Changes in v3: - Remove availabe field. - Remove fsl,pwm-avaliable-chs property. - ... Changes in v2: - Remove PWM CPWM/EPWM feature and sysfs. - Remove some redundant code. - Revise some code for more readable. - Remove fsl,pwm-clk-ps, fsl,pwm-number, fsl,pwm-channels,etc. - Add fsl,pwm-avaliable-chs, fsl,pwm-counter-clk, etc. - Support 8 channels default in dtsi file. - Add counter clock source selection. - Rename some function name, macro name, etc. - Use PWM's and OF's existing function interfaces. - Split clk_unprepare_enable to clk_unprepare and clk_enable,etc. - ... Added in v1: - Add Freescale FTM PWM driver support. - Add Freescale FTM PWM node for VF610. - Enable Enables FTM PWM device for Vybrid VF610 TOWER. - Add device tree bindings for Freescale. ___ 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/
