Re: [PATCH V2 1/4] mfd: Initial support for Texas Instruments AICfamily of CODECs

From: Mark Brown
Date: Wed May 01 2013 - 11:44:27 EST


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.

Attachment: signature.asc
Description: Digital signature