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.