lkml.org 
[lkml]   [2017]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RESEND PATCH v4 1/2] platform: Add driver for RAVE Supervisory Processor
On Tue, Jul 25, 2017 at 9:44 PM, Andrey Smirnov
<andrew.smirnov@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.

Some comments below, and FWIW

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> +#include <asm/unaligned.h>

Usually we put it below linux/* type of headers.

> +#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?

> +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.

> +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++;

> +
> + *crc = 1 + ~(*crc);
> +}

> +static void csum_ccitt(const u8 *buf, size_t size, u8 *crc)
> +{

> +

Remove this line.

> + 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.

> + */

> +}

> +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.

> +
> + 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.

> +}

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

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2017-07-27 11:03    [W:0.069 / U:9.472 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site