Re: [PATCH 1/3] mfd: rave-sp: Add code to print firmware versions

2018-03-08 Thread Andrey Smirnov 
On Wed, Mar 7, 2018 at 8:51 AM, Lee Jones  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 
>> Cc: Lee Jones 
>> Cc: Guenter Roeck 
>> Signed-off-by: Andrey Smirnov 
>> ---
>>
>> 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

Re: [PATCH 1/3] mfd: rave-sp: Add code to print firmware versions

2018-03-08 Thread Andrey Smirnov 
On Wed, Mar 7, 2018 at 8:51 AM, Lee Jones  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 
>> Cc: Lee Jones 
>> Cc: Guenter Roeck 
>> Signed-off-by: Andrey Smirnov 
>> ---
>>
>> 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

Re: [PATCH 1/3] mfd: rave-sp: Add code to print firmware versions

2018-03-07 Thread Lee Jones 
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 
> Cc: Lee Jones 
> Cc: Guenter Roeck 
> Signed-off-by: Andrey Smirnov 
> ---
> 
> 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

Re: [PATCH 1/3] mfd: rave-sp: Add code to print firmware versions

2018-03-07 Thread Lee Jones 
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 
> Cc: Lee Jones 
> Cc: Guenter Roeck 
> Signed-off-by: Andrey Smirnov 
> ---
> 
> 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

Re: [PATCH 1/3] mfd: rave-sp: Add code to print firmware versions

2018-03-06 Thread Lucas Stach 
Am Montag, den 26.02.2018, 07:07 -0800 schrieb Andrey Smirnov:
> 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 
> Cc: Lee Jones 
> Cc: Guenter Roeck 
> Signed-off-by: Andrey Smirnov 

Tested-by: Lucas Stach 

> ---
> 
> 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'.
> 
> Thanks,
> Andrey Smirnov
> 
> 
>  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)
> +
> + u8  deprecated1;
> + u8  power_led_status;
> + u8  deprecated2;
> + u8  periph_power_shutoff;
> +} __packed;
> +
>  static bool rave_sp_id_is_event(u8 code)
>  {
>   return (code & 0xF0) == RAVE_SP_EVNT_BASE;
> @@ -609,6 +645,52 @@ static int rave_sp_default_cmd_translate(enum
> rave_sp_command command)
>   }
>  }
>  
> +static const char *devm_rave_sp_version(struct device *dev,
> + struct rave_sp_version
> *version)
> +{
> + /*
> +  * NOTE: The format string below uses %02d to display u16
> +  * intentionally for the sake of backwards compatibility
> with
> +  * legacy software.
> +  */
> + return devm_kasprintf(dev, GFP_KERNEL,
> "%02d%02d%02d.%c%c\n",
> +   version->hardware,
> +   le16_to_cpu(version->major),
> +   version->minor,
> +   version->letter[0],
> +   version->letter[1]);
> +}
> +
> +static int rave_sp_get_status(struct rave_sp *sp)
> +{
> + struct device *dev = >serdev->dev;
> + u8 cmd[] = {
> + [0] = RAVE_SP_CMD_STATUS,
> + [1] = 0
> + };
> + struct rave_sp_status status;
> + const char *version;
> + int ret;
> +
> + ret = rave_sp_exec(sp, cmd, sizeof(cmd), ,
> sizeof(status));
> + if (ret)
> + return ret;
> +
> + version = devm_rave_sp_version(dev,
> _version);
> + if (!version)
> + return -ENOMEM;
> +
> + sp->part_number_firmware = version;
> +
> + version = devm_rave_sp_version(dev,
> _version);
> + if (!version)
> + return -ENOMEM;
> +
> + sp->part_number_bootloader = version;
> +
> + return 0;
> +}
> +
>  static const struct rave_sp_checksum rave_sp_checksum_8b2c = {
>   .length = 1,
>   .subroutine = csum_8b2c,
> @@ -657,6 +739,7 @@ static const struct serdev_device_ops
> rave_sp_serdev_device_ops = {
>  static int rave_sp_probe(struct serdev_device *serdev)
>  {
>   struct device *dev = >dev;
> + const char *unknown = "unknown\n";
>   struct rave_sp *sp;
>   u32 baud;
>   int ret;
> @@ -689,6 +772,20 @@ static int rave_sp_probe(struct serdev_device
> *serdev)
>  
>   serdev_device_set_baudrate(serdev, baud);
>  
> + ret = rave_sp_get_status(sp);
> + if (ret) {
> + dev_warn(dev, "Failed to get firmware status: %d\n",
> ret);
> + sp->part_number_firmware   = unknown;
> +

Re: [PATCH 1/3] mfd: rave-sp: Add code to print firmware versions

2018-03-06 Thread Lucas Stach 
Am Montag, den 26.02.2018, 07:07 -0800 schrieb Andrey Smirnov:
> 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 
> Cc: Lee Jones 
> Cc: Guenter Roeck 
> Signed-off-by: Andrey Smirnov 

Tested-by: Lucas Stach 

> ---
> 
> 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'.
> 
> Thanks,
> Andrey Smirnov
> 
> 
>  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)
> +
> + u8  deprecated1;
> + u8  power_led_status;
> + u8  deprecated2;
> + u8  periph_power_shutoff;
> +} __packed;
> +
>  static bool rave_sp_id_is_event(u8 code)
>  {
>   return (code & 0xF0) == RAVE_SP_EVNT_BASE;
> @@ -609,6 +645,52 @@ static int rave_sp_default_cmd_translate(enum
> rave_sp_command command)
>   }
>  }
>  
> +static const char *devm_rave_sp_version(struct device *dev,
> + struct rave_sp_version
> *version)
> +{
> + /*
> +  * NOTE: The format string below uses %02d to display u16
> +  * intentionally for the sake of backwards compatibility
> with
> +  * legacy software.
> +  */
> + return devm_kasprintf(dev, GFP_KERNEL,
> "%02d%02d%02d.%c%c\n",
> +   version->hardware,
> +   le16_to_cpu(version->major),
> +   version->minor,
> +   version->letter[0],
> +   version->letter[1]);
> +}
> +
> +static int rave_sp_get_status(struct rave_sp *sp)
> +{
> + struct device *dev = >serdev->dev;
> + u8 cmd[] = {
> + [0] = RAVE_SP_CMD_STATUS,
> + [1] = 0
> + };
> + struct rave_sp_status status;
> + const char *version;
> + int ret;
> +
> + ret = rave_sp_exec(sp, cmd, sizeof(cmd), ,
> sizeof(status));
> + if (ret)
> + return ret;
> +
> + version = devm_rave_sp_version(dev,
> _version);
> + if (!version)
> + return -ENOMEM;
> +
> + sp->part_number_firmware = version;
> +
> + version = devm_rave_sp_version(dev,
> _version);
> + if (!version)
> + return -ENOMEM;
> +
> + sp->part_number_bootloader = version;
> +
> + return 0;
> +}
> +
>  static const struct rave_sp_checksum rave_sp_checksum_8b2c = {
>   .length = 1,
>   .subroutine = csum_8b2c,
> @@ -657,6 +739,7 @@ static const struct serdev_device_ops
> rave_sp_serdev_device_ops = {
>  static int rave_sp_probe(struct serdev_device *serdev)
>  {
>   struct device *dev = >dev;
> + const char *unknown = "unknown\n";
>   struct rave_sp *sp;
>   u32 baud;
>   int ret;
> @@ -689,6 +772,20 @@ static int rave_sp_probe(struct serdev_device
> *serdev)
>  
>   serdev_device_set_baudrate(serdev, baud);
>  
> + ret = rave_sp_get_status(sp);
> + if (ret) {
> + dev_warn(dev, "Failed to get firmware status: %d\n",
> ret);
> + sp->part_number_firmware   = unknown;
> + sp->part_number_bootloader = unknown;
> + }
> +
> + /*
> +  * Those strings already have a \n embedded, so there's no
> +

[PATCH 1/3] mfd: rave-sp: Add code to print firmware versions

2018-02-26 Thread Andrey Smirnov 
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 
Cc: Lee Jones 
Cc: Guenter Roeck 
Signed-off-by: Andrey Smirnov 
---

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

Thanks,
Andrey Smirnov


 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_MODEBIT(1)
+
+   u8  deprecated1;
+   u8  power_led_status;
+   u8  deprecated2;
+   u8  periph_power_shutoff;
+} __packed;
+
 static bool rave_sp_id_is_event(u8 code)
 {
return (code & 0xF0) == RAVE_SP_EVNT_BASE;
@@ -609,6 +645,52 @@ static int rave_sp_default_cmd_translate(enum 
rave_sp_command command)
}
 }
 
+static const char *devm_rave_sp_version(struct device *dev,
+   struct rave_sp_version *version)
+{
+   /*
+* NOTE: The format string below uses %02d to display u16
+* intentionally for the sake of backwards compatibility with
+* legacy software.
+*/
+   return devm_kasprintf(dev, GFP_KERNEL, "%02d%02d%02d.%c%c\n",
+ version->hardware,
+ le16_to_cpu(version->major),
+ version->minor,
+ version->letter[0],
+ version->letter[1]);
+}
+
+static int rave_sp_get_status(struct rave_sp *sp)
+{
+   struct device *dev = >serdev->dev;
+   u8 cmd[] = {
+   [0] = RAVE_SP_CMD_STATUS,
+   [1] = 0
+   };
+   struct rave_sp_status status;
+   const char *version;
+   int ret;
+
+   ret = rave_sp_exec(sp, cmd, sizeof(cmd), , sizeof(status));
+   if (ret)
+   return ret;
+
+   version = devm_rave_sp_version(dev, _version);
+   if (!version)
+   return -ENOMEM;
+
+   sp->part_number_firmware = version;
+
+   version = devm_rave_sp_version(dev, _version);
+   if (!version)
+   return -ENOMEM;
+
+   sp->part_number_bootloader = version;
+
+   return 0;
+}
+
 static const struct rave_sp_checksum rave_sp_checksum_8b2c = {
.length = 1,
.subroutine = csum_8b2c,
@@ -657,6 +739,7 @@ static const struct serdev_device_ops 
rave_sp_serdev_device_ops = {
 static int rave_sp_probe(struct serdev_device *serdev)
 {
struct device *dev = >dev;
+   const char *unknown = "unknown\n";
struct rave_sp *sp;
u32 baud;
int ret;
@@ -689,6 +772,20 @@ static int rave_sp_probe(struct serdev_device *serdev)
 
serdev_device_set_baudrate(serdev, baud);
 
+   ret = rave_sp_get_status(sp);
+   if (ret) {
+   dev_warn(dev, "Failed to get firmware status: %d\n", ret);
+   sp->part_number_firmware   = unknown;
+   sp->part_number_bootloader = unknown;
+   }
+
+   /*
+* Those strings already have a \n embedded, so there's no
+* need to have one in format string.
+*/
+   dev_info(dev, "Firmware version: %s",   sp->part_number_firmware);
+   dev_info(dev, "Bootloader version:

[PATCH 1/3] mfd: rave-sp: Add code to print firmware versions

2018-02-26 Thread Andrey Smirnov 
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 
Cc: Lee Jones 
Cc: Guenter Roeck 
Signed-off-by: Andrey Smirnov 
---

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

Thanks,
Andrey Smirnov


 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_MODEBIT(1)
+
+   u8  deprecated1;
+   u8  power_led_status;
+   u8  deprecated2;
+   u8  periph_power_shutoff;
+} __packed;
+
 static bool rave_sp_id_is_event(u8 code)
 {
return (code & 0xF0) == RAVE_SP_EVNT_BASE;
@@ -609,6 +645,52 @@ static int rave_sp_default_cmd_translate(enum 
rave_sp_command command)
}
 }
 
+static const char *devm_rave_sp_version(struct device *dev,
+   struct rave_sp_version *version)
+{
+   /*
+* NOTE: The format string below uses %02d to display u16
+* intentionally for the sake of backwards compatibility with
+* legacy software.
+*/
+   return devm_kasprintf(dev, GFP_KERNEL, "%02d%02d%02d.%c%c\n",
+ version->hardware,
+ le16_to_cpu(version->major),
+ version->minor,
+ version->letter[0],
+ version->letter[1]);
+}
+
+static int rave_sp_get_status(struct rave_sp *sp)
+{
+   struct device *dev = >serdev->dev;
+   u8 cmd[] = {
+   [0] = RAVE_SP_CMD_STATUS,
+   [1] = 0
+   };
+   struct rave_sp_status status;
+   const char *version;
+   int ret;
+
+   ret = rave_sp_exec(sp, cmd, sizeof(cmd), , sizeof(status));
+   if (ret)
+   return ret;
+
+   version = devm_rave_sp_version(dev, _version);
+   if (!version)
+   return -ENOMEM;
+
+   sp->part_number_firmware = version;
+
+   version = devm_rave_sp_version(dev, _version);
+   if (!version)
+   return -ENOMEM;
+
+   sp->part_number_bootloader = version;
+
+   return 0;
+}
+
 static const struct rave_sp_checksum rave_sp_checksum_8b2c = {
.length = 1,
.subroutine = csum_8b2c,
@@ -657,6 +739,7 @@ static const struct serdev_device_ops 
rave_sp_serdev_device_ops = {
 static int rave_sp_probe(struct serdev_device *serdev)
 {
struct device *dev = >dev;
+   const char *unknown = "unknown\n";
struct rave_sp *sp;
u32 baud;
int ret;
@@ -689,6 +772,20 @@ static int rave_sp_probe(struct serdev_device *serdev)
 
serdev_device_set_baudrate(serdev, baud);
 
+   ret = rave_sp_get_status(sp);
+   if (ret) {
+   dev_warn(dev, "Failed to get firmware status: %d\n", ret);
+   sp->part_number_firmware   = unknown;
+   sp->part_number_bootloader = unknown;
+   }
+
+   /*
+* Those strings already have a \n embedded, so there's 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);
+
return devm_of_platform_populate(dev);
 }
 
--