Hi thanks for the review! I'll address these in v8. Looks like you
missed my v7 of this patch
On Wed, Sep 25, 2024 at 11:07:00PM GMT, Uwe Kleine-König wrote:
> Hello,
>
> On Thu, Sep 05, 2024 at 08:44:00AM -0400, Alex Lanzano wrote:
> > +static void sharp_memory_crtc_enable(struct drm_crtc *crtc,
> > + struct drm_atomic_state *state)
> > +{
> > + struct pwm_state pwm_state;
> > + struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
> > +
> > + sharp_memory_clear_display(smd);
> > +
> > + if (smd->enable_gpio)
> > + gpiod_set_value(smd->enable_gpio, 1);
> > +
> > + switch (smd->vcom_mode) {
> > + case SHARP_MEMORY_SOFTWARE_VCOM:
> > + smd->sw_vcom_signal = kthread_run(sharp_memory_sw_vcom_signal_thread,
> > + smd, "sw_vcom_signal");
> > + break;
> > +
> > + case SHARP_MEMORY_EXTERNAL_VCOM:
> > + break;
> > +
> > + case SHARP_MEMORY_PWM_VCOM:
> > + pwm_get_state(smd->pwm_vcom_signal, &pwm_state);
>
> I'd prefer using pwm_init_state() here instead of pwm_get_state(), The
> former only depends on machine description (probably device tree), the
> latter depends on what happend before to the PWM. While it probably
> doesn't make a difference in practise, the former is more deterministic.
>
Will fix in v8.
> > + pwm_state.period = 1000000000;
> > + pwm_state.duty_cycle = 100000000;
>
> Unusual indention.
>
Will fix
> The device tree (and also ACPI) defines a default period for a PWM. If
> you used pwm_init_state() -- as suggested above -- you could just use
> pwm_set_relative_duty_cycle(smd->pwm_vcom_signal, 1, 10); here.
>
Will fix
> > + pwm_state.enabled = true;
> > + pwm_apply_might_sleep(smd->pwm_vcom_signal, &pwm_state);
> > + break;
> > + }
> > +}
> > +
> > +static void sharp_memory_crtc_disable(struct drm_crtc *crtc,
> > + struct drm_atomic_state *state)
> > +{
> > + struct sharp_memory_device *smd = drm_to_sharp_memory_device(crtc->dev);
> > +
> > + sharp_memory_clear_display(smd);
> > +
> > + if (smd->enable_gpio)
> > + gpiod_set_value(smd->enable_gpio, 0);
> > +
> > + switch (smd->vcom_mode) {
> > + case SHARP_MEMORY_SOFTWARE_VCOM:
> > + kthread_stop(smd->sw_vcom_signal);
> > + break;
> > +
> > + case SHARP_MEMORY_EXTERNAL_VCOM:
> > + break;
> > +
> > + case SHARP_MEMORY_PWM_VCOM:
> > + pwm_disable(smd->pwm_vcom_signal);
>
> What is the objective here? Do you want to save energy and don't care
> about the output? Or do you want the PWM to emit the inactive level?
> Note that for the second case, pwm_disable() is wrong, as depending on
> the underlying hardware the PWM might continue to toggle or emit a
> constant active level.
>
I want the PWM to stop emitting to save energy.
> > + break;
> > + }
> > +}
> > +
> > [...]
> > +
> > +static int sharp_memory_init_pwm_vcom_signal(struct sharp_memory_device *smd)
> > +{
> > + struct pwm_state state;
> > + struct device *dev = &smd->spi->dev;
> > +
> > + smd->pwm_vcom_signal = devm_pwm_get(dev, NULL);
> > + if (IS_ERR(smd->pwm_vcom_signal))
> > + return dev_err_probe(dev, -EINVAL, "Could not get pwm device\n");
> > +
> > + pwm_init_state(smd->pwm_vcom_signal, &state);
> > + state.enabled = false;
> > + pwm_apply_might_sleep(smd->pwm_vcom_signal, &state);
>
> Same question as above. If you care about the output level, use
>
> {
> .period = ...,
> .duty_cycle = 0,
> .enabled = true,
> }
>
See answer above!
> > +
> > + return 0;
> > +}
>
> Best regards
> Uwe
