* [PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x @ 2019-05-09 19:51 Tobias Klausmann 2019-05-12 14:53 ` Tobias Klausmann 2019-05-29 16:56 ` [PATCH v2] " Tobias Klausmann 0 siblings, 2 replies; 8+ messages in thread From: Tobias Klausmann @ 2019-05-09 19:51 UTC (permalink / raw) To: linux-kernel, linux-media, mchehab; +Cc: 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x 2019-05-09 19:51 [PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x Tobias Klausmann @ 2019-05-12 14:53 ` Tobias Klausmann 2019-05-26 9:33 ` Sean Young 2019-05-29 16:56 ` [PATCH v2] " Tobias Klausmann 1 sibling, 1 reply; 8+ messages in thread From: Tobias Klausmann @ 2019-05-12 14:53 UTC (permalink / raw) To: linux-kernel, linux-media, mchehab 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 */ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x 2019-05-12 14:53 ` Tobias Klausmann @ 2019-05-26 9:33 ` Sean Young 2019-05-26 13:11 ` Tobias Klausmann 0 siblings, 1 reply; 8+ messages in thread From: Sean Young @ 2019-05-26 9:33 UTC (permalink / raw) To: Tobias Klausmann; +Cc: linux-kernel, linux-media, mchehab 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x 2019-05-26 9:33 ` Sean Young @ 2019-05-26 13:11 ` Tobias Klausmann 0 siblings, 0 replies; 8+ messages in thread From: Tobias Klausmann @ 2019-05-26 13:11 UTC (permalink / raw) To: Sean Young; +Cc: linux-kernel, linux-media, mchehab 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] drivers/media/dvb-frontends: Implement probe/remove for stv6110x 2019-05-09 19:51 [PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x Tobias Klausmann 2019-05-12 14:53 ` Tobias Klausmann @ 2019-05-29 16:56 ` Tobias Klausmann 2019-05-29 18:45 ` Joe Perches 1 sibling, 1 reply; 8+ messages in thread From: Tobias Klausmann @ 2019-05-29 16:56 UTC (permalink / raw) To: linux-kernel, linux-media, mchehab, sean; +Cc: 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drivers/media/dvb-frontends: Implement probe/remove for stv6110x 2019-05-29 16:56 ` [PATCH v2] " Tobias Klausmann @ 2019-05-29 18:45 ` Joe Perches 2019-05-29 19:03 ` Tobias Klausmann 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2019-05-29 18:45 UTC (permalink / raw) To: Tobias Klausmann, linux-kernel, linux-media, mchehab, sean 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drivers/media/dvb-frontends: Implement probe/remove for stv6110x 2019-05-29 18:45 ` Joe Perches @ 2019-05-29 19:03 ` Tobias Klausmann 2019-05-30 4:20 ` Joe Perches 0 siblings, 1 reply; 8+ messages in thread From: Tobias Klausmann @ 2019-05-29 19:03 UTC (permalink / raw) To: Joe Perches, linux-kernel, linux-media, mchehab, sean 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drivers/media/dvb-frontends: Implement probe/remove for stv6110x 2019-05-29 19:03 ` Tobias Klausmann @ 2019-05-30 4:20 ` Joe Perches 0 siblings, 0 replies; 8+ messages in thread From: Joe Perches @ 2019-05-30 4:20 UTC (permalink / raw) To: Tobias Klausmann, linux-kernel, linux-media, mchehab, sean 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-05-30 4:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-09 19:51 [PATCH] drivers/media/dvb-frontends: Implement probe/remove for stv6110x Tobias Klausmann 2019-05-12 14:53 ` Tobias Klausmann 2019-05-26 9:33 ` Sean Young 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
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).