Re: [PATCH v7 2/5] MFD: RK808: Add new mfd driver for RK808

[Lee Jones]

On Mon, 01 Sep 2014, Chris Zhong wrote:

> The RK808 chip is a power management IC for multimedia and handheld
> devices. It contains the following components:
> 
> - Regulators
> - RTC
> - Clkout
> 
> The RK808 core driver is registered as a platform driver and provides
> communication through I2C with the host device for the different
> components.
> 
> Signed-off-by: Chris Zhong <zyw@xxxxxxxxxxxxxx>
> Signed-off-by: Zhang Qing <zhangqing@xxxxxxxxxxxxxx>

Couple of nits.  Once fixed you can apply my:

Acked-by: Lee Jones <lee.jones@xxxxxxxxxx>

[...]

> +/*
> + * MFD core driver for Rockchip RK808
> + *
> + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd

Author?

> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */

[...]

> +static int rk808_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *id)
> +{
> +	int i;
> +	int ret;
> +	int pm_off = 0;
> +	struct rk808 *rk808;
> +	struct device_node *np = client->dev.of_node;

Reverse these declarations please, structs at the top etc.

[...]

> +	rk808->i2c = client;
> +	i2c_set_clientdata(client, rk808);

'\n' here.

> +	ret = mfd_add_devices(&client->dev, -1,
> +			      rk808s, ARRAY_SIZE(rk808s),
> +			      NULL, 0, regmap_irq_get_domain(rk808->irq_data));
> +	if (ret) {
> +		dev_err(&client->dev, "failed to add MFD devices %d\n", ret);
> +		goto err_irq;
> +	}
> +
> +	if (np) {

There has to be an 'np'.  The driver depends on OF.

> +		pm_off = of_property_read_bool(np,
> +					"rockchip,system-power-controller");
> +		if (pm_off && !pm_power_off) {
> +			rk808_i2c_client = client;
> +			pm_power_off = rk808_device_shutdown;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_irq:
> +	regmap_del_irq_chip(client->irq, rk808->irq_data);
> +	return ret;
> +}

[...]

> +static struct i2c_driver rk808_i2c_driver = {
> +	.driver = {
> +		.name = "rk808",
> +		.of_match_table = of_match_ptr(rk808_of_match),

No need to use of_match_ptr() now that you depend on OF.

> +	},
> +	.probe    = rk808_probe,
> +	.remove   = rk808_remove,
> +	.id_table = rk808_ids,
> +};
> +
> +module_i2c_driver(rk808_i2c_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Chris Zhong <zyw@xxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Zhang Qing <zhangqing@xxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("RK808 PMIC driver");

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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/