linux-kernel - Re: [PATCH 3/4] mfd: 88pm800: add device tree support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 16 Aug 2013 09:28:44 +0800
From:	Chao Xie <xiechao.mail@...il.com>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	Chao Xie <chao.xie@...vell.com>, sameo@...ux.intel.com,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4] mfd: 88pm800: add device tree support

On Thu, Aug 15, 2013 at 6:07 PM, Lee Jones <lee.jones@...aro.org> wrote:
>> >> +Optional parent device properties:
>> >> +- marvell,88pm800-irq-write-clear: inicates whether interrupt status is cleared by write
>> >> +- marvell,88pm800-battery-detection: indicats whether need 88pm800 to support battery
>> >> +                             detection or not.
>> >
>> > Not sure what these are. This is why you need to CC the Device Tree
>> > guys.
>> >
>> It is the 88pm805's own configuration.
>> 88pm800-irq-write-clear: when irq happens, the status register is
>> write clear or read clear.
>> 88pm800-battery-detection: whether the battery is connected to chip.
>> It means that whether
>> the chip be aware of battery or not.
>
> As you are adding vendor specific bindings, you need to Cc the Device
> Tree mailing list.
>
>> >> +     if (IS_ENABLED(CONFIG_OF)) {
>> >> +             if (!pdata) {
>> >> +                     pdata = devm_kzalloc(&client->dev,
>> >> +                                          sizeof(*pdata), GFP_KERNEL);
>> >> +                     if (!pdata)
>> >> +                             return -ENOMEM;
>> >> +             }
>> >> +             ret = pm800_dt_init(node, &client->dev, pdata);
>> >> +             if (ret)
>> >> +                     return ret;
>> >> +     } else if (!pdata) {
>> >> +             return -EINVAL;
>> >> +     }
>> >
>> > Replace with:
>> >
>> >         if (!pdata) {
>> >                 if (node)
>> >                         /* <blah> populate pdata with DT </blah> */
>> >                 else
>> >                         return -EINVAL;
>> >         }
>> >
>> The orignial code will cover the following situation.
>> 1. DT enabled, and user pass pdata
>> 2. DT enabled, but user do not pass pdata
>> 3. DT disabled, user pass pdata
>> 4. DT disabled, user do not pass pdata.
>>
>> 88pm805 has a callback for config the it based on platform requirment.
>> I do not want to remove this callback now, because it includes so many
>> configurations.
>> So i allow user can pass pdata with callback if the platform needs to
>> configure the chip.
>
> Mixing DT with pdata is a bad idea. If you need to pass a call-back
> pointer, then _only_ use pdata i.e. get all of your platform specific
> information from pdata, rather than just over-writing sections of it
> with information retrieved from Device Tree.
>
> So:
>
> If pdata  - use pdata and ignore DT completely
> If !pdata:
>    If DT  - use DT
>    If !DT - return -EINVAL
>
> Out of interest, what does your call-back do?
>
Without the callback, the soc still can work.
The callback does job relates to power saving and CP's requirment.
1. LPM configure for the chip based on AP/CP's requriment.
2. 88pm800 OSC configuration
3. Some output pin configuration of 88pm800, for example reset_out_n pin

I want to abstract the callback step by step, so the first step are the patches
that enable DT first.

For the patch 0001 and 0002 are fixes, so if these two patches are all
right, can you
merge them? Then i will submit the 2 DT related patches again with cc
to device tree maillist.

Thanks.

> --
> Lee Jones
> Linaro ST-Ericsson Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/