| Message ID | 20210623032755.1170809-1-bjorn.andersson@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4,1/2] pwm: Introduce single-PWM of_xlate function | expand |
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
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
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,
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(-)