On Tue, Apr 16, 2013 at 07:13:36PM +0530, Mehar Bajwa wrote:
> +/**
> + * set_aic_book: change book which we have to write/read to.
> + *
> + * @aic: Device to write/read to.
> + * @book: Book to write/read to.
> + */
> +int set_aic_book(struct aic *aic, int book)
> +{
> + int ret = 0;
> + u8 page_buf[] = { 0x0, 0x0 };
> + u8 book_buf[] = { 0x7f, 0x0 };
> +
> + ret = regmap_write(aic->regmap, page_buf[0], page_buf[1]);
> +
> + if (ret < 0)
> + return ret;
> + book_buf[1] = book;
> + ret = regmap_write(aic->regmap, book_buf[0], book_buf[1]);
This looks like just like register paging - there's support in the core
for that, if it's not adequate then just extend the framework. Looks
like the pages are multi-level so perhaps that's required.
> + dev_info(aic->dev, "%s\n", devname);
It's useful to display something like a device revision but if you're
just announcing that the driver has been loaded that's not so useful and
should be removed. Similarly for the others.
> +#ifdef CONFIG_MFD_AIC_IRQ
> + if (aic->irq) {
> + ret = aic_irq_init(aic);
> + if (ret < 0)
> + goto err_irq;
> + }
> +#endif
This doesn't look like it should be a config option? If it should be
then these should be done with inline functions in the headers for stubs
rather than ifdefs in the code.
signature.asc
Description: Digital signature
