Patchwork [PATCHv2,2/2] pwm: imx: support polarity inversion

login
register
mail settings
Submitter Russell King - ARM Linux
Date Jan. 23, 2014, 11:52 a.m.
Message ID <20140123115203.GV15937@n2100.arm.linux.org.uk>
Download mbox | patch
Permalink /patch/313569/
State New
Headers show

Comments

Russell King - ARM Linux - Jan. 23, 2014, 11:52 a.m.
On Thu, Jan 23, 2014 at 08:37:14AM +0100, Lothar Waßmann wrote:
> This wouldn't buy much without a material change to of_pwm_get().
> The function of_parse_phandle_with_args() called by of_pwm_get()
> requires the number of args in the pwms property be greater or equal to
> the #pwm-cells property in the pwm node. Thus, the interesting case of
> having #pwm-cells = <3> without changing the existing users is
> prohibited by of_parse_phandle_with_args().

I really don't think that's a problem we need to be concerned with at
the moment.  What we need is for the kernel to be able to parse files
with #pwm-cells = <2> with the pwms property containing two arguments,
and when they're updated to #pwm-cells = <3> with the pwms property
containing three arguments.

Yes, that means all the board dt files need to be updated at the same
time to include the additional argument, but I don't see that as a big
problem.

What we do need to do is to adjust the PWM parsing code such that it's
possible to use either specification without causing any side effects.

I would test this, but as u-boot is rather fscked at the moment and the
networking has broken on my cubox-i as a result... and it seems that the
u-boot developers have pissed off cubox-i u-boot hackers soo much that
they've dropped u-boot in favour of barebox...

 drivers/pwm/core.c  | 59 +++++++++++++++++++++++++++++------------------------
 include/linux/pwm.h |  2 ++
 2 files changed, 34 insertions(+), 27 deletions(-)
Russell King - ARM Linux - Jan. 23, 2014, 12:33 p.m.
On Thu, Jan 23, 2014 at 11:52:03AM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 23, 2014 at 08:37:14AM +0100, Lothar Waßmann wrote:
> > This wouldn't buy much without a material change to of_pwm_get().
> > The function of_parse_phandle_with_args() called by of_pwm_get()
> > requires the number of args in the pwms property be greater or equal to
> > the #pwm-cells property in the pwm node. Thus, the interesting case of
> > having #pwm-cells = <3> without changing the existing users is
> > prohibited by of_parse_phandle_with_args().
> 
> I really don't think that's a problem we need to be concerned with at
> the moment.  What we need is for the kernel to be able to parse files
> with #pwm-cells = <2> with the pwms property containing two arguments,
> and when they're updated to #pwm-cells = <3> with the pwms property
> containing three arguments.
> 
> Yes, that means all the board dt files need to be updated at the same
> time to include the additional argument, but I don't see that as a big
> problem.
> 
> What we do need to do is to adjust the PWM parsing code such that it's
> possible to use either specification without causing any side effects.
> 
> I would test this, but as u-boot is rather fscked at the moment and the
> networking has broken on my cubox-i as a result... and it seems that the
> u-boot developers have pissed off cubox-i u-boot hackers soo much that
> they've dropped u-boot in favour of barebox...

Oh, and another reason... the u-boot video settings are totally and utterly
buggered to the point that it doesn't produce correct timings, and it seems
that u-boot people have zero interest in fixing that, so u-boot mainline is
basically refusing to fix this - another reason to stay away from it.

(1024x768 @ 60Hz produces 70Hz refresh on iMX6Q here - I've seen it produce
51Hz on iMX6S, both of which are far enough out that lots of display devices
will not accept it as a valid signal.)
Russell King - ARM Linux - Jan. 23, 2014, 4:44 p.m.
On Thu, Jan 23, 2014 at 11:52:03AM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 23, 2014 at 08:37:14AM +0100, Lothar Waßmann wrote:
> > This wouldn't buy much without a material change to of_pwm_get().
> > The function of_parse_phandle_with_args() called by of_pwm_get()
> > requires the number of args in the pwms property be greater or equal to
> > the #pwm-cells property in the pwm node. Thus, the interesting case of
> > having #pwm-cells = <3> without changing the existing users is
> > prohibited by of_parse_phandle_with_args().
> 
> I really don't think that's a problem we need to be concerned with at
> the moment.  What we need is for the kernel to be able to parse files
> with #pwm-cells = <2> with the pwms property containing two arguments,
> and when they're updated to #pwm-cells = <3> with the pwms property
> containing three arguments.
> 
> Yes, that means all the board dt files need to be updated at the same
> time to include the additional argument, but I don't see that as a big
> problem.
> 
> What we do need to do is to adjust the PWM parsing code such that it's
> possible to use either specification without causing any side effects.
> 
> I would test this, but as u-boot is rather fscked at the moment and the
> networking has broken on my cubox-i as a result... and it seems that the
> u-boot developers have pissed off cubox-i u-boot hackers soo much that
> they've dropped u-boot in favour of barebox...

Okay, finally confirmed that this works with #pwm-cells = 2.

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 2ca95042a0b9..40adbce8ef0c 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -132,14 +132,11 @@  static int pwm_device_request(struct pwm_device *pwm, const char *label)
 }
 
 struct pwm_device *
-of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
+of_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
-	if (pc->of_pwm_n_cells < 3)
-		return ERR_PTR(-EINVAL);
-
-	if (args->args[0] >= pc->npwm)
+	if (args->args_count < 2)
 		return ERR_PTR(-EINVAL);
 
 	pwm = pwm_request_from_chip(pc, args->args[0], NULL);
@@ -148,33 +145,45 @@  of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 
 	pwm_set_period(pwm, args->args[1]);
 
-	if (args->args[2] & PWM_POLARITY_INVERTED)
-		pwm_set_polarity(pwm, PWM_POLARITY_INVERSED);
-	else
-		pwm_set_polarity(pwm, PWM_POLARITY_NORMAL);
+	if (args->args_count > 2) {
+		int err;
+
+		if (args->args[2] & PWM_POLARITY_INVERTED)
+			err = pwm_set_polarity(pwm, PWM_POLARITY_INVERSED);
+		else
+			err = pwm_set_polarity(pwm, PWM_POLARITY_NORMAL);
+
+		pwm_put(pwm);
+		return ERR_PTR(err);
+	}
 
 	return pwm;
 }
+EXPORT_SYMBOL(of_pwm_xlate);
+
+struct pwm_device *
+of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
+{
+	if (pc->of_pwm_n_cells < 3)
+		return ERR_PTR(-EINVAL);
+
+	if (args->args_count != pc->of_pwm_n_cells)
+		return ERR_PTR(-EINVAL);
+
+	return of_pwm_xlate(pc, args);
+}
 EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
 
 static struct pwm_device *
 of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 {
-	struct pwm_device *pwm;
-
 	if (pc->of_pwm_n_cells < 2)
 		return ERR_PTR(-EINVAL);
 
-	if (args->args[0] >= pc->npwm)
+	if (args->args_count != pc->of_pwm_n_cells)
 		return ERR_PTR(-EINVAL);
 
-	pwm = pwm_request_from_chip(pc, args->args[0], NULL);
-	if (IS_ERR(pwm))
-		return pwm;
-
-	pwm_set_period(pwm, args->args[1]);
-
-	return pwm;
+	return of_pwm_xlate(pc, args);
 }
 
 static void of_pwmchip_add(struct pwm_chip *chip)
@@ -536,16 +545,12 @@  struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
 		goto put;
 	}
 
-	if (args.args_count != pc->of_pwm_n_cells) {
-		pr_debug("%s: wrong #pwm-cells for %s\n", np->full_name,
-			 args.np->full_name);
-		pwm = ERR_PTR(-EINVAL);
-		goto put;
-	}
-
 	pwm = pc->of_xlate(pc, &args);
-	if (IS_ERR(pwm))
+	if (IS_ERR(pwm)) {
+		pr_debug("%s: of_xlate failed for %s: %d\n", np->full_name,
+			 args.np->full_name, (int)PTR_ERR(pwm));
 		goto put;
+	}
 
 	/*
 	 * If a consumer name was not given, try to look it up from the
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index f0feafd184a0..14a823f77c31 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -188,6 +188,8 @@  struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 					 unsigned int index,
 					 const char *label);
 
+struct pwm_device *of_pwm_xlate(struct pwm_chip *pc,
+		const struct of_phandle_args *args);
 struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc,
 		const struct of_phandle_args *args);