lkml.org 
[lkml]   [2018]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] mfd: rave-sp: Add code to print firmware versions
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: cphealy@gmail.com
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@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.

--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

\
 
 \ /
  Last update: 2018-03-07 17:52    [W:0.064 / U:2.128 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site