lkml.org 
[lkml]   [2011]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCHv3 1/4] drivers/otp: add initial support for OTP memory
Hi Mike,

On Fri, Mar 25, 2011 at 05:58:05PM -0400, Mike Frysinger wrote:
> 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

Doh!

> > +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.

I think it's OK because it's a menuconfig thing so the toplevel OTP
thing is at the same level as misc devices and staging drivers etc.

> > +/* We'll allow OTP devices to be named otpa-otpz. */
> > +#define MAX_OTP_DEVICES 26
>
> mmm is that still true ?

I think so - the actual devices should be otpa-otpz, but when you
register regions they could be otpa1, otpa2, otpb1, otpb2 etc.

>
> > +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 ...

OK, so it would be nice to get rid of this but afaict we still need to
do some accounting of available minor numbers in the range that we've
allocated. We could do a simple increment % 255 for the minor number
but if OTP devices are removed at runtime then that may get fragmented
and we would need to do retries of device_register() which feels a bit
too easy to mess up.

Certainly allocating one major number for OTP devices then allocating
the minors one by one would be much better than what I have now.

We probably also want it so that if you remove the OTP device that has
had regions called otpaN then reinsert it they doesn't suddenly become
otpbN.

>
> > + 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",
> ........
> };

Yes, that would be a lot cleaner.

> > +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 ...

OK, will do.

>
> > + 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 ?

Sounds like a plan! Thanks again for the review!

Jamie


\
 
 \ /
  Last update: 2011-03-25 23:49    [W:0.060 / U:0.276 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site