| Message ID | 20210423213304.1371143-1-bjorn.andersson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] pwm: Introduce single-PWM of_xlate function | expand |
Hello, On Fri, Apr 23, 2021 at 04:33:04PM -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> I'm OK with the idea as such. I'd like to see the semantic expanded a bit however such that the function can parse pwms = <&mypwm 50000>; and also pwms = <&mypwm 500000 PWM_POLARITY_INVERTED>; . You suggetion only covers the former. See https://lore.kernel.org/r/20210315111124.2475274-2-u.kleine-koenig@pengutronix.de for my first attempt to unify of_pwm_xlate_with_flags and of_pwm_simple_xlate accordingly. Best regards Uwe
On Sat 24 Apr 06:32 CDT 2021, Uwe Kleine-K?nig wrote: > Hello, > > On Fri, Apr 23, 2021 at 04:33:04PM -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> > > I'm OK with the idea as such. I'd like to see the semantic expanded a > bit however such that the function can parse > > pwms = <&mypwm 50000>; > > and also > > pwms = <&mypwm 500000 PWM_POLARITY_INVERTED>; > > . You suggetion only covers the former. One concern though is that a single-channel pwm with the optional flag would syntactically be indistinguishable from a multi-channel property without flags. Presumably the values are out of range though, so I suppose there's no problem in practice. Please let me know if you think there's any merit to this concern and I'll respin the patch accordingly. Thanks, Bjorn > > See > https://lore.kernel.org/r/20210315111124.2475274-2-u.kleine-koenig@pengutronix.de > for my first attempt to unify of_pwm_xlate_with_flags and > of_pwm_simple_xlate accordingly. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Apr 26, 2021 at 10:24:27AM -0500, Bjorn Andersson wrote: > On Sat 24 Apr 06:32 CDT 2021, Uwe Kleine-K?nig wrote: > > > Hello, > > > > On Fri, Apr 23, 2021 at 04:33:04PM -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> > > > > I'm OK with the idea as such. I'd like to see the semantic expanded a > > bit however such that the function can parse > > > > pwms = <&mypwm 50000>; > > > > and also > > > > pwms = <&mypwm 500000 PWM_POLARITY_INVERTED>; > > > > . You suggetion only covers the former. > > One concern though is that a single-channel pwm with the optional flag > would syntactically be indistinguishable from a multi-channel property > without flags. Presumably the values are out of range though, so I > suppose there's no problem in practice. I personally have no problem with it, for clk-providers it is also normal that some need an id and others don't. If we have such concerns (Thierry?) we should insist that new drivers always require an channel id (which is always 0 for single-channel PWMs). Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index c4d5c0667137..25ed23cdf0d3 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -176,6 +176,30 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) return pwm; } +struct pwm_device * +of_pwm_single_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) +{ + struct pwm_device *pwm; + + /* sanity check driver is single PWM */ + if (pc->of_pwm_n_cells != 1) + return ERR_PTR(-EINVAL); + + /* validate that one cell is specified */ + 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; + + 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 5bb90af4997e..0ac242a5b5e4 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -405,6 +405,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,
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 v1: - Included the pwm.h change in the commit... drivers/pwm/core.c | 24 ++++++++++++++++++++++++ drivers/pwm/pwm-pxa.c | 16 +--------------- include/linux/pwm.h | 2 ++ 3 files changed, 27 insertions(+), 15 deletions(-)