[RFC] Removing VLAs in rtc-s5m

[RFC] Removing VLAs in rtc-s5m

[Gustavo A. R. Silva]

Hi Alexandre,

I'm trying to figure out the best way to fix the following VLA warnings:

drivers/rtc/rtc-s5m.c:370:2: warning: ISO C90 forbids variable length 
array ‘data’ [-Wvla]
drivers/rtc/rtc-s5m.c:416:2: warning: ISO C90 forbids variable length 
array ‘data’ [-Wvla]
drivers/rtc/rtc-s5m.c:453:2: warning: ISO C90 forbids variable length 
array ‘data’ [-Wvla]
drivers/rtc/rtc-s5m.c:503:2: warning: ISO C90 forbids variable length 
array ‘data’ [-Wvla]
drivers/rtc/rtc-s5m.c:548:2: warning: ISO C90 forbids variable length 
array ‘data’ [-Wvla]
drivers/rtc/rtc-s5m.c:601:2: warning: ISO C90 forbids variable length 
array ‘data’ [-Wvla]


So I took a look into the following piece of code at 
drivers/rtc/rtc-s5m.c:31, please see my comments below.

/*
  * Registers used by the driver which are different between chipsets.
  *
  * Operations like read time and write alarm/time require updating
  * specific fields in UDR register. These fields usually are auto-cleared
  * (with some exceptions).
  *
  * Table of operations per device:
  *
  * Device     | Write time | Read time | Write alarm
  * =================================================
  * S5M8767    | UDR + TIME |           | UDR
  * S2MPS11/14 | WUDR       | RUDR      | WUDR + RUDR
  * S2MPS13    | WUDR       | RUDR      | WUDR + AUDR
  * S2MPS15    | WUDR       | RUDR      | AUDR
  */
struct s5m_rtc_reg_config {
	/* Number of registers used for setting time/alarm0/alarm1 */
	unsigned int regs_count;


Once the maximum number of registers for devices S5M8767, S2MPS11/14 
S2MPS13 and S2MPS15 are 8, 7, 7 and 7 correspondingly, I think we can 
safely change the type of regs_count to u8/uint8_t. And then define a 
macro like the following:

#define MAX_NUM_REGS 8 or 10

so we can replace this kind of code:

u8 data[info->regs->regs_count];

with this:

u8 data[MAX_NUM_REGS];

What do you think?

Is there any chance a new device of this kind with more than 10 or 15 
registers can be added in the future?


	/* First register for time, seconds */
	unsigned int time;
	/* RTC control register */
	unsigned int ctrl;
	/* First register for alarm 0, seconds */
	unsigned int alarm0;
	/* First register for alarm 1, seconds */
	unsigned int alarm1;
	/*
	 * Register for update flag (UDR). Typically setting UDR field to 1
	 * will enable update of time or alarm register. Then it will be
	 * auto-cleared after successful update.
	 */
	unsigned int udr_update;
	/* Auto-cleared mask in UDR field for writing time and alarm */
	unsigned int autoclear_udr_mask;
	/*
	 * Masks in UDR field for time and alarm operations.
	 * The read time mask can be 0. Rest should not.
	 */
	unsigned int read_time_udr_mask;
	unsigned int write_time_udr_mask;
	unsigned int write_alarm_udr_mask;
};

/* Register map for S5M8763 and S5M8767 */
static const struct s5m_rtc_reg_config s5m_rtc_regs = {
	.regs_count		= 8,
	.time			= S5M_RTC_SEC,
	.ctrl			= S5M_ALARM1_CONF,
	.alarm0			= S5M_ALARM0_SEC,
	.alarm1			= S5M_ALARM1_SEC,
	.udr_update		= S5M_RTC_UDR_CON,
	.autoclear_udr_mask	= S5M_RTC_UDR_MASK,
	.read_time_udr_mask	= 0, /* Not needed */
	.write_time_udr_mask	= S5M_RTC_UDR_MASK | S5M_RTC_TIME_EN_MASK,
	.write_alarm_udr_mask	= S5M_RTC_UDR_MASK,
};

/* Register map for S2MPS13 */
static const struct s5m_rtc_reg_config s2mps13_rtc_regs = {
	.regs_count		= 7,
	.time			= S2MPS_RTC_SEC,
	.ctrl			= S2MPS_RTC_CTRL,
	.alarm0			= S2MPS_ALARM0_SEC,
	.alarm1			= S2MPS_ALARM1_SEC,
	.udr_update		= S2MPS_RTC_UDR_CON,
	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
	.write_time_udr_mask	= S2MPS_RTC_WUDR_MASK,
	.write_alarm_udr_mask	= S2MPS_RTC_WUDR_MASK | S2MPS13_RTC_AUDR_MASK,
};

/* Register map for S2MPS11/14 */
static const struct s5m_rtc_reg_config s2mps14_rtc_regs = {
	.regs_count		= 7,
	.time			= S2MPS_RTC_SEC,
	.ctrl			= S2MPS_RTC_CTRL,
	.alarm0			= S2MPS_ALARM0_SEC,
	.alarm1			= S2MPS_ALARM1_SEC,
	.udr_update		= S2MPS_RTC_UDR_CON,
	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
	.write_time_udr_mask	= S2MPS_RTC_WUDR_MASK,
	.write_alarm_udr_mask	= S2MPS_RTC_WUDR_MASK | S2MPS_RTC_RUDR_MASK,
};

/*
  * Register map for S2MPS15 - in comparison to S2MPS14 the WUDR and 
AUDR bits
  * are swapped.
  */
static const struct s5m_rtc_reg_config s2mps15_rtc_regs = {
	.regs_count		= 7,
	.time			= S2MPS_RTC_SEC,
	.ctrl			= S2MPS_RTC_CTRL,
	.alarm0			= S2MPS_ALARM0_SEC,
	.alarm1			= S2MPS_ALARM1_SEC,
	.udr_update		= S2MPS_RTC_UDR_CON,
	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
	.write_time_udr_mask	= S2MPS15_RTC_WUDR_MASK,
	.write_alarm_udr_mask	= S2MPS15_RTC_AUDR_MASK,
};

Thanks
--
Gustavo

Re: [RFC] Removing VLAs in rtc-s5m

[Krzysztof Kozlowski]

On Thu, Mar 8, 2018 at 4:18 AM, Gustavo A. R. Silva
<garsilva@embeddedor.com> wrote:
> Hi Alexandre,
>
> I'm trying to figure out the best way to fix the following VLA warnings:
>
> drivers/rtc/rtc-s5m.c:370:2: warning: ISO C90 forbids variable length array
> ‘data’ [-Wvla]
> drivers/rtc/rtc-s5m.c:416:2: warning: ISO C90 forbids variable length array
> ‘data’ [-Wvla]
> drivers/rtc/rtc-s5m.c:453:2: warning: ISO C90 forbids variable length array
> ‘data’ [-Wvla]
> drivers/rtc/rtc-s5m.c:503:2: warning: ISO C90 forbids variable length array
> ‘data’ [-Wvla]
> drivers/rtc/rtc-s5m.c:548:2: warning: ISO C90 forbids variable length array
> ‘data’ [-Wvla]
> drivers/rtc/rtc-s5m.c:601:2: warning: ISO C90 forbids variable length array
> ‘data’ [-Wvla]
>
>
> So I took a look into the following piece of code at
> drivers/rtc/rtc-s5m.c:31, please see my comments below.
>
> /*
>  * Registers used by the driver which are different between chipsets.
>  *
>  * Operations like read time and write alarm/time require updating
>  * specific fields in UDR register. These fields usually are auto-cleared
>  * (with some exceptions).
>  *
>  * Table of operations per device:
>  *
>  * Device     | Write time | Read time | Write alarm
>  * =================================================
>  * S5M8767    | UDR + TIME |           | UDR
>  * S2MPS11/14 | WUDR       | RUDR      | WUDR + RUDR
>  * S2MPS13    | WUDR       | RUDR      | WUDR + AUDR
>  * S2MPS15    | WUDR       | RUDR      | AUDR
>  */
> struct s5m_rtc_reg_config {
>         /* Number of registers used for setting time/alarm0/alarm1 */
>         unsigned int regs_count;
>
>
> Once the maximum number of registers for devices S5M8767, S2MPS11/14 S2MPS13
> and S2MPS15 are 8, 7, 7 and 7 correspondingly, I think we can safely change
> the type of regs_count to u8/uint8_t.

There is no benefit of changing it to u8. You will not get smaller
struct and unsigned int is pretty obvious for counting elements.
Unlike u8.

> And then define a macro like the
> following:
>
> #define MAX_NUM_REGS 8 or 10
>
> so we can replace this kind of code:
>
> u8 data[info->regs->regs_count];
>
> with this:
>
> u8 data[MAX_NUM_REGS];

Yes, 8 makes sense (but name like MAX_NUM_TIME_REGS). I think removing
the additional complexity (and error-prone solution) is worth of
living with wasted one-byte for some of devices. :)

>
> What do you think?
>
> Is there any chance a new device of this kind with more than 10 or 15
> registers can be added in the future?

I think not because Samsung is copying this RTC block from design to
design and just changes few things. More registers would mean that
time and alarms are provided with better precision (milli seconds?
more registers for years?) or with some different calendars. If such
device appears, we can extend the array and accept the waste of
additional bytes. Really, complexity of code is not worth these few
bytes.

Best regards,
Krzysztof

Re: [RFC] Removing VLAs in rtc-s5m

[Gustavo A. R. Silva]

Hi Krzysztof,

On 03/08/2018 02:37 AM, Krzysztof Kozlowski wrote:
> On Thu, Mar 8, 2018 at 4:18 AM, Gustavo A. R. Silva
> <garsilva@embeddedor.com> wrote:
>> Hi Alexandre,
>>
>> I'm trying to figure out the best way to fix the following VLA warnings:
>>
>> drivers/rtc/rtc-s5m.c:370:2: warning: ISO C90 forbids variable length array
>> ‘data’ [-Wvla]
>> drivers/rtc/rtc-s5m.c:416:2: warning: ISO C90 forbids variable length array
>> ‘data’ [-Wvla]
>> drivers/rtc/rtc-s5m.c:453:2: warning: ISO C90 forbids variable length array
>> ‘data’ [-Wvla]
>> drivers/rtc/rtc-s5m.c:503:2: warning: ISO C90 forbids variable length array
>> ‘data’ [-Wvla]
>> drivers/rtc/rtc-s5m.c:548:2: warning: ISO C90 forbids variable length array
>> ‘data’ [-Wvla]
>> drivers/rtc/rtc-s5m.c:601:2: warning: ISO C90 forbids variable length array
>> ‘data’ [-Wvla]
>>
>>
>> So I took a look into the following piece of code at
>> drivers/rtc/rtc-s5m.c:31, please see my comments below.
>>
>> /*
>>   * Registers used by the driver which are different between chipsets.
>>   *
>>   * Operations like read time and write alarm/time require updating
>>   * specific fields in UDR register. These fields usually are auto-cleared
>>   * (with some exceptions).
>>   *
>>   * Table of operations per device:
>>   *
>>   * Device     | Write time | Read time | Write alarm
>>   * =================================================
>>   * S5M8767    | UDR + TIME |           | UDR
>>   * S2MPS11/14 | WUDR       | RUDR      | WUDR + RUDR
>>   * S2MPS13    | WUDR       | RUDR      | WUDR + AUDR
>>   * S2MPS15    | WUDR       | RUDR      | AUDR
>>   */
>> struct s5m_rtc_reg_config {
>>          /* Number of registers used for setting time/alarm0/alarm1 */
>>          unsigned int regs_count;
>>
>>
>> Once the maximum number of registers for devices S5M8767, S2MPS11/14 S2MPS13
>> and S2MPS15 are 8, 7, 7 and 7 correspondingly, I think we can safely change
>> the type of regs_count to u8/uint8_t.
> 
> There is no benefit of changing it to u8. You will not get smaller
> struct and unsigned int is pretty obvious for counting elements.
> Unlike u8.
>

I got it.

>> And then define a macro like the
>> following:
>>
>> #define MAX_NUM_REGS 8 or 10
>>
>> so we can replace this kind of code:
>>
>> u8 data[info->regs->regs_count];
>>
>> with this:
>>
>> u8 data[MAX_NUM_REGS];
> 
> Yes, 8 makes sense (but name like MAX_NUM_TIME_REGS). I think removing
> the additional complexity (and error-prone solution) is worth of
> living with wasted one-byte for some of devices. :)
> 
>>
>> What do you think?
>>
>> Is there any chance a new device of this kind with more than 10 or 15
>> registers can be added in the future?
> 
> I think not because Samsung is copying this RTC block from design to
> design and just changes few things. More registers would mean that
> time and alarms are provided with better precision (milli seconds?
> more registers for years?) or with some different calendars. If such
> device appears, we can extend the array and accept the waste of
> additional bytes. Really, complexity of code is not worth these few
> bytes.
> 


I got it. I'll send a patch for this shortly.

Thanks for the feedback.
I appreciate it.
--
Gustavo