| Message ID | 20210924021225.846197-1-bjorn.andersson@linaro.org |
|---|---|
| State | Accepted |
| Commit | 3ab7b6ac5d829e60c3b89d415811ff1c9f358c8e |
| Headers | show |
| Series | [v5,1/2] pwm: Introduce single-PWM of_xlate function | expand |
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/ |
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 --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,
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