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
