* [PATCH 1/4] mfd: update i2c driver for max8925 @ 2010-01-25 11:07 Haojian Zhuang 2010-01-29 19:54 ` Samuel Ortiz 0 siblings, 1 reply; 6+ messages in thread From: Haojian Zhuang @ 2010-01-25 11:07 UTC (permalink / raw) To: linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4] mfd: update i2c driver for max8925 2010-01-25 11:07 [PATCH 1/4] mfd: update i2c driver for max8925 Haojian Zhuang @ 2010-01-29 19:54 ` Samuel Ortiz 2010-02-02 14:22 ` Haojian Zhuang 0 siblings, 1 reply; 6+ messages in thread From: Samuel Ortiz @ 2010-01-29 19:54 UTC (permalink / raw) To: linux-arm-kernel Hi Haojian, On Mon, Jan 25, 2010 at 06:07:08AM -0500, Haojian Zhuang wrote: > static inline int max8925_read_device(struct i2c_client *i2c, > int reg, int bytes, void *dest) > { > - unsigned char data; > - unsigned char *buf; > int ret; > > - buf = kzalloc(bytes + 1, GFP_KERNEL); > - if (!buf) > - return -ENOMEM; > - > - data = (unsigned char)reg; > - ret = i2c_master_send(i2c, &data, 1); > - if (ret < 0) > - return ret; > - > - ret = i2c_master_recv(i2c, buf, bytes + 1); > - if (ret < 0) > - return ret; > - memcpy(dest, buf, bytes); > - return 0; > + if (bytes > 1) > + ret = i2c_smbus_read_i2c_block_data(i2c, reg, bytes, dest); > + else { > + ret = i2c_smbus_read_byte_data(i2c, reg); > + if (ret < 0) > + return ret; > + *(unsigned char *)dest = (unsigned char)ret; > + } > + return ret; Your read_device routine looks much better now :) > @@ -142,28 +138,56 @@ static int __devinit max8925_probe(struct > i2c_client *client, > const struct i2c_device_id *id) > { > struct max8925_platform_data *pdata = client->dev.platform_data; > - struct max8925_chip *chip; > + struct i2c_board_info rtc_info, adc_info; > + const unsigned short addr_rtc[] = { > + RTC_I2C_ADDR, > + I2C_CLIENT_END, > + }; > + const unsigned short addr_adc[] = { > + ADC_I2C_ADDR, > + I2C_CLIENT_END, > + }; If you know the devices adress, why do you call i2c_new_probed_device() instead of i2c_new_device() ? > + static struct max8925_chip *chip; > + static int init_gpm = 0, init_adc = 0, init_rtc = 0, count = 0; Those static here are not looking good at all... > + if (!init_gpm) { > + init_gpm = 1; > + chip = kzalloc(sizeof(struct max8925_chip), GFP_KERNEL); > + if (chip == NULL) > + return -ENOMEM; > + chip->i2c = client; > + chip->dev = &client->dev; > + i2c_set_clientdata(client, chip); > + dev_set_drvdata(chip->dev, chip); > + mutex_init(&chip->io_lock); > + } > + if (!init_rtc) { > + init_rtc = 1; > + memset(&rtc_info, 0, sizeof(struct i2c_board_info)); > + strlcpy(rtc_info.type, "max8925", I2C_NAME_SIZE); > + rtc_info.platform_data = chip->i2c->dev.platform_data; > + chip->rtc = i2c_new_probed_device(chip->i2c->adapter, > + &rtc_info, addr_rtc); > + i2c_set_clientdata(chip->rtc, chip); > + } else if (!init_adc) { > + init_adc = 1; > + memset(&adc_info, 0, sizeof(struct i2c_board_info)); > + strlcpy(adc_info.type, "max8925", I2C_NAME_SIZE); > + adc_info.platform_data = chip->i2c->dev.platform_data; > + if (pdata->tsc_irq > 0) > + adc_info.irq = pdata->tsc_irq; > + chip->adc = i2c_new_probed_device(chip->i2c->adapter, > + &adc_info, addr_adc); > + i2c_set_clientdata(chip->adc, chip); > + } > + /* Initialize max8925 until all of three I2C clients are ready */ > + if (++count == 3) > + max8925_device_init(chip, pdata); This has to be rewritten, but first let me try to understand something: You need the max9825 core to have pointers to the other i2c devices because of how it handles IRQs right ? In your max9825 irq handler you need to be able to access the rtc and the adc devices, right ? And then on yourrtc and power drivers, you actually only access the max8925 core i2c registers ? I would appreciate if most of your answers to those questions could morph into code comments to this probe routine. Then, about the code design: Having your probe routine relying on those static variable is not ok. If I undersand you correctly, what you trying to achieve here is not calling max8925_device_init() before you actually have the core, the i2c and the adc pointer setup ? If that's so, why not simply use i2c_new_dummy() instead of having to handle recursive calls of your probe routine ? Cheers, Samuel. > return 0; > } > @@ -171,10 +195,23 @@ static int __devinit max8925_probe(struct > i2c_client *client, > static int __devexit max8925_remove(struct i2c_client *client) > { > struct max8925_chip *chip = i2c_get_clientdata(client); > + static int init_gpm = 1, init_adc = 1, init_rtc = 1; > > - max8925_device_exit(chip); > - i2c_set_clientdata(client, NULL); > - kfree(chip); > + if (client->addr == ADC_I2C_ADDR) > + init_adc = 0; > + else if (client->addr == RTC_I2C_ADDR) > + init_rtc = 0; > + else > + init_gpm = 0; > + if (!init_gpm && !init_rtc && !init_adc) { > + max8925_device_exit(chip); > + i2c_unregister_device(chip->adc); > + i2c_unregister_device(chip->rtc); > + i2c_set_clientdata(chip->adc, NULL); > + i2c_set_clientdata(chip->rtc, NULL); > + i2c_set_clientdata(chip->i2c, NULL); > + kfree(chip); > + } > return 0; > } > > -- > 1.5.6.5 -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4] mfd: update i2c driver for max8925 2010-01-29 19:54 ` Samuel Ortiz @ 2010-02-02 14:22 ` Haojian Zhuang 2010-02-02 15:15 ` Samuel Ortiz 2010-02-05 9:06 ` Samuel Ortiz 0 siblings, 2 replies; 6+ messages in thread From: Haojian Zhuang @ 2010-02-02 14:22 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jan 29, 2010 at 2:54 PM, Samuel Ortiz <sameo@linux.intel.com> wrote: > Hi Haojian, > > This has to be rewritten, but first let me try to understand something: You > need the max9825 core to have pointers to the other i2c devices because of how > it handles IRQs right ? In your max9825 irq handler you need to be able to > access the rtc and the adc devices, right ? And then on yourrtc and power > drivers, you actually only access the max8925 core i2c registers ? > I would appreciate if most of your answers to those questions could morph into > code comments to this probe routine. > > Then, about the code design: Having your probe routine relying on those static > variable is not ok. If I undersand you correctly, what you trying to achieve > here is not calling max8925_device_init() before you actually have the core, > the i2c and the adc pointer setup ? > If that's so, why not simply use i2c_new_dummy() instead of having to handle > recursive calls of your probe routine ? > I wants to initialize max8925 after all three components probed. In core driver, irq handling needs to access all three components. In power supply driver, both adc and generic components are accessed. It's the reason that using three i2c clients in one driver. Yes, the code is not very clear. Now I use i2c_new_dummy() now. Now I attache new patches now. Updates are in below. 1. use i2c_new_dummy() to replace i2c_new_probed_device(). 2. add onkey driver 3. remove unused i2c pointer in rtc driver Thanks Haojian -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-mfd-update-i2c-driver-for-max8925.patch Type: text/x-patch Size: 3590 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100202/ef1c8d48/attachment-0005.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0002-mfd-update-irq-handler-in-max8925.patch Type: text/x-patch Size: 29175 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100202/ef1c8d48/attachment-0006.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0003-rtc-enable-rtc-in-max8925.patch Type: text/x-patch Size: 10219 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100202/ef1c8d48/attachment-0007.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0004-power-enable-power-supply-of-max8925.patch Type: text/x-patch Size: 16348 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100202/ef1c8d48/attachment-0008.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0005-input-enable-onkey-driver-of-max8925.patch Type: text/x-patch Size: 5956 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100202/ef1c8d48/attachment-0009.bin> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4] mfd: update i2c driver for max8925 2010-02-02 14:22 ` Haojian Zhuang @ 2010-02-02 15:15 ` Samuel Ortiz 2010-02-03 1:44 ` Haojian Zhuang 2010-02-05 9:06 ` Samuel Ortiz 1 sibling, 1 reply; 6+ messages in thread From: Samuel Ortiz @ 2010-02-02 15:15 UTC (permalink / raw) To: linux-arm-kernel Hi Haojian, On Tue, Feb 02, 2010 at 09:22:58AM -0500, Haojian Zhuang wrote: > On Fri, Jan 29, 2010 at 2:54 PM, Samuel Ortiz <sameo@linux.intel.com> wrote: > I wants to initialize max8925 after all three components probed. In > core driver, irq handling needs to access all three components. In > power supply driver, both adc and generic components are accessed. > It's the reason that using three i2c clients in one driver. Ok, that's what I understood. Adding appropriate comments to the code to describe this would be really helpful. > Yes, the code is not very clear. Now I use i2c_new_dummy() now. Now I > attache new patches now. > > Updates are in below. > 1. use i2c_new_dummy() to replace i2c_new_probed_device(). > 2. add onkey driver > 3. remove unused i2c pointer in rtc driver Very good, I just have one last comment: > @@ -142,27 +138,28 @@ static int __devinit max8925_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > struct max8925_platform_data *pdata = client->dev.platform_data; > - struct max8925_chip *chip; > + static struct max8925_chip *chip; That no longer needs to be static. The rest of the code looks fine to me. Once you remove that static definition, and you add some comments explaining why you need those 3 i2c pointers, I'll merge it. Thanks for your work. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4] mfd: update i2c driver for max8925 2010-02-02 15:15 ` Samuel Ortiz @ 2010-02-03 1:44 ` Haojian Zhuang 0 siblings, 0 replies; 6+ messages in thread From: Haojian Zhuang @ 2010-02-03 1:44 UTC (permalink / raw) To: linux-arm-kernel On Tue, Feb 2, 2010 at 10:15 AM, Samuel Ortiz <sameo@linux.intel.com> wrote: >> Yes, the code is not very clear. Now I use i2c_new_dummy() now. Now I >> attache new patches now. >> >> Updates are in below. >> 1. use i2c_new_dummy() to replace i2c_new_probed_device(). >> 2. add onkey driver >> 3. remove unused i2c pointer in rtc driver > Very good, I just have one last comment: > > >> @@ -142,27 +138,28 @@ static int __devinit max8925_probe(struct i2c_client *client, >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?const struct i2c_device_id *id) >> ?{ >> ? ? ? struct max8925_platform_data *pdata = client->dev.platform_data; >> - ? ? struct max8925_chip *chip; >> + ? ? static struct max8925_chip *chip; > That no longer needs to be static. > The rest of the code looks fine to me. Once you remove that static > definition, and you add some comments explaining why you need those 3 i2c > pointers, I'll merge it. > Thanks a lot. I update the patch. Now the fix is in below. 1. Remove static on max8925_chip. 2. Add more comments on using 3 i2c pointers. Best Regards Haojian -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-mfd-update-i2c-driver-for-max8925.patch Type: text/x-patch Size: 3590 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100202/363c745d/attachment.bin> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4] mfd: update i2c driver for max8925 2010-02-02 14:22 ` Haojian Zhuang 2010-02-02 15:15 ` Samuel Ortiz @ 2010-02-05 9:06 ` Samuel Ortiz 1 sibling, 0 replies; 6+ messages in thread From: Samuel Ortiz @ 2010-02-05 9:06 UTC (permalink / raw) To: linux-arm-kernel Hi Haojian On Tue, Feb 02, 2010 at 09:22:58AM -0500, Haojian Zhuang wrote: > Yes, the code is not very clear. Now I use i2c_new_dummy() now. Now I > attache new patches now. > > Updates are in below. > 1. use i2c_new_dummy() to replace i2c_new_probed_device(). > 2. add onkey driver > 3. remove unused i2c pointer in rtc driver > > Thanks > Haojian All 4 patches applied, many thanks. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-02-05 9:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-01-25 11:07 [PATCH 1/4] mfd: update i2c driver for max8925 Haojian Zhuang 2010-01-29 19:54 ` Samuel Ortiz 2010-02-02 14:22 ` Haojian Zhuang 2010-02-02 15:15 ` Samuel Ortiz 2010-02-03 1:44 ` Haojian Zhuang 2010-02-05 9:06 ` Samuel Ortiz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).