diff mbox series

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

Message ID 20210924021225.846197-1-bjorn.andersson@linaro.org
State New
Headers show
Series [v5,1/2] pwm: Introduce single-PWM of_xlate function | expand

Commit Message

Bjorn Andersson Sept. 24, 2021, 2:12 a.m. UTC
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 v4:
- 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.32.0

Comments

Uwe Kleine-König Sept. 24, 2021, 7:16 a.m. UTC | #1
On Thu, Sep 23, 2021 at 09:12:24PM -0500, Bjorn Andersson 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 v4:

> - 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 4527f09a5c50..2c6b155002a2 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;


of_pwm_xlate_with_flags is a bit more complicated. Translating
accordingly this would yield:

	if (pc->of_pwm_n_cells >= 2) {
		if (args->args_count > 1 && args->args[1] & PWM_POLARITY_INVERTED)
			pwm->args.polarity = PWM_POLARITY_INVERSED;
	}

Given that pc->of_pwm_n_cells isn't used when a phandle is parsed (in
of_pwm_get()) I think your variant is fine.

So I think technically the patch is good, for me the question is if we
want to make new drivers of_pwm_xlate_with_flags for consistency even
though this would mean that the first argument has to be 0 for all
phandles. Thierry? Lee?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Bjorn Andersson Sept. 24, 2021, 2:30 p.m. UTC | #2
On Fri 24 Sep 00:16 PDT 2021, Uwe Kleine-K?nig wrote:

> On Thu, Sep 23, 2021 at 09:12:24PM -0500, Bjorn Andersson 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 v4:

> > - 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 4527f09a5c50..2c6b155002a2 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;

> 

> of_pwm_xlate_with_flags is a bit more complicated. Translating

> accordingly this would yield:

> 

> 	if (pc->of_pwm_n_cells >= 2) {

> 		if (args->args_count > 1 && args->args[1] & PWM_POLARITY_INVERTED)

> 			pwm->args.polarity = PWM_POLARITY_INVERSED;

> 	}

> 

> Given that pc->of_pwm_n_cells isn't used when a phandle is parsed (in

> of_pwm_get()) I think your variant is fine.

> 


Right, the difference from of_pwm_xlate_with_flags is that this version
will pick up the flags even if the driver says it has n_cells = 1.

I didn't see a strong reason for doing the extra check and the drawback
with it is that if I then write in my dts that my channel should be
INVERTED the driver won't be able to bump the n_cells to 2, because that
would cause a regression.

Would you like me to add this extra check? Or perhaps ensure that the
commit message captures my reasoning here?

> So I think technically the patch is good, for me the question is if we

> want to make new drivers of_pwm_xlate_with_flags for consistency even

> though this would mean that the first argument has to be 0 for all

> phandles. Thierry? Lee?

> 


I find it typical for single entity providers to be defined with
#foo-cells = <0> (or 1 if you have flags) and not pass a "dummy" 0.

We did talk about this with Rob in a previous version of this patch and
came to the conclusion that this was the appropriate thing to do...

Thanks,
Bjorn
diff mbox series

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 4527f09a5c50..2c6b155002a2 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 a9efdcf839ae..238ec88c130b 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)
 	pc->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
 
 	if (IS_ENABLED(CONFIG_OF)) {
-		pc->chip.of_xlate = pxa_pwm_of_xlate;
+		pc->chip.of_xlate = of_pwm_single_xlate;
 		pc->chip.of_pwm_n_cells = 1;
 	}
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 725c9b784e60..dd51d4931fdc 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -414,6 +414,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,