[v4,1/2] pwm: Introduce single-PWM of_xlate function

Message ID 20210623032755.1170809-1-bjorn.andersson@linaro.org
State New
Headers show
Series
  • [v4,1/2] pwm: Introduce single-PWM of_xlate function
Related show

Commit Message

Bjorn Andersson June 23, 2021, 3:27 a.m.
The existing pxa driver and the upcoming addition of PWM support in the
TI sn565dsi86 DSI/eDP bridge driver both has a single PWM channel and
thereby a need for a of_xlate function with the period as its single
argument.

Introduce a common helper function in the core that can be used as
of_xlate by such drivers and migrate the pxa driver to use this.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

---

Changes since v3:
- None

Changes since v2:
- None

 drivers/pwm/core.c    | 26 ++++++++++++++++++++++++++
 drivers/pwm/pwm-pxa.c | 16 +---------------
 include/linux/pwm.h   |  2 ++
 3 files changed, 29 insertions(+), 15 deletions(-)

-- 
2.31.0

Comments

Doug Anderson June 23, 2021, 10:19 p.m. | #1
Hi,

On Tue, Jun 22, 2021 at 8:28 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>

> The existing pxa driver and the upcoming addition of PWM support in the

> TI sn565dsi86 DSI/eDP bridge driver both has a single PWM channel and

> thereby a need for a of_xlate function with the period as its single

> argument.

>

> Introduce a common helper function in the core that can be used as

> of_xlate by such drivers and migrate the pxa driver to use this.

>

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---

>

> Changes since v3:

> - None

>

> Changes since v2:

> - None

>

>  drivers/pwm/core.c    | 26 ++++++++++++++++++++++++++

>  drivers/pwm/pwm-pxa.c | 16 +---------------

>  include/linux/pwm.h   |  2 ++

>  3 files changed, 29 insertions(+), 15 deletions(-)

>

> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c

> index a42999f877d2..5e9c876fccc4 100644

> --- a/drivers/pwm/core.c

> +++ b/drivers/pwm/core.c

> @@ -152,6 +152,32 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)

>  }

>  EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);

>

> +struct pwm_device *

> +of_pwm_single_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)


It's probably up to PWM folks, but to make it symmetric to
of_pwm_xlate_with_flags() I probably would have named it with the
"_with_flags" suffix.


> +{

> +       struct pwm_device *pwm;

> +

> +       if (pc->of_pwm_n_cells < 1)

> +               return ERR_PTR(-EINVAL);

> +

> +       /* validate that one cell is specified, optionally with flags */

> +       if (args->args_count != 1 && args->args_count != 2)

> +               return ERR_PTR(-EINVAL);


I don't know all the rules for attempted forward compatibility, but
unless there's a strong reason I'd expect to match the rules for
of_pwm_xlate_with_flags(). That function doesn't consider it to be an
error if either "pc->of_pwm_n_cells" or "args->args_count" is bigger
than you need. Unless there's a reason to be inconsistent, it seems
like we should be consistent between the two functions. That would
make the test:

if (args->args_count < 1)
  return ERR_PTR(-EINVAL);


> +

> +       pwm = pwm_request_from_chip(pc, 0, NULL);

> +       if (IS_ERR(pwm))

> +               return pwm;

> +

> +       pwm->args.period = args->args[0];

> +       pwm->args.polarity = PWM_POLARITY_NORMAL;

> +

> +       if (args->args_count == 2 && args->args[2] & PWM_POLARITY_INVERTED)


Similar to above, should this be ">= 2" rather than "== 2" ?

I also notice that in commit cf38c978cf1d ("pwm: Make
of_pwm_xlate_with_flags() work with #pwm-cells = <2>") Uwe added a
check for "pc->of_pwm_n_cells" in of_pwm_xlate_with_flags() right
around here. You're not checking it in your function.

I _think_ your code is fine because I can't see how "args->args_count"
could ever be greater than "pc->of_pwm_n_cells" but maybe I'm not
seeing something. Assuming your code is correct then maybe the right
thing to do is to remove the extra check from
of_pwm_xlate_with_flags() to make the two functions more similar.


-Doug
Bjorn Andersson June 24, 2021, 2:10 a.m. | #2
On Wed 23 Jun 17:19 CDT 2021, Doug Anderson wrote:

> Hi,

> 

> On Tue, Jun 22, 2021 at 8:28 PM Bjorn Andersson

> <bjorn.andersson@linaro.org> wrote:

> >

> > The existing pxa driver and the upcoming addition of PWM support in the

> > TI sn565dsi86 DSI/eDP bridge driver both has a single PWM channel and

> > thereby a need for a of_xlate function with the period as its single

> > argument.

> >

> > Introduce a common helper function in the core that can be used as

> > of_xlate by such drivers and migrate the pxa driver to use this.

> >

> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> > ---

> >

> > Changes since v3:

> > - None

> >

> > Changes since v2:

> > - None

> >

> >  drivers/pwm/core.c    | 26 ++++++++++++++++++++++++++

> >  drivers/pwm/pwm-pxa.c | 16 +---------------

> >  include/linux/pwm.h   |  2 ++

> >  3 files changed, 29 insertions(+), 15 deletions(-)

> >

> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c

> > index a42999f877d2..5e9c876fccc4 100644

> > --- a/drivers/pwm/core.c

> > +++ b/drivers/pwm/core.c

> > @@ -152,6 +152,32 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)

> >  }

> >  EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);

> >

> > +struct pwm_device *

> > +of_pwm_single_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)

> 

> It's probably up to PWM folks, but to make it symmetric to

> of_pwm_xlate_with_flags() I probably would have named it with the

> "_with_flags" suffix.

> 


I don't see a reason for having the no-flags variant of this, but you're
right in that it does look more uniform.

> 

> > +{

> > +       struct pwm_device *pwm;

> > +

> > +       if (pc->of_pwm_n_cells < 1)

> > +               return ERR_PTR(-EINVAL);

> > +

> > +       /* validate that one cell is specified, optionally with flags */

> > +       if (args->args_count != 1 && args->args_count != 2)

> > +               return ERR_PTR(-EINVAL);

> 

> I don't know all the rules for attempted forward compatibility, but

> unless there's a strong reason I'd expect to match the rules for

> of_pwm_xlate_with_flags(). That function doesn't consider it to be an

> error if either "pc->of_pwm_n_cells" or "args->args_count" is bigger

> than you need. Unless there's a reason to be inconsistent, it seems

> like we should be consistent between the two functions. That would

> make the test:

> 

> if (args->args_count < 1)

>   return ERR_PTR(-EINVAL);

> 


My crystal ball is foggy, but I guess I could follow suite even though I
don't see what that might be.

> 

> > +

> > +       pwm = pwm_request_from_chip(pc, 0, NULL);

> > +       if (IS_ERR(pwm))

> > +               return pwm;

> > +

> > +       pwm->args.period = args->args[0];

> > +       pwm->args.polarity = PWM_POLARITY_NORMAL;

> > +

> > +       if (args->args_count == 2 && args->args[2] & PWM_POLARITY_INVERTED)

> 

> Similar to above, should this be ">= 2" rather than "== 2" ?

> 

> I also notice that in commit cf38c978cf1d ("pwm: Make

> of_pwm_xlate_with_flags() work with #pwm-cells = <2>") Uwe added a

> check for "pc->of_pwm_n_cells" in of_pwm_xlate_with_flags() right

> around here. You're not checking it in your function.

> 

> I _think_ your code is fine because I can't see how "args->args_count"

> could ever be greater than "pc->of_pwm_n_cells" but maybe I'm not

> seeing something. Assuming your code is correct then maybe the right

> thing to do is to remove the extra check from

> of_pwm_xlate_with_flags() to make the two functions more similar.

> 


I guess the way of_pwm_xlate_with_flags() is written the optional flags
will only be considered if the driver has stated that it supports the
3rd field.

The way I wrote this means that I don't care if the drivers supports
flags I will pick up that INVERTED bit. I suppose this means that if a
driver where to increment of_pwm_n_cells we suddenly start to care about
a cell that we previously never looked at...

But it would be consistent to follow this, and I don't really have an
opinion about these nuances.

Thanks for your feedback Doug.

Regards,
Bjorn

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index a42999f877d2..5e9c876fccc4 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -152,6 +152,32 @@  of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 }
 EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
 
+struct pwm_device *
+of_pwm_single_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	if (pc->of_pwm_n_cells < 1)
+		return ERR_PTR(-EINVAL);
+
+	/* validate that one cell is specified, optionally with flags */
+	if (args->args_count != 1 && args->args_count != 2)
+		return ERR_PTR(-EINVAL);
+
+	pwm = pwm_request_from_chip(pc, 0, NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	pwm->args.period = args->args[0];
+	pwm->args.polarity = PWM_POLARITY_NORMAL;
+
+	if (args->args_count == 2 && args->args[2] & PWM_POLARITY_INVERTED)
+		pwm->args.polarity = PWM_POLARITY_INVERSED;
+
+	return pwm;
+}
+EXPORT_SYMBOL_GPL(of_pwm_single_xlate);
+
 static void of_pwmchip_add(struct pwm_chip *chip)
 {
 	if (!chip->dev || !chip->dev->of_node)
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index cfb683827d32..8cd82fb54483 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -148,20 +148,6 @@  static const struct platform_device_id *pxa_pwm_get_id_dt(struct device *dev)
 	return id ? id->data : NULL;
 }
 
-static struct pwm_device *
-pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
-{
-	struct pwm_device *pwm;
-
-	pwm = pwm_request_from_chip(pc, 0, NULL);
-	if (IS_ERR(pwm))
-		return pwm;
-
-	pwm->args.period = args->args[0];
-
-	return pwm;
-}
-
 static int pwm_probe(struct platform_device *pdev)
 {
 	const struct platform_device_id *id = platform_get_device_id(pdev);
@@ -187,7 +173,7 @@  static int pwm_probe(struct platform_device *pdev)
 	pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
 
 	if (IS_ENABLED(CONFIG_OF)) {
-		pwm->chip.of_xlate = pxa_pwm_of_xlate;
+		pwm->chip.of_xlate = of_pwm_single_xlate;
 		pwm->chip.of_pwm_n_cells = 1;
 	}
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 5a73251d28e3..6aff1fa4fe5d 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -411,6 +411,8 @@  struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 
 struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc,
 		const struct of_phandle_args *args);
+struct pwm_device *of_pwm_single_xlate(struct pwm_chip *pc,
+				       const struct of_phandle_args *args);
 
 struct pwm_device *pwm_get(struct device *dev, const char *con_id);
 struct pwm_device *of_pwm_get(struct device *dev, struct device_node *np,