[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <201210091153.00324.hverkuil@xs4all.nl>
Date: Tue, 9 Oct 2012 11:53:00 +0200
From: Hans Verkuil <hverkuil@...all.nl>
To: Andrey Smirnov <andrey.smirnov@...vergeddevices.net>
Cc: andrey.smrinov@...vergeddevices.net, mchehab@...hat.com,
sameo@...ux.intel.com, broonie@...nsource.wolfsonmicro.com,
perex@...ex.cz, tiwai@...e.de, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/6] Add header files and Kbuild plumbing for SI476x MFD core
On Mon 8 October 2012 20:38:01 Andrey Smirnov wrote:
> On 10/08/2012 01:43 AM, Hans Verkuil wrote:
> > On Sat October 6 2012 03:54:57 Andrey Smirnov wrote:
> >> This patch adds all necessary header files and Kbuild plumbing for the
> >> 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 <andrey.smirnov@...vergeddevices.net>
> >> ---
> >> drivers/mfd/Kconfig | 14 ++
> >> drivers/mfd/Makefile | 3 +
> >> include/linux/mfd/si476x-core.h | 529 +++++++++++++++++++++++++++++++++++++++
> >> include/media/si476x.h | 449 +++++++++++++++++++++++++++++++++
> >> 4 files changed, 995 insertions(+)
> >> create mode 100644 include/linux/mfd/si476x-core.h
> >> create mode 100644 include/media/si476x.h
> >>
<snip>
> >> diff --git a/include/linux/mfd/si476x-core.h b/include/linux/mfd/si476x-core.h
> >> new file mode 100644
> >> index 0000000..eb6f52a
> >> --- /dev/null
> >> +++ b/include/linux/mfd/si476x-core.h
<snip>
> >> +#define SI476X_IOC_GET_RSQ _IOWR('V', BASE_VIDIOC_PRIVATE + 0, \
> >> + struct si476x_rsq_status_report)
> >> +
> >> +#define SI476X_IOC_SET_PHDIV_MODE _IOW('V', BASE_VIDIOC_PRIVATE + 1, \
> >> + enum si476x_phase_diversity_mode)
> >> +
> >> +#define SI476X_IOC_GET_PHDIV_STATUS _IOWR('V', BASE_VIDIOC_PRIVATE + 2, \
> >> + int)
> >> +
> >> +#define SI476X_IOC_GET_RSQ_PRIMARY _IOWR('V', BASE_VIDIOC_PRIVATE + 3, \
> >> + struct si476x_rsq_status_report)
> >> +
> >> +#define SI476X_IOC_GET_ACF _IOWR('V', BASE_VIDIOC_PRIVATE + 4, \
> >> + struct si476x_acf_status_report)
> >> +
> >> +#define SI476X_IOC_GET_AGC _IOWR('V', BASE_VIDIOC_PRIVATE + 5, \
> >> + struct si476x_agc_status_report)
> >> +
> >> +#define SI476X_IOC_GET_RDS_BLKCNT _IOWR('V', BASE_VIDIOC_PRIVATE + 6, \
> >> + struct si476x_rds_blockcount_report)
> > There is no documentation at all for these private ioctls. At the very least
> > these should be documentated (both the ioctl and the structs they receive).
> >
> > More importantly, are these ioctls really needed?
>
> I apologize for not writing the documentation for those ioctls. I
> thought I at least did so for all data structures returned by those
> calls, I guess I never got around to doing it. I'll document all the
> ioctls in the next version of the patch.
>
> SI476X_IOC_SET_PHDIV_MODE, SI476X_IOC_GET_PHDIV_STATUS are definitely
> needed since they are used to control antenna phase diversity modes of
> the tuners. In that mode two Si4764 chips connected to different
> antennas can act as a single tuner and use both signal to improve
> sensibility.
Interesting. But I wonder if this can be implemented as controls? That seems
to me to be a better fit.
> The rest is useful to get different radio signal parameters from the
> chip. We used it for during RF performance evaluation of the
> boards(probably using it in EOL testing).
>
> > If the purpose is to return
> > status information for debugging, then you should consider implementing
> > VIDIOC_LOG_STATUS instead.
>
> For me, the problem with using VIDIOC_LOG_STATUS is that it dumps all
> the debugging information in kernel log buffer, meaning that if one
> wants to pass that information to some other application they would have
> to resort to screen-scraping of the output of dmesg. Unfortunately the
> people who were using this driver/as a part of a software suite during
> aforementioned RF performance testing are not Linux-savvy enough to be
> asked to use dmesg and they want a GUI solution. That small test utility
> was written and it uses those those ioctls to gather and display all
> signal related information. I guess the other solution for that problem
> would be to create corresponding files in sysfs, but I'm not sure if
> creating multitude of files in sysfs tree is a better option.
Definitely not, although using debugfs might be a good idea.
> Since this version of the driver has not been integrated into our
> software package the we install on laptops to do all the board testing I
> guess I can remove those 'ioctl', but I would love to find acceptable
> solution so that later I would be able to integrate upstream driver int
> our package and not use "special" version of the driver for our purposes.
Can you split off the implementation of these ioctls from the rest of the
driver? And I think you should look into debugfs to see if that isn't a better
fit then custom ioctls. debugfs has the advantage that you can make changes
later as it isn't considered to be a public API. And this is used primarily
for debugging/testing after all.
But I think it is best to split this off and concentrate on getting the driver
itself in first. In general we don't like custom ioctls unless there is a
really good reason for it. They tend to be poorly maintained and documented
and can become a bit of a nuisance over time.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists