Re: [PATCH v3 1/2] platform: Add driver for RAVE Supervisory Processor

[Andrey Smirnov]

On Mon, Jul 24, 2017 at 10:25 AM, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
> 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.

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