Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Oliver Schinagl <oliver+list@schinagl.nl>]

On 17-06-13 13:25, Russell King - ARM Linux wrote:
> On Mon, Jun 17, 2013 at 12:36:47PM +0200, Oliver Schinagl wrote:
>> On 15-06-13 12:28, Tomasz Figa wrote:
>>> What is this version thingy?
>>>
>>> Is there a versioning scheme defined for this driver? Do you expect it to
>>> be changed every modification of this driver?
>>>
>>> I don't see any point of having such thing in a project with a version
>>> control system, where you have all change history.
>> Well we export something to userspace, while trivial there is the
>> possibility it changes over time. Say A40 which outputs 256 bits instead
>> of the current 128 bits. That would validate a bump in version number.
>> It's purely so the user can be aware of differences in the driver. So
>> maybe DRV_A[BP]I_VERSION would be better?
>
> What is better to do is to export such things as properties, or
> design the API in such a way that the length of the ID is reportable.
>
> However, it's actually quite easy to do if you only care about the
> number of bytes - you just arrange for the read() function to return
> the number of bytes read.  So in the case of 128 bits available, that's
> 16 bytes, so a read() of the sysfs attribute with a buffer of (say)
> 256 bytes should report only 16 bytes read.
>
> If it were to become 256 bytes later, then the read() would return
> 32 bytes read.  So there's no need for any new APIs to do this.
That makes sense for the sysfs bit and as the only user, I guess makes 
the version information obsolete for now.
>
> Also, this is over-complicated:
>
> +       for (i = 0; i < size; i++) {
> +               if ((pos + i) >= SID_SIZE || (pos < 0))
> +                       break;
> +               buf[i] = sunxi_sid_read_byte(sid_reg_base, pos + i);
> +       }
>
> Maybe:
> 	if (pos < 0 || pos >= SID_SIZE)
> 		return 0;
> 	if (size > SID_SIZE - pos)
> 		size = SID_SIZE - pos;
>
> 	for (i = 0; i < size; i++)
> 		buf[i] = sunxi_sid_read_byte(sid_reg_base, pos + i);
>
> 	return size;
>
I do like your approach, but takes a second to read ;) How is it less 
complicated though? It's more LOC i suppose. I do appreciate that we 
only perform the read function when our size is correct, thus making the 
for loop only execute the minimally required code. While in this driver 
is insignificant and not important, I am a proponent of it.
Consider it changed.

Will wait a bit for Thomaz to optionally reply and then send yet a 
nother version ;)

Thanks for your time,

Oliver