On Mon, Jul 24, 2017 at 10:25 AM, Andy Shevchenko <andy.shevche...@gmail.com> wrote: > On Mon, Jul 24, 2017 at 6:09 PM, Andrey Smirnov > <andrew.smir...@gmail.com> wrote: >> Add a driver for RAVE Supervisory Processor, an MCU implementing >> varoius bits of housekeeping functionality (watchdoging, backlight >> control, LED control, etc) on RAVE family of products by Zodiac >> Inflight Innovations. >> >> This driver implementes core MFD/serdev device as well as >> communication subroutines necessary for commanding the device. > > Thanks for an update! > > My (minor) comments below (most of them are related to spelling issues). > >> +#define RAVE_SP_RX_BUFFER_SIZE (RAVE_SP_MAX_DATA_SIZE + \ >> + RAVE_SP_CHECKSUM_SIZE) > > Slightly better style is
> > #define RAVE_SP_RX_BUFFER_SIZE \ > (RAVE_SP_MAX_DATA_SIZE + RAVE_SP_CHECKSUM_SIZE) > >> +#define RAVE_SP_TX_BUFFER_SIZE (RAVE_SP_STX_ETX_SIZE + \ >> + 2 * RAVE_SP_RX_BUFFER_SIZE) > > Ditto. Sure, will fix in v4. > >> +/** >> + * enum rave_sp_deframer_state - Possible state for de-framer >> + * >> + * @RAVE_SP_EXPECT_SOF: scanning data stream for start of frame >> + * marker > > One line? > >> + * @RAVE_SP_EXPECT_DATA: start of frame marker detected, collecting >> + * frame > > Ditto > >> + * @RAVE_SP_EXPECT_ESCAPED_DATA: escape character received, collecting >> + * escaped byte > > Ditto. Sure. Will do in v4. > >> + */ > >> +/** >> + * struct rave_sp_variant_cmds - Variant specific command vtable >> + * >> + * @translate: Subroutine to translate common command identifiers to >> + * device specific variants. Different generations of the >> + * ICD employed different numberical constants to > > Typo: numerical > >> + * represent same/similar commands and this function is >> + * there to address that. >> + * >> + * @get_boot_source: Variant dependent implementaion of "get boot >> + * source" operation > > Typo implementation > >> + * @set_boot_source: Variant dependent implementaion of "set boot >> + * source" operation > > Ditto. > > Field descriptions are supposed to be _short_. > >> + */ > >> +/** >> + * struct rave_sp_variant - RAVE supervisory processor core variant >> + * >> + * @checksum: Variant specific checksum implementation >> + * @cmd: Variant specific command vtable > > Decode vtable. > >> + * @init: Variant specific initialization sequence implementation >> + * @group: Attrubute group for exposed sysfs entries > > Run spell checker over the file, please. > Hmm, could've sworn I had it as a my pre-commit hook, but it looks like I didn't. Will do in v4. >> + */ > >> + * @part_number_firmware: >> + * @part_number_bootloader: >> + * @reset_reason: >> + * @copper_rev_rmb: >> + * @copper_rev_deb: >> + * @silicon_devid: >> + * @silicon_devrev: >> + * @copper_mod_rmb: >> + * @copper_mod_deb: > > ??? > That's my interpretation of you advice to describe those fields in detailed comment below. >> + * >> + * @variant: Device variant specific parameters and >> + * functions >> + * @event_notifier_list: Input event notification chain (used with >> + * corresponding input MFD cell driver) > > Make them _short_. OK, will do in v4. > >> + * @group: Attrubute group for exposed sysfs entries >> + * >> + * >> + * part_number_*, reset_reason, copper_*, and silicon_* fields are all > >> + * strings retrived from the device during device probing and made >> + * availible for later userspace consumption via sysfs > > Spell checker. > Ditto. >> + * >> + */ > >> +int devm_rave_sp_register_event_notifier(struct device *dev, >> + struct notifier_block *nb) >> +{ >> + struct rave_sp *sp = dev_get_drvdata(dev->parent); >> + struct notifier_block **rcnb; >> + int ret; >> + >> + rcnb = devres_alloc(rave_sp_unregister_event_notifier, >> + sizeof(*rcnb), GFP_KERNEL); >> + if (!rcnb) >> + return -ENOMEM; >> + >> + ret = blocking_notifier_chain_register(&sp->event_notifier_list, nb); >> + if (!ret) { >> + *rcnb = nb; >> + devres_add(dev, rcnb); >> + } else { >> + devres_free(rcnb); >> + } >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(devm_rave_sp_register_event_notifier); > > Did I miss > > EXPORT_SYMBOL_GPL(devm_rave_sp_unregister_event_notifier); > > ? No, you did not, as I mentioned in my reply for v2 to you, there's no use-case for having that function, there's only one MFD-cell driver that calls devm_rave_sp_register_event_notifier() and it does so as the last step of its probe(), so there's not going to be any users of devm_rave_sp_unregister_event_notifier(). > >> +static const char *devm_rave_sp_version(struct device *dev, const char *buf) >> +{ >> + /* >> + * NOTE: Ther format string below uses %02d to display u16 >> + * intentionally for the sake of backwards compatibility with >> + * legacy software. >> + */ >> + return devm_kasprintf(dev, GFP_KERNEL, "%02d%02d%02d.%c%c\n", >> + buf[0], le16_to_cpup((const __le16 *)&buf[1]), >> + buf[3], buf[4], buf[5]); >> +} > > One more question about le16_to_cpup() use. Is the variable in the > buffer guaranteed to be always in little endian format? > Okay, looks like it's protocol which is little endian. Ok. > > By the way, here it might be needed to call get_unaligned(). > Sure, I'll add that just in case. >> +static ssize_t >> +i2c_device_status_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + ssize_t ret; >> + struct rave_sp *sp = dev_get_drvdata(dev); >> + u8 status[2]; >> + u8 cmd[] = { >> + [0] = RAVE_SP_CMD_GET_I2C_DEVICE_STATUS, >> + [1] = 0 >> + }; >> + >> + ret = rave_sp_exec(sp, cmd, sizeof(cmd), &status, sizeof(status)); >> + if (ret < 0) >> + return ret; >> + >> + return sprintf(buf, "%04x\n", le16_to_cpup((__le16 *)status)); >> +} > > Ditto. > Will add in v4. >> +static int rave_sp_write(struct rave_sp *sp, const u8 *data, u8 data_size) >> +{ > >> + size_t length; > > Slightly better to put this line after *dest definition. > OK, coming in v4. >> + const size_t checksum_length = sp->variant->checksum->length; >> + unsigned char crc[checksum_length]; >> + unsigned char frame[RAVE_SP_TX_BUFFER_SIZE]; >> + unsigned char *dest = frame; > >> +static u8 rave_sp_reply_code(u8 command) >> +{ >> + /* > >> + * There isn't a signle rule that describes command code -> > > Typo: single. > >> + * ACK code transformation, but, going through various >> + * versions of ICDs, there appear to be three distinct groups >> + * that can be described by simple transformation. >> + */ > >> + switch (command) { >> + case 0xA0 ... 0xBE: >> + /* >> + * Commands implemented by firmware found in RDU1 and >> + * older devices all seem to obey the following rule >> + */ >> + return command + 0x20; >> + case 0xE0 ... 0xEF: >> + /* >> + * Events emitted by all version of the firmare use >> + * least signifiact bit to get an ACK code > > Typos. > Will fix in v4. >> + */ >> + return command | 0x01; >> + default: >> + /* >> + * Commands implemented by firmware found in RDU2 are >> + * similar to "old" commands, but they use slightly >> + * different offset >> + */ >> + return command + 0x40; >> + } >> +} > >> + >> +int rave_sp_exec(struct rave_sp *sp, >> + void *__data, size_t data_size, >> + void *reply_data, size_t reply_data_size) >> +{ >> + int ret = 0; >> + unsigned char *data = __data; >> + const u8 ackid = (u8)atomic_inc_return(&sp->ackid); >> + const int command = sp->variant->cmd.translate(data[0]); >> + struct rave_sp_reply reply = { >> + .code = rave_sp_reply_code((u8)command), >> + .ackid = ackid, >> + .data = reply_data, >> + .length = reply_data_size, >> + .received = COMPLETION_INITIALIZER_ONSTACK(reply.received), >> + }; >> + > >> + if (command < 0) >> + return command; > > > Shouldn't be like > > command = sp->variant->cmd.translate(data[0]); > if (command < 0) > return command; > > reply.code = rave_sp_reply_code((u8)command); > > ? Shouldn't really make any difference, rave_sp_reply_code() will either return -EINVAL or some ACK code but in either case it would not be used due to early return on "command" being less that 0. > >> + >> + mutex_lock(&sp->bus_lock); >> + >> + mutex_lock(&sp->reply_lock); >> + sp->reply = &reply; >> + mutex_unlock(&sp->reply_lock); >> + > >> + data[0] = (u8)command; > > unsigned char implies u8. Fair point. Will remove the typecast. > >> + data[1] = ackid; >> + >> + rave_sp_write(sp, data, data_size); >> + >> + if (!wait_for_completion_timeout(&reply.received, HZ)) { >> + dev_err(&sp->serdev->dev, "Command timeout\n"); >> + ret = -ETIMEDOUT; >> + >> + mutex_lock(&sp->reply_lock); >> + sp->reply = NULL; >> + mutex_unlock(&sp->reply_lock); >> + } >> + >> + mutex_unlock(&sp->bus_lock); >> + return ret; >> +} > >> +static int rave_sp_rdu2_cmd_translate(enum rave_sp_command command) >> +{ >> + if (command >= RAVE_SP_CMD_GET_FIRMWARE_VERSION && >> + command <= RAVE_SP_CMD_GET_GPIO_STATE) >> + return command; >> + > >> + if (command == RAVE_SP_CMD_REQ_COPPER_REV) >> + return 0x28; > > Comment for use magic values? > Definition for it? > >> + >> + return rave_sp_rdu1_cmd_translate(command); >> +} > >> +static int rave_sp_default_cmd_translate(enum rave_sp_command command) >> +{ >> + switch (command) { >> + case RAVE_SP_CMD_GET_FIRMWARE_VERSION: >> + return 0x11; >> + case RAVE_SP_CMD_GET_BOOTLOADER_VERSION: >> + return 0x12; >> + case RAVE_SP_CMD_BOOT_SOURCE: >> + return 0x14; >> + case RAVE_SP_CMD_SW_WDT: >> + return 0x1C; >> + case RAVE_SP_CMD_RESET: >> + return 0x1E; >> + case RAVE_SP_CMD_RESET_REASON: >> + return 0x1F; > > Ditto. I'll add comment explaining the origin of the magic values. > >> + default: >> + return -EINVAL; >> + } >> +} > >> +static const char *rave_sp_silicon_to_string(struct device *dev, >> + const __le32 *version) >> +{ >> + return devm_kasprintf(dev, GFP_KERNEL, "%08x\n", >> le32_to_cpup(version)); > > I don't see why you can't use version value instead of pointer. > Sure, I can do that. >> +} > >> +static const char *rave_sp_copper_to_string(struct device *dev, uint8_t >> version) >> +{ >> + return devm_kasprintf(dev, GFP_KERNEL, "%02x\n", version); >> +} >> + >> + > > Remove extra empty line. > >> + .driver = { >> + .name = KBUILD_MODNAME, > > This is bad idea. It might be someone can use a platform driver (as > part of MFD, for example) and someone else rename driver at some > point. > I understand that in practice it's quite unlikely, though better not > to put potential issues in the first place. > Sure, I can convert it to a explicit string. >> + .of_match_table = rave_sp_dt_ids, >> + }, > >> +static inline u8 rave_sp_action_unpack_event(unsigned long action) >> +{ >> + return action & 0xff; >> +} >> + >> +static inline u8 rave_sp_action_unpack_value(unsigned long action) >> +{ >> + return (action >> 8) & 0xff; >> +} > > You can drop & 0xff in both cases as it's implied by u8. > Will remove in v4. Thanks, Andrey Smirnov