On Wed, Mar 7, 2018 at 8:51 AM, Lee Jones <lee.jo...@linaro.org> wrote: > On Mon, 26 Feb 2018, Andrey Smirnov wrote: > >> Add code that would query and print out bootloader and application >> firmware version info. >> >> Cc: linux-kernel@vger.kernel.org >> Cc: cphe...@gmail.com >> Cc: Lucas Stach <l.st...@pengutronix.de> >> Cc: Lee Jones <lee.jo...@linaro.org> >> Cc: Guenter Roeck <li...@roeck-us.net> >> Signed-off-by: Andrey Smirnov <andrew.smir...@gmail.com> >> --- >> >> Lee: >> >> The reason 'part_number_firmware' and 'part_number_firmware' are a >> part of struct rave_sp is because there exists another patch on top of >> this one that exposes those fields via sysfs. The latter patch is not >> currently being upstreamed (might be in the future), so if keeping >> this arrangement is too ugly, let me know, and I'll get rid of those >> fields in 'rave_sp'. > > That's okay. > >> drivers/mfd/rave-sp.c | 97 >> +++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 97 insertions(+) >> >> diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c >> index c8173de5653a..9e4c83ff2aec 100644 >> --- a/drivers/mfd/rave-sp.c >> +++ b/drivers/mfd/rave-sp.c >> @@ -160,6 +160,8 @@ struct rave_sp_variant { >> * @variant: Device variant specific information >> * @event_notifier_list: Input event notification chain >> * >> + * @part_number_firmware: Firmware version >> + * @part_number_bootloader: Bootloader version >> */ >> struct rave_sp { >> struct serdev_device *serdev; >> @@ -171,8 +173,42 @@ struct rave_sp { >> >> const struct rave_sp_variant *variant; >> struct blocking_notifier_head event_notifier_list; >> + >> + const char *part_number_firmware; >> + const char *part_number_bootloader; >> }; >> >> +struct rave_sp_version { >> + u8 hardware; >> + __le16 major; >> + u8 minor; >> + u8 letter[2]; >> +} __packed; >> + >> +struct rave_sp_status { >> + struct rave_sp_version bootloader_version; >> + struct rave_sp_version firmware_version; >> + u16 rdu_eeprom_flag; >> + u16 dds_eeprom_flag; >> + u8 pic_flag; >> + u8 orientation; >> + u32 etc; >> + s16 temp[2]; >> + u8 backlight_current[3]; >> + u8 dip_switch; >> + u8 host_interrupt; >> + u16 voltage_28; >> + u8 i2c_device_status; >> + u8 power_status; >> + u8 general_status; >> +#define RAVE_SP_STATUS_GS_FIRMWARE_MODE BIT(1) > > I'm more concerned with the seemingly unused #define shoved in the > middle of a struct definition -- which I am not a fan of. > > Better to introduce it when you start using it and outside of the > struct definition please. > > The remainder of the patch looks okay.
OK, will fix in v2. Thanks, Andrey Smirnov