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@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.
>
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
