Messages in this thread | | | Date | Tue, 23 Oct 2012 16:18:25 -0700 | From | Christopher Heiny <> | Subject | Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation |
| |
On 10/11/2012 08:32 AM, Linus Walleij wrote: > On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny <Cheiny@synaptics.com> 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.
OK, after reviewing the spec I'll accept that. We'll make the change.
>>>> +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...
What's the old saying? "I want to live in Theory. Everything is always so nice there..." :-)
Anyway, see my reply to Dmitry a bit ago. These are no longer needed, so we'll drop them.
| |