On Mon, Sep 16, 2019 at 08:25:24PM +0200, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, Sep 13, 2019 at 03:57:43PM -0700, Guru Das Srinagesh wrote:
> > From: Fenglin Wu <fengl...@codeaurora.org>
> >
> > Normally, PWM channel has fixed output until software request to change
> > its settings. There are some PWM devices which their outputs could be
> > changed autonomously according to a predefined pattern programmed in
> > hardware. Add pwm_output_type enum type to identify these two different
> > PWM types and add relevant helper functions to set and get PWM output
> > types and pattern.
>
> I have problems to understand what your modulated mode does even after
> reading your commit log and the patch.
Hi Uwe,
I have posted a reply to Thierry's email in which I provide some
background and details about this patch for your review. Hopefully that
clarifies things - I can provide some more clarifications if necessary.
>
> Also you should note here what is the intended usage and add support for
> it for at least one (preferably more) drivers to make this actually
> usable.
The PWM_OUTPUT_MODULATED mode is useful for PWM peripherals like
Qualcomm's Light Pulse Generator (LPG) that can output a duty-cycle
pattern in hardware as well as output a fixed duty-cycle signal. All
this mode really does is to provide a way for drivers to carry out some
actions for the "pattern" mode of operation that are not required (or
relevant) for the "fixed" mode of operation.
>
> > Change-Id: Ia1f914a45ab4f4dd7be037a395eeb89d0e65a80e
>
> In Linux we don't use these. You're making it easier to apply your patch
> if you drop the change-id lines.
Sorry, forgot to strip these before sending out the patch.
>
> > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> > index 2389b86..ab703f2 100644
> > --- a/drivers/pwm/sysfs.c
> > +++ b/drivers/pwm/sysfs.c
> > @@ -215,11 +215,60 @@ static ssize_t capture_show(struct device *child,
> > return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
> > }
> >
> > +static ssize_t output_type_show(struct device *child,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + const struct pwm_device *pwm = child_to_pwm_device(child);
> > + const char *output_type = "unknown";
> > + struct pwm_state state;
> > +
> > + pwm_get_state(pwm, &state);
> > + switch (state.output_type) {
> > + case PWM_OUTPUT_FIXED:
> > + output_type = "fixed";
> > + break;
> > + case PWM_OUTPUT_MODULATED:
> > + output_type = "modulated";
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return snprintf(buf, PAGE_SIZE, "%s\n", output_type);
> > +}
> > +
> > +static ssize_t output_type_store(struct device *child,
> > + struct device_attribute *attr,
> > + const char *buf, size_t size)
> > +{
> > + struct pwm_export *export = child_to_pwm_export(child);
> > + struct pwm_device *pwm = export->pwm;
> > + struct pwm_state state;
> > + int ret = -EINVAL;
> > +
> > + mutex_lock(&export->lock);
> > + pwm_get_state(pwm, &state);
> > + if (sysfs_streq(buf, "fixed"))
> > + state.output_type = PWM_OUTPUT_FIXED;
> > + else if (sysfs_streq(buf, "modulated"))
> > + state.output_type = PWM_OUTPUT_MODULATED;
> > + else
> > + goto unlock;
> > +
> > + ret = pwm_apply_state(pwm, &state);
> > +unlock:
> > + mutex_unlock(&export->lock);
> > +
> > + return ret ? : size;
> > +}
>
> So in sysfs you cannot set a pattern. Doesn't that mean this makes using
> modulated mode hardly useful?
The pattern has to be set through the devicetree and not sysfs. Will
change the output type sysfs parameter to read-only.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
