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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]