Re: [PATCH v3 2/6] Add the main bulk of core driver for SI476x code

2012-11-16 Thread Hans Verkuil 
Hi Andrey,

I'm really sorry for the long delay, but I finally have time to review v4 of 
this
code.

On Tue October 23 2012 20:44:28 Andrey Smirnov wrote:
 This patch adds main part(out of three) of the I2C driver for the
 core of MFD device.
 
 Signed-off-by: Andrey Smirnov andrey.smir...@convergeddevices.net
 ---
  drivers/mfd/si476x-i2c.c |  966 
 ++
  1 file changed, 966 insertions(+)
  create mode 100644 drivers/mfd/si476x-i2c.c
 
 diff --git a/drivers/mfd/si476x-i2c.c b/drivers/mfd/si476x-i2c.c
 new file mode 100644
 index 000..6d581bd
 --- /dev/null
 +++ b/drivers/mfd/si476x-i2c.c
 @@ -0,0 +1,966 @@
 +/*
 + * include/media/si476x-i2c.c -- Core device driver for si476x MFD
 + * device
 + *
 + * Copyright (C) 2012 Innovative Converged Devices(ICD)
 + *
 + * Author: Andrey Smirnov andrey.smir...@convergeddevices.net
 + *
 + * 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 linux/module.h
 +
 +#include linux/slab.h
 +#include linux/interrupt.h
 +#include linux/delay.h
 +#include linux/gpio.h
 +#include linux/regulator/consumer.h
 +#include linux/i2c.h
 +#include linux/err.h
 +
 +#include linux/mfd/si476x-core.h
 +
 +/* Command Timeouts */
 +#define DEFAULT_TIMEOUT  10
 +#define TIMEOUT_TUNE 70
 +#define TIMEOUT_POWER_UP 33
 +
 +#define MAX_IO_ERRORS 10
 +
 +#define SI476X_DRIVER_RDS_FIFO_DEPTH 128
 +
 +#define SI476X_STATUS_POLL_US 0
 +
 +/**
 + * si476x_core_config_pinmux() - pin function configuration function
 + *
 + * @core: Core device structure
 + *
 + * Configure the functions of the pins of the radio chip.
 + *
 + * The function returns zero in case of succes or negative error code
 + * otherwise.
 + */
 +static int si476x_core_config_pinmux(struct si476x_core *core)
 +{
 + int err;
 + dev_dbg(core-client-dev, Configuring pinmux\n);
 + err = si476x_core_cmd_dig_audio_pin_cfg(core,
 + core-pinmux.dclk,
 + core-pinmux.dfs,
 + core-pinmux.dout,
 + core-pinmux.xout);
 + if (err  0) {
 + dev_err(core-client-dev,
 + Failed to configure digital audio pins(err = %d)\n,
 + err);
 + return err;
 + }
 +
 + err = si476x_core_cmd_zif_pin_cfg(core,
 +   core-pinmux.iqclk,
 +   core-pinmux.iqfs,
 +   core-pinmux.iout,
 +   core-pinmux.qout);
 + if (err  0) {
 + dev_err(core-client-dev,
 + Failed to configure ZIF pins(err = %d)\n,
 + err);
 + return err;
 + }
 +
 + err = si476x_core_cmd_ic_link_gpo_ctl_pin_cfg(core,
 +   core-pinmux.icin,
 +   core-pinmux.icip,
 +   core-pinmux.icon,
 +   core-pinmux.icop);
 + if (err  0) {
 + dev_err(core-client-dev,
 + Failed to configure IC-Link/GPO pins(err = %d)\n,
 + err);
 + return err;
 + }
 +
 + err = si476x_core_cmd_ana_audio_pin_cfg(core,
 + core-pinmux.lrout);
 + if (err  0) {
 + dev_err(core-client-dev,
 + Failed to configure analog audio pins(err = %d)\n,
 + err);
 + return err;
 + }
 +
 + err = si476x_core_cmd_intb_pin_cfg(core,
 +core-pinmux.intb,
 +core-pinmux.a1);
 + if (err  0) {
 + dev_err(core-client-dev,
 + Failed to configure interrupt pins(err = %d)\n,
 + err);
 + return err;
 + }
 +
 + return 0;
 +}
 +
 +static inline void si476x_core_schedule_polling_work(struct si476x_core 
 *core)
 +{
 + schedule_delayed_work(core-status_monitor,
 + usecs_to_jiffies(atomic_read(core-polling_interval)));
 +}
 +
 +/**
 + * si476x_core_start() - early chip startup function
 + * @core: Core device structure
 + * @soft: When set, this flag forces soft startup, where soft

Re: [PATCH v3 2/6] Add the main bulk of core driver for SI476x code

2012-10-27 Thread Mark Brown 
On Thu, Oct 25, 2012 at 03:26:02PM -0700, Andrey Smirnov wrote:
 On 10/25/2012 12:45 PM, Mark Brown wrote:

  This really makes little sense to me, why are you doing this?  Does the
  device *really* layer a byte stream on top of I2C for sending messages
  that look like marshalled register reads and writes?

 The SI476x chips has a concept of a property. Each property having
 16-bit address and 16-bit value. At least a portion of a chip
 configuration is done by modifying those properties. In order to

Right, that's what I remembered from previous code.  There's no way this
should be a regmap bus - a bus is something that gets data serialised by
the core into a byte stream, having the data rendered down into a byte
stream and then reparsing it is a bit silly.  The device should be
hooking in before the data gets marshalled which we can't currently do
but it shouldn't be too hard to make it so that we can have register
read and write functions supplied in the regmap config.

 Also due to the way the driver uses the chip it is only powered up when
 the corresponding file in devfs(e.g. /dev/radio0) is opened at least by
 one user which means that unless there is a user who opened the file all
 the SET/GET_PROPERTY commands sent to it will be lost. The codec driver
 for that chip does not have any say in the power management policy(while
 all the audio configuration is done via properties) if the chip is not
 powered up the driver has to cache the configuration values it has so
 that they can be applied later.

This is very normal, indeed modern CODEC drivers can leave the chip
powered down whenever it's not performing some function.

 So, since I have to implement a caching functionality in the driver, in
 order to avoid reinventing the wheel I opted for using 'regmap' API for
 this.

 Of course, It is possible that I misunderstood the purpose and
 capabilities of the 'regmap' framework, which would make my code look
 very silly indeed. If that is the case I'll just re-implement it using
 some sort of ad-hoc version of caching.

No, what you're doing is totally sensible.  It needs a bit of extension
to the framework before you can do it though.


signature.asc
Description: Digital signature

Re: [PATCH v3 2/6] Add the main bulk of core driver for SI476x code

2012-10-27 Thread Andrey Smirnov 
On 10/27/2012 02:31 PM, Mark Brown wrote:
 On Thu, Oct 25, 2012 at 03:26:02PM -0700, Andrey Smirnov wrote:
 On 10/25/2012 12:45 PM, Mark Brown wrote:
 This really makes little sense to me, why are you doing this?  Does the
 device *really* layer a byte stream on top of I2C for sending messages
 that look like marshalled register reads and writes?
 The SI476x chips has a concept of a property. Each property having
 16-bit address and 16-bit value. At least a portion of a chip
 configuration is done by modifying those properties. In order to
 Right, that's what I remembered from previous code.  There's no way this
 should be a regmap bus - a bus is something that gets data serialised by
 the core into a byte stream, having the data rendered down into a byte
 stream and then reparsing it is a bit silly.  The device should be
 hooking in before the data gets marshalled which we can't currently do
 but it shouldn't be too hard to make it so that we can have register
 read and write functions supplied in the regmap config.

Oh, now I think I see what you mean. I have two agree with you, I don't
think I like what I am doing in my code.
I'll try to familiarize myself with 'regmap' code and come up with the
way to extend the framework.

And I just wanted to upstream my simple radio driver... :-)



--
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 v3 2/6] Add the main bulk of core driver for SI476x code

2012-10-25 Thread Mark Brown 
On Tue, Oct 23, 2012 at 11:44:28AM -0700, Andrey Smirnov wrote:

 + core-regmap = devm_regmap_init_si476x(core);
 + if (IS_ERR(core-regmap)) {

This really makes little sense to me, why are you doing this?  Does the
device *really* layer a byte stream on top of I2C for sending messages
that look like marshalled register reads and writes?


signature.asc
Description: Digital signature

Re: [PATCH v3 2/6] Add the main bulk of core driver for SI476x code

2012-10-25 Thread Andrey Smirnov 
On 10/25/2012 12:45 PM, Mark Brown wrote:
 On Tue, Oct 23, 2012 at 11:44:28AM -0700, Andrey Smirnov wrote:

 +core-regmap = devm_regmap_init_si476x(core);
 +if (IS_ERR(core-regmap)) {
 This really makes little sense to me, why are you doing this?  Does the
 device *really* layer a byte stream on top of I2C for sending messages
 that look like marshalled register reads and writes?
The SI476x chips has a concept of a property. Each property having
16-bit address and 16-bit value. At least a portion of a chip
configuration is done by modifying those properties. In order to
manipulate those properties user is expected to issue what is called a
command. For manipulating properties there are two commands:
- SET_PROPERTY which is I2C write of 6 bytes of the following layout:
| 0x13 | 0x00 | Address High Byte | Address Low Byte | Property Data
High Byte | Property Data Low Byte |
After the command is finished being executed the 1 byte read would
contain the status byte

followed by 1 byte read which contain status byte
- GET_PROPERTY which is I2C write of 4 bytes of the following layout:
| 0x13 | 0x00 | Address High Byte | Address Low Byte |
After the command is finished being executed the 4 byte read would
have the following layout:
| Status Byte | Reserved Byte | Property Value High Byte | Property
Value Low Byte |

The chip does not operate continuously in the AM/FM frequency range,
instead the user is expected to power-down/power-up the chip in a
certain mode which in my case can be either AM or FM tuner. There are
two ways of doing that one is to send a power down command to the
device, the second one is to toggle reset line of the chip. Both methods
will reset the values of the aforementioned properties. Because V4L2
user-space interface presents a tuner as the one continuously operating
in the whole AM/FM range it is necessary for me to do those AM/FM
switches transparently when handling tuning or seeking requests from the
user. That means that I need to cache the values of the properties I
care about in the driver and restore them when user switches to the
mode(for example when AM-FM-AM transition happens)

The other quirk of that chip is that some properties are only accessible
in certain modes(for example it is impossible to configure RDS interrupt
sources, which is FM specific, in AM mode), but some of the controls I
expose to user-land change the values of the properties and since AM/FM
switches happen transparently to the user in the situation when FM
specific property is changed while tuner is in AM mode, the driver has
to cache the value and write it when the switch to FM would take place.

Also due to the way the driver uses the chip it is only powered up when
the corresponding file in devfs(e.g. /dev/radio0) is opened at least by
one user which means that unless there is a user who opened the file all
the SET/GET_PROPERTY commands sent to it will be lost. The codec driver
for that chip does not have any say in the power management policy(while
all the audio configuration is done via properties) if the chip is not
powered up the driver has to cache the configuration values it has so
that they can be applied later.

So, since I have to implement a caching functionality in the driver, in
order to avoid reinventing the wheel I opted for using 'regmap' API for
this.
Of course, It is possible that I misunderstood the purpose and
capabilities of the 'regmap' framework, which would make my code look
very silly indeed. If that is the case I'll just re-implement it using
some sort of ad-hoc version of caching.



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

[PATCH v3 2/6] Add the main bulk of core driver for SI476x code

2012-10-23 Thread Andrey Smirnov 
This patch adds main part(out of three) of the I2C driver for the
core of MFD device.

Signed-off-by: Andrey Smirnov andrey.smir...@convergeddevices.net
---
 drivers/mfd/si476x-i2c.c |  966 ++
 1 file changed, 966 insertions(+)
 create mode 100644 drivers/mfd/si476x-i2c.c

diff --git a/drivers/mfd/si476x-i2c.c b/drivers/mfd/si476x-i2c.c
new file mode 100644
index 000..6d581bd
--- /dev/null
+++ b/drivers/mfd/si476x-i2c.c
@@ -0,0 +1,966 @@
+/*
+ * include/media/si476x-i2c.c -- Core device driver for si476x MFD
+ * device
+ *
+ * Copyright (C) 2012 Innovative Converged Devices(ICD)
+ *
+ * Author: Andrey Smirnov andrey.smir...@convergeddevices.net
+ *
+ * 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 linux/module.h
+
+#include linux/slab.h
+#include linux/interrupt.h
+#include linux/delay.h
+#include linux/gpio.h
+#include linux/regulator/consumer.h
+#include linux/i2c.h
+#include linux/err.h
+
+#include linux/mfd/si476x-core.h
+
+/* Command Timeouts */
+#define DEFAULT_TIMEOUT10
+#define TIMEOUT_TUNE   70
+#define TIMEOUT_POWER_UP   33
+
+#define MAX_IO_ERRORS 10
+
+#define SI476X_DRIVER_RDS_FIFO_DEPTH   128
+
+#define SI476X_STATUS_POLL_US 0
+
+/**
+ * si476x_core_config_pinmux() - pin function configuration function
+ *
+ * @core: Core device structure
+ *
+ * Configure the functions of the pins of the radio chip.
+ *
+ * The function returns zero in case of succes or negative error code
+ * otherwise.
+ */
+static int si476x_core_config_pinmux(struct si476x_core *core)
+{
+   int err;
+   dev_dbg(core-client-dev, Configuring pinmux\n);
+   err = si476x_core_cmd_dig_audio_pin_cfg(core,
+   core-pinmux.dclk,
+   core-pinmux.dfs,
+   core-pinmux.dout,
+   core-pinmux.xout);
+   if (err  0) {
+   dev_err(core-client-dev,
+   Failed to configure digital audio pins(err = %d)\n,
+   err);
+   return err;
+   }
+
+   err = si476x_core_cmd_zif_pin_cfg(core,
+ core-pinmux.iqclk,
+ core-pinmux.iqfs,
+ core-pinmux.iout,
+ core-pinmux.qout);
+   if (err  0) {
+   dev_err(core-client-dev,
+   Failed to configure ZIF pins(err = %d)\n,
+   err);
+   return err;
+   }
+
+   err = si476x_core_cmd_ic_link_gpo_ctl_pin_cfg(core,
+ core-pinmux.icin,
+ core-pinmux.icip,
+ core-pinmux.icon,
+ core-pinmux.icop);
+   if (err  0) {
+   dev_err(core-client-dev,
+   Failed to configure IC-Link/GPO pins(err = %d)\n,
+   err);
+   return err;
+   }
+
+   err = si476x_core_cmd_ana_audio_pin_cfg(core,
+   core-pinmux.lrout);
+   if (err  0) {
+   dev_err(core-client-dev,
+   Failed to configure analog audio pins(err = %d)\n,
+   err);
+   return err;
+   }
+
+   err = si476x_core_cmd_intb_pin_cfg(core,
+  core-pinmux.intb,
+  core-pinmux.a1);
+   if (err  0) {
+   dev_err(core-client-dev,
+   Failed to configure interrupt pins(err = %d)\n,
+   err);
+   return err;
+   }
+
+   return 0;
+}
+
+static inline void si476x_core_schedule_polling_work(struct si476x_core *core)
+{
+   schedule_delayed_work(core-status_monitor,
+   usecs_to_jiffies(atomic_read(core-polling_interval)));
+}
+
+/**
+ * si476x_core_start() - early chip startup function
+ * @core: Core device structure
+ * @soft: When set, this flag forces soft startup, where soft
+ * power down is the one done by sending appropriate command instead
+ * of using reset pin of the tuner
+ *
+ * Perform required startup sequence to correctly power
+ * up the