Hello,
and thank you for the comments.
On Fri, 2010-11-26 at 12:56 +0100, ext Samuel Ortiz wrote:
> > +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.
I added a bit of explanation and also renamed the config.
> > +#include <linux/delay.h>
> Not needed
Removed all unnecessary includes.
> > + struct mfd_cell *cell = &core->cells[children];
> Please add a line between your variable declarations and the rest of the code.
Eventually I moved these definitions to the beginning of the function.
> > + } 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.
Moved this test to the beginning of the function.
> > + if (!r)
> > + return 0;
>
> I'd prefere:
>
> if (children == 0) {
> dev_err(....);
> r = -ENODEV;
> goto err;
> }
>
>
> dev_dbg(....);
>
> return mfd_add_devices();
>
> err:
> ....
I did more or less what Samuel suggested but kept the test for the
mfd_add_devices return value to be able to free resources in case
of error.
> > + return 0;
> Here you can just return r.
Yes.
> > + flush_scheduled_work();
>
> What is that for ?
I forgot that when refactoring the code. Removed.
> > + * include/media/radio/radio-wl1273.h
> This is include/linux/mfd/wl1273-core.h
Correct.
> > + * Copyright (C) Nokia Corporation
> You want to specify a year here.
Added year to all copyright statements.
> > +#ifndef RADIO_WL1273_H
> #ifndef WL1273_CORE_H, please.
OK.
> > +#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.
Both are directly needed in this header so I kept these includes.
> > +struct wl1273_fw_packet{
> > + int len;
> > + __u8 *buf;
> > +};
> Do you really need to export this structure ?
No. It actually become completely obsoleted so: removed.
> > + 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 ?
They are defined in the V4L2 driver. Previously they were defined here
in the core...
On Fri, 2010-11-19 at 18:33 +0530, ext Raja Mani wrote:
> > +static int radio_nr = -1;
>
> You have assigned -1 to radio_nr here. But your description says "The
> default is 0".
> I am bit confused...
I was also confused, so I removed the initialization.
> > +MODULE_PARM_DESC(radio_nr, "Radio Nr");
>
> Can we add clear comments instead "Radio Nr" ?. I know other driver
> has this kind of description
> What you added for "rds_buf" is more clear. This is only a suggestion..
Wrote a longer explanation.
> > + struct i2c_client *client = core->client;
> > +
>
> No space here
>
> > + u8 buf[] = { (param >> 8) & 0xff, param & 0xff }
OK.
> > + r = i2c_transfer(client->adapter, &msg, 1);
> > +
>
> No space here
>
> > + if (r != 1) {
OK.
> > + val = volume;
> Really do we need val variable here ?
I wanted to have a pointer to u16...
> > + r = wl1273_fm_read_reg(core, WL1273_RSSI_LVL_GET, &level);
> > +
>
> No space here
> > + if (r)
OK.
> > + INIT_COMPLETION(radio->busy);
> > + /* Set the current tx channel */
> > + r = wl1273_fm_write_cmd(core, WL1273_CHANL_SET, freq / 10);
> > + if (r)
> > + return r;
> > +
>
> Better , radio->busy init can be moved here.. there can be a chance to
> hit above return..
OK. I also did the other similar changes.
> > + INIT_COMPLETION(radio->busy);
> > + r = wl1273_fm_write_cmd(core, WL1273_TUNER_MODE_SET,
> > TUNER_MODE_PRESET);
> > + if (r) {
> > + dev_err(radio->dev, "TUNER_MODE_SET fails\n");
>
> why complete here ? radio->busy is just initialized .. who is waiting
> on radio->busy ?
A good question. I removed the complete call and moved the init after the test.
> > +
> > + return 0;
>
> probably, you have to return negative error . not Zero. Isn't it
> mandatory to download FM Firmware ?
I kept this as it was because we are uploading only a firmware patch,
which may not be necessary. Added a comment, however.
> > + goto out;
> > + }
>
> should we need "packets " ? . Can't we deal with fw_p->data itself for
> firmware download?
> we can avoid eating system memory.. Give a thought on this..
I got rid of the packets.
Cheers,
Matti
Matti J. Aaltonen (2):
MFD: WL1273 FM Radio: MFD driver for the FM radio.
V4L2: WL1273 FM Radio: TI WL1273 FM radio driver
drivers/media/radio/Kconfig | 16 +
drivers/media/radio/Makefile | 1 +
drivers/media/radio/radio-wl1273.c | 2331 ++++++++++++++++++++++++++++++++++++
drivers/mfd/Kconfig | 10 +
drivers/mfd/Makefile | 1 +
drivers/mfd/wl1273-core.c | 148 +++
include/linux/mfd/wl1273-core.h | 288 +++++
7 files changed, 2795 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/radio/radio-wl1273.c
create mode 100644 drivers/mfd/wl1273-core.c
create mode 100644 include/linux/mfd/wl1273-core.h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
