Re: [PATCHv2] drivers/misc: Altera Cyclone active serialimplementation

[Baruch Siach]

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@xxxxxxxxxx - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/