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

[Andrey Smirnov]

On Thu, Jul 27, 2017 at 2:03 AM, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
> On Tue, Jul 25, 2017 at 9:44 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.
>
> Some comments below, and FWIW
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>
>> +#include <asm/unaligned.h>
>
> Usually we put it below linux/* type of headers.
>

OK, will do in v5.

>> +#include <linux/atomic.h>
>
>> + * @variant:                   Device variant specific parameters and
>> + *                             functions
>> + * @event_notifier_list:       Input event notification chain (used with
>> + *                             corresponding input MFD cell driver)
>
> Perhaps make them oneliners?
>

Sure, coming in v5.

>> +static DEVICE_ATTR(boot_source, 0640,
>> +                  rave_sp_show_boot_source, rave_sp_store_boot_source);
>
> DEVICE_ATTR_RW() ?
> Otherwise a comment is needed here why you can't use it.
>

DEVICE_ATTR_RW should work just fine, so I'll convert to that in v5.

>> +static void csum_8b2c(const u8 *buf, size_t size, u8 *crc)
>> +{
>> +       *crc = *buf++;
>> +       size--;
>> +
>> +       while (size--)
>> +               *crc += *buf++;
>
> I don't remember the answer to a proposal to convert this like
>
> *crc = 0;
>
> while (size--)
>  *crc += *buf++;
>

I unrolled first iteration of the loop specifically to avoid having to
do "*crc = 0".

>> +
>> +       *crc = 1 + ~(*crc);
>> +}
>
>> +static void csum_ccitt(const u8 *buf, size_t size, u8 *crc)
>> +{
>
>> +
>
> Remove this line.
>

Sure.

>> +       const u16 calculated = crc_ccitt_false(0xffff, buf, size);
>> +
>> +       /*
>> +        * While the rest of the wire protocol is little-endian,
>> +        * CCITT-16 CRC in RDU2 device is sent out in big-endian order.
>> +        */
>> +       put_unaligned_be16(calculated, crc);
>> +}
>
>> +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) {
>> +               /*
>> +                * As per RDU2 ICD 3.4.47 CMD_GET_COPPER_REV code is
>> +                * different from that for RDU1 and it is set to 0x28
>
> + period at the end => "...set to 0x28."
>
>> +                */
>> +               return 0x28;
>> +       }
>> +
>> +       return rave_sp_rdu1_cmd_translate(command);
>> +}
>
>> +
>> +static int rave_sp_default_cmd_translate(enum rave_sp_command command)
>> +{
>> +       /*
>> +        * All of the following command codes were taken from "Table :
>> +        * Communications Protocol Message Types" in section 3.3
>> +        * "MESSAGE TYPES" of Rave PIC24 ICD
>
> Ditto.
>

Will fix both in v5.

>> +        */
>
>> +}
>
>> +static void rave_sp_load_silicon_rev(struct rave_sp *sp)
>> +{
>> +       struct device *dev = &sp->serdev->dev;
>> +       u8 cmd[] = {
>> +               [0] = RAVE_SP_CMD_GET_SP_SILICON_REV,
>> +               [1] = 0
>> +       };
>> +       struct {
>> +               __le32 devid;
>> +               __le32 devrev;
>> +       } __packed reply;
>> +       int ret;
>> +
>> +       ret = rave_sp_exec(sp, cmd, sizeof(cmd), &reply, sizeof(reply));
>> +       if (ret) {
>> +               dev_err(dev, "CMD_GET_SP_SILICON_REV failed %d\n", ret);
>> +               return;
>> +       }
>> +
>
>> +       reply.devid  = le32_to_cpu(reply.devid);
>> +       reply.devrev = le32_to_cpu(reply.devrev);
>
> Can you run sparse with endianess check?
> I doubt it will be happy about these lines.
>

You're absolutely right, it did not like it. Will fix in v5.

>> +
>> +       sp->silicon_devid  = rave_sp_silicon_to_string(dev, reply.devid);
>> +       sp->silicon_devrev = rave_sp_silicon_to_string(dev, reply.devrev);
>> +}
>
>> +static int rave_sp_probe(struct serdev_device *serdev)
>> +{
>
>> +       /*
>> +        * Those strings already have a \n embedded so no need to have
>> +        * one in format string.
>> +        */
>> +       dev_info(dev, "Firmware version: %s", sp->part_number_firmware);
>> +       dev_info(dev, "Bootloader version: %s", sp->part_number_bootloader);
>
> \n missed in both cases.

Did you see the comment above the two lines? ;-)

>
>> +}
>
> P.S. Btw, have you consider to use remoteproc framework? Does it suit here?
>

RAVE SP is just a small PIC uC connected to the SoC over UART, it's
not really a true co-processor and, while it is technically possible,
it's firmware, stored in flash, is rarely re-programmed and it mostly
operates as a black box device.

Thanks,
Andrey Smirnov