| Message ID | 1485726855-16236-4-git-send-email-lukma@denx.de |
|---|---|
| State | Accepted |
| Headers | show |
On Sun, Jan 29, 2017 at 10:54:07PM +0100, Lukasz Majewski wrote: > From: Lukasz Majewski <l.majewski@majess.pl> > > This patch provides separate set of pwm ops utilized by > i.MX's PWMv1 and PWMv2. > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > Acked-by: Shawn Guo <shawn.guo@linaro.org> > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > Changes for v5: > - None > > Changes for v4: > - None > > Changes for v3: > - Adjust the code to work with ipg clock removed > > Changes for v2: > - New patch > --- > drivers/pwm/pwm-imx.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > index b1d1e50..0fa480d 100644 > --- a/drivers/pwm/pwm-imx.c > +++ b/drivers/pwm/pwm-imx.c > @@ -239,7 +239,14 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > clk_disable_unprepare(imx->clk_per); > } > > -static struct pwm_ops imx_pwm_ops = { > +static struct pwm_ops imx_pwm_ops_v1 = { > + .enable = imx_pwm_enable, > + .disable = imx_pwm_disable, > + .config = imx_pwm_config, > + .owner = THIS_MODULE, > +}; > + > +static struct pwm_ops imx_pwm_ops_v2 = { Can't these two be const? No need to respin for only this, just let me know and I can make the change while applying. Thierry
On Mon, Jan 30, 2017 at 08:23:12AM +0100, Thierry Reding wrote: > On Sun, Jan 29, 2017 at 10:54:07PM +0100, Lukasz Majewski wrote: > > From: Lukasz Majewski <l.majewski@majess.pl> > > > > This patch provides separate set of pwm ops utilized by > > i.MX's PWMv1 and PWMv2. > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com> > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > > Acked-by: Shawn Guo <shawn.guo@linaro.org> > > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > Changes for v5: > > - None > > > > Changes for v4: > > - None > > > > Changes for v3: > > - Adjust the code to work with ipg clock removed > > > > Changes for v2: > > - New patch > > --- > > drivers/pwm/pwm-imx.c | 17 ++++++++++++++--- > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > > index b1d1e50..0fa480d 100644 > > --- a/drivers/pwm/pwm-imx.c > > +++ b/drivers/pwm/pwm-imx.c > > @@ -239,7 +239,14 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > > clk_disable_unprepare(imx->clk_per); > > } > > > > -static struct pwm_ops imx_pwm_ops = { > > +static struct pwm_ops imx_pwm_ops_v1 = { > > + .enable = imx_pwm_enable, > > + .disable = imx_pwm_disable, > > + .config = imx_pwm_config, > > + .owner = THIS_MODULE, > > +}; > > + > > +static struct pwm_ops imx_pwm_ops_v2 = { > > Can't these two be const? No need to respin for only this, just let me > know and I can make the change while applying. Nevermind that. I just remembered that I had picked up a patch to make the original imx_pwm_ops a const and things still work fine if I make both of the above const, so I just had to manually apply your patch, but other than that it seems fine. Let me apply the rest of this set and push out. It'd be great if you could check afterwards that it's all still what you expect. Thierry
Hi Thierry, > On Mon, Jan 30, 2017 at 08:23:12AM +0100, Thierry Reding wrote: > > On Sun, Jan 29, 2017 at 10:54:07PM +0100, Lukasz Majewski wrote: > > > From: Lukasz Majewski <l.majewski@majess.pl> > > > > > > This patch provides separate set of pwm ops utilized by > > > i.MX's PWMv1 and PWMv2. > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com> > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > > > Acked-by: Shawn Guo <shawn.guo@linaro.org> > > > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de> > > > --- > > > Changes for v5: > > > - None > > > > > > Changes for v4: > > > - None > > > > > > Changes for v3: > > > - Adjust the code to work with ipg clock removed > > > > > > Changes for v2: > > > - New patch > > > --- > > > drivers/pwm/pwm-imx.c | 17 ++++++++++++++--- > > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > > > index b1d1e50..0fa480d 100644 > > > --- a/drivers/pwm/pwm-imx.c > > > +++ b/drivers/pwm/pwm-imx.c > > > @@ -239,7 +239,14 @@ static void imx_pwm_disable(struct pwm_chip > > > *chip, struct pwm_device *pwm) > > > clk_disable_unprepare(imx->clk_per); } > > > > > > -static struct pwm_ops imx_pwm_ops = { > > > +static struct pwm_ops imx_pwm_ops_v1 = { > > > + .enable = imx_pwm_enable, > > > + .disable = imx_pwm_disable, > > > + .config = imx_pwm_config, > > > + .owner = THIS_MODULE, > > > +}; > > > + > > > +static struct pwm_ops imx_pwm_ops_v2 = { > > > > Can't these two be const? No need to respin for only this, just let > > me know and I can make the change while applying. > > Nevermind that. I just remembered that I had picked up a patch to make > the original imx_pwm_ops a const and things still work fine if I make > both of the above const, so I just had to manually apply your patch, > but other than that it seems fine. Let me apply the rest of this set > and push out. It'd be great if you could check afterwards that it's > all still what you expect. I will do that. Thanks for integrating the patch series :-). > > Thierry Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On Mon, Jan 30, 2017 at 09:36:49AM +0100, Lukasz Majewski wrote: > Hi Thierry, > > > On Mon, Jan 30, 2017 at 08:23:12AM +0100, Thierry Reding wrote: > > > On Sun, Jan 29, 2017 at 10:54:07PM +0100, Lukasz Majewski wrote: > > > > From: Lukasz Majewski <l.majewski@majess.pl> > > > > > > > > This patch provides separate set of pwm ops utilized by > > > > i.MX's PWMv1 and PWMv2. > > > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com> > > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > > > > Acked-by: Shawn Guo <shawn.guo@linaro.org> > > > > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > --- > > > > Changes for v5: > > > > - None > > > > > > > > Changes for v4: > > > > - None > > > > > > > > Changes for v3: > > > > - Adjust the code to work with ipg clock removed > > > > > > > > Changes for v2: > > > > - New patch > > > > --- > > > > drivers/pwm/pwm-imx.c | 17 ++++++++++++++--- > > > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > > > > index b1d1e50..0fa480d 100644 > > > > --- a/drivers/pwm/pwm-imx.c > > > > +++ b/drivers/pwm/pwm-imx.c > > > > @@ -239,7 +239,14 @@ static void imx_pwm_disable(struct pwm_chip > > > > *chip, struct pwm_device *pwm) > > > > clk_disable_unprepare(imx->clk_per); } > > > > > > > > -static struct pwm_ops imx_pwm_ops = { > > > > +static struct pwm_ops imx_pwm_ops_v1 = { > > > > + .enable = imx_pwm_enable, > > > > + .disable = imx_pwm_disable, > > > > + .config = imx_pwm_config, > > > > + .owner = THIS_MODULE, > > > > +}; > > > > + > > > > +static struct pwm_ops imx_pwm_ops_v2 = { > > > > > > Can't these two be const? No need to respin for only this, just let > > > me know and I can make the change while applying. > > > > Nevermind that. I just remembered that I had picked up a patch to make > > the original imx_pwm_ops a const and things still work fine if I make > > both of the above const, so I just had to manually apply your patch, > > but other than that it seems fine. Let me apply the rest of this set > > and push out. It'd be great if you could check afterwards that it's > > all still what you expect. > > I will do that. Thanks for integrating the patch series :-). Thanks for sticking with it. I know the initial patches for optional polarity support have been around for years, and it took a really long time for this all to come together. But I think the end result is sound and looks really good. The one remaining bit that I'm not 100% happy about is that the v1 support is not atomic while the v2 support is. Not a blocker, but it looks as if it should be easy to convert over v1 as well. Any takers? Thierry
Hi Thierry, > On Mon, Jan 30, 2017 at 09:36:49AM +0100, Lukasz Majewski wrote: > > Hi Thierry, > > > > > On Mon, Jan 30, 2017 at 08:23:12AM +0100, Thierry Reding wrote: > > > > On Sun, Jan 29, 2017 at 10:54:07PM +0100, Lukasz Majewski wrote: > > > > > From: Lukasz Majewski <l.majewski@majess.pl> > > > > > > > > > > This patch provides separate set of pwm ops utilized by > > > > > i.MX's PWMv1 and PWMv2. > > > > > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > > > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com> > > > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > > > > > Acked-by: Shawn Guo <shawn.guo@linaro.org> > > > > > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > > --- > > > > > Changes for v5: > > > > > - None > > > > > > > > > > Changes for v4: > > > > > - None > > > > > > > > > > Changes for v3: > > > > > - Adjust the code to work with ipg clock removed > > > > > > > > > > Changes for v2: > > > > > - New patch > > > > > --- > > > > > drivers/pwm/pwm-imx.c | 17 ++++++++++++++--- > > > > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > > > > > index b1d1e50..0fa480d 100644 > > > > > --- a/drivers/pwm/pwm-imx.c > > > > > +++ b/drivers/pwm/pwm-imx.c > > > > > @@ -239,7 +239,14 @@ static void imx_pwm_disable(struct > > > > > pwm_chip *chip, struct pwm_device *pwm) > > > > > clk_disable_unprepare(imx->clk_per); } > > > > > > > > > > -static struct pwm_ops imx_pwm_ops = { > > > > > +static struct pwm_ops imx_pwm_ops_v1 = { > > > > > + .enable = imx_pwm_enable, > > > > > + .disable = imx_pwm_disable, > > > > > + .config = imx_pwm_config, > > > > > + .owner = THIS_MODULE, > > > > > +}; > > > > > + > > > > > +static struct pwm_ops imx_pwm_ops_v2 = { > > > > > > > > Can't these two be const? No need to respin for only this, just > > > > let me know and I can make the change while applying. > > > > > > Nevermind that. I just remembered that I had picked up a patch to > > > make the original imx_pwm_ops a const and things still work fine > > > if I make both of the above const, so I just had to manually > > > apply your patch, but other than that it seems fine. Let me apply > > > the rest of this set and push out. It'd be great if you could > > > check afterwards that it's all still what you expect. > > > > I will do that. Thanks for integrating the patch series :-). > > Thanks for sticking with it. I know the initial patches for optional > polarity support have been around for years, and it took a really long > time for this all to come together. > > But I think the end result is sound and looks really good. You are welcome :-) > > The one remaining bit that I'm not 100% happy about is that the v1 > support is not atomic while the v2 support is. Here the only limitation is the lack of v1 HW. > Not a blocker, but it > looks as if it should be easy to convert over v1 as well. Any takers? > > Thierry Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On Mon, Jan 30, 2017 at 09:55:04AM +0100, Lukasz Majewski wrote: > Hi Thierry, > > > On Mon, Jan 30, 2017 at 09:36:49AM +0100, Lukasz Majewski wrote: > > > Hi Thierry, > > > > > > > On Mon, Jan 30, 2017 at 08:23:12AM +0100, Thierry Reding wrote: > > > > > On Sun, Jan 29, 2017 at 10:54:07PM +0100, Lukasz Majewski wrote: > > > > > > From: Lukasz Majewski <l.majewski@majess.pl> > > > > > > > > > > > > This patch provides separate set of pwm ops utilized by > > > > > > i.MX's PWMv1 and PWMv2. > > > > > > > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > > > > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com> > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl> > > > > > > Acked-by: Shawn Guo <shawn.guo@linaro.org> > > > > > > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > > > --- > > > > > > Changes for v5: > > > > > > - None > > > > > > > > > > > > Changes for v4: > > > > > > - None > > > > > > > > > > > > Changes for v3: > > > > > > - Adjust the code to work with ipg clock removed > > > > > > > > > > > > Changes for v2: > > > > > > - New patch > > > > > > --- > > > > > > drivers/pwm/pwm-imx.c | 17 ++++++++++++++--- > > > > > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c > > > > > > index b1d1e50..0fa480d 100644 > > > > > > --- a/drivers/pwm/pwm-imx.c > > > > > > +++ b/drivers/pwm/pwm-imx.c > > > > > > @@ -239,7 +239,14 @@ static void imx_pwm_disable(struct > > > > > > pwm_chip *chip, struct pwm_device *pwm) > > > > > > clk_disable_unprepare(imx->clk_per); } > > > > > > > > > > > > -static struct pwm_ops imx_pwm_ops = { > > > > > > +static struct pwm_ops imx_pwm_ops_v1 = { > > > > > > + .enable = imx_pwm_enable, > > > > > > + .disable = imx_pwm_disable, > > > > > > + .config = imx_pwm_config, > > > > > > + .owner = THIS_MODULE, > > > > > > +}; > > > > > > + > > > > > > +static struct pwm_ops imx_pwm_ops_v2 = { > > > > > > > > > > Can't these two be const? No need to respin for only this, just > > > > > let me know and I can make the change while applying. > > > > > > > > Nevermind that. I just remembered that I had picked up a patch to > > > > make the original imx_pwm_ops a const and things still work fine > > > > if I make both of the above const, so I just had to manually > > > > apply your patch, but other than that it seems fine. Let me apply > > > > the rest of this set and push out. It'd be great if you could > > > > check afterwards that it's all still what you expect. > > > > > > I will do that. Thanks for integrating the patch series :-). > > > > Thanks for sticking with it. I know the initial patches for optional > > polarity support have been around for years, and it took a really long > > time for this all to come together. > > > > But I think the end result is sound and looks really good. > > You are welcome :-) > > > > > The one remaining bit that I'm not 100% happy about is that the v1 > > support is not atomic while the v2 support is. > > Here the only limitation is the lack of v1 HW. That doesn't have to be a blocker. If you're willing to invest some more work to do the additional conversion (I think it would be a fairly minor change, looking at the existing v1 code), I'm sure we can find someone with the hardware to test it. Sascha, Lothar, Fabio, Shawn: do you guys have access to v1 hardware, or know of anyone who might? Thierry
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c index b1d1e50..0fa480d 100644 --- a/drivers/pwm/pwm-imx.c +++ b/drivers/pwm/pwm-imx.c @@ -239,7 +239,14 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) clk_disable_unprepare(imx->clk_per); } -static struct pwm_ops imx_pwm_ops = { +static struct pwm_ops imx_pwm_ops_v1 = { + .enable = imx_pwm_enable, + .disable = imx_pwm_disable, + .config = imx_pwm_config, + .owner = THIS_MODULE, +}; + +static struct pwm_ops imx_pwm_ops_v2 = { .enable = imx_pwm_enable, .disable = imx_pwm_disable, .config = imx_pwm_config, @@ -250,16 +257,19 @@ struct imx_pwm_data { int (*config)(struct pwm_chip *chip, struct pwm_device *pwm, int duty_ns, int period_ns); void (*set_enable)(struct pwm_chip *chip, bool enable); + struct pwm_ops *pwm_ops; }; static struct imx_pwm_data imx_pwm_data_v1 = { .config = imx_pwm_config_v1, .set_enable = imx_pwm_set_enable_v1, + .pwm_ops = &imx_pwm_ops_v1, }; static struct imx_pwm_data imx_pwm_data_v2 = { .config = imx_pwm_config_v2, .set_enable = imx_pwm_set_enable_v2, + .pwm_ops = &imx_pwm_ops_v2, }; static const struct of_device_id imx_pwm_dt_ids[] = { @@ -281,6 +291,8 @@ static int imx_pwm_probe(struct platform_device *pdev) if (!of_id) return -ENODEV; + data = of_id->data; + imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL); if (imx == NULL) return -ENOMEM; @@ -292,7 +304,7 @@ static int imx_pwm_probe(struct platform_device *pdev) return PTR_ERR(imx->clk_per); } - imx->chip.ops = &imx_pwm_ops; + imx->chip.ops = data->pwm_ops; imx->chip.dev = &pdev->dev; imx->chip.base = -1; imx->chip.npwm = 1; @@ -303,7 +315,6 @@ static int imx_pwm_probe(struct platform_device *pdev) if (IS_ERR(imx->mmio_base)) return PTR_ERR(imx->mmio_base); - data = of_id->data; imx->config = data->config; imx->set_enable = data->set_enable;