Re: [PATCH v3 08/10] mfd: da9063: Register RTC only on DA9063L

[Marek Vasut]

On 06/05/2018 09:53 AM, Lee Jones wrote:
> On Sat, 02 Jun 2018, Marek Vasut wrote:
> 
>> The DA9063L does not contain RTC block, unlike the full DA9063.
>> Split the RTC block into separate mfd cell and register it only
>> on DA9063.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
>> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>> Cc: Lee Jones <lee.jones@xxxxxxxxxx>
>> Cc: Mark Brown <broonie@xxxxxxxxxx>
>> Cc: Steve Twiss <stwiss.opensource@xxxxxxxxxxx>
>> Cc: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
>> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
>> ---
>> V2: No change
>> V3: Rework of mfd: da9063: Disallow RTC on DA9063L
>> ---
>>  drivers/mfd/da9063-core.c | 30 +++++++++++++++++++++++-------
>>  1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
>> index eebca3442cf3..b05910c797af 100644
>> --- a/drivers/mfd/da9063-core.c
>> +++ b/drivers/mfd/da9063-core.c
>> @@ -76,7 +76,7 @@ static struct resource da9063_hwmon_resources[] = {
>>  };
>>  
>>  
>> -static const struct mfd_cell da9063_devs[] = {
>> +static const struct mfd_cell da9063_common_devs[] = {
>>  	{
>>  		.name		= DA9063_DRVNAME_REGULATORS,
> 
> Appreciate that these are historical, but these device name defines
> make me shudder.  They only serve to act as an obfuscation layer when
> debugging at platform level.  Please consider getting rid of them.

The macro can be shared between the core and the drivers, so the names
never run out of sync.

>>  		.num_resources	= ARRAY_SIZE(da9063_regulators_resources),
>> @@ -100,15 +100,19 @@ static const struct mfd_cell da9063_devs[] = {
>>  		.resources	= da9063_onkey_resources,
>>  		.of_compatible = "dlg,da9063-onkey",
>>  	},
>> +	{
>> +		.name		= DA9063_DRVNAME_VIBRATION,
>> +	},
> 
> Place this on a single line please.

This would only make the style inconsistent with the ie. LEDs entry.

>         { .name	= DA9063_DRVNAME_VIBRATION },
> 
>> +};
>> +
>> +/* Only present on DA9063 , not on DA9063L */
>> +static const struct mfd_cell da9063_devs[] = {
>>  	{
>>  		.name		= DA9063_DRVNAME_RTC,
>>  		.num_resources	= ARRAY_SIZE(da9063_rtc_resources),
>>  		.resources	= da9063_rtc_resources,
>>  		.of_compatible	= "dlg,da9063-rtc",
>>  	},
>> -	{
>> -		.name		= DA9063_DRVNAME_VIBRATION,
>> -	},
>>  };
>>  
>>  static int da9063_clear_fault_log(struct da9063 *da9063)
>> @@ -225,16 +229,28 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
>>  
>>  	da9063->irq_base = regmap_irq_chip_get_base(da9063->regmap_irq);
>>  
>> -	ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
>> -			      ARRAY_SIZE(da9063_devs), NULL, da9063->irq_base,
>> -			      NULL);
>> +	ret = mfd_add_devices(da9063->dev, -1, da9063_common_devs,
> 
> Please consider updating the -1's in this file with the appropriate
> define in a separate patch.

Done

>> +			      ARRAY_SIZE(da9063_common_devs),
>> +			      NULL, da9063->irq_base, NULL);
>>  	if (ret) {
>>  		dev_err(da9063->dev, "Cannot add MFD cells\n");
>>  		goto err_irq_exit;
>>  	}
>>  
>> +	if (da9063->type == PMIC_TYPE_DA9063) {
>> +		ret = mfd_add_devices(da9063->dev, -1, da9063_devs,
>> +				      ARRAY_SIZE(da9063_devs),
>> +				      NULL, da9063->irq_base, NULL);
>> +		if (ret) {
>> +			dev_err(da9063->dev, "Cannot add MFD cells\n");
> 
> Better to be more general here.
> 
> "Failed to add child devices" or such.
> 
> Users don't tend to care about MFD cells.

Hum, done.

>> +			goto err_mfd_cleanup;
>> +		}
>> +	}
>> +
>>  	return ret;
>>  
>> +err_mfd_cleanup:
>> +	mfd_remove_devices(da9063->dev);
> 
> Any reason why you can't use devm_*?

Because we need to undo the MFD setup before the IRQ setup.

>>  err_irq_exit:
>>  	da9063_irq_exit(da9063);
>>  	return ret;
> 


-- 
Best regards,
Marek Vasut