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
