Messages in this thread | | | From | Andrey Smirnov <> | Date | Thu, 27 Jul 2017 14:14:58 -0700 | Subject | Re: [RESEND PATCH v4 1/2] platform: Add driver for RAVE Supervisory Processor |
| |
On Thu, Jul 27, 2017 at 2:03 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > 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. >
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
| |