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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 23 Oct 2011 19:39:36 +0900
From:	Sangbeom Kim <sbkim73@...sung.com>
To:	'Mark Brown' <broonie@...nsource.wolfsonmicro.com>
Cc:	'Samuel Ortiz' <sameo@...ux.intel.com>,
	linux-kernel@...r.kernel.org
Subject: RE: [PATCH 1/3] mfd: Add S5M core driver

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/