linux-kernel - RE: [RESEND PATCH V11 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 for Android: free password hash cracker in your pocket

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

Message-ID: <DB6PR1001MB1077F44B22FD486B04AF21B185A80@DB6PR1001MB1077.EURPRD10.PROD.OUTLOOK.COM>
Date:   Sat, 2 May 2020 15:32:04 +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>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: RE: [RESEND PATCH V11 3/3] Input: new da7280 haptic driver

Friday, May 1, 2020 4:46 AM, Uwe Kleine-König wrote:
> On Mon, Apr 27, 2020 at 09:57:12AM +0900, Roy Im wrote:
> > +	/* It is recommended to update the patterns
> > +	 * during haptic is not working in order to avoid conflict
> > +	 */
> 
> I thought only in net related code the comment style is like the above and in all other areas the /* is on a line for itself.

OK, I will change the comment to a line.

[...]
> > +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;
> 
> If enabled == false haptics->gain is unused. Does it make sense to only error out for enabled == true?

Yes, you are right and it make sense, I will add the condition(enabled == true) into that.

[...]
> > +static int da7280_probe(struct i2c_client *client,
> > +			const struct i2c_device_id *id)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct da7280_haptic *haptics;
> > +	struct input_dev *input_dev;
> > +	struct ff_device *ff;
> > +	struct pwm_state state;
> > +	unsigned int period2freq;
> 
> This variable could be local to the if body below.

Ok. That would be better. I will move it below(to Here).

> 
> > +	int error;
> > +
> > +	haptics = devm_kzalloc(dev, sizeof(*haptics), GFP_KERNEL);
> > +	if (!haptics)
> > +		return -ENOMEM;
> > +	haptics->dev = dev;
> > +
> > +	if (!client->irq) {
> > +		dev_err(dev, "No IRQ configured\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	da7280_parse_properties(dev, haptics);
> > +
> > +	if (haptics->const_op_mode == DA7280_PWM_MODE) {
		unsigned int period2freq;
Here.

> > +		haptics->pwm_dev = devm_pwm_get(dev, NULL);
> > +		if (IS_ERR(haptics->pwm_dev)) {
> > [...]

> > +	error = devm_request_threaded_irq(dev, client->irq, NULL,
> > +					  da7280_irq_handler,
> > +					  IRQF_ONESHOT,
> > +					  "da7280-haptics", haptics);
> > +	if (error)
> > +		dev_err(dev, "Failed to request IRQ : %d\n", client->irq);
> 
> I'd say emitting the error code would be helpful here, the actual irq number not so.

I will do so

> > +static struct i2c_driver da7280_driver = {
> > +	.driver		= {
> > +		.name	= "da7280",
> > +		.of_match_table = of_match_ptr(da7280_of_match),
> > +		.pm	= &da7280_pm_ops,
> > +	},
> > +	.probe	= da7280_probe,
> > +	.id_table	= da7280_i2c_id,
> 
> Inconsistent alignment of the = signs. My preference is a single space before the equal sign, but aligning them in the same
> column is another usual style.

I will align as your preference, it looks better in this case. Thanks for your comments.

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Kind regards,
Roy