Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 27 Jul 2017 12:03:03 +0300 | Subject | Re: [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
| |