Re: [PATCH] v0.3 Support for tuner FC0012

["Michael Büsch" <m@bues.ch>]

[-- Attachment #1: Type: text/plain, Size: 3514 bytes --]

On Tue, 20 Mar 2012 23:14:35 +0100
"Hans-Frieder Vogt" <hfvogt@gmx.net> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]