RE: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
> Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support > > On Tue, Sep 03, 2013 at 04:17:09AM +, Xiubo Li-B47053 wrote: > > > Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support > > > > > > You simply don't need the available field. You don't need to track > > > whether they are available. If a user enables a pwm which is not > > > routed out of the SoC (disabled in the iomux) simply nothing will > > > happen except for a slightly increased power consumption. > > > > > If the there is not need to explicitly specify the channels are > > available or not, so there is no doubt that the 'available' field will > > be dropt. Why I added this here is because that the 4th and 5th > > channels' pinctrls are used as UART TX and RX as I have mentioned > > before, so here if you configure these two pinctrls, the UART TX and > > RX will be polluted, there maybe some other cases like this. > > If you misconfigure your iomux then usually unexptected things happen. > That is not the problem of the PWM driver, but the problem of the one > writing the devicetree. The kernel will print a message for conflicting > iomux settings. That should be hint enough to fix it. > That sounds good. Actully there isn't any conflicting messages will be printed. I will think it over. -- 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 1/4] pwm: Add Freescale FTM PWM driver support
On Tue, Sep 03, 2013 at 04:17:09AM +, Xiubo Li-B47053 wrote: > > Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support > > > > You simply don't need the available field. You don't need to track > > whether they are available. If a user enables a pwm which is not routed > > out of the SoC (disabled in the iomux) simply nothing will happen except > > for a slightly increased power consumption. > > > If the there is not need to explicitly specify the channels are > available or not, so there is no doubt that the 'available' field will > be dropt. Why I added this here is because that the 4th and 5th > channels' pinctrls are used as UART TX and RX as I have mentioned > before, so here if you configure these two pinctrls, the UART TX and > RX will be polluted, there maybe some other cases like this. If you misconfigure your iomux then usually unexptected things happen. That is not the problem of the PWM driver, but the problem of the one writing the devicetree. The kernel will print a message for conflicting iomux settings. That should be hint enough to fix it. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 1/4] pwm: Add Freescale FTM PWM driver support
On Tue, Sep 03, 2013 at 04:17:09AM +, Xiubo Li-B47053 wrote: Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support You simply don't need the available field. You don't need to track whether they are available. If a user enables a pwm which is not routed out of the SoC (disabled in the iomux) simply nothing will happen except for a slightly increased power consumption. If the there is not need to explicitly specify the channels are available or not, so there is no doubt that the 'available' field will be dropt. Why I added this here is because that the 4th and 5th channels' pinctrls are used as UART TX and RX as I have mentioned before, so here if you configure these two pinctrls, the UART TX and RX will be polluted, there maybe some other cases like this. If you misconfigure your iomux then usually unexptected things happen. That is not the problem of the PWM driver, but the problem of the one writing the devicetree. The kernel will print a message for conflicting iomux settings. That should be hint enough to fix it. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- 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 1/4] pwm: Add Freescale FTM PWM driver support
Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support On Tue, Sep 03, 2013 at 04:17:09AM +, Xiubo Li-B47053 wrote: Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support You simply don't need the available field. You don't need to track whether they are available. If a user enables a pwm which is not routed out of the SoC (disabled in the iomux) simply nothing will happen except for a slightly increased power consumption. If the there is not need to explicitly specify the channels are available or not, so there is no doubt that the 'available' field will be dropt. Why I added this here is because that the 4th and 5th channels' pinctrls are used as UART TX and RX as I have mentioned before, so here if you configure these two pinctrls, the UART TX and RX will be polluted, there maybe some other cases like this. If you misconfigure your iomux then usually unexptected things happen. That is not the problem of the PWM driver, but the problem of the one writing the devicetree. The kernel will print a message for conflicting iomux settings. That should be hint enough to fix it. That sounds good. Actully there isn't any conflicting messages will be printed. I will think it over. -- 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 1/4] pwm: Add Freescale FTM PWM driver support
> Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
>
> On Mon, Sep 02, 2013 at 03:33:37AM +, Xiubo Li-B47053 wrote:
> >
> > > > +static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device
> > > > +*pwm) {
> > > > + struct fsl_pwm_chip *fpc;
> > > > + struct fsl_pwm_data *pwm_data;
> > > > +
> > > > + fpc = to_fsl_chip(chip);
> > > > +
> > > > + pwm_data = pwm_get_chip_data(pwm);
> > > > + if (!pwm_data)
> > > > + return;
> > >
> > > THis check seems unnecessary.
> > >
> >
> > But if do not check it here, I must check it in the following code.
> >
> > > > +
> > > > + if (pwm_data->available != FSL_AVAILABLE)
> > > > + return;
> > > > +
> >
> > So the ' struct fsl_pwm_data' may be removed in the future.
> >
> > >
> > > > +
> > > > +
> > > > + pwm_data->period_cycles = period_cycles;
> > > > + pwm_data->duty_cycles = duty_cycles;
> > >
> > > These fields are set but never read. Please drop them.
> > >
> > > If you drop the 'available' field also the you can drop chip_data
> > > completely.
> > >
> >
> > I think I may move the 'available' field to the PWM driver data struct.
>
> You simply don't need the available field. You don't need to track
> whether they are available. If a user enables a pwm which is not routed
> out of the SoC (disabled in the iomux) simply nothing will happen except
> for a slightly increased power consumption.
>
If the there is not need to explicitly specify the channels are available or
not, so there is no doubt that the 'available' field will be dropt.
Why I added this here is because that the 4th and 5th channels' pinctrls are
used as UART TX and RX as I have mentioned before, so here if you configure
these two pinctrls, the UART TX and RX will be polluted, there maybe some other
cases like this.
So, if there is no need to worry about this in PWM driver, the customer should
be aware of it and be responsible for the potential risk.
I will think it over and optimize it then.
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: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
On Mon, Sep 02, 2013 at 03:33:37AM +, Xiubo Li-B47053 wrote:
>
> > > +static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device
> > > +*pwm) {
> > > + struct fsl_pwm_chip *fpc;
> > > + struct fsl_pwm_data *pwm_data;
> > > +
> > > + fpc = to_fsl_chip(chip);
> > > +
> > > + pwm_data = pwm_get_chip_data(pwm);
> > > + if (!pwm_data)
> > > + return;
> >
> > THis check seems unnecessary.
> >
>
> But if do not check it here, I must check it in the following code.
>
> > > +
> > > + if (pwm_data->available != FSL_AVAILABLE)
> > > + return;
> > > +
>
> So the ' struct fsl_pwm_data' may be removed in the future.
>
> >
> > > +
> > > +
> > > + pwm_data->period_cycles = period_cycles;
> > > + pwm_data->duty_cycles = duty_cycles;
> >
> > These fields are set but never read. Please drop them.
> >
> > If you drop the 'available' field also the you can drop chip_data
> > completely.
> >
>
> I think I may move the 'available' field to the PWM driver data struct.
You simply don't need the available field. You don't need to track
whether they are available. If a user enables a pwm which is not routed
out of the SoC (disabled in the iomux) simply nothing will happen except
for a slightly increased power consumption.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 1/4] pwm: Add Freescale FTM PWM driver support
On Mon, Sep 02, 2013 at 03:33:37AM +, Xiubo Li-B47053 wrote:
+static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device
+*pwm) {
+ struct fsl_pwm_chip *fpc;
+ struct fsl_pwm_data *pwm_data;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return;
THis check seems unnecessary.
But if do not check it here, I must check it in the following code.
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return;
+
So the ' struct fsl_pwm_data' may be removed in the future.
+
+
+ pwm_data-period_cycles = period_cycles;
+ pwm_data-duty_cycles = duty_cycles;
These fields are set but never read. Please drop them.
If you drop the 'available' field also the you can drop chip_data
completely.
I think I may move the 'available' field to the PWM driver data struct.
You simply don't need the available field. You don't need to track
whether they are available. If a user enables a pwm which is not routed
out of the SoC (disabled in the iomux) simply nothing will happen except
for a slightly increased power consumption.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 1/4] pwm: Add Freescale FTM PWM driver support
Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
On Mon, Sep 02, 2013 at 03:33:37AM +, Xiubo Li-B47053 wrote:
+static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device
+*pwm) {
+ struct fsl_pwm_chip *fpc;
+ struct fsl_pwm_data *pwm_data;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return;
THis check seems unnecessary.
But if do not check it here, I must check it in the following code.
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return;
+
So the ' struct fsl_pwm_data' may be removed in the future.
+
+
+ pwm_data-period_cycles = period_cycles;
+ pwm_data-duty_cycles = duty_cycles;
These fields are set but never read. Please drop them.
If you drop the 'available' field also the you can drop chip_data
completely.
I think I may move the 'available' field to the PWM driver data struct.
You simply don't need the available field. You don't need to track
whether they are available. If a user enables a pwm which is not routed
out of the SoC (disabled in the iomux) simply nothing will happen except
for a slightly increased power consumption.
If the there is not need to explicitly specify the channels are available or
not, so there is no doubt that the 'available' field will be dropt.
Why I added this here is because that the 4th and 5th channels' pinctrls are
used as UART TX and RX as I have mentioned before, so here if you configure
these two pinctrls, the UART TX and RX will be polluted, there maybe some other
cases like this.
So, if there is no need to worry about this in PWM driver, the customer should
be aware of it and be responsible for the potential risk.
I will think it over and optimize it then.
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: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
> > +static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device
> > +*pwm) {
> > + struct fsl_pwm_chip *fpc;
> > + struct fsl_pwm_data *pwm_data;
> > +
> > + fpc = to_fsl_chip(chip);
> > +
> > + pwm_data = pwm_get_chip_data(pwm);
> > + if (!pwm_data)
> > + return;
>
> THis check seems unnecessary.
>
But if do not check it here, I must check it in the following code.
> > +
> > + if (pwm_data->available != FSL_AVAILABLE)
> > + return;
> > +
So the ' struct fsl_pwm_data' may be removed in the future.
>
> > +
> > +
> > + pwm_data->period_cycles = period_cycles;
> > + pwm_data->duty_cycles = duty_cycles;
>
> These fields are set but never read. Please drop them.
>
> If you drop the 'available' field also the you can drop chip_data
> completely.
>
I think I may move the 'available' field to the PWM driver data struct.
> > +
> > + writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm));
> > +
> > + writel(0xF0, fpc->base + FTM_OUTMASK);
> > + writel(0x0F, fpc->base + FTM_OUTINIT);
> > + writel(FTM_CNTIN_VAL, fpc->base + FTM_CNTIN);
> > +
> > + writel(period_cycles + cntin - 1, fpc->base + FTM_MOD);
> > + writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm));
> > +
> > + return 0;
> > +}
> > +
> > +static int fsl_pwm_set_polarity(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > + enum pwm_polarity polarity)
> > +{
> > + unsigned long reg;
> > + struct fsl_pwm_data *pwm_data;
> > + struct fsl_pwm_chip *fpc;
> > +
> > + fpc = to_fsl_chip(chip);
> > +
> > + pwm_data = pwm_get_chip_data(pwm);
> > + if (!pwm_data)
> > + return -EINVAL;
> > +
> > + if (pwm_data->available != FSL_AVAILABLE)
> > + return -EINVAL;
> > +
> > + reg = readl(fpc->base + FTM_POL);
> > + reg &= ~BIT(pwm->hwpwm);
>
> Either drop this line...
>
This is just for unmasking this bit field.
Here it's not needed, so I will revise it.
> > + if (polarity == PWM_POLARITY_INVERSED)
> > + reg |= BIT(pwm->hwpwm);
> > + else
> > + reg &= ~BIT(pwm->hwpwm);
>
> ...or this one
>
> > +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device
> > +*pwm) {
> > + int ret;
> > + struct fsl_pwm_chip *fpc;
> > + struct pinctrl_state *pins_state;
> > + struct fsl_pwm_data *pwm_data;
> > + const char *statename;
> > +
> > + fpc = to_fsl_chip(chip);
> > +
> > + pwm_data = pwm_get_chip_data(pwm);
> > + if (!pwm_data)
> > + return -EINVAL;
> > +
> > + if (pwm_data->available != FSL_AVAILABLE)
> > + return -EINVAL;
> > +
> > + statename = kasprintf(GFP_KERNEL, "ch%d-active", pwm->hwpwm);
>
> You loose memory here and in fsl_pwm_disable aswell.
>
Yes, I will revise it.
> > + pins_state = pinctrl_lookup_state(fpc->pinctrl,
> > + statename);
> > + /* enable pins to be muxed in and configured */
> > + if (!IS_ERR(pins_state)) {
> > + ret = pinctrl_select_state(fpc->pinctrl, pins_state);
> > + if (ret)
> > + dev_warn(chip->dev, "could not set default pins\n");
> > + } else
> > + dev_warn(chip->dev, "could not get default pinstate\n");
>
> Either it's ok to do without pinctrl or it's not ok, so either return an
> error or drop the warnings. Polluting the kernel log with such messages
> from a frequently called function is not a good idea.
>
Well, I will just print out some error logs and return the error.
--
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 1/4] pwm: Add Freescale FTM PWM driver support
+static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device
+*pwm) {
+ struct fsl_pwm_chip *fpc;
+ struct fsl_pwm_data *pwm_data;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return;
THis check seems unnecessary.
But if do not check it here, I must check it in the following code.
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return;
+
So the ' struct fsl_pwm_data' may be removed in the future.
+
+
+ pwm_data-period_cycles = period_cycles;
+ pwm_data-duty_cycles = duty_cycles;
These fields are set but never read. Please drop them.
If you drop the 'available' field also the you can drop chip_data
completely.
I think I may move the 'available' field to the PWM driver data struct.
+
+ writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc-base + FTM_CSC(pwm-hwpwm));
+
+ writel(0xF0, fpc-base + FTM_OUTMASK);
+ writel(0x0F, fpc-base + FTM_OUTINIT);
+ writel(FTM_CNTIN_VAL, fpc-base + FTM_CNTIN);
+
+ writel(period_cycles + cntin - 1, fpc-base + FTM_MOD);
+ writel(duty_cycles + cntin, fpc-base + FTM_CV(pwm-hwpwm));
+
+ return 0;
+}
+
+static int fsl_pwm_set_polarity(struct pwm_chip *chip, struct
pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ unsigned long reg;
+ struct fsl_pwm_data *pwm_data;
+ struct fsl_pwm_chip *fpc;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return -EINVAL;
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return -EINVAL;
+
+ reg = readl(fpc-base + FTM_POL);
+ reg = ~BIT(pwm-hwpwm);
Either drop this line...
This is just for unmasking this bit field.
Here it's not needed, so I will revise it.
+ if (polarity == PWM_POLARITY_INVERSED)
+ reg |= BIT(pwm-hwpwm);
+ else
+ reg = ~BIT(pwm-hwpwm);
...or this one
+static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device
+*pwm) {
+ int ret;
+ struct fsl_pwm_chip *fpc;
+ struct pinctrl_state *pins_state;
+ struct fsl_pwm_data *pwm_data;
+ const char *statename;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return -EINVAL;
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return -EINVAL;
+
+ statename = kasprintf(GFP_KERNEL, ch%d-active, pwm-hwpwm);
You loose memory here and in fsl_pwm_disable aswell.
Yes, I will revise it.
+ pins_state = pinctrl_lookup_state(fpc-pinctrl,
+ statename);
+ /* enable pins to be muxed in and configured */
+ if (!IS_ERR(pins_state)) {
+ ret = pinctrl_select_state(fpc-pinctrl, pins_state);
+ if (ret)
+ dev_warn(chip-dev, could not set default pins\n);
+ } else
+ dev_warn(chip-dev, could not get default pinstate\n);
Either it's ok to do without pinctrl or it's not ok, so either return an
error or drop the warnings. Polluting the kernel log with such messages
from a frequently called function is not a good idea.
Well, I will just print out some error logs and return the error.
--
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 1/4] pwm: Add Freescale FTM PWM driver support
On Fri, Aug 30, 2013 at 05:48:49PM +0800, Xiubo Li wrote:
> +
> +#define FTM_CSC_BASE0x0C
> +#define FTM_CSC(_channel) \
> + (FTM_CSC_BASE + ((_channel) * 8))
Put into a single line.
> +#define FTMCnSC_MSB BIT(5)
> +#define FTMCnSC_MSA BIT(4)
> +#define FTMCnSC_ELSBBIT(3)
> +#define FTMCnSC_ELSABIT(2)
> +
> +#define FTM_CV_BASE 0x10
> +#define FTM_CV(_channel) \
> + (FTM_CV_BASE + ((_channel) * 8))
ditto.
> +static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + int ret;
> + struct fsl_pwm_chip *fpc;
> + struct fsl_pwm_data *pwm_data;
> +
> + fpc = to_fsl_chip(chip);
> +
> + pwm_data = pwm_get_chip_data(pwm);
> + if (!pwm_data)
> + return -EINVAL;
> +
> + if (pwm_data->available != FSL_AVAILABLE)
> + return -EINVAL;
> +
> + ret = clk_enable(fpc->sys_clk);
> + if (ret)
> + return ret;
This is non atomic context. You can also prepare the clocks here instead
of doing it in probe().
> +
> + return 0;
> +}
> +
> +static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct fsl_pwm_chip *fpc;
> + struct fsl_pwm_data *pwm_data;
> +
> + fpc = to_fsl_chip(chip);
> +
> + pwm_data = pwm_get_chip_data(pwm);
> + if (!pwm_data)
> + return;
THis check seems unnecessary.
> +
> + if (pwm_data->available != FSL_AVAILABLE)
> + return;
> +
> + clk_disable(fpc->sys_clk);
> +}
> +
[...]
> +
> +
> + pwm_data->period_cycles = period_cycles;
> + pwm_data->duty_cycles = duty_cycles;
These fields are set but never read. Please drop them.
If you drop the 'available' field also the you can drop chip_data
completely.
> +
> + writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm));
> +
> + writel(0xF0, fpc->base + FTM_OUTMASK);
> + writel(0x0F, fpc->base + FTM_OUTINIT);
> + writel(FTM_CNTIN_VAL, fpc->base + FTM_CNTIN);
> +
> + writel(period_cycles + cntin - 1, fpc->base + FTM_MOD);
> + writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm));
> +
> + return 0;
> +}
> +
> +static int fsl_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device
> *pwm,
> + enum pwm_polarity polarity)
> +{
> + unsigned long reg;
> + struct fsl_pwm_data *pwm_data;
> + struct fsl_pwm_chip *fpc;
> +
> + fpc = to_fsl_chip(chip);
> +
> + pwm_data = pwm_get_chip_data(pwm);
> + if (!pwm_data)
> + return -EINVAL;
> +
> + if (pwm_data->available != FSL_AVAILABLE)
> + return -EINVAL;
> +
> + reg = readl(fpc->base + FTM_POL);
> + reg &= ~BIT(pwm->hwpwm);
Either drop this line...
> + if (polarity == PWM_POLARITY_INVERSED)
> + reg |= BIT(pwm->hwpwm);
> + else
> + reg &= ~BIT(pwm->hwpwm);
...or this one
> +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + int ret;
> + struct fsl_pwm_chip *fpc;
> + struct pinctrl_state *pins_state;
> + struct fsl_pwm_data *pwm_data;
> + const char *statename;
> +
> + fpc = to_fsl_chip(chip);
> +
> + pwm_data = pwm_get_chip_data(pwm);
> + if (!pwm_data)
> + return -EINVAL;
> +
> + if (pwm_data->available != FSL_AVAILABLE)
> + return -EINVAL;
> +
> + statename = kasprintf(GFP_KERNEL, "ch%d-active", pwm->hwpwm);
You loose memory here and in fsl_pwm_disable aswell.
> + pins_state = pinctrl_lookup_state(fpc->pinctrl,
> + statename);
> + /* enable pins to be muxed in and configured */
> + if (!IS_ERR(pins_state)) {
> + ret = pinctrl_select_state(fpc->pinctrl, pins_state);
> + if (ret)
> + dev_warn(chip->dev, "could not set default pins\n");
> + } else
> + dev_warn(chip->dev, "could not get default pinstate\n");
Either it's ok to do without pinctrl or it's not ok, so either return
an error or drop the warnings. Polluting the kernel log with such
messages from a frequently called function is not a good idea.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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 1/4] pwm: Add Freescale FTM PWM driver support
On Fri, Aug 30, 2013 at 05:48:49PM +0800, Xiubo Li wrote:
+
+#define FTM_CSC_BASE0x0C
+#define FTM_CSC(_channel) \
+ (FTM_CSC_BASE + ((_channel) * 8))
Put into a single line.
+#define FTMCnSC_MSB BIT(5)
+#define FTMCnSC_MSA BIT(4)
+#define FTMCnSC_ELSBBIT(3)
+#define FTMCnSC_ELSABIT(2)
+
+#define FTM_CV_BASE 0x10
+#define FTM_CV(_channel) \
+ (FTM_CV_BASE + ((_channel) * 8))
ditto.
+static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ int ret;
+ struct fsl_pwm_chip *fpc;
+ struct fsl_pwm_data *pwm_data;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return -EINVAL;
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return -EINVAL;
+
+ ret = clk_enable(fpc-sys_clk);
+ if (ret)
+ return ret;
This is non atomic context. You can also prepare the clocks here instead
of doing it in probe().
+
+ return 0;
+}
+
+static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct fsl_pwm_chip *fpc;
+ struct fsl_pwm_data *pwm_data;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return;
THis check seems unnecessary.
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return;
+
+ clk_disable(fpc-sys_clk);
+}
+
[...]
+
+
+ pwm_data-period_cycles = period_cycles;
+ pwm_data-duty_cycles = duty_cycles;
These fields are set but never read. Please drop them.
If you drop the 'available' field also the you can drop chip_data
completely.
+
+ writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc-base + FTM_CSC(pwm-hwpwm));
+
+ writel(0xF0, fpc-base + FTM_OUTMASK);
+ writel(0x0F, fpc-base + FTM_OUTINIT);
+ writel(FTM_CNTIN_VAL, fpc-base + FTM_CNTIN);
+
+ writel(period_cycles + cntin - 1, fpc-base + FTM_MOD);
+ writel(duty_cycles + cntin, fpc-base + FTM_CV(pwm-hwpwm));
+
+ return 0;
+}
+
+static int fsl_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device
*pwm,
+ enum pwm_polarity polarity)
+{
+ unsigned long reg;
+ struct fsl_pwm_data *pwm_data;
+ struct fsl_pwm_chip *fpc;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return -EINVAL;
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return -EINVAL;
+
+ reg = readl(fpc-base + FTM_POL);
+ reg = ~BIT(pwm-hwpwm);
Either drop this line...
+ if (polarity == PWM_POLARITY_INVERSED)
+ reg |= BIT(pwm-hwpwm);
+ else
+ reg = ~BIT(pwm-hwpwm);
...or this one
+static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ int ret;
+ struct fsl_pwm_chip *fpc;
+ struct pinctrl_state *pins_state;
+ struct fsl_pwm_data *pwm_data;
+ const char *statename;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return -EINVAL;
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return -EINVAL;
+
+ statename = kasprintf(GFP_KERNEL, ch%d-active, pwm-hwpwm);
You loose memory here and in fsl_pwm_disable aswell.
+ pins_state = pinctrl_lookup_state(fpc-pinctrl,
+ statename);
+ /* enable pins to be muxed in and configured */
+ if (!IS_ERR(pins_state)) {
+ ret = pinctrl_select_state(fpc-pinctrl, pins_state);
+ if (ret)
+ dev_warn(chip-dev, could not set default pins\n);
+ } else
+ dev_warn(chip-dev, could not get default pinstate\n);
Either it's ok to do without pinctrl or it's not ok, so either return
an error or drop the warnings. Polluting the kernel log with such
messages from a frequently called function is not a good idea.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
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/
