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

[Andrey Smirnov]

On Tue, Jul 18, 2017 at 11:48 AM, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
> On Tue, Jul 18, 2017 at 8:56 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.
>
>> +/**
>> + * struct rave_sp - RAVE supervisory processor core
>> + *
>> + * @serdev:                    Pointer to underlying serdev
>> + * @deframer:                  Stored state of the protocol deframer
>> + * @ackid:                     ACK ID used in last reply sent to the device
>> + * @bus_lock:                  Lock to serialize access to the device
>> + * @reply_lock:                        Lock protecting @reply
>> + * @reply:                     Pointer to memory to store reply payload
>
>> + *
>
> Do you need it?

Not sure what this is in reference too. The @reply field?

>
>> + * @part_number_firmware:      String containing firmware part number
>
>> + *                             (retrived once during probing)
>
> No need to repeat this over each parameter. Just move it to (long)
> description part and tell that
> "part_*, copper_*, and silicon_* parameters are retrived once during probing."
>

OK, will change in v3.

>> +static void
>> +devm_rave_sp_unregister_event_notifier(struct device *dev, void *res)
>
> static devm_*?!
> It looks let's say interesting... (yes, better to provide releasing
> devres API as well)
>

It's just a callback provided to devres_alloc call below. It could and
should not be used as external API, but I can remove devm_ prefix from
the name if that makes things better. As for providing API for manual
unregistered -- there are no potential users of that functionality in
any of the corresponding MFD cell drivers, so I'd rather not add code
that'll never get used.

>> +static const char *devm_rave_sp_version(struct device *dev, const char *buf)
>> +{
>> +       return devm_kasprintf(dev, GFP_KERNEL, "%02d%02d%02d.%c%c\n",
>> +                             buf[0], le16_to_cpup((const __le16 *)&buf[1]),
>
> To cpu_P_?!

Yes, I am not sure why you are surprised. It's used with &buf[1] and
it all seems legit, did I miss something?

> __le16 and %02d?
> Are you sure?
>

Yeah, legacy userspace daemon that, among other things, was in charge
of implementing similar to this driver's functionality was reporting
that particular version string in such buggy way. For the sake of
backwards compatibility I do the same. I'll add some commentary to
explain that.

>> +                             buf[3], buf[4], buf[5]);
>> +}
>
>> +static ssize_t
>> +rave_sp_show_part_number(char *buf, const char *version, size_t version_length)
>> +{
>> +       memcpy(buf, version, version_length + 1);
>> +       return version_length;
>
> Looks suspicious. If it's string, use snprintf(), it it might have
> garbage, use %pE.
>

Will change to snprintf in v3.

>> +}
>
>> +       return sprintf(buf, "%04x\n", le16_to_cpup((__le16 *)status));
>
> cpu_P_?!
>

Status is declared as "u8 status[2]", so this should be OK, no?

>> +static int rave_sp_common_get_boot_source(struct rave_sp *sp)
>> +{
>> +       u8 cmd[] = {
>> +               [0] = RAVE_SP_CMD_BOOT_SOURCE,
>> +               [1] = 0,
>> +               [2] = RAVE_SP_BOOT_SOURCE_SET,
>> +               [3] = 0,
>> +       };
>> +       u8 boot_source;
>
>> +       const int ret = rave_sp_exec(sp, cmd, sizeof(cmd),
>> +                                    &boot_source, sizeof(boot_source));
>
> Why not
>
>     const int ret;
>
>     ret = rave_sp_exec(sp, cmd, sizeof(cmd), &boot_source, sizeof(boot_source));
>

You'd have to drop the "const" qualifier from "ret" in order for that
snippet to compile, which I was trying to avoid. I'll drop the const
and move the call one line down in v3.

>> +       return (ret < 0) ? ret : boot_source;
>
> Ditto for every similar cases.
>
>> +}
>
>> +static void csum_8b2c(const u8 *buf, size_t size, u8 *crc)
>> +{
>
>> +       *crc = *buf++;
>
>> +       size--;
>> +
>> +       while (size--)
>
> *crc = 0;
>
> while (size--)
>     *crc += *buf++;
>
>  ?

I partially unrolled that look specifically because I wanted to avoid
having that "useless" "*crc = 0" line.

>
>> +               *crc += *buf++;
>> +
>> +       *crc = 1 + ~(*crc);
>> +}
>
> I dunno if lib/crc8.c helps here, worth to check.
>

I did check it out, but I am not sure I can use it: I am hesitant to
outright deny the possibility, but I can't think of a straight
conversion from "2's complement of sum of all bytes" to polynomial
division. Besides csum_8b2c() implements checksum algorithm as it is
described in ICD, so it has the advantage of being easier to double
check.

>> +static void *stuff(unsigned char *dest, const unsigned char *src, size_t n)
>> +{
>
>> +       size_t i;
>> +
>> +       for (i = 0; i < n; i++) {
>
> Useless i.
>
> while (n--) {
>  ...
> }
>

Will change in v3.

>> +               const unsigned char byte = *src++;
>> +
>> +               switch (byte) {
>> +               case RAVE_SP_STX:
>> +               case RAVE_SP_ETX:
>> +               case RAVE_SP_DLE:
>> +                       *dest++ = RAVE_SP_DLE;
>> +               default:
>> +                       *dest++ = byte;
>> +               }
>> +       }
>> +
>> +       return dest;
>> +}
>
>> +static u8 rave_sp_reply_code(u8 command)
>> +{
>> +       switch (command) {
>> +       case 0xA0 ... 0xBE:
>> +               return command + 0x20;
>
>> +       case 0xE0 ... 0xEF:
>
>> +               return command | 0x01;
>
> These all look strange. Perhaps this function needs more comments.
>

Yeah, you're right, I'll add mover verbiage to explain what's going on there.

>> +       default:
>> +               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),
>> +       };
>> +
>
> + command = ...; here!
>

"reply.code" initialization depends on the value of "command", so I
can't easily move it down here.

>> +       if (command < 0)
>> +               return command;
>
>> +static void rave_sp_receive_reply(struct rave_sp *sp,
>> +                                 const unsigned char *data, size_t length)
>> +{
>> +       struct device *dev = &sp->serdev->dev;
>> +       struct rave_sp_reply *reply;
>> +       const  size_t payload_length = length - 2;
>> +
>> +       mutex_lock(&sp->reply_lock);
>> +       reply = sp->reply;
>> +
>> +       if (reply && reply->code == data[0] && reply->ackid == data[1] &&
>> +           payload_length >= reply->length) {
>> +               /*
>> +                * We are relying on memcpy(dst, src, 0) to be a no-op
>> +                * when handling commands that have a no-payload reply
>> +                */
>> +               memcpy(reply->data, &data[2], reply->length);
>> +               complete(&reply->received);
>> +               sp->reply = NULL;
>> +       } else {
>> +               dev_err(dev, "Ignoring incorrect reply\n");
>> +               dev_dbg(dev, "Code:   expected = 0x%08x received = 0x%08x\n",
>> +                       reply->code, data[0]);
>
> NULL pointer dereference.
>
>> +               dev_dbg(dev, "ACK ID: expected = 0x%08x received = 0x%08x\n",
>> +                       reply->ackid, data[1]);
>> +               dev_dbg(dev, "Length: expected = %zu received = %zu\n",
>> +                       reply->length, payload_length);
>
> ... in all cases.
>

Good catch, thanks! Will fix in v3.

>> +       }
>> +
>> +       mutex_unlock(&sp->reply_lock);
>> +}
>> +
>> +static void rave_sp_receive_frame(struct rave_sp *sp,
>> +                                 const unsigned char *data,
>> +                                 size_t length)
>> +{
>
>> +       if (rave_sp_id_is_event(data[0]))
>> +               rave_sp_receive_event(sp, data, length);
>> +       else
>> +               rave_sp_receive_reply(sp, data, length);
>> +}
>> +
>
>> +static int rave_sp_rdu1_cmd_translate(enum rave_sp_command command)
>> +{
>> +       if (command >= RAVE_SP_CMD_STATUS &&
>> +           command <= RAVE_SP_CMD_CONTROL_EVENTS)
>> +               return command;
>
>> +       else
>
> Redundant.
>
>> +               return -EINVAL;
>> +}
>> +
>> +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;
>
>> +       else if (command == RAVE_SP_CMD_REQ_COPPER_REV)
>> +               return 0x28;
>> +       else
>> +               return rave_sp_rdu1_cmd_translate(command);
>
> Redundant else in both cases.
>

Will fix both in v3.

>> +}
>
>> +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_RESET_REASON:
>
>> +               return 0x1F;
>> +       case RAVE_SP_CMD_BOOT_SOURCE:
>> +               return 0x14;
>> +       case RAVE_SP_CMD_RESET:
>> +               return 0x1E;
>> +       case RAVE_SP_CMD_SW_WDT:
>> +               return 0x1C;
>
> Can you set them in order of returned value?
>

Sure, sounds like a good idea, will fix in v3.

>> +       default:
>> +               return -EINVAL;
>> +       }
>> +}
>> +
>
>> +static void rave_sp_common_init(struct rave_sp *sp)
>> +{
>
>> +       if (!ret)
>
> Other way around, please.
>
>> +               sp->part_number_firmware =
>> +                       devm_rave_sp_version(dev, version);
>> +       else
>> +               dev_warn(dev, "CMD_GET_FIRMWARE_VERSION failed %d\n", ret);
>> +
>> +       cmd[0] = RAVE_SP_CMD_GET_BOOTLOADER_VERSION;
>> +       ret = rave_sp_exec(sp, cmd, sizeof(cmd), version, sizeof(version));
>
>> +       if (!ret)
>
> Ditto.
>
> Do this for every similar place.
>

Will do in v3.

>> +               sp->part_number_bootloader =
>> +                       devm_rave_sp_version(dev, version);
>> +       else
>> +               dev_warn(dev, "CMD_GET_BOOTLOADER_VERSION failed %d\n", ret);
>> +}
>> +
>
>> +static void rave_sp_rdu2_init(struct rave_sp *sp)
>> +{
>
>> +       ret = rave_sp_exec(sp, cmd, sizeof(cmd),
>> +                          &copper_rev, sizeof(copper_rev));
>
>> +       if (!ret) {
>
> This is a bit unusual.
>
>> +               sp->copper_rev_rmb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
>> +                                                   copper_rev & 0x1F);
>
> %01x and & 0x1f ?!
>

Ugh, missed this. Will change to "%02x" in v3.

>> +               sp->copper_mod_rmb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
>> +                                                   copper_rev >> 5);
>> +       } else {
>> +               dev_warn(dev,
>> +                        "RAVE_SP_CMD_REQ_COPPER_REV(RMB) failed %d\n", ret);
>> +       }
>> +
>> +       cmd[2] = RAVE_SP_RDU2_BOARD_TYPE_DEB;
>> +
>> +       ret = rave_sp_exec(sp, cmd, sizeof(cmd),
>> +                          &copper_rev, sizeof(copper_rev));
>> +       if (!ret) {
>
>> +               sp->copper_rev_deb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
>> +                                                   copper_rev & 0x1F);
>> +               sp->copper_mod_deb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
>> +                                                   copper_rev >> 5);
>
> Duplicate! Perhaps, helper function.
>

Will fix in v3.

>> +       } else {
>> +               dev_warn(dev,
>> +                        "RAVE_SP_CMD_REQ_COPPER_REV(DEB) failed %d\n", ret);
>> +       }
>> +}
>
>> +static int rave_sp_probe(struct serdev_device *serdev)
>> +{
>
>> +       static const struct serdev_device_ops serdev_device_ops = {
>> +               .receive_buf  = rave_sp_receive_buf,
>> +               .write_wakeup = serdev_device_write_wakeup,
>> +       };
>
> Please, move outside.
>

Sure, will fix in v3.

>> +       struct rave_sp *sp;
>
>> +       struct device *dev = &serdev->dev;
>
> Make it first line in this function.
>

OK, will fix in v3.

>> +       u32 baud;
>> +       int ret;
>
>> +       mutex_init(&sp->bus_lock);
>> +       mutex_init(&sp->reply_lock);
>
>> +static struct serdev_device_driver rave_sp_drv = {
>> +       .probe                  = rave_sp_probe,
>> +       .remove                 = rave_sp_remove,
>> +       .driver = {
>> +               .name           = KBUILD_MODNAME,
>
>> +               .owner          = THIS_MODULE,
>
> Redundant I suppose.
>

OK, will drop in v3.

>> +               .of_match_table = rave_sp_dt_ids,
>> +       },
>> +};
>
>> +static inline unsigned long rave_sp_action(u8 event, u8 value)
>
> _pack_action?
>
>> +{
>> +       return ((unsigned long)value << 8) | event;
>> +}
>> +
>> +static inline u8 rave_sp_action_get_event(unsigned long action)
>
> _unpack_event?
>
>> +{
>> +       return action & 0xff;
>> +}
>> +
>> +static inline u8 rave_sp_action_get_value(unsigned long action)
>
> _unpack_value?
>
>> +{
>> +       return (action >> 8) & 0xff;
>> +}
>

Yeah, sounds reasonable, will rename in v3.

Thanks,
Andrey Smirnov