Hello, from the PWM POV I'm happy now. Just a few minor comments that I noticed while checking the PWM details. On Thu, Jun 25, 2020 at 01:59:29AM +0900, Roy Im wrote: > + val = haptics->ps_seq_id << DA7280_PS_SEQ_ID_SHIFT | > + haptics->ps_seq_loop << DA7280_PS_SEQ_LOOP_SHIFT; If you write this as: val = FIELD_PREP(DA7280_PS_SEQ_ID_MASK, haptics->ps_seq_id) | FIELD_PREP(DA7280_PS_SEQ_LOOP_MASK, haptics->ps_seq_loop); you get some additional checks for free and can drop all defines for ..._SHIFT . > +static u8 da7280_haptic_of_gpi_pol_str(struct device *dev, > + const char *str) > +{ > + if (!strcmp(str, "Rising-edge")) > + return 0; > + else if (!strcmp(str, "Falling-edge")) > + return 1; > + else if (!strcmp(str, "Both-edge")) > + return 2; > + > + dev_warn(dev, "Invalid string - set to default\n"); Maybe mention "Rising-edge" being the default? > + return 0; > +} Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature