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?

Attachments:

(No filename) (344.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-10-25 22:26:10

by Andrey Smirnov

[permalink] [raw]

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

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.

2012-10-27 21:31:18

On 11/16/2012 07:02 AM, Hans Verkuil wrote:
> Hi Andrey,
>
> Some more comments...
>
> On Tue October 23 2012 20:44:31 Andrey Smirnov wrote:
>> This commit adds a driver that exposes all the radio related
>> functionality of the Si476x series of chips via the V4L2 subsystem.
>>
>> Signed-off-by: Andrey Smirnov <[email protected]>
>> ---
>> drivers/media/radio/Kconfig | 17 +
>> drivers/media/radio/Makefile | 1 +
>> drivers/media/radio/radio-si476x.c | 1549 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 1567 insertions(+)
>> create mode 100644 drivers/media/radio/radio-si476x.c
>>
> <cut>
>
>> diff --git a/drivers/media/radio/radio-si476x.c b/drivers/media/radio/radio-si476x.c
>> new file mode 100644
>> index 0000000..c8fa90f
>> --- /dev/null
>> +++ b/drivers/media/radio/radio-si476x.c
>> @@ -0,0 +1,1549 @@
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/slab.h>
>> +#include <linux/atomic.h>
>> +#include <linux/videodev2.h>
>> +#include <linux/mutex.h>
>> +#include <linux/debugfs.h>
>> +#include <media/v4l2-common.h>
>> +#include <media/v4l2-ioctl.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-event.h>
>> +#include <media/v4l2-device.h>
>> +
>> +
>> +#include <linux/mfd/si476x-core.h>
>> +
>> +#define FM_FREQ_RANGE_LOW 64000000
>> +#define FM_FREQ_RANGE_HIGH 108000000
>> +
>> +#define AM_FREQ_RANGE_LOW 520000
>> +#define AM_FREQ_RANGE_HIGH 30000000
>> +
>> +#define PWRLINEFLTR (1 << 8)
>> +
>> +#define FREQ_MUL (10000000 / 625)
>> +
>> +#define SI476X_PHDIV_STATUS_LINK_LOCKED(status) (0b10000000 & (status))
>> +
>> +#define DRIVER_NAME "si476x-radio"
>> +#define DRIVER_CARD "SI476x AM/FM Receiver"
>> +
>> +enum si476x_freq_bands {
>> + SI476X_BAND_FM,
>> + SI476X_BAND_AM,
>> +};
>> +
>> +static const struct v4l2_frequency_band si476x_bands[] = {
>> + [SI476X_BAND_FM] = {
>> + .type = V4L2_TUNER_RADIO,
>> + .index = SI476X_BAND_FM,
>> + .capability = V4L2_TUNER_CAP_LOW
>> + | V4L2_TUNER_CAP_STEREO
>> + | V4L2_TUNER_CAP_RDS
>> + | V4L2_TUNER_CAP_RDS_BLOCK_IO
>> + | V4L2_TUNER_CAP_FREQ_BANDS,
>> + .rangelow = 64 * FREQ_MUL,
>> + .rangehigh = 108 * FREQ_MUL,
>> + .modulation = V4L2_BAND_MODULATION_FM,
>> + },
>> + [SI476X_BAND_AM] = {
>> + .type = V4L2_TUNER_RADIO,
>> + .index = SI476X_BAND_AM,
>> + .capability = V4L2_TUNER_CAP_LOW | V4L2_TUNER_CAP_FREQ_BANDS,
>> + .rangelow = 0.52 * FREQ_MUL,
>> + .rangehigh = 30 * FREQ_MUL,
>> + .modulation = V4L2_BAND_MODULATION_AM,
>> + },
>> +};
>> +
>> +static inline bool si476x_radio_freq_is_inside_of_the_band(u32 freq, int band)
>> +{
>> + return freq >= si476x_bands[band].rangelow &&
>> + freq <= si476x_bands[band].rangehigh;
>> +}
>> +
>> +static inline bool si476x_radio_range_is_inside_of_the_band(u32 low, u32 high, int band)
>> +{
>> + return low >= si476x_bands[band].rangelow &&
>> + high <= si476x_bands[band].rangehigh;
>> +}
>> +
>> +#define PRIVATE_CTL_IDX(x) (x - V4L2_CID_PRIVATE_BASE)
> No. The new control IDs need to be added to include/uapi/linux/v4l2-controls.h
> with unique IDs. V4L2_CID_PRIVATE_BASE must not be used anymore for new controls.
>
> Since Halli Manjunatha hasn't worked on a new version of his patch with the new
> fm controls it might be something you want to take on (I'm referring to the FM RX
> control class).

Should I move all controls there, even chip specific ones, like
SI476X_CID_HARMONICS_COUNT or SI476X_CID_DIVERSITY_MODE?