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

[Mark Brown]

On Sun, Oct 23, 2011 at 05:07:22PM +0900, Sangbeom Kim wrote:

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

Naughty Outlook!

> +int s5m_reg_read(struct s5m87xx_dev *s5m87xx, u8 reg, u8 *dest)
> +{
> +	int ret;
> +	unsigned int val;
> +
> +	ret = regmap_read(s5m87xx->regmap, reg, &val);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	ret &= 0xff;
> +	*dest = ret;

This should be manipulating val rather than ret.  You shouldn't need the
&= 0xff bit but it's understandable for paranoia.

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.

> +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.

> +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.

> +	s5m87xx->dev = &i2c->dev;
> +	s5m87xx->i2c = i2c;
> +	s5m87xx->irq = i2c->irq;

Is SPI supported?

> +	dev_info(s5m87xx->dev ,"SAMSUNG S5M MFD\n");

Probably best to remove this for mainline, it's not really adding
anything.

> +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.

> +static int s5m_suspend(struct i2c_client *i2c, pm_message_t state)
> +{
> +	struct s5m87xx_dev *s5m87xx = i2c_get_clientdata(i2c);
> +
> +	if (s5m87xx->wakeup)
> +		enable_irq_wake(s5m87xx->irq);
> +
> +	disable_irq(s5m87xx->irq);

Enabling wake and disabling the IRQ looks odd...  perhaps an else?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/