lkml.org 
[lkml]   [2012]   [Oct]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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.


    \
     
     \ /
      Last update: 2012-10-27 04:21    [W:2.896 / U:0.132 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site