LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sean Young <sean@mess.org>
To: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
Cc: linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
	mchehab@kernel.org
Subject: Re: [PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x
Date: Sun, 26 May 2019 10:33:57 +0100	[thread overview]
Message-ID: <20190526093355.ednxp725ui6lwoji@gofer.mess.org> (raw)
In-Reply-To: <ba914388-04d1-8a57-77eb-3f303941b9bf@mni.thm.de>

Hi Tobias,

On Sun, May 12, 2019 at 04:53:06PM +0200, Tobias Klausmann wrote:
> Ping,
> 
> comments for this patch are appreciated!

Sorry for not back to you earlier. 

Please run script/checkpatch.pl --strict on your patch. There are several
cosmetic changes needed.
> 
> Thanks,
> 
> Tobias
> 
> 
> On 09.05.19 21:51, Tobias Klausmann wrote:
> > Refactor out the common parts of stv6110x_probe() and stv6110x_attach() into
> > separate functions.
> > 
> > This provides the needed functionality to use dvb_module_probe() instead of
> > dvb_attach()!

The lines shouldn't be longer than 75 characters.

This is a great improvement. It would be nice to see an actual driver use
dvb_module_probe() rather than dvb_attach(), so that the new code paths
are used. Do you have hardware to test this?

> > 
> > Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
> > ---
> >   drivers/media/dvb-frontends/stv6110x.c      | 125 ++++++++++++++++----
> >   drivers/media/dvb-frontends/stv6110x.h      |   3 +
> >   drivers/media/dvb-frontends/stv6110x_priv.h |   3 +-
> >   3 files changed, 109 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/media/dvb-frontends/stv6110x.c b/drivers/media/dvb-frontends/stv6110x.c
> > index 82c002d3833a..17bc7adf3771 100644
> > --- a/drivers/media/dvb-frontends/stv6110x.c
> > +++ b/drivers/media/dvb-frontends/stv6110x.c
> > @@ -345,6 +345,33 @@ static void stv6110x_release(struct dvb_frontend *fe)
> >   	kfree(stv6110x);
> >   }
> > +void st6110x_init_regs(struct stv6110x_state *stv6110x)
> > +{
> > +	u8 default_regs[] = {0x07, 0x11, 0xdc, 0x85, 0x17, 0x01, 0xe6, 0x1e};
> > +
> > +	memcpy(stv6110x->regs, default_regs, 8);
> > +}
> > +
> > +void stv6110x_setup_divider(struct stv6110x_state *stv6110x)
> > +{
> > +	switch (stv6110x->config->clk_div) {
> > +	default:
> > +	case 1:
> > +		STV6110x_SETFIELD(stv6110x->regs[STV6110x_CTRL2], CTRL2_CO_DIV, 0);
> > +		break;
> > +	case 2:
> > +		STV6110x_SETFIELD(stv6110x->regs[STV6110x_CTRL2], CTRL2_CO_DIV, 1);
> > +		break;
> > +	case 4:
> > +		STV6110x_SETFIELD(stv6110x->regs[STV6110x_CTRL2], CTRL2_CO_DIV, 2);
> > +		break;
> > +	case 8:
> > +	case 0:
> > +		STV6110x_SETFIELD(stv6110x->regs[STV6110x_CTRL2], CTRL2_CO_DIV, 3);
> > +		break;
> > +	}
> > +}
> > +
> >   static const struct dvb_tuner_ops stv6110x_ops = {
> >   	.info = {
> >   		.name		  = "STV6110(A) Silicon Tuner",
> > @@ -354,7 +381,7 @@ static const struct dvb_tuner_ops stv6110x_ops = {
> >   	.release		= stv6110x_release
> >   };
> > -static const struct stv6110x_devctl stv6110x_ctl = {
> > +static struct stv6110x_devctl stv6110x_ctl = {
> >   	.tuner_init		= stv6110x_init,
> >   	.tuner_sleep		= stv6110x_sleep,
> >   	.tuner_set_mode		= stv6110x_set_mode,
> > @@ -368,39 +395,77 @@ static const struct stv6110x_devctl stv6110x_ctl = {
> >   	.tuner_get_status	= stv6110x_get_status,
> >   };
> > +void stv6110x_set_frontend_opts(struct stv6110x_state *stv6110x)
> > +{
> > +	stv6110x->frontend->tuner_priv		= stv6110x;
> > +	stv6110x->frontend->ops.tuner_ops	= stv6110x_ops;
> > +}
> > +
> > +static struct stv6110x_devctl *stv6110x_get_devctl(struct i2c_client *client)
> > +{
> > +	struct stv6110x_state *stv6110x = i2c_get_clientdata(client);
> > +
> > +	dev_dbg(&client->dev, "\n");
> > +
> > +	return stv6110x->devctl;
> > +}
> > +
> > +static int stv6110x_probe(struct i2c_client *client,
> > +			const struct i2c_device_id *id)
> > +{
> > +	struct stv6110x_config *config = client->dev.platform_data;
> > +
> > +	struct stv6110x_state *stv6110x;
> > +
> > +	stv6110x = kzalloc(sizeof(struct stv6110x_state), GFP_KERNEL);

This should be:
	stv6110x = kzalloc(sizeof(*stv6110x), GFP_KERNEL);

> > +	if (!stv6110x)
> > +		return -ENOMEM;
> > +
> > +	stv6110x->frontend	= config->frontend;
> > +	stv6110x->i2c		= client->adapter;
> > +	stv6110x->config	= config;
> > +	stv6110x->devctl	= &stv6110x_ctl;
> > +
> > +	st6110x_init_regs(stv6110x);
> > +	stv6110x_setup_divider(stv6110x);
> > +	stv6110x_set_frontend_opts(stv6110x);
> > +
> > +	printk(KERN_INFO "%s: Probed STV6110x\n", __func__);

Please use dev_info().

> > +
> > +	i2c_set_clientdata(client, stv6110x);
> > +
> > +	/* setup callbacks */
> > +	config->get_devctl = stv6110x_get_devctl;
> > +
> > +	return 0;
> > +}
> > +
> > +static int stv6110x_remove(struct i2c_client *client)
> > +{
> > +	struct stv6110x_state *stv6110x = i2c_get_clientdata(client);
> > +
> > +	stv6110x_release(stv6110x->frontend);
> > +	return 0;
> > +}
> > +
> >   const struct stv6110x_devctl *stv6110x_attach(struct dvb_frontend *fe,
> >   					const struct stv6110x_config *config,
> >   					struct i2c_adapter *i2c)
> >   {
> >   	struct stv6110x_state *stv6110x;
> > -	u8 default_regs[] = {0x07, 0x11, 0xdc, 0x85, 0x17, 0x01, 0xe6, 0x1e};
> > -	stv6110x = kzalloc(sizeof (struct stv6110x_state), GFP_KERNEL);
> > +	stv6110x = kzalloc(sizeof(struct stv6110x_state), GFP_KERNEL);

	stv6110x = kzalloc(sizeof(*stv6110x), GFP_KERNEL);

Maybe we can patch up the dvb_attach() call sites and do away with
stv6110x_attach completely. Do you have dvb hardware with this frontend
to test?


> >   	if (!stv6110x)
> >   		return NULL;
> > +	stv6110x->frontend	= fe;
> >   	stv6110x->i2c		= i2c;
> >   	stv6110x->config	= config;
> >   	stv6110x->devctl	= &stv6110x_ctl;
> > -	memcpy(stv6110x->regs, default_regs, 8);
> > -	/* setup divider */
> > -	switch (stv6110x->config->clk_div) {
> > -	default:
> > -	case 1:
> > -		STV6110x_SETFIELD(stv6110x->regs[STV6110x_CTRL2], CTRL2_CO_DIV, 0);
> > -		break;
> > -	case 2:
> > -		STV6110x_SETFIELD(stv6110x->regs[STV6110x_CTRL2], CTRL2_CO_DIV, 1);
> > -		break;
> > -	case 4:
> > -		STV6110x_SETFIELD(stv6110x->regs[STV6110x_CTRL2], CTRL2_CO_DIV, 2);
> > -		break;
> > -	case 8:
> > -	case 0:
> > -		STV6110x_SETFIELD(stv6110x->regs[STV6110x_CTRL2], CTRL2_CO_DIV, 3);
> > -		break;
> > -	}
> > +	st6110x_init_regs(stv6110x);
> > +	stv6110x_setup_divider(stv6110x);
> > +	stv6110x_set_frontend_opts(stv6110x);
> >   	fe->tuner_priv		= stv6110x;
> >   	fe->ops.tuner_ops	= stv6110x_ops;
> > @@ -410,6 +475,24 @@ const struct stv6110x_devctl *stv6110x_attach(struct dvb_frontend *fe,
> >   }
> >   EXPORT_SYMBOL(stv6110x_attach);
> > +static const struct i2c_device_id stv6110x_id_table[] = {
> > +	{"stv6110x", 0},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, stv6110x_id_table);
> > +
> > +static struct i2c_driver stv6110x_driver = {
> > +	.driver = {
> > +		.name	= "stv6110x",
> > +		.suppress_bind_attrs = true,
> > +	},
> > +	.probe		= stv6110x_probe,
> > +	.remove		= stv6110x_remove,
> > +	.id_table	= stv6110x_id_table,
> > +};
> > +
> > +module_i2c_driver(stv6110x_driver);
> > +
> >   MODULE_AUTHOR("Manu Abraham");
> >   MODULE_DESCRIPTION("STV6110x Silicon tuner");
> >   MODULE_LICENSE("GPL");
> > diff --git a/drivers/media/dvb-frontends/stv6110x.h b/drivers/media/dvb-frontends/stv6110x.h
> > index 696b6e5b9e7b..7714adea5242 100644
> > --- a/drivers/media/dvb-frontends/stv6110x.h
> > +++ b/drivers/media/dvb-frontends/stv6110x.h
> > @@ -27,6 +27,9 @@ struct stv6110x_config {
> >   	u8	addr;
> >   	u32	refclk;
> >   	u8	clk_div; /* divisor value for the output clock */
> > +	struct dvb_frontend		*frontend;
> > +
> > +	struct stv6110x_devctl* (*get_devctl)(struct i2c_client *);

The i2c_client needs an argument name.

> >   };
> >   enum tuner_mode {
> > diff --git a/drivers/media/dvb-frontends/stv6110x_priv.h b/drivers/media/dvb-frontends/stv6110x_priv.h
> > index 109dfaf4ba42..383549d25268 100644
> > --- a/drivers/media/dvb-frontends/stv6110x_priv.h
> > +++ b/drivers/media/dvb-frontends/stv6110x_priv.h
> > @@ -66,11 +66,12 @@
> >   #define REFCLOCK_MHz				(stv6110x->config->refclk / 1000000)
> >   struct stv6110x_state {
> > +	struct dvb_frontend		*frontend;
> >   	struct i2c_adapter		*i2c;
> >   	const struct stv6110x_config	*config;
> >   	u8				regs[8];
> > -	const struct stv6110x_devctl	*devctl;
> > +	struct stv6110x_devctl	*devctl;
> >   };
> >   #endif /* __STV6110x_PRIV_H */


Thanks,

Sean

  reply	other threads:[~2019-05-26  9:34 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-09 19:51 Tobias Klausmann
2019-05-12 14:53 ` Tobias Klausmann
2019-05-26  9:33   ` Sean Young [this message]
2019-05-26 13:11     ` Tobias Klausmann
2019-05-29 16:56 ` [PATCH v2] " Tobias Klausmann
2019-05-29 18:45   ` Joe Perches
2019-05-29 19:03     ` Tobias Klausmann
2019-05-30  4:20       ` Joe Perches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190526093355.ednxp725ui6lwoji@gofer.mess.org \
    --to=sean@mess.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=tobias.johannes.klausmann@mni.thm.de \
    --subject='Re: [PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).