Re: [PATCH 01/11] MFD: DA9052 MFD core module v8

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Fri, Nov 18, 2011 at 02:49:54PM +0530, Ashish Jangam wrote:

There's still a few issues in here.  It'd be very much easier to review
this stuff if you split the patch down into smaller patches, for example
having the ADC, SPI and I2C as separate patches.  The bigger each
individual e-mail is the more work it is to review.

Please also try to fix whatever problems you're having posting patch
serieses - this claims to be patch 1/11 but it looked like only patches
numbered 1, 4 and 5 got posted and none of those were threaded together.

> +int da9052_adc_manual_read(struct da9052 *da9052, unsigned char channel)
> +{
> +	struct da9052_auxadc_req *req;
> +	int ret;
> +	unsigned short calc_data;
> +	unsigned short data;
> +	unsigned char mux_sel;
> +
> +	req = kzalloc(sizeof(*req), GFP_KERNEL);
> +	if (!req)
> +		return -ENOMEM;

This function sometimes returns errnos but at other times...

> +	ret = da9052_reg_read(da9052, DA9052_ADC_RES_H_REG);
> +	if (ret < 0)
> +		return IRQ_NONE;

...it returns irqreturn_t values.  In the above case I'd *really* expect
to see the error from the return passed back to the caller.  It also
looks like this error case returns with the auxadc_lock held which is
going to deadlock the next time someone tries to use the ADC - the same
problem exists for some other code.

> +int da9052_add_regulator_devices(struct da9052 *da9052,
> +				  struct da9052_pdata *pdata)
> +{
> +	struct platform_device *pdev;
> +	int i, ret;
> +
> +	for (i = 0; i < pdata->num_regulators; i++) {
> +		pdev = platform_device_alloc("da9052-regulator", i);
> +		if (!pdev)
> +			return -ENOMEM;
> +
> +		pdev->dev.parent = da9052->dev;
> +		ret = platform_device_add(pdev);
> +		if (ret) {
> +			platform_device_put(pdev);
> +			return ret;
> +		}

As I think has been previously said it's better style to unconditionally
register all the regulators the device has.  The core should cope fine
with missing platform data and this gives readback support for the
regulators that aren't software configured yet.

> +static struct mfd_cell __initdata da9052_subdev_info[] = {
> +	{"da9052-onkey", .resources = &da9052_onkey_resource,
> +	 .num_resources = 1},

Spaces around the { }.

> +	ret = request_threaded_irq(pdata->irq_base + 13,
> +				   NULL, da9052_auxadc_irq,

Should replace the magic number with a define.

> +	da9052_i2c->bustype = BUS_I2C;

bustype should be redundant now, it certianly doesn't seem to be
referred to in this patch.

> +/*
> + * Interrupt controller support for Dilaog DA9052 PMICs.

This looks very much like it could be replaced with regmap-irq.  The
code would be slightly less efficient due to the support for sparse
interrupt registers but it'd be less code.

> +static struct spi_driver da9053_ba_spi_driver = {
> +	.probe = da9052_spi_probe,
> +	.remove = __devexit_p(da9052_spi_remove),
> +	.driver = {
> +		.name = "da9053-ba",
> +		.bus = &spi_bus_type,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +static struct spi_driver da9053_bb_spi_driver = {

Use spi_device_id rather than registering multiple driver instances.