lkml.org 
[lkml]   [2011]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC PATCHv3 1/4] drivers/otp: add initial support for OTP memory
    On Fri, Mar 25, 2011 at 13:14, Jamie Iles wrote:
    > --- /dev/null
    > +++ b/drivers/otp/Kconfig
    > + may have different characterstics. This provides a character device

    characterstics -> characteristics

    > +if OTP
    > +
    > +config WRITE_ENABLE
    > + bool "Enable writing support of OTP pages"
    > + default n
    > + help

    does this show correctly in the kconfig by putting this under "if otp"
    instead of "depends otp" ? it should show the write option indented
    rather than at the same level.

    > +/* We'll allow OTP devices to be named otpa-otpz. */
    > +#define MAX_OTP_DEVICES 26

    mmm is that still true ?

    > +static unsigned long registered_otp_map[BITS_TO_LONGS(MAX_OTP_DEVICES)];
    > +static DEFINE_MUTEX(otp_register_mutex);

    do we really need this ? if we let the minor number dictate
    availability, then we can increment until that errors/wraps, and we
    dont need to do any tracking ...

    > + if (fmt == OTP_REDUNDANCY_FMT_SINGLE_ENDED)
    > + fmt_string = "single-ended";
    > + else if (fmt == OTP_REDUNDANCY_FMT_REDUNDANT)
    > + fmt_string = "redundant";
    > + else if (fmt == OTP_REDUNDANCY_FMT_DIFFERENTIAL)
    > + fmt_string = "differential";
    > + else if (fmt == OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT)
    > + fmt_string = "differential-redundant";
    > + else if (fmt == OTP_REDUNDANCY_FMT_ECC)
    > + fmt_string = "ecc";
    > + else
    > + return -EINVAL;

    i wonder if the code would be simpler if we had a local static array.
    then the show/store funcs would simply walk that tree, and when you
    add a new format in the future, you only have to update one place.

    static const char * const otp_redundancy_str[] = {
    [OTP_REDUNDANCY_FMT_SINGLE_ENDED] = "single-ended",
    [OTP_REDUNDANCY_FMT_REDUNDANT] = "redundant",
    ........
    };

    > +static ssize_t otp_num_regions_show(struct device *dev,
    > + struct device_attribute *attr, char *buf)
    > +{
    > + int nr_regions;
    > +
    > + nr_regions = otp_dev->ops->get_nr_regions(otp_dev);
    > +
    > + if (nr_regions < 0)
    > + return (ssize_t)nr_regions;

    we could make get_nr_regions() return a ssize_t ...

    > + err = alloc_chrdev_region(&otp_dev->devno, 0, max_regions, "otp");

    hmm, i was thinking that we'd have 1 major for otp devices. isnt this
    how MTD does it ?

    > --- /dev/null
    > +++ b/include/linux/otp.h
    > +/**
    > + * enum otp_device_flags - Flags to indicate capabilities for the OTP device.
    > + *
    > + * @OTP_DEVICE_FNO_SUBWORD_WRITE: only full word sized writes may be
    > + * performed. Don't use
    > + * read-modify-write cycles for
    > + * performing unaligned writes.
    > + */
    > +enum otp_device_flags {
    > + OTP_DEVICE_FNO_SUBWORD_WRITE = (1 << 0),
    > +};

    use OTP_CAPS_xxx instead ?
    -mike


    \
     
     \ /
      Last update: 2011-03-25 23:01    [W:0.025 / U:90.172 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site