linux-kernel - Re: [PATCH v6 2/2] drm/tiny: Add driver for Sharp Memory LCD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <znyfkop6lm5a734hsuko56xcmeeessqrkhbjhf5iwbtllmli2k@kcwbo7fh6yah>
Date: Sun, 29 Sep 2024 15:48:11 -0400
From: Alex Lanzano <lanzano.alex@...il.com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Mehdi Djait <mehdi.djait@...tlin.com>, christophe.jaillet@...adoo.fr, dri-devel@...ts.freedesktop.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v6 2/2] drm/tiny: Add driver for Sharp Memory LCD

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