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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 */
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
---
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
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?
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
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.