RE: [RESEND PATCH V11 3/3] Input: new da7280 haptic driver

[Roy Im]

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