[PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x

[PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x

[Tobias Klausmann]

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()!

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);
+	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__);
+
+	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);
 	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 *);
 };
 
 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 */
-- 
2.21.0

Re: [PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x

[Tobias Klausmann]

Ping,

comments for this patch are appreciated!

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()!
>
> 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);
> +	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__);
> +
> +	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);
>   	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 *);
>   };
>   
>   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 */

Re: [PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x

[Sean Young]

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

Re: [PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x

[Tobias Klausmann]

Hello,

answers, if appropriate, inline!


On 26.05.19 11:33, Sean Young wrote:
> 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.

No problem, thanks for reviewing!

>
> Please run script/checkpatch.pl --strict on your patch. There are several
> cosmetic changes needed.

Will do!

>> 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?

I have hardware for a driver living out of tree (sadly): saa716x. So 
that driver was used to test the new functionality provided by this 
patch. I could convert the drivers in-tree to use the new 
dvb_module_probe(), yet without actually testing it.

>
>>> 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?

See answer above!


>
>
>>>    	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


for the other comments: Adaptations will come with a v2 patch!

Thanks,

Tobias

[PATCH v2] drivers/media/dvb-frontends: Implement probe/remove for stv6110x

[Tobias Klausmann]

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()!

v2:
- Impovments based on comments by Sean Young
- Fix checkpatch.pl --strict errors

Signed-off-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
---
 drivers/media/dvb-frontends/stv6110x.c      | 135 ++++++++++++++++----
 drivers/media/dvb-frontends/stv6110x.h      |   3 +
 drivers/media/dvb-frontends/stv6110x_priv.h |   3 +-
 3 files changed, 118 insertions(+), 23 deletions(-)

diff --git a/drivers/media/dvb-frontends/stv6110x.c b/drivers/media/dvb-frontends/stv6110x.c
index 0126cfae2e03..f2368ed20bc1 100644
--- a/drivers/media/dvb-frontends/stv6110x.c
+++ b/drivers/media/dvb-frontends/stv6110x.c
@@ -333,6 +333,41 @@ 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",
@@ -342,7 +377,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,
@@ -356,48 +391,104 @@ 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(*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);
+
+	dev_info(&stv6110x->i2c->dev, "Probed STV6110x\n");
+
+	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(*stv6110x), GFP_KERNEL);
 	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;
 
-	printk(KERN_INFO "%s: Attaching STV6110x\n", __func__);
+	dev_info(&stv6110x->i2c->dev, "Attaching STV6110x\n");
 	return stv6110x->devctl;
 }
 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 1630e55255fd..1feade3158c2 100644
--- a/drivers/media/dvb-frontends/stv6110x.h
+++ b/drivers/media/dvb-frontends/stv6110x.h
@@ -15,6 +15,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 *i2c);
 };
 
 enum tuner_mode {
diff --git a/drivers/media/dvb-frontends/stv6110x_priv.h b/drivers/media/dvb-frontends/stv6110x_priv.h
index 909094df28df..b27769558f78 100644
--- a/drivers/media/dvb-frontends/stv6110x_priv.h
+++ b/drivers/media/dvb-frontends/stv6110x_priv.h
@@ -54,11 +54,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 */
-- 
2.21.0

Re: [PATCH v2] drivers/media/dvb-frontends: Implement probe/remove for stv6110x

[Joe Perches]

On Wed, 2019-05-29 at 18:56 +0200, 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()!
> 
> v2:
> - Impovments based on comments by Sean Young
> - Fix checkpatch.pl --strict errors

trivia:

> diff --git a/drivers/media/dvb-frontends/stv6110x.c b/drivers/media/dvb-frontends/stv6110x.c
[]
> @@ -333,6 +333,41 @@ 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};

static const u8...

> +
> +	memcpy(stv6110x->regs, default_regs, 8);

	memcpy(stv6110x->regs, default_regs, ARRAY_SIZE(default_regs));

> +}
> +
> +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;
> +	}
> +}

Probably more sensible (and smaller object code) written using
an automatic like:

{
	int div;

	switch (stv6110x->config->clk_div) {
	case 8:
		div = 3;
		break;
	case 4:
		div = 2;
		break;
	case 2:
		div = 1;
		break;
	case 1:
	default:
		div = 0;
		break;
	}
	STV6110x_SETFIELD(stv6110x->regs[STV6110x_CTRL2], CTRL2_CO_DIV, div);
}

> diff --git a/drivers/media/dvb-frontends/stv6110x_priv.h b/drivers/media/dvb-frontends/stv6110x_priv.h
[]
> @@ -54,11 +54,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];

Perhaps this 8 should be a define?

Re: [PATCH v2] drivers/media/dvb-frontends: Implement probe/remove for stv6110x

[Tobias Klausmann]

On 29.05.19 20:45, Joe Perches wrote:
> On Wed, 2019-05-29 at 18:56 +0200, 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()!
>>
>> v2:
>> - Impovments based on comments by Sean Young
>> - Fix checkpatch.pl --strict errors
> trivia:
>
>> diff --git a/drivers/media/dvb-frontends/stv6110x.c b/drivers/media/dvb-frontends/stv6110x.c
> []
>> @@ -333,6 +333,41 @@ 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};
> static const u8...
>
>> +
>> +	memcpy(stv6110x->regs, default_regs, 8);
> 	memcpy(stv6110x->regs, default_regs, ARRAY_SIZE(default_regs));
>
>> +}
>> +
>> +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;
>> +	}
>> +}
> Probably more sensible (and smaller object code) written using
> an automatic like:
>
> {
> 	int div;
>
> 	switch (stv6110x->config->clk_div) {
> 	case 8:
> 		div = 3;
> 		break;
> 	case 4:
> 		div = 2;
> 		break;
> 	case 2:
> 		div = 1;
> 		break;
> 	case 1:
> 	default:
> 		div = 0;
> 		break;
> 	}
> 	STV6110x_SETFIELD(stv6110x->regs[STV6110x_CTRL2], CTRL2_CO_DIV, div);
> }
>
>> diff --git a/drivers/media/dvb-frontends/stv6110x_priv.h b/drivers/media/dvb-frontends/stv6110x_priv.h
> []
>> @@ -54,11 +54,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];
> Perhaps this 8 should be a define?
>
>

Hi,

thanks for the comments! If really desired i can change the code 
further, adapting to your comments, but note that the code was 
essentially just moved around to cater to both _probe() and attach(), 
intentionally leaving it as it was before the patch!

Greetings,

Tobias

Re: [PATCH v2] drivers/media/dvb-frontends: Implement probe/remove for stv6110x

[Joe Perches]

On Wed, 2019-05-29 at 21:03 +0200, Tobias Klausmann wrote:
> thanks for the comments! If really desired i can change the code 
> further, adapting to your comments, but note that the code was 
> essentially just moved around to cater to both _probe() and attach(), 
> intentionally leaving it as it was before the patch!

Up to you.
My general preference is for human intelligible and simple code.