RE: [PATCH V1] mfd: da9063: Add device tree support

[Opensource [Steve Twiss]]

On 19 January 2015 09:54, Lee Jones wrote:

> On Wed, 14 Jan 2015, Steve Twiss wrote:
> > From: Steve Twiss <stwiss.opensource@xxxxxxxxxxx>
> >
> > ---
> > Checks performed with linux-next/v3.19-rc4/scripts/checkpatch.pl
> >  da9063.txt                total: 0 errors, 0 warnings, 94 lines checked
> >  da9063-core.c             total: 0 errors, 0 warnings, 192 lines checked
> >  da9063-i2c.c              total: 0 errors, 0 warnings, 277 lines checked
> >  core.h                    total: 0 errors, 0 warnings, 99 lines checked
> 
> There is no need to put this in here really.  It is assumed that
> checkpatch.pl has been run and that no warnings/errors exists.
> 

ok -- I will remove this in future

[...]

> > +Required properties:
> > +
> > +- compatible : Should be "dlg,da9063-ca", "dlg,da9063-bb" or/and
> > +  "dlg,da9063-ad".
> 
> What are 'ca', 'bb' and 'ad'?

There are multiple variants of the DA9063 chip -- historical support.
The AD chip has a different register map to BB and CA  silicon and the
variant information is read from the chip at run-time and used to set
up the correct regmap_config tables during initialisation.

[...]

> > +++ b/drivers/mfd/da9063-core.c
> > @@ -85,6 +85,7 @@ static const struct mfd_cell da9063_devs[] = {
> >  		.name		= DA9063_DRVNAME_LEDS,
> >  	},
> >  	{
> > +		.of_compatible = "dlg,da9063-watchdog",
> 
> Can you put the of_compatible attribute at the end of the structure
> please?
> 
> >  		.name		= DA9063_DRVNAME_WATCHDOG,
> >  	},
> >  	{
> > @@ -98,6 +99,7 @@ static const struct mfd_cell da9063_devs[] = {
> >  		.resources	= da9063_onkey_resources,
> >  	},
> >  	{
> > +		.of_compatible = "dlg,da9063-rtc",
> 
> Same here.

Sure.

[...]
> >
> > +static const struct of_device_id da9063_dt_ids[] = {
> > +	{ .compatible = "dlg,da9063-ad", },
> > +	{ .compatible = "dlg,da9063-bb", },
> > +	{ .compatible = "dlg,da9063-ca", },
> 
> Why is there a need to differientiae between 'ad', 'bb' and 'ca'
> (whatever they are)?

As described above.

> > +	{ /* sentinel */ }
> 
> Nit: You can drop this comment.

ok.

[...]

I will also make changes to separate into multiple patches
as described in https://lkml.org/lkml/2015/1/19/149
Will re-send as patch V2 ...

Regards,
Steve