lkml.org 
[lkml]   [2010]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv2] drivers/misc: Altera Cyclone active serial implementation
    Hi Indan,

    On Tue, Nov 09, 2010 at 09:15:09PM +0100, Indan Zupancic wrote:
    > On Tue, November 9, 2010 16:35, Baruch Siach wrote:

    [snip]

    > >> > +static struct {
    > >> > + int minor;
    > >> > + struct cyclone_as_device *drvdata;
    > >> > +} cyclone_as_devs[AS_MAX_DEVS];
    > >> > +
    > >> > +static struct cyclone_as_device *get_as_dev(int minor)
    > >> > +{
    > >> > + int i;
    > >> > +
    > >> > + for (i = 0; i < ARRAY_SIZE(cyclone_as_devs); i++)
    > >>
    > >> To me AS_MAX_DEVS looks clearer than ARRAY_SIZE(cyclone_as_devs).
    > >
    > > Not to me. The fact that AS_MAX_DEVS happens to be equal to
    > > ARRAY_SIZE(cyclone_as_devs) is not relevant to the understanding of this
    > > procedure. It just forces the reader to lookup the AS_MAX_DEVS value, and
    > > distracts the reader's attention.
    >
    > Then just leave the AS_MAX_DEVS define away, it doesn't add anything.

    I use AS_MAX_DEVS to set the size of cyclone_as_devs. It documents why this
    array is of that length.

    > Also change the:
    >
    > + if (pdata->id >= AS_MAX_DEVS)
    > + return -ENODEV;
    >
    > to
    >
    > + if (pdata->id >= ARRAY_SIZE(cyclone_as_devs))
    > + return -ENODEV;
    >
    > After all, what you're really checking is if the id falls within the
    > array.

    The current code seems more clear to me. AS_MAX_DEVS expresses the reason for
    not letting id exceed this value, and for having -ENODEV as return value. The
    cyclone_as_devs boundary check is just a side effect.

    [snip]

    > >> > + /* writes must be page aligned */
    > >> > + if (!AS_ALIGNED(*ppos) || !AS_ALIGNED(count))
    > >> > + return -EINVAL;
    >
    > As you're only incrementing ppos with the page size, is that first check
    > really needed? I don't think anything else can change ppos.

    I'd rather be on the safe side here. Future extensions (like read or lseek
    implementations) may change *ppos.

    > >> > + page_count = *ppos / AS_PAGE_SIZE;
    > >> > +
    > >> > + page_buf = kmalloc(AS_PAGE_SIZE, GFP_KERNEL);
    > >> > + if (page_buf == NULL)
    > >> > + return -ENOMEM;
    > >> > +
    > >> > + if (*ppos == 0) {
    > >> > + u8 as_status;
    > >> > +
    > >> > + xmit_cmd(drvdata->gpios, AS_WRITE_ENABLE);
    > >> > + xmit_cmd(drvdata->gpios, AS_ERASE_BULK);
    > >> > +
    > >> > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
    > >> > + xmit_byte(drvdata->gpios, AS_READ_STATUS);
    > >> > + for (i = 0; i < AS_ERASE_TIMEOUT; i++) {
    > >> > + as_status = recv_byte(drvdata->gpios);
    > >> > + if ((as_status & AS_STATUS_WIP) == 0)
    > >> > + break; /* erase done */
    > >> > + if (msleep_interruptible(1000) > 0) {
    > >> > + err = -EINTR;
    > >> > + break;
    > >> > + }
    > >> > + }
    > >> > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
    > >> > + if ((as_status & AS_STATUS_WIP) && err == 0)
    > >> > + err = -EIO; /* erase timeout */
    > >> > + if (err)
    > >> > + goto out;
    > >> > + ndelay(300);
    > >> > + }
    > >>
    > >> I'd move this into its own function, erase_device() or something.
    > >
    > > OK.
    > >
    > >> (And drop the ndelay.)
    > >
    > > See above.
    >
    > In that case I'd move it to just after the gpio_set*.

    OK. My original rationale was to skip the delay in case of error, but this
    micro-optimization doesn't worth the code obfuscation.

    baruch

    > > Thanks,
    > >
    > > baruch
    > >
    >
    > You're welcome.
    >
    > Greetings,
    >
    > Indan

    --
    ~. .~ Tk Open Systems
    =}------------------------------------------------ooO--U--Ooo------------{=
    - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -


    \
     
     \ /
      Last update: 2010-11-10 06:57    [W:0.027 / U:8.744 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site