linux-kernel - RE: [PATCH V9 3/3] Input: new da7280 haptic driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <VE1PR10MB30859CB04A6F9DB58D921F7485E90@VE1PR10MB3085.EURPRD10.PROD.OUTLOOK.COM>
Date:   Sat, 29 Feb 2020 00:59:20 +0000
From:   Roy Im <roy.im.opensource@...semi.com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Roy Im <roy.im.opensource@...semi.com>
CC:     Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Brian Masney <masneyb@...tation.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Lee Jones <lee.jones@...aro.org>, Luca Weiss <luca@...tu.xyz>,
        Maximilian Luz <luzmaximilian@...il.com>,
        Pascal PAILLET-LME <p.paillet@...com>,
        Rob Herring <robh@...nel.org>,
        Samuel Ortiz <sameo@...ux.intel.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Support Opensource <Support.Opensource@...semi.com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>
Subject: RE: [PATCH V9 3/3] Input: new da7280 haptic driver

On Thursday, Feb 27, 2020 at 1:13 AM, Uwe Kleine-König wrote: 

> On Fri, Feb 21, 2020 at 04:27:05PM +0900, Roy Im wrote:
> > Adds support for the Dialog DA7280 LRA/ERM Haptic Driver with multiple
> > mode and integrated waveform memory and wideband support.
> > It communicates via an I2C bus to the device.
> >
> > Signed-off-by: Roy Im <roy.im.opensource@...semi.com>
> >
> > ---
> > v9:
> > 	- Removed the header file and put the definitions into the c file.
> > 	- Updated the pwm code and error logs with %pE
> > +
> > +struct da7280_haptic {
> > +	struct regmap *regmap;
> > +	struct input_dev *input_dev;
> > +	struct device *dev;
> > +	struct i2c_client *client;
> > +	struct pwm_device *pwm_dev;
> > +
> > +	bool legacy;
> > +	int pwm_id;
> 
> pwm_id is unused?

Okay, I will remove this.

[...]
> > +static int da7280_haptic_set_pwm(struct da7280_haptic *haptics, bool
> > +enabled) {
> > +	struct pwm_state state;
> > +	u64 period_mag_multi;
> > +	int error;
> > +
> > +	if (!haptics->gain) {
> > +		dev_err(haptics->dev,
> > +			"Please set the gain first for the pwm mode\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	pwm_get_state(haptics->pwm_dev, &state);
> > +	state.enabled = enabled;
> > +	period_mag_multi = enabled ? state.period * haptics->gain : 0;
> 
> if .enabled is false .duty_cycle has no effect on the output. So there is no good reason for setting the duty_cyle to
> something different for a disabled PWM.

Okay, I will add a condition for this.

> > +	state.duty_cycle  = (haptics->acc_en) ?
> > +		(unsigned int)(period_mag_multi >> 15) :
> > +		(unsigned int)((period_mag_multi >> 15)
> > +			+ state.period) / 2;
> 
> This can be written in a more readable way:
> 
> 	/* would be great to have a symbolic name for 15 */
> 	period_mag_multi >>= 15;
> 
> 	/* comment with a rational here */
> 	if (!haptics->acc_en) {
> 		period_mag_multi += state.period;
> 		period_mag_multi /= 2;
> 	}
> 
> 	state.duty_cycle = period_mag_multi;

Okay, thanks. I have tried to update that as below.
Could I get your comment if you still see anything on this?

	/* Maximum gain is 0x7fff for PWM mode */
	#define MAX_MAGNITUDE_SHIFT		15
       [...]
	period_mag_multi >>= MAX_MAGNITUDE_SHIFT;

	/* The interpretation of duty cycle depends on the acc_en,
	* it should be from 50% to 100% for acc_en = 0.
	* See datasheet 'PWM mode' section for more details.
	*/
	if (!haptics->acc_en) {
		period_mag_multi += state.period;
		period_mag_multi /= 2;
	}

> > [...]
> > +		pwm_init_state(haptics->pwm_dev, &state);
> > +		state.enabled = false;
> > +		error = pwm_apply_state(haptics->pwm_dev, &state);
> > +		if (error) {
> > +			dev_err(dev,
> > +				"failed to apply initial PWM state: %pE\n",
> > +				ERR_PTR(error));
> > +			return error;
> > +		}
> > +
> > +		/* Check PWM Period, it must be in 10k ~ 250kHz */
> > +		period2freq = 1000000 / pwm_get_period(haptics->pwm_dev);
> 
> You already know the return value of pwm_get_period(), it's state.period.

Right, I will replace that to state.period.

> > +		if (period2freq < DA7280_MIN_PWM_FREQ_KHZ ||
> > +		    period2freq > DA7280_MAX_PWM_FREQ_KHZ) {
> > [...]
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Hi Uwe,

Thanks a lot for your comments.

Best regards,
Roy