Re: [PATCH v2 18/21] mfd: htc-pasic3: Prepare driver for leds-pasic3

[Petr Cvek <petr.cvek@tul.cz>]

Dne 18.8.2015 v 08:52 Lee Jones napsal(a):
> On Tue, 18 Aug 2015, Petr Cvek wrote:
> 
>> Fix register definitions and prepare structures for new leds-pasic3
>> driver.
>>
>> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
>> ---
>>  drivers/mfd/htc-pasic3.c       | 54 ++++++++++++++++-----------
>>  include/linux/mfd/htc-pasic3.h | 85 +++++++++++++++++++++++++++++++-----------
>>  2 files changed, 97 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/mfd/htc-pasic3.c b/drivers/mfd/htc-pasic3.c
>> index e88d4f6..16e156d 100644
>> --- a/drivers/mfd/htc-pasic3.c
>> +++ b/drivers/mfd/htc-pasic3.c
>> @@ -3,6 +3,9 @@
>>   *
>>   * Copyright (C) 2006 Philipp Zabel <philipp.zabel@gmail.com>
>>   *
>> + * 2015:	Added registers for LED and RESET, see htc-pasic3.h
>> + *		-- Petr Cvek
>> + *
> 
> This is pretty unconventional.
> 
> Please look to see what others have done.

LED support: Petr Cvek <petr.cvek@tul.cz>

OK?

>> @@ -130,6 +140,7 @@ static int __init pasic3_probe(struct platform_device *pdev)
>>  	struct device *dev = &pdev->dev;
>>  	struct pasic3_data *asic;
>>  	struct resource *r;
>> +	struct pasic3_leds_pdata *leds_pdata;
>>  	int ret;
>>  	int irq = 0;
>>  
>> @@ -162,6 +173,8 @@ static int __init pasic3_probe(struct platform_device *pdev)
>>  	/* calculate bus shift from mem resource */
>>  	asic->bus_shift = (resource_size(r) - 5) >> 3;
>>  
>> +	spin_lock_init(&asic->lock);
>> +
>>  	if (pdata && pdata->clock_rate) {
>>  		ds1wm_pdata.clock_rate = pdata->clock_rate;
>>  		/* the first 5 PASIC3 registers control the DS1WM */
>> @@ -172,13 +185,12 @@ static int __init pasic3_probe(struct platform_device *pdev)
>>  			dev_warn(dev, "failed to register DS1WM\n");
>>  	}
>>  
>> -	if (pdata && pdata->led_pdata) {
>> -		led_cell.platform_data = pdata->led_pdata;
>> -		led_cell.pdata_size = sizeof(struct pasic3_leds_machinfo);
>> -		ret = mfd_add_devices(&pdev->dev, pdev->id, &led_cell, 1, r,
>> -				      0, NULL);
>> -		if (ret < 0)
>> -			dev_warn(dev, "failed to register LED device\n");
>> +	if (pdata && pdata->pasic3_leds) {
>> +		leds_pdata = dev_get_platdata(&pdata->pasic3_leds->dev);
> 
> Whoa!  You're passing a pointer to a device through pdata?
> 
> I don't like that at all.  Please explain.

> 
>> +		if (leds_pdata) {
>> +			leds_pdata->mfd_dev = dev;
>> +			platform_device_register(pdata->pasic3_leds);
> 
> What's the idea here?

Actually, I don't know how to do this in a clean way as pasic3_read_register() and pasic3_write_register() require device struct to pass address for accessing the registers. Only way would be to rewrite all functions which call pasic3_*_register() (removing struct device *dev and change it to struct pasic3_data *asic).

Petr