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

[Lee Jones <lee.jones@linaro.org>]

On Fri, 16 Aug 2013, Chao Xie wrote:

> On Thu, Aug 15, 2013 at 6:07 PM, Lee Jones <lee.jones@linaro.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.

I think the first step is to fix the call-back. I can't say for sure
as I haven't seen it, but the chances are that it can be implemented
in a different way and eradicated. I'm keen not to accept the code
above, as I believe it's fundamentally broken.

> 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.

I don't think you sent patches 1 and 2 to me? Can you resend them as a
separate patch-set please?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog