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