[PATCH v6 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Alex Lanzano]

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