From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1758266AbcFHRW0 (ORCPT <rfc822;w@1wt.eu>);
	Wed, 8 Jun 2016 13:22:26 -0400
Received: from mail-oi0-f44.google.com ([209.85.218.44]:33169 "EHLO
	mail-oi0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1758232AbcFHRWZ (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 8 Jun 2016 13:22:25 -0400
MIME-Version: 1.0
In-Reply-To: <20160608143124.GI14888@dell>
References: <1464816460-18750-1-git-send-email-john.stultz@linaro.org>
 <1464816460-18750-3-git-send-email-john.stultz@linaro.org> <20160608143124.GI14888@dell>
From: John Stultz <john.stultz@linaro.org>
Date: Wed, 8 Jun 2016 10:22:23 -0700
Message-ID: <CALAqxLXiyn1KOvE4C6acU8n-0ANT=WWf0sNxP3-CnuAPybnSEg@mail.gmail.com>
Subject: Re: [RFC][PATCH 2/5] hi655x-pmic: Make hi655x pmic logic probe child
 nodes in the dt
To: Lee Jones <lee.jones@linaro.org>
Cc: lkml <linux-kernel@vger.kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Ian Campbell <ijc+devicetree@hellion.org.uk>,
        Kumar Gala <galak@codeaurora.org>,
        Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>,
        Wei Xu <xuwei5@hisilicon.com>, Guodong Xu <guodong.xu@linaro.org>,
        Chen Feng <puck.chen@hisilicon.com>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Jun 8, 2016 at 7:31 AM, Lee Jones <lee.jones@linaro.org> wrote:
>
> Please use full buffer width.  Wrapping at 50 chars is not necessary.

But then how would you be able to review patches on your phone?! :)


>> --- a/drivers/mfd/hi655x-pmic.c
>> +++ b/drivers/mfd/hi655x-pmic.c
>> @@ -39,6 +39,11 @@ static const struct regmap_irq hi655x_irqs[] = {
>>       { .reg_offset = 0, .mask = RESERVE_INT },
>>  };
>>
>> +static const struct of_device_id of_hi655x_pmic_child_match_tbl[] = {
>> +     { .compatible = "hisilicon,hi6552-powerkey", },
>> +     { /* end */ }
>> +};
>> +
>>  static const struct regmap_irq_chip hi655x_irq_chip = {
>>       .name = "hi655x-pmic",
>>       .irqs = hi655x_irqs,
>> @@ -122,6 +127,9 @@ static int hi655x_pmic_probe(struct platform_device *pdev)
>>
>>       platform_set_drvdata(pdev, pmic);
>>
>> +     /* populate sub nodes */
>> +     of_platform_populate(np, of_hi655x_pmic_child_match_tbl, NULL, dev);
>> +
>
> Oh, holey poop, no!
>
> Please don't mix DT and MFD registration like this.
>
> Drivers should contain either of_platform_populate() or
> mfd_add_devices(), but never both.
>
>>       ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, hi655x_pmic_devs,
>>                             ARRAY_SIZE(hi655x_pmic_devs), NULL, 0, NULL);
>>       if (ret) {


So apologies for my ignorance here. I'm not quite sure I see what the
right thing to do. I tried adding a "hi65xx-powerkey" entry in the
hi655x_pmic_devs, but then mfd_add_devices doesn't seem to provide any
of the irq info from the DT.

The only way I can think to make it work w/o calling to populate
sub-nodes is to pull the powerkey subnode out of the pmic node. It
seems to work, but I'm not sure if it properly describes the hardware
then.  Sorry again for being a bit clueless here.

Thanks for the review!
-john