RE: [PATCH 1/3] mfd: Add S5M core driver

[Sangbeom Kim <sbkim73@samsung.com>]

Hi!

On Sun, Oct 23, 2011 at 05:37 PM +0900, Mark Brown wrote:

> >  drivers/mfd/s5m-core.c               |  235 +++++++++++++++++++++++++
> >  include/linux/mfd/s5m87xx/s5m-core.h |  313
> > ++++++++++++++++++++++++++++++++++
> 
> Naughty Outlook!

Sorry, I hope that I can send next version by git send-email.

> Actually, looking at this what I'm thinking is that we should put some
> inline functions in the regmap header which let you write this as
> something like
> 
> 	int s5m_reg_read(struct s5m87xx_dev *s5m87xx, u8 reg, u8 *dest)
> 	{
> 		return regmap_read_u8(s5m87xx->dev, reg, dest);
> 	}
> (which might be inline itself) as most of what you're doing here is the
> type conversion for the arguments.

OK, I will modify it.
> 
> > +EXPORT_SYMBOL(s5m_bulk_read);
> 
> All this stuff should be _GPL as the regmap core is _GPL - you shouldn't
> wrap a _GPL function with a non-GPL one.

You mean that EXPORT_SYMBOL should be replace with EXPORT_SYMBOL_GPL?

> > +static struct mfd_cell s5m87xx_devs[] = {
> > +	{
> > +		.name = "s5m8763-pmic",
> > +	}, {
> > +		.name = "s5m8767-pmic",
> > +	}, {
> 
> It looks a bit odd to have both simultaneously but I guess this will
> become more obvious later on?  I guess what I'd expect is one of these
> arrays per device variant.

My intention of this mfd driver is supporting all samsung mfd.
It is desirable that a core driver handle various driver to prevent produce
similar code.
So, If I want to implement like above concept, What kind of approach can be
advised?
 
> > +	s5m87xx->dev = &i2c->dev;
> > +	s5m87xx->i2c = i2c;
> > +	s5m87xx->irq = i2c->irq;
> 
> Is SPI supported?

SPI isn't supported by Samsung mfd

> 
> > +	dev_info(s5m87xx->dev ,"SAMSUNG S5M MFD\n");
> 
> Probably best to remove this for mainline, it's not really adding
> anything.

OK, I will remove it.

> > +static const struct i2c_device_id s5m87xx_i2c_id[] = {
> > +	{ "s5m87xx", 0 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, s5m87xx_i2c_id);
> 
> It'd be better to have one entry per chip explicitly naming the device,
> that way boards are describing which they have and the kernel can apply
> any device specific configuration.
> 
As I write above, I want to use a core driver for various mfd.
What kind of approach can be advised?

I'm very thank for your kindly review.
Sangbeom.