Hi Matti,
On Wed, Nov 17, 2010 at 03:42:00PM +0200, Matti J. Aaltonen wrote:
> This is the core of the WL1273 FM radio driver, it connects
> the two child modules.
>
> This is a parent for two child drivers:
> drivers/media/radio/radio-wl1273.c and sound/soc/codecs/wl1273.c
>
> Radio-wl1273 implements the V4L2 interface and the communication to the device.
> The ALSA codec is for digital audio. Without the codec only analog audio
> is available.
>
> Signed-off-by: Matti J. Aaltonen <matti.j.aaltonen@xxxxxxxxx>
My review:
> +config WL1273_CORE
> + tristate
> + depends on I2C
> + select MFD_CORE
> + default n
> +
You need to be a lot more verbose here. Nobody knows what this wl1273 core
driver is for.
Also, please rename your config to MFD_WL1273_CORE.
> +
> +#include <linux/delay.h>
Not needed.
> +#include <linux/mfd/wl1273-core.h>
> +#include <linux/slab.h>
> +#include <media/v4l2-common.h>
Not needed neither.
> +#define DRIVER_DESC "WL1273 FM Radio Core"
> +
> +static struct i2c_device_id wl1273_driver_id_table[] = {
> + { WL1273_FM_DRIVER_NAME, 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, wl1273_driver_id_table);
> +
> +static int wl1273_core_remove(struct i2c_client *client)
> +{
> + struct wl1273_core *core = i2c_get_clientdata(client);
> +
> + dev_dbg(&client->dev, "%s\n", __func__);
> +
> + mfd_remove_devices(&client->dev);
> + i2c_set_clientdata(client, NULL);
> + kfree(core);
> +
> + return 0;
> +}
> +
> +static int __devinit wl1273_core_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct wl1273_fm_platform_data *pdata = client->dev.platform_data;
> + int r = 0;
> + struct wl1273_core *core;
> + int children = 0;
> +
> + dev_dbg(&client->dev, "%s\n", __func__);
> +
> + if (!pdata) {
> + dev_err(&client->dev, "No platform data.\n");
> + return -EINVAL;
> + }
> +
> + core = kzalloc(sizeof(*core), GFP_KERNEL);
> + if (!core)
> + return -ENOMEM;
> +
> + core->pdata = pdata;
> + core->client = client;
> + mutex_init(&core->lock);
> +
> + i2c_set_clientdata(client, core);
> +
> + /* the radio child must be first */
> + if (pdata->children & WL1273_RADIO_CHILD) {
> + struct mfd_cell *cell = &core->cells[children];
Please add a line between your variable declarations and the rest of the code.
> + dev_dbg(&client->dev, "%s: Have V4L2.\n", __func__);
> +
> + cell->name = "wl1273_fm_radio";
> + cell->platform_data = &core;
> + cell->data_size = sizeof(core);
> + children++;
> + } else {
> + dev_err(&client->dev, "Cannot function without radio child.\n");
> + r = -EINVAL;
> + goto err_radio_child;
> + }
So I'd prefer something like:
if (!pdata->children & WL1273_RADIO_CHILD) {
dev_err(...);
return -EINVAL;
}
before you actually allocate the core pointer.
> + if (pdata->children & WL1273_CODEC_CHILD) {
> + struct mfd_cell *cell = &core->cells[children];
A new line here as well, please.
> + dev_dbg(&client->dev, "%s: Have codec.\n", __func__);
> + cell->name = "wl1273-codec";
> + cell->platform_data = &core;
> + cell->data_size = sizeof(core);
> + children++;
> + }
> +
> + if (children) {
> + dev_dbg(&client->dev, "%s: Have children.\n", __func__);
> + r = mfd_add_devices(&client->dev, -1, core->cells,
> + children, NULL, 0);
> + } else {
> + dev_err(&client->dev, "No platform data found for children.\n");
> + r = -ENODEV;
> + }
> +
> + if (!r)
> + return 0;
I'd prefere:
if (children == 0) {
dev_err(....);
r = -ENODEV;
goto err;
}
dev_dbg(....);
return mfd_add_devices();
err:
....
> +err_radio_child:
> + i2c_set_clientdata(client, NULL);
> + pdata->free_resources();
> + kfree(core);
> +
> + dev_dbg(&client->dev, "%s\n", __func__);
> +
> + return r;
> +}
> +
> +static struct i2c_driver wl1273_core_driver = {
> + .driver = {
> + .name = WL1273_FM_DRIVER_NAME,
> + },
> + .probe = wl1273_core_probe,
> + .id_table = wl1273_driver_id_table,
> + .remove = __devexit_p(wl1273_core_remove),
> +};
> +
> +static int __init wl1273_core_init(void)
> +{
> + int r;
> +
> + r = i2c_add_driver(&wl1273_core_driver);
> + if (r) {
> + pr_err(WL1273_FM_DRIVER_NAME
> + ": driver registration failed\n");
> + return r;
> + }
> +
> + return 0;
Here you can just return r.
> +}
> +
> +static void __exit wl1273_core_exit(void)
> +{
> + flush_scheduled_work();
What is that for ?
> + i2c_del_driver(&wl1273_core_driver);
> +}
> +late_initcall(wl1273_core_init);
> +module_exit(wl1273_core_exit);
> +
> +MODULE_AUTHOR("Matti Aaltonen <matti.j.aaltonen@xxxxxxxxx>");
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/wl1273-core.h b/include/linux/mfd/wl1273-core.h
> new file mode 100644
> index 0000000..c2c44dc
> --- /dev/null
> +++ b/include/linux/mfd/wl1273-core.h
> @@ -0,0 +1,298 @@
> +/*
> + * include/media/radio/radio-wl1273.h
This is include/linux/mfd/wl1273-core.h
> + * Some definitions for the wl1273 radio receiver/transmitter chip.
> + *
> + * Copyright (C) Nokia Corporation
You want to specify a year here.
> +#ifndef RADIO_WL1273_H
#ifndef WL1273_CORE_H, please.
> +#define RADIO_WL1273_H
> +
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
You don't want all your drivers to include mfd/core.h. And that may apply to
i2c.h as well.
> +struct wl1273_fw_packet{
> + int len;
> + __u8 *buf;
> +};
Do you really need to export this structure ?
> +struct wl1273_core {
> + struct mfd_cell cells[WL1273_FM_CORE_CELLS];
> + struct wl1273_fm_platform_data *pdata;
> +
> + unsigned int mode;
> + unsigned int i2s_mode;
> + unsigned int volume;
> + unsigned int audio_mode;
> + unsigned int channel_number;
> + struct mutex lock; /* for serializing fm radio operations */
> +
> + struct i2c_client *client;
> +
> + int (*write)(struct wl1273_core *core, u8, u16);
> + int (*set_audio)(struct wl1273_core *core, unsigned int);
> + int (*set_volume)(struct wl1273_core *core, unsigned int);
So who is defining those routines ?
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
