| Message ID | 20240905124432.834831-1-lanzano.alex@gmail.com |
|---|---|
| Headers | show |
| Series | Add driver for Sharp Memory LCD | expand |
Hi all, Just a friendly ping. Please send feedback whenever possible. Best regards, Alex On Thu, Sep 05, 2024 at 08:43:58AM GMT, 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/71a9dbf4609dbba46026a31f60261830163a0b99.1701267411.git.mehdi.djait@bootlin.com/ > https://www.sharpsde.com/fileadmin/products/Displays/2016_SDE_App_Note_for_Memory_LCD_programming_V1.3.pdf > > Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com> > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com> > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> > --- > 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 | 682 ++++++++++++++++++ > 5 files changed, 801 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml > create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c > > -- > 2.46.0 >
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. > + pwm_state.period = 1000000000; > + pwm_state.duty_cycle = 100000000; Unusual indention. 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. > + 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. > + 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, } > + > + return 0; > +} Best regards Uwe
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