On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny <Cheiny@xxxxxxxxxxxxx> wrote:
> Linus Walleij wrote:
>> But please use arithmetic operators (I think I said this on the last
>> review):
>>
>> dest[0] = src & 0xFF;
>> dest[1] = src >> 8;
>>
>> Doing it the above way makes artithmetic look like maths, and it isn't.
>> Besides it's done this way in most parts of the kernel and we're
>> familiar with it.
>
> Yes, you mentioned it previously. I'm somewhat paranoid, though, and
> don't trust the shift/mask method to work correctly on big-endian
> machines. If the shifts can be relied on to behave (I'm guessing the
> answer is "yes", since you say this idiom is used widely in the
> kernel), then I'll change it.
If the behaviour was not consistent across different endianness
it would not be part of the C language specification...
<< means shift left in the accumulator or whatever you have.
It will work the same no matter how bits are laid out in
memory.
>> > +static inline ssize_t rmi_store_error(struct device *dev,
>> > + struct device_attribute *attr,
>> > + const char *buf, size_t count)
>> > +{
>> > + dev_warn(dev,
>> > + "WARNING: Attempt to write %d characters to read-only
>> > attribute %s.", + count, attr->attr.name);
>> > + return -EPERM;
>> > +}
>>
>> Here it looks like you're hiding a lot of stuff that should be dev_warn()?
>> Consider my earlier point about dynamic debug.
>
> In previous patch submissions, we always used these warning functions.
> But in the feedback on those patches, we were asked to just make
> sysfs show/store NULL if the attribute is write/read only. However,
> during their development process, our customers want to see the
> warnings if the attributes are accessed incorrectly. So we made
> these warnings a debug option.
See Dmitry's comment ...
Basically my stance is that you should not lower yourself to the
level of others not getting the point of your technical solution
by making unelegant compromises, what
you should do is to bring them up to your level so they
understand that your solution is elegant.
Maybe a bit utopist I know...
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
