Re: [PATCH V6 2/8] mfd: max77620: add core driver for MAX77620/MAX20024

[Laxman Dewangan]

On Monday 01 February 2016 02:29 PM, Lee Jones wrote:
> On Fri, 29 Jan 2016, Laxman Dewangan wrote:
>
> > Thanks Lee for review.
> > I will take care of most of stuff on next version of patch.
> >
> > However, I have some query form your comment.
> > On Friday 29 January 2016 02:36 PM, Lee Jones wrote:
> > > On Thu, 28 Jan 2016, Laxman Dewangan wrote:
> > >
> > >
> > > > +	}
> > > > +
> > > > +#define MAX20024_SUB_MODULE_NO_RES(_name, _id)			\
> > > > +	[_id] = {						\
> > > > +		.name = "max20024-"#_name,			\
> > > > +		.id = _id,					\
> > > > +	}
> > > I don't want people hand-rolling this stuff.  If it's useful to you,
> > > it's useful to others, so great a generic implementation that lives in
> > > the kernel headers directory.
> > yaah, generic implementation possible. I can put the new defines in
> > the mfd/core.h.
> >
> > This will be similar to
> > +/* Define mfd cells with name and resource */
> > +#define DEFINE_MFD_CELL_NAME_RESOURCE(_name, _res)             \
> > +       {                                                       \
> > +               .name = (_name),                                \
> > +               .num_resources = ARRAY_SIZE((res)),             \
> > +               .resources = (_res),                            \
> > +       }
> > +
> > +/* Define mfd cells with name */
> > +#define DEFINE_MFD_CELL_NAME(_name)                            \
> > +       {                                                       \
> > +               .name = (_name),                                \
> > +       }
> > +
> >
> > This will be separate patch and should be applied before this series.
> > Does it look fine?
> Hmm... Actually, I have my own ideas of how this should look.  How do
> you feel about me submitting my own patch.  I'll keep you on Cc, so
> you can review and make use of it in your set.

Sure, I am fine with this. Please send the patch and CC me so that I can 
make my patch on top of it and void my mfd/core.h patch.
Thanks for taking care.


> > > > +static const struct i2c_device_id max77620_id[] = {
> > > > +	{"max77620", MAX77620},
> > > > +	{"max20024", MAX20024},
> > > > +	{},
> > > > +};
> > > > +MODULE_DEVICE_TABLE(i2c, max77620_id);
> > > > +
> > > > +static const struct of_device_id max77620_of_match[] = {
> > > > +	{
> > > > +		.compatible = "maxim,max77620",
> > > > +		.data = &max77620_cells,
> > > > +	}, {
> > > > +		.compatible = "maxim,max20024",
> > > > +		.data = &max20024_cells,
> > > > +	}, {
> > > > +	},
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, max77620_of_match);
> > > This is not acceptable.  EITHER use DT OR MFD methods of registering
> > > devices, do not mix the two.
> > You mean I need to either provide the i2c_device_id table or the
> > of_device_id table, not both?
> > Do I need to protect it by CONFIG_OF?
> >
> > This only support the DT method of registration. So do I need to
> > remove i2c_device_id?
> No, I mean I don't want you providing platform data via an MFD cell
> and passing it through the OF .data attribute.

This is not platform data, this is chip specific data.
However, In patch V7, I removed this and use the id_table for the chip 
data to optimized the chip ID.
This is not required.