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

Uwe Kleine-König
2024-10-03T09:27:48Z

[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]