[v8,0/2] Add driver for Sharp Memory LCD

Message

Alex Lanzano Oct. 2, 2024, 3:37 a.m. UTC

This patch series add support for the monochrome Sharp Memory LCD
panels. This series is based off of the work done by Mehdi Djait.

References:
https://lore.kernel.org/dri-devel/71a[e-mail]/
https://www.sharpsde.com/fileadmin/products/Displays/2016_SDE_App_Note_for_Memory_LCD_programming_V1.3.pdf

Co-developed-by: Mehdi Djait <[e-mail]>
Signed-off-by: Mehdi Djait <[e-mail]>
Signed-off-by: Alex Lanzano <[e-mail]>
---
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

Changes in v7:
- Add Reviewed-by tag back to dt-binding patch

Changes in v6:
- Rebase off latest drm-misc-next
- Replace pwm_apply_state with pwm_apply_might_sleep

Changes in v5:
- Address minor style issues in sharp-memory.c

Changes in v4:
- Remove redundant dev_err

Changes in v3:
- Fix file path in MAINTAINERS file
- Address review comments
- Simplify mode selection based on match data instead of model

Changes in v2:
- Credited Mehdi Djait in commit messages
- Renamed sharp,sharp-memory.yaml to sharp,ls010b7dh04.yaml
- Using strings instead of int for vcom-mode in dt-binding
- Fixed indentation of binding example
- Removed binding header
- Removed extra whitespace in sharp-memory.c
- Fixed error handling in sharp-memory.c
- Added match data to of_device_id table to be in-sync with spi_device_id table
- Replaced redundant function with spi_get_device_match_data
- Sorted header files in sharp-memory.c
---

Alex Lanzano (2):
  dt-bindings: display: Add Sharp Memory LCD bindings
  drm/tiny: Add driver for Sharp Memory LCD

 .../bindings/display/sharp,ls010b7dh04.yaml   |  92 +++
 MAINTAINERS                                   |   6 +
 drivers/gpu/drm/tiny/Kconfig                  |  20 +
 drivers/gpu/drm/tiny/Makefile                 |   1 +
 drivers/gpu/drm/tiny/sharp-memory.c           | 681 ++++++++++++++++++
 5 files changed, 800 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
 create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c

Comments

Uwe Kleine-König Oct. 2, 2024, 7:56 a.m. UTC | #1

Hello,

On Tue, Oct 01, 2024 at 11:37:35PM -0400, Alex Lanzano wrote:
> This patch series add support for the monochrome Sharp Memory LCD
> panels. This series is based off of the work done by Mehdi Djait.
> 
> References:
> https://lore.kernel.org/dri-devel/71a[e-mail]/
> https://www.sharpsde.com/fileadmin/products/Displays/2016_SDE_App_Note_for_Memory_LCD_programming_V1.3.pdf
> 
> Co-developed-by: Mehdi Djait <[e-mail]>
> Signed-off-by: Mehdi Djait <[e-mail]>
> Signed-off-by: Alex Lanzano <[e-mail]>
> ---
> 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.

Best regards
Uwe

Alex Lanzano Oct. 3, 2024, 2:33 a.m. UTC | #2

On Wed, Oct 02, 2024 at 09:56:38AM GMT, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Oct 01, 2024 at 11:37:35PM -0400, Alex Lanzano wrote:
> > This patch series add support for the monochrome Sharp Memory LCD
> > panels. This series is based off of the work done by Mehdi Djait.
> > 
> > References:
> > https://lore.kernel.org/dri-devel/71a[e-mail]/
> > https://www.sharpsde.com/fileadmin/products/Displays/2016_SDE_App_Note_for_Memory_LCD_programming_V1.3.pdf
> > 
> > Co-developed-by: Mehdi Djait <[e-mail]>
> > Signed-off-by: Mehdi Djait <[e-mail]>
> > Signed-off-by: Alex Lanzano <[e-mail]>
> > ---
> > 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.

Best regards,
Alex

Uwe Kleine-König Oct. 3, 2024, 9:27 a.m. UTC | #3

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

Alex Lanzano Oct. 3, 2024, 3:45 p.m. UTC | #4

On Thu, Oct 03, 2024 at 11:27:43AM GMT, Uwe Kleine-König wrote:
> 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);
> 	}
> 

Ahh, understood. I looked over the datasheet again. It looks like in the
case where no enable gpio pin is defined we'll need to keep the pwm pulsing
since display hardware would still be enabled.

I'll fix this up in the disable and probe functions.

Best regards,
Alex