[PATCH 1/4] mfd: update i2c driver for max8925

[PATCH 1/4] mfd: update i2c driver for max8925

[Haojian Zhuang]

[PATCH 1/4] mfd: update i2c driver for max8925

[Samuel Ortiz]

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/

[PATCH 1/4] mfd: update i2c driver for max8925

[Haojian Zhuang]

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>

[PATCH 1/4] mfd: update i2c driver for max8925

[Samuel Ortiz]

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/

[PATCH 1/4] mfd: update i2c driver for max8925

[Haojian Zhuang]

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>

[PATCH 1/4] mfd: update i2c driver for max8925

[Samuel Ortiz]

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/