Re: [PATCH v8 0/2] Add driver for Sharp Memory LCD

[Uwe Kleine-König]

Hello Alex,

On Wed, Oct 02, 2024 at 10:33:13PM -0400, Alex Lanzano wrote:
> On Wed, Oct 02, 2024 at 09:56:38AM GMT, Uwe Kleine-König wrote:
> > On Tue, Oct 01, 2024 at 11:37:35PM -0400, Alex Lanzano wrote:
> > > Changes in v8:
> > > - Addressed review comments from Uwe
> > >     - Replace pwm_get_state with pwm_init_state
> > >     - Use pwm_set_relative_duty_cycle instead of manually setting period 
> > > and duty cycle
> > 
> > You didn't explicitly mention that it's fine if the PWM doesn't emit the
> > inactive state when you call pwm_disable(). You're code should continue
> > to work if you drop all calls to pwm_disable().
> > 
> > Ideally you mention that in a code comment to make others reading your
> > code understand that.
> 
> Sorry about that! The intent of the code is to stop the pwm from outputing
> when the display is disabled since the signal is no longer needed. If
> it's best to emit the inactive state rather than calling pwm_disable()
> I'm fine with making that change.

Calling pwm_disable() is best iff you don't care about the output any
more. If however you rely on it to emit the inactive level,
pwm_disable() is wrong. I don't know enough about your display to judge
from here.

The code to disable the display looks (simplified) as follows:

        if (smd->enable_gpio)
                gpiod_set_value(smd->enable_gpio, 0);

        pwm_disable(smd->pwm_vcom_signal);

so maybe the logic you need is:

        if (smd->enable_gpio) {
                gpiod_set_value(smd->enable_gpio, 0);

                /*
                 * In the presence of a GPIO to disable the display the
                 * behaviour of the PWM doesn't matter and we can
                 * just disable it.
                 */
                pwm_disable(smd->pwm_vcom_signal);
        } else {
                struct pwm_state state;

                /*
                 * However without a GPIO driving the display's output
                 * enable pin the PWM must emit the inactive level,
                 * which isn't guaranteed when calling pwm_disable(), so
                 * configure it for duty_cycle = 0.
                 */
                 pwm_init_state(smd->pwm_vcom_signal, &state);
                 state.duty_cycle = 0;
                 state.enabled = true;
                 pwm_apply_might_sleep(smd->pwm_vcom_signal, &state);
        }

?

Best regards
Uwe

signature.asc
Description: PGP signature