Hi,
On Tue, Jun 22, 2021 at 8:28 PM Bjorn Andersson
<bjorn.andersson@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>
> 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
