Re: [PATCH v3 1/2] platform: Add driver for RAVE Supervisory Processor
From: Andy Shevchenko
Date: Mon Jul 24 2017 - 13:28:53 EST
On Mon, Jul 24, 2017 at 6:09 PM, Andrey Smirnov
<andrew.smirnov@xxxxxxxxx> 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.
> +/**
> + * 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.
> + */
> +/**
> + * 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.
> + */
> + * @part_number_firmware:
> + * @part_number_bootloader:
> + * @reset_reason:
> + * @copper_rev_rmb:
> + * @copper_rev_deb:
> + * @silicon_devid:
> + * @silicon_devrev:
> + * @copper_mod_rmb:
> + * @copper_mod_deb:
???
> + *
> + * @variant: Device variant specific parameters and
> + * functions
> + * @event_notifier_list: Input event notification chain (used with
> + * corresponding input MFD cell driver)
Make them _short_.
> + * @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.
> + *
> + */
> +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);
?
> +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().
> +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.
> +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.
> + 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.
> + */
> + 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);
?
> +
> + 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.
> + 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.
> + 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.
> +}
> +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.
> + .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.
--
With Best Regards,
Andy Shevchenko