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
> +