Messages in this thread | | | From | Sangbeom Kim <> | Subject | RE: [PATCH 1/3] mfd: Add S5M core driver | Date | Sun, 23 Oct 2011 19:39:36 +0900 |
| |
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.
| |