Re: [PATCH 1/3] Add a core driver for SI476x MFD
On 09/13/2012 11:44 PM, Hans Verkuil wrote:
> Hi Andrey!
>
> Thanks for posting this driver. One request for the future: please split this
> patch up in smaller pieces: one for each c source for example. That makes it
> easier to review.
Will do for next version.
> +
> +/**
> + * __core_send_command() - sends a command to si476x and waits its
> + * response
> + * @core:si476x_device structure for the device we are
> + *communicating with
> + * @command: command id
> + * @args: command arguments we are sending
> + * @argn: actual size of @args
> + * @response: buffer to place the expected response from the device
> + * @respn:actual size of @response
> + * @usecs:amount of time to wait before reading the response (in
> + *usecs)
> + *
> + * Function returns 0 on succsess and negative error code on
> + * failure
> + */
> +static int __core_send_command(struct si476x_core *core,
> + const u8 command,
> + const u8 args[],
> + const int argn,
> + u8 resp[],
> + const int respn,
> + const int usecs)
> +{
> + struct i2c_client *client = core->client;
> + int err;
> + u8 data[CMD_MAX_ARGS_COUNT + 1];
> +
> + if (argn > CMD_MAX_ARGS_COUNT) {
> + err = -ENOMEM;
> + goto exit;
> Why goto exit? There is no clean up after the exit label, so just return
> immediately. Ditto for all the other goto exit's in this function.
To have only just on point of exit from the function that's just
personal coding style preference.
There are no technical reasons behind that, I can change that.
>
>> +}
>> +
>> +if (!client->adapter) {
>> +err = -ENODEV;
>> +goto exit;
>> +}
>> +
>> +/* First send the command and its arguments */
>> +data[0] = command;
>> +memcpy(&data[1], args, argn);
>> +DBG_BUFFER(&client->dev, "Command:\n", data, argn + 1);
>> +
>> +err = si476x_i2c_xfer(core, SI476X_I2C_SEND, (char *) data, argn + 1);
>> +if (err != argn + 1) {
>> +dev_err(&core->client->dev,
>> +"Error while sending command 0x%02x\n",
>> +command);
>> +err = (err >= 0) ? -EIO : err;
>> +goto exit;
>> +}
>> +/* Set CTS to zero only after the command is send to avoid
>> + * possible racing conditions when working in polling mode */
>> +atomic_set(&core->cts, 0);
>> +
>> +if (!wait_event_timeout(core->command,
>> +atomic_read(&core->cts),
>> +usecs_to_jiffies(usecs) + 1))
>> +dev_warn(&core->client->dev,
>> + "(%s) [CMD 0x%02x] Device took too much time to
>> answer.\n",
>> + __func__, command);
>> +
>> +/*
>> + When working in polling mode, for some reason the tuner will
>> + report CTS bit as being set in the first status byte read,
>> + but all the consequtive ones will return zros until the
>> + tuner is actually completed the POWER_UP command. To
>> + workaround that we wait for second CTS to be reported
>> + */
>> +if (unlikely(!core->client->irq && command == CMD_POWER_UP)) {
>> +if (!wait_event_timeout(core->command,
>> +atomic_read(&core->cts),
>> +usecs_to_jiffies(usecs) + 1))
>> +dev_warn(&core->client->dev,
>> + "(%s) Power up took too much time.\n",
>> + __func__);
>> +}
>> +
>> +/* Then get the response */
>> +err = si476x_i2c_xfer(core, SI476X_I2C_RECV, resp, respn);
>> +if (err != respn) {
>> +dev_err(&core->client->dev,
>> +"Error while reading response for command 0x%02x\n",
>> +command);
>> +err = (err >= 0) ? -EIO : err;
>> +goto exit;
>> +}
>> +DBG_BUFFER(&client->dev, "Response:\n", resp, respn);
>> +
>> +err = 0;
>> +
>> +if (resp[0] & SI476X_ERR) {
>> +dev_err(&core->client->dev, "Chip set error flag\n");
>> +err = si476x_core_parse_and_nag_about_error(core);
>> +goto exit;
>> +}
>> +
>> +if (!(resp[0] & SI476X_CTS))
>> +err = -EBUSY;
>> +exit:
>> +return err;
>> +}
>> +
>> +#define CORE_SEND_COMMAND(core, cmd, args, resp, timeout) \
>> +__core_send_command(core, cmd, args,\
>> +ARRAY_SIZE(args), \
>> +resp, ARRAY_SIZE(resp), \
>> +timeout)
>> +
>> +
>> +static int __cmd_tune_seek_freq(struct si476x_core *core,
>> +uint8_t cmd,
>> +
Re: [PATCH 1/3] Add a core driver for SI476x MFD
On Fri September 21 2012 03:05:41 andrey.smir...@convergeddevices.net wrote:
> On 09/13/2012 11:44 PM, Hans Verkuil wrote:
> > Hi Andrey!
> >
> > Thanks for posting this driver. One request for the future: please split
> > this
> > patch up in smaller pieces: one for each c source for example. That makes it
> > easier to review.
>
> Will do for next version.
>
> > +
> > +/**
> > + * __core_send_command() - sends a command to si476x and waits its
> > + * response
> > + * @core:si476x_device structure for the device we are
> > + *communicating with
> > + * @command: command id
> > + * @args: command arguments we are sending
> > + * @argn: actual size of @args
> > + * @response: buffer to place the expected response from the device
> > + * @respn:actual size of @response
> > + * @usecs:amount of time to wait before reading the response (in
> > + *usecs)
> > + *
> > + * Function returns 0 on succsess and negative error code on
> > + * failure
> > + */
> > +static int __core_send_command(struct si476x_core *core,
> > + const u8 command,
> > + const u8 args[],
> > + const int argn,
> > + u8 resp[],
> > + const int respn,
> > + const int usecs)
> > +{
> > + struct i2c_client *client = core->client;
> > + int err;
> > + u8 data[CMD_MAX_ARGS_COUNT + 1];
> > +
> > + if (argn > CMD_MAX_ARGS_COUNT) {
> > + err = -ENOMEM;
> > + goto exit;
> > Why goto exit? There is no clean up after the exit label, so just return
> > immediately. Ditto for all the other goto exit's in this function.
>
> To have only just on point of exit from the function that's just
> personal coding style preference.
> There are no technical reasons behind that, I can change that.
>
> >
> >> + }
> >> +
> >> + if (!client->adapter) {
> >> + err = -ENODEV;
> >> + goto exit;
> >> + }
> >> +
> >> + /* First send the command and its arguments */
> >> + data[0] = command;
> >> + memcpy(&data[1], args, argn);
> >> + DBG_BUFFER(&client->dev, "Command:\n", data, argn + 1);
> >> +
> >> + err = si476x_i2c_xfer(core, SI476X_I2C_SEND, (char *) data, argn + 1);
> >> + if (err != argn + 1) {
> >> + dev_err(&core->client->dev,
> >> + "Error while sending command 0x%02x\n",
> >> + command);
> >> + err = (err >= 0) ? -EIO : err;
> >> + goto exit;
> >> + }
> >> + /* Set CTS to zero only after the command is send to avoid
> >> + * possible racing conditions when working in polling mode */
> >> + atomic_set(&core->cts, 0);
> >> +
> >> + if (!wait_event_timeout(core->command,
> >> + atomic_read(&core->cts),
> >> + usecs_to_jiffies(usecs) + 1))
> >> + dev_warn(&core->client->dev,
> >> + "(%s) [CMD 0x%02x] Device took too much time to
> >> answer.\n",
> >> + __func__, command);
> >> +
> >> + /*
> >> +When working in polling mode, for some reason the tuner will
> >> +report CTS bit as being set in the first status byte read,
> >> +but all the consequtive ones will return zros until the
> >> +tuner is actually completed the POWER_UP command. To
> >> +workaround that we wait for second CTS to be reported
> >> + */
> >> + if (unlikely(!core->client->irq && command == CMD_POWER_UP)) {
> >> + if (!wait_event_timeout(core->command,
> >> + atomic_read(&core->cts),
> >> + usecs_to_jiffies(usecs) + 1))
> >> + dev_warn(&core->client->dev,
> >> + "(%s) Power up took too much time.\n",
> >> + __func__);
> >> + }
> >> +
> >> + /* Then get the response */
> >> + err = si476x_i2c_xfer(core, SI476X_I2C_RECV, resp, respn);
> >> + if (err != respn) {
> >> + dev_err(&core->client->dev,
> >> + "Error while reading response for command 0x%02x\n",
> >> + command);
> >> + err = (err >= 0) ? -EIO : err;
> >> + goto exit;
> >> + }
> >> + DBG_BUFFER(&client->dev, "Response:\n", resp, respn);
> >> +
> >> + err = 0;
> >> +
> >> + if (resp[0] & SI476X_ERR) {
> >> + dev_err(&core->client->dev, "Chip set error flag\n");
> >> + err = si476x_core_parse_and_nag_about_error(core);
> >> + goto exit;
> >> + }
> >> +
> >> + if (!(resp[0] & SI476X_CTS))
> >> + err = -EBUSY;
> >> +exit:
> >> + return err;
> >> +}
> >> +
> >> +#define CORE_SEND_COMMAND(core, cmd, args, resp, timeout) \
> >> + __core_send_command(core, cmd, args,\
> >> + ARRAY_SIZE(args), \
> >> + resp, ARRAY_SIZE(resp),
Re: [PATCH 1/3] Add a core driver for SI476x MFD
On 09/21/2012 12:31 AM, Hans Verkuil wrote:
> On Fri September 21 2012 03:05:41 andrey.smir...@convergeddevices.net wrote:
>> On 09/13/2012 11:44 PM, Hans Verkuil wrote:
>>> Hi Andrey!
>>>
>>> Thanks for posting this driver. One request for the future: please split
>>> this
>>> patch up in smaller pieces: one for each c source for example. That makes it
>>> easier to review.
>> Will do for next version.
>>
>>> +
>>> +/**
>>> + * __core_send_command() - sends a command to si476x and waits its
>>> + * response
>>> + * @core:si476x_device structure for the device we are
>>> + *communicating with
>>> + * @command: command id
>>> + * @args: command arguments we are sending
>>> + * @argn: actual size of @args
>>> + * @response: buffer to place the expected response from the device
>>> + * @respn:actual size of @response
>>> + * @usecs:amount of time to wait before reading the response (in
>>> + *usecs)
>>> + *
>>> + * Function returns 0 on succsess and negative error code on
>>> + * failure
>>> + */
>>> +static int __core_send_command(struct si476x_core *core,
>>> + const u8 command,
>>> + const u8 args[],
>>> + const int argn,
>>> + u8 resp[],
>>> + const int respn,
>>> + const int usecs)
>>> +{
>>> + struct i2c_client *client = core->client;
>>> + int err;
>>> + u8 data[CMD_MAX_ARGS_COUNT + 1];
>>> +
>>> + if (argn > CMD_MAX_ARGS_COUNT) {
>>> + err = -ENOMEM;
>>> + goto exit;
>>> Why goto exit? There is no clean up after the exit label, so just return
>>> immediately. Ditto for all the other goto exit's in this function.
>> To have only just on point of exit from the function that's just
>> personal coding style preference.
>> There are no technical reasons behind that, I can change that.
>>
+ }
+
+ if (!client->adapter) {
+ err = -ENODEV;
+ goto exit;
+ }
+
+ /* First send the command and its arguments */
+ data[0] = command;
+ memcpy(&data[1], args, argn);
+ DBG_BUFFER(&client->dev, "Command:\n", data, argn + 1);
+
+ err = si476x_i2c_xfer(core, SI476X_I2C_SEND, (char *) data, argn + 1);
+ if (err != argn + 1) {
+ dev_err(&core->client->dev,
+ "Error while sending command 0x%02x\n",
+ command);
+ err = (err >= 0) ? -EIO : err;
+ goto exit;
+ }
+ /* Set CTS to zero only after the command is send to avoid
+ * possible racing conditions when working in polling mode */
+ atomic_set(&core->cts, 0);
+
+ if (!wait_event_timeout(core->command,
+ atomic_read(&core->cts),
+ usecs_to_jiffies(usecs) + 1))
+ dev_warn(&core->client->dev,
+ "(%s) [CMD 0x%02x] Device took too much time to
answer.\n",
+ __func__, command);
+
+ /*
+When working in polling mode, for some reason the tuner will
+report CTS bit as being set in the first status byte read,
+but all the consequtive ones will return zros until the
+tuner is actually completed the POWER_UP command. To
+workaround that we wait for second CTS to be reported
+ */
+ if (unlikely(!core->client->irq && command == CMD_POWER_UP)) {
+ if (!wait_event_timeout(core->command,
+ atomic_read(&core->cts),
+ usecs_to_jiffies(usecs) + 1))
+ dev_warn(&core->client->dev,
+ "(%s) Power up took too much time.\n",
+ __func__);
+ }
+
+ /* Then get the response */
+ err = si476x_i2c_xfer(core, SI476X_I2C_RECV, resp, respn);
+ if (err != respn) {
+ dev_err(&core->client->dev,
+ "Error while reading response for command 0x%02x\n",
+ command);
+ err = (err >= 0) ? -EIO : err;
+ goto exit;
+ }
+ DBG_BUFFER(&client->dev, "Response:\n", resp, respn);
+
+ err = 0;
+
+ if (resp[0] & SI476X_ERR) {
+ dev_err(&core->client->dev, "Chip set error flag\n");
+ err = si476x_core_parse_and_nag_about_error(core);
+ goto exit;
+ }
+
+ if (!(resp[0] & SI476X_CTS))
+ err = -EBUSY;
+exit:
+ return err;
+}
+
+#define CORE_SEND_COMMAND(core, cmd, args, resp, timeout) \
+ __core_send_command(core, cmd, args,\
+ ARRAY_SIZE(args), \
+
Re: [PATCH 1/3] Add a core driver for SI476x MFD
On Fri September 21 2012 18:33:45 andrey.smir...@convergeddevices.net wrote:
> On 09/21/2012 12:31 AM, Hans Verkuil wrote:
> > On Fri September 21 2012 03:05:41 andrey.smir...@convergeddevices.net wrote:
> >> On 09/13/2012 11:44 PM, Hans Verkuil wrote:
> >>> Hi Andrey!
> >>>
> >>> Thanks for posting this driver. One request for the future: please split
> >>> this
> >>> patch up in smaller pieces: one for each c source for example. That makes
> >>> it
> >>> easier to review.
> >> Will do for next version.
> >>
> >>> +
> >>> +/**
> >>> + * __core_send_command() - sends a command to si476x and waits its
> >>> + * response
> >>> + * @core:si476x_device structure for the device we are
> >>> + *communicating with
> >>> + * @command: command id
> >>> + * @args: command arguments we are sending
> >>> + * @argn: actual size of @args
> >>> + * @response: buffer to place the expected response from the device
> >>> + * @respn:actual size of @response
> >>> + * @usecs:amount of time to wait before reading the response (in
> >>> + *usecs)
> >>> + *
> >>> + * Function returns 0 on succsess and negative error code on
> >>> + * failure
> >>> + */
> >>> +static int __core_send_command(struct si476x_core *core,
> >>> + const u8 command,
> >>> + const u8 args[],
> >>> + const int argn,
> >>> + u8 resp[],
> >>> + const int respn,
> >>> + const int usecs)
> >>> +{
> >>> + struct i2c_client *client = core->client;
> >>> + int err;
> >>> + u8 data[CMD_MAX_ARGS_COUNT + 1];
> >>> +
> >>> + if (argn > CMD_MAX_ARGS_COUNT) {
> >>> + err = -ENOMEM;
> >>> + goto exit;
> >>> Why goto exit? There is no clean up after the exit label, so just return
> >>> immediately. Ditto for all the other goto exit's in this function.
> >> To have only just on point of exit from the function that's just
> >> personal coding style preference.
> >> There are no technical reasons behind that, I can change that.
> >>
> +}
> +
> +if (!client->adapter) {
> +err = -ENODEV;
> +goto exit;
> +}
> +
> +/* First send the command and its arguments */
> +data[0] = command;
> +memcpy(&data[1], args, argn);
> +DBG_BUFFER(&client->dev, "Command:\n", data, argn + 1);
> +
> +err = si476x_i2c_xfer(core, SI476X_I2C_SEND, (char *) data,
> argn + 1);
> +if (err != argn + 1) {
> +dev_err(&core->client->dev,
> +"Error while sending command 0x%02x\n",
> +command);
> +err = (err >= 0) ? -EIO : err;
> +goto exit;
> +}
> +/* Set CTS to zero only after the command is send to avoid
> + * possible racing conditions when working in polling mode */
> +atomic_set(&core->cts, 0);
> +
> +if (!wait_event_timeout(core->command,
> +atomic_read(&core->cts),
> +usecs_to_jiffies(usecs) + 1))
> +dev_warn(&core->client->dev,
> + "(%s) [CMD 0x%02x] Device took too much time
> to answer.\n",
> + __func__, command);
> +
> +/*
> + When working in polling mode, for some reason the tuner will
> + report CTS bit as being set in the first status byte read,
> + but all the consequtive ones will return zros until the
> + tuner is actually completed the POWER_UP command. To
> + workaround that we wait for second CTS to be reported
> + */
> +if (unlikely(!core->client->irq && command == CMD_POWER_UP)) {
> +if (!wait_event_timeout(core->command,
> +atomic_read(&core->cts),
> +usecs_to_jiffies(usecs) + 1))
> +dev_warn(&core->client->dev,
> + "(%s) Power up took too much time.\n",
> + __func__);
> +}
> +
> +/* Then get the response */
> +err = si476x_i2c_xfer(core, SI476X_I2C_RECV, resp, respn);
> +if (err != respn) {
> +dev_err(&core->client->dev,
> +"Error while reading response for command
> 0x%02x\n",
> +command);
> +err = (err >= 0) ? -EIO : err;
> +goto exit;
> +}
> +DBG_BUFFER(&client->dev, "Response:\n", resp, respn);
> +
> >
Re: [PATCH 1/3] Add a core driver for SI476x MFD
On Thu, Sep 13, 2012 at 03:40:11PM -0700, Andrey Smirnov wrote:
> + core = kzalloc(sizeof(*core), GFP_KERNEL);
devm_kzalloc()
> + if (!core) {
> + pr_err("si476x-core: failed to allocate " \
> +"'struct si476x_core'\n");
> + return -ENOMEM;
> + }
Splitting error messages over multiple lines like this just makes things
hard to grep for.
> + core->supplies.vio1 = regulator_get(&client->dev, "vio1");
> + if (IS_ERR_OR_NULL(core->supplies.vio1)) {
> + dev_info(&client->dev, "No vio1 regulator found\n");
> + core->supplies.vio1 = NULL;
> + }
This and all the usages of the regulator API in the driver are broken,
the driver should treat failures to get the supplies as errors. There
are more than enough ways to stub things out in the core.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] Add a core driver for SI476x MFD
Hi Andrey! Thanks for posting this driver. One request for the future: please split this patch up in smaller pieces: one for each c source for example. That makes it easier to review. On Fri September 14 2012 00:40:11 Andrey Smirnov wrote: > This patch adds a core driver for Silicon Laboratories Si476x series > of AM/FM tuner chips. The driver as a whole is implemented as an MFD device > and this patch adds a core portion of it that provides all the necessary > functionality to the two other drivers that represent radio and audio > codec subsystems of the chip. > > Signed-off-by: Andrey Smirnov > --- > drivers/mfd/Kconfig | 14 + > drivers/mfd/Makefile|3 + > drivers/mfd/si476x-cmd.c| 1509 > +++ > drivers/mfd/si476x-i2c.c| 1033 +++ > drivers/mfd/si476x-prop.c | 477 + > include/linux/mfd/si476x-core.h | 522 ++ > include/media/si476x.h | 455 > 7 files changed, 4013 insertions(+) > create mode 100644 drivers/mfd/si476x-cmd.c > create mode 100644 drivers/mfd/si476x-i2c.c > create mode 100644 drivers/mfd/si476x-prop.c > create mode 100644 include/linux/mfd/si476x-core.h > create mode 100644 include/media/si476x.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index b1a1462..3fab06d 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -895,6 +895,20 @@ config MFD_WL1273_CORE > driver connects the radio-wl1273 V4L2 module and the wl1273 > audio codec. > > +config MFD_SI476X_CORE > + tristate "Support for Silicon Laboratories 4761/64/68 AM/FM radio." > + depends on I2C > + select MFD_CORE > + default n > + help > + This is the core driver for the SI476x series of AM/FM radio. This MFD > + driver connects the radio-si476x V4L2 module and the si476x > + audio codec. > + > + To compile this driver as a module, choose M here: the > + module will be called si476x-core. > + > + > config MFD_OMAP_USB_HOST > bool "Support OMAP USBHS core driver" > depends on USB_EHCI_HCD_OMAP || USB_OHCI_HCD_OMAP3 > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 79dd22d..942257b 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -132,3 +132,6 @@ obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o > obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o > obj-$(CONFIG_MFD_ANATOP) += anatop-mfd.o > obj-$(CONFIG_MFD_LM3533) += lm3533-core.o lm3533-ctrlbank.o > + > +si476x-core-objs := si476x-cmd.o si476x-prop.o si476x-i2c.o > +obj-$(CONFIG_MFD_SI476X_CORE)+= si476x-core.o > diff --git a/drivers/mfd/si476x-cmd.c b/drivers/mfd/si476x-cmd.c > new file mode 100644 > index 000..defe1f5 > --- /dev/null > +++ b/drivers/mfd/si476x-cmd.c > @@ -0,0 +1,1509 @@ > +/* > + * include/media/si476x-cmd.c -- Subroutines implementing command > + * protocol of si476x series of chips > + * > + * Copyright (C) 2012 Innovative Converged Devices(ICD) > + * > + * Author: Andrey Smirnov > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License. > + * > + * This program is distributed in the hope that 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. > + * > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +#define msb(x) ((u8)((u16) x >> 8)) > +#define lsb(x) ((u8)((u16) x & 0x00FF)) > + > + > + > +#define CMD_POWER_UP 0x01 > +#define CMD_POWER_UP_A10_NRESP 1 > +#define CMD_POWER_UP_A10_NARGS 5 > + > +#define CMD_POWER_UP_A20_NRESP 1 > +#define CMD_POWER_UP_A20_NARGS 5 > + > +#define POWER_UP_DELAY_MS110 > + > +#define CMD_POWER_DOWN 0x11 > +#define CMD_POWER_DOWN_A10_NRESP 1 > + > +#define CMD_POWER_DOWN_A20_NRESP 1 > +#define CMD_POWER_DOWN_A20_NARGS 1 > + > +#define CMD_FUNC_INFO0x12 > +#define CMD_FUNC_INFO_NRESP 7 > + > +#define CMD_SET_PROPERTY 0x13 > +#define CMD_SET_PROPERTY_NARGS 5 > +#define CMD_SET_PROPERTY_NRESP 1 > + > +#define CMD_GET_PROPERTY 0x14 > +#define CMD_GET_PROPERTY_NARGS 3 > +#define CMD_GET_PROPERTY_NRESP 4 > + > +#define CMD_AGC_STATUS 0x17 > +#define CMD_AGC_STAT
