On Tue, 20 Mar 2012 23:14:35 +0100
"Hans-Frieder Vogt" <hfvogt@xxxxxxx> wrote:
> +/*
> + buf[0] is the first register address
> + */
Just for me to understand this:
How does this work? Does the hardware auto-increment the register address
automatically after each received byte? If so, we could probably document
that here in the comment.
> +static int fc0012_writeregs(struct fc0012_priv *priv, u8 *buf, int len)
> +{
> + struct i2c_msg msg = {
> + .addr = priv->addr, .flags = 0, .buf = buf, .len = len
> + };
> +
> + if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
> + err("I2C write regs failed, reg: %02x, val[0]: %02x",
> + buf[0], len > 1 ? buf[1] : 0);
> + return -EREMOTEIO;
> + }
> + return 0;
> +}
> +static int fc0012_set_params(struct dvb_frontend *fe)
> +{
> +
> + priv->frequency = freq;
I think this either needs a freq*1000, or priv->frequency=p->frequency.
> + priv->bandwidth = p->bandwidth_hz;
> +
> +error_out:
> + return ret;
> +}
> +static const struct dvb_tuner_ops fc0012_tuner_ops = {
> + .info = {
> + .name = "Fitipower FC0012",
> +
> + .frequency_min = 170000000,
> + .frequency_max = 860000000,
> + .frequency_step = 0,
> + },
> +
> + .release = fc0012_release,
> +
> + .init = fc0012_init,
> + .sleep = fc0012_sleep,
> +
> + .set_params = fc0012_set_params,
> +
> + .get_frequency = fc0012_get_frequency,
> + .get_if_frequency = fc0012_get_if_frequency,
> + .get_bandwidth = fc0012_get_bandwidth,
> +};
> +
> +struct dvb_frontend * fc0012_attach(struct dvb_frontend *fe,
> + struct i2c_adapter *i2c, u8 i2c_address,
> + enum fc0012_xtal_freq xtal_freq)
> +{
> + struct fc0012_priv *priv = NULL;
Should use tab indention here and in the tuner_ops struct above.
> +
> +#ifndef _FC0012_H_
> +#define _FC0012_H_
> +
> +#include "dvb_frontend.h"
> +
> +enum fc0012_xtal_freq {
> + FC_XTAL_27_MHZ, /* 27000000 */
> + FC_XTAL_28_8_MHZ, /* 28800000 */
> + FC_XTAL_36_MHZ, /* 36000000 */
> +};
> +
> +#if defined(CONFIG_MEDIA_TUNER_FC0012) || \
> + (defined(CONFIG_MEDIA_TUNER_FC0012_MODULE) && defined(MODULE))
Why check for defined(MODULE) here?
> +extern struct dvb_frontend *fc0012_attach(struct dvb_frontend *fe,
> + struct i2c_adapter *i2c,
> + u8 i2c_address,
> + enum fc0012_xtal_freq xtal_freq);
> +#else
> +#define LOG_PREFIX "fc0012"
> +
> +#define dprintk(var, level, args...) \
> + do { \
> + if ((var & level)) \
> + printk(args); \
> + } while (0)
> +
> +#define deb_info(args...) dprintk(fc0012_debug, 0x01, args)
> +
> +#undef err
> +#define err(f, arg...) printk(KERN_ERR LOG_PREFIX": " f "\n" , ## arg)
> +#undef info
> +#define info(f, arg...) printk(KERN_INFO LOG_PREFIX": " f "\n" , ## arg)
> +#undef warn
> +#define warn(f, arg...) printk(KERN_WARNING LOG_PREFIX": " f "\n" , ## arg)
I think you should get rid of all these custom print helpers and use dev_err,
dev_info and friends. Those are more ideomatic.
> +struct fc0012_priv {
> + struct i2c_adapter *i2c;
> + u8 addr;
> + u8 xtal_freq;
> +
> + u32 frequency;
> + u32 bandwidth;
> +};
Here seems to be some whitespace mixing. tab indention vs. space indention.
--
Greetings, Michael.
PGP encryption is encouraged / 908D8B0E
Attachment:
signature.asc
Description: PGP signature
