Re: [PATCH V2 2/2] mfd: 88pm88x: initialize 88pm886/88pm880 base support

[Lee Jones]

On Fri, 26 Jun 2015, Yi Zhang wrote:
> On Thu, Jun 25, 2015 at 09:32:48AM +0100, Lee Jones wrote:
> > On Fri, 12 Jun 2015, Yi Zhang wrote:
> > 
> > > 88pm886 and 88pm880 are combo PMIC chip, which integrates
> > > regulator, onkey, rtc, gpadc, charger, fuelgauge function;
> > > 
> > > this patch add the basic support for them, adding related resource, such as
> > > interrupt, preparing for the client-device driver
> > > 
> > > Signed-off-by: Yi Zhang <yizhang@xxxxxxxxxxx>
> > > ---
> > >  drivers/mfd/88pm880-table.c     | 173 ++++++++++++
> > >  drivers/mfd/88pm886-table.c     | 173 ++++++++++++
> > >  drivers/mfd/88pm88x-core.c      | 584 ++++++++++++++++++++++++++++++++++++++++
> > >  drivers/mfd/88pm88x-i2c.c       | 167 ++++++++++++
> > >  drivers/mfd/88pm88x-irq.c       | 171 ++++++++++++
> > >  drivers/mfd/88pm88x.h           |  51 ++++
> 
>   Thanks very much for your patience on such a long patch, it is my
>   fault, I will modify according to your precious comments and split
>   them in the next version, thanks very much;

You are welcome.

[...] /* Cut 100's of lines where you were agreeing with me */

In future, please cut all of the lines that you agree with me and only
leave in the useful or contentious items.

> > > +	chip->ldo_page_addr = client->addr + 1;
> > > +	chip->power_page_addr = client->addr + 1;
> > > +	chip->gpadc_page_addr = client->addr + 2;
> > > +	chip->battery_page_addr = client->addr + 3;
> > > +	chip->buck_page_addr = client->addr + 4;
> > > +	chip->test_page_addr = client->addr + 7;
> > 
> > I have no idea what's going on here, but it's almost certainly not
> > correct.  Instead of separating these out per device have a base
> > address and create DEFINES for each of them.
> 
>   it is because this PMIC has several i2c address, each has different
>   function, the offset vesus the client->addr is fixed, the client->addr
>   is 0x30 from device tree, then the gpadc_page_addr is 0x32, that is
>   why I use client->addr + 2;
> 
>   you mean the DEFINES are better?

I do mean that, yes.  If you don't want supply nodes for each of the
client devices (what is the reason for that?), then just supply the
base address and use DEFINES to obtain the offsets.

[...] /* Lots more 'yes's removed */

> > > +	ret = mfd_add_devices(chip->dev, 0, common_cell_devs,
> > 
> > What does 0 mean?
> 
>   I should use -1 here, though it _seems_ works..

Please use the pre-allocated platform DEFINEs for them.

[...]

> > > +	for (;;)
> > > +		cpu_relax();
> > 
> > What's the point of this?
> 
>   I just would like to busy loop at the end of the power_off callback;

Is that when you're meant to do?  Why can't you just return?

[...]

> > > +#define CELL_DEV(_name, _r, _compatible, _id) { \
> > > +	.name = _name, \
> > > +	.of_compatible = _compatible, \
> > > +	.num_resources = ARRAY_SIZE(_r), \
> > > +	.resources = _r, \
> > > +	.id = _id, \
> > > +	}
> > 
> > This is not required.
> > 
> > If you feel the need for an MFD Cell macro, you probably have too many
> > MFD cells and you have done something wrong..
> 
>   Yes, I should use one MFD to cover regulators and one MFD to cover
>   LEDs, I will remember this lesson; thanks;

That wasn't what I was saying.  Just remove the MACRO and add the
information into the cells like all the other MFD drivers do.

[...]

-- 
Lee Jones
Linaro STMicroelectronics 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/