| Message ID | 20200109152408.919325-2-dwlsalmeida@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement a DVB Dummy Tuner | expand |
On Thu, Jan 09, 2020 at 12:24:08PM -0300, Daniel W. S. Almeida wrote: > +static int dvb_dummy_tuner_i2c_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + int ret = 0; > + struct dvb_dummy_tuner_config *config = client->dev.platform_data; > + struct dvb_frontend *fe = config->fe; > + struct dvb_dummy_tuner_dev *tuner_dev = NULL; > + > + tuner_dev = kzalloc(sizeof(*tuner_dev), GFP_KERNEL); > + if (!tuner_dev) { > + ret = -ENOMEM; > + goto err; > + } > + > + i2c_set_clientdata(client, tuner_dev); > + tuner_dev->fe = config->fe; > + > + memcpy(&fe->ops.tuner_ops, > + &dvb_dummy_tuner_ops, > + sizeof(struct dvb_tuner_ops)); > + > + fe->tuner_priv = client; > + > + pr_debug("%s: Successfully probed %s\n", __func__, client->name); As you are a driver, you should never need to call any pr_* calls, instead use dev_*(). For this, you can use dev_dbg(), but really, why is that even needed except for your debugging bringup. And for that, you can use ftrace, right? So no need for any printing of anything here. > + return ret; > + > +err: > + pr_err("%s: failed\n", __func__); Again, dev_err() would be proper, but there's no need for any error message here. Don't you need to register the tuner ops with something in this function? > + return ret; > +} > + > +static int dvb_dummy_tuner_i2c_remove(struct i2c_client *client) > +{ > + struct dvb_dummy_tuner_dev *tuner_dev = i2c_get_clientdata(client); > + struct dvb_frontend *fe = tuner_dev->fe; > + > + memset(&fe->ops.tuner_ops, 0, sizeof(struct dvb_tuner_ops)); > + fe->tuner_priv = NULL; > + kfree(tuner_dev); > + Don't you need to unregister the tuner ops in here? thanks, greg k-h
Em Thu, 9 Jan 2020 12:24:08 -0300 "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu: > From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> > > Implement the skeleton for the tuner driver in a separate file. > > Signed-off-by: Daniel W. S. Almeida <dwlsalmeida@gmail.com> > --- > drivers/media/tuners/Kconfig | 7 ++ > drivers/media/tuners/Makefile | 1 + > drivers/media/tuners/dvb_dummy_tuner.c | 153 +++++++++++++++++++++++++ > drivers/media/tuners/dvb_dummy_tuner.h | 20 ++++ > 4 files changed, 181 insertions(+) > create mode 100644 drivers/media/tuners/dvb_dummy_tuner.c > create mode 100644 drivers/media/tuners/dvb_dummy_tuner.h > > diff --git a/drivers/media/tuners/Kconfig b/drivers/media/tuners/Kconfig > index e104bb7766e1..efa1591fe0ae 100644 > --- a/drivers/media/tuners/Kconfig > +++ b/drivers/media/tuners/Kconfig > @@ -296,4 +296,11 @@ config MEDIA_TUNER_QM1D1B0004 > default m if !MEDIA_SUBDRV_AUTOSELECT > help > Sharp QM1D1B0004 ISDB-S tuner driver. > + > +config MEDIA_TUNER_DVB_DUMMY_TUNER > + tristate "Dummy tuner" > + depends on MEDIA_SUPPORT && I2C > + default m if !MEDIA_SUBDRV_AUTOSELECT Actually, this is a dummy driver. It should not be auto-selected, as no real hardware depends on it. We don't want production distros to come with those test drivers. When we add a bridge driver (let's say we call it DVB_DUMMY_BRIDGE), then what we could do here would be something like: depends on DVB_DUMMY_BRIDGE default y Cheers, Mauro
Hi Daniel, On 1/9/20 8:24 AM, Daniel W. S. Almeida wrote: > From: "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> > > Implement the skeleton for the tuner driver in a separate file. Please add more detail here on what this driver does. What this driver is used for and so on. > > Signed-off-by: Daniel W. S. Almeida <dwlsalmeida@gmail.com> > --- > drivers/media/tuners/Kconfig | 7 ++ > drivers/media/tuners/Makefile | 1 + > drivers/media/tuners/dvb_dummy_tuner.c | 153 +++++++++++++++++++++++++ > drivers/media/tuners/dvb_dummy_tuner.h | 20 ++++ > 4 files changed, 181 insertions(+) > create mode 100644 drivers/media/tuners/dvb_dummy_tuner.c > create mode 100644 drivers/media/tuners/dvb_dummy_tuner.h > > diff --git a/drivers/media/tuners/Kconfig b/drivers/media/tuners/Kconfig > index e104bb7766e1..efa1591fe0ae 100644 > --- a/drivers/media/tuners/Kconfig > +++ b/drivers/media/tuners/Kconfig > @@ -296,4 +296,11 @@ config MEDIA_TUNER_QM1D1B0004 > default m if !MEDIA_SUBDRV_AUTOSELECT > help > Sharp QM1D1B0004 ISDB-S tuner driver. > + > +config MEDIA_TUNER_DVB_DUMMY_TUNER > + tristate "Dummy tuner" > + depends on MEDIA_SUPPORT && I2C > + default m if !MEDIA_SUBDRV_AUTOSELECT > + help > + Dummy tuner driver for common TV standards. Samehere. Give more details on what this driver does. Add enough detail to help users decide why they should enable it or not. > endmenu > diff --git a/drivers/media/tuners/Makefile b/drivers/media/tuners/Makefile > index 7b4f8423501e..f98de1cf2e19 100644 > --- a/drivers/media/tuners/Makefile > +++ b/drivers/media/tuners/Makefile > @@ -44,5 +44,6 @@ obj-$(CONFIG_MEDIA_TUNER_QM1D1C0042) += qm1d1c0042.o > obj-$(CONFIG_MEDIA_TUNER_QM1D1B0004) += qm1d1b0004.o > obj-$(CONFIG_MEDIA_TUNER_M88RS6000T) += m88rs6000t.o > obj-$(CONFIG_MEDIA_TUNER_TDA18250) += tda18250.o > +obj-$(CONFIG_MEDIA_TUNER_DVB_DUMMY_TUNER) += dvb_dummy_tuner.o > > ccflags-y += -I$(srctree)/drivers/media/dvb-frontends > diff --git a/drivers/media/tuners/dvb_dummy_tuner.c b/drivers/media/tuners/dvb_dummy_tuner.c > new file mode 100644 > index 000000000000..63d2e47d4739 > --- /dev/null > +++ b/drivers/media/tuners/dvb_dummy_tuner.c > @@ -0,0 +1,153 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Driver for Dummy Frontend > + * > + * Written by Daniel W. S. Almeida <dwlsalmeida@gmail.com> General practice is the following. Copyright (c) 2020 Daniel W. S. Almeida <dwlsalmeida@gmail.com> > + */ > + > +#include <linux/errno.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/types.h> > +#include <media/dvb_frontend.h> > +#include "dvb_dummy_tuner.h" > + > +struct dvb_dummy_tuner_dev { > + struct dvb_frontend *fe; > +}; > + > +static void dvb_dummy_tuner_release(struct dvb_frontend *fe) > +{ > +} > + > +static int dvb_dummy_tuner_init(struct dvb_frontend *fe) > +{ > + return 0; > +} > + > +static int dvb_dummy_tuner_sleep(struct dvb_frontend *fe) > +{ > + return 0; > +} > + > +static int dvb_dummy_tuner_suspend(struct dvb_frontend *fe) > +{ > + return 0; > +} > + > +static int dvb_dummy_tuner_resume(struct dvb_frontend *fe) > +{ > + return 0; > +} > + > +static int dvb_dummy_tuner_set_params(struct dvb_frontend *fe) > +{ > + return 0; > +} > + > +static int dvb_dummy_tuner_set_config(struct dvb_frontend *fe, void *priv_cfg) > +{ > + return 0; > +} > + > +static int dvb_dummy_tuner_get_frequency(struct dvb_frontend *fe, > + u32 *frequency) > +{ > + return 0; > +} > + > +static int dvb_dummy_tuner_get_bandwidth(struct dvb_frontend *fe, > + u32 *bandwidth) > +{ > + return 0; > +} > + > +static int dvb_dummy_tuner_get_if_frequency(struct dvb_frontend *fe, > + u32 *frequency) > +{ > + return 0; > +} > + > +static int dvb_dummy_tuner_get_status(struct dvb_frontend *fe, u32 *status) > +{ > + return 0; > +} > + > +static const struct dvb_tuner_ops dvb_dummy_tuner_ops = { > + .release = dvb_dummy_tuner_release, > + .init = dvb_dummy_tuner_init, > + .sleep = dvb_dummy_tuner_sleep, > + .suspend = dvb_dummy_tuner_suspend, > + .resume = dvb_dummy_tuner_resume, > + .set_params = dvb_dummy_tuner_set_params, > + .set_config = dvb_dummy_tuner_set_config, > + .get_bandwidth = dvb_dummy_tuner_get_bandwidth, > + .get_frequency = dvb_dummy_tuner_get_frequency, > + .get_if_frequency = dvb_dummy_tuner_get_if_frequency, > + .get_status = dvb_dummy_tuner_get_status, > +}; > + > +static const struct i2c_device_id dvb_dummy_tuner_i2c_id_table[] = { > + {"dvb_dummy_tuner", 0}, > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, dvb_dummy_tuner_i2c_id_table); > + > +static int dvb_dummy_tuner_i2c_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + int ret = 0; > + struct dvb_dummy_tuner_config *config = client->dev.platform_data; > + struct dvb_frontend *fe = config->fe; > + struct dvb_dummy_tuner_dev *tuner_dev = NULL; > + > + tuner_dev = kzalloc(sizeof(*tuner_dev), GFP_KERNEL); > + if (!tuner_dev) { > + ret = -ENOMEM; > + goto err; > + } > + > + i2c_set_clientdata(client, tuner_dev); > + tuner_dev->fe = config->fe; > + > + memcpy(&fe->ops.tuner_ops, > + &dvb_dummy_tuner_ops, > + sizeof(struct dvb_tuner_ops)); > + > + fe->tuner_priv = client; > + > + pr_debug("%s: Successfully probed %s\n", __func__, client->name); > + return ret; > + > +err: > + pr_err("%s: failed\n", __func__); > + return ret; > +} > + > +static int dvb_dummy_tuner_i2c_remove(struct i2c_client *client) > +{ > + struct dvb_dummy_tuner_dev *tuner_dev = i2c_get_clientdata(client); > + struct dvb_frontend *fe = tuner_dev->fe; > + > + memset(&fe->ops.tuner_ops, 0, sizeof(struct dvb_tuner_ops)); > + fe->tuner_priv = NULL; > + kfree(tuner_dev); > + > + return 0; > +} > + > +static struct i2c_driver dvb_dummy_tuner_i2c_driver = { > + .driver = { > + .name = "dvb_dummy_tuner", > + .suppress_bind_attrs = true, > + }, > + .probe = dvb_dummy_tuner_i2c_probe, > + .remove = dvb_dummy_tuner_i2c_remove, > + .id_table = dvb_dummy_tuner_i2c_id_table, > +}; > +module_i2c_driver(dvb_dummy_tuner_i2c_driver); > + > +MODULE_DESCRIPTION("DVB Dummy Tuner"); > +MODULE_AUTHOR("Daniel W. S. Almeida"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/media/tuners/dvb_dummy_tuner.h b/drivers/media/tuners/dvb_dummy_tuner.h > new file mode 100644 > index 000000000000..fde7628258fa > --- /dev/null > +++ b/drivers/media/tuners/dvb_dummy_tuner.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * DVB Dummy Tuner > + * Please add details about the driver similar to the .c file. > + */ > +#ifndef __TUNER_DVB_DUMMY_TUNER_H__ > +#define __TUNER_DVB_DUMMY_TUNER_H__ > + > +#include <media/dvb_frontend.h> > +/** > + * struct dvb_dummy_tuner_config - configuration parameters for the dummy tuner > + * > + * @fe: > + * frontend returned by driver > + */ > +struct dvb_dummy_tuner_config { > + struct dvb_frontend *fe; > +}; > + > +#endif /*__TUNER_DVB_DUMMY_TUNER_H__ */ > Looks like there is more code to be added to register/unregister thanks, -- Shuah
Hi Greg! Thanks for chiming in. > As you are a driver, you should never need to call any pr_* calls, > instead use dev_*(). For this, you can use dev_dbg(), but really, why > is that even needed except for your debugging bringup. And for that, > you can use ftrace, right? So no need for any printing of anything > here. > Again, dev_err() would be proper, but there's no need for any error > message here. Let's take these out in v2 then. > Don't you need to register the tuner ops with something in this > function? > Don't you need to unregister the tuner ops in here? It is my understanding that bridge drivers are the ones responsible for this. For instance, I don't see this with either xc4000.c, xc5000.c or mt2060.c. I could be wrong, though. Maybe Mauro could clarify this? What I did miss in this patch was an attach function. Let's also add this in v2. Thanks again, - Daniel.
Hi Mauro! > Actually, this is a dummy driver. It should not be auto-selected, as no > real hardware depends on it. We don't want production distros to come with > those test drivers. Let's address this in v2. Thanks.
Hi Shuah, thank you for you input! > > Looks like there is more code to be added to register/unregister Just to be clear, attach() is missing, but register / unregister belong in the bridge driver. Is this correct? > > Please add more detail here on what this driver does. What this > driver is used for and so on. > > > Samehere. Give more details on what this driver does. Add enough detail > to help users decide why they should enable it or not. > > General practice is the following. > > Copyright (c) 2020 Daniel W. S. Almeida <dwlsalmeida@gmail.com> > Please add details about the driver similar to the .c file. OK. Thanks - Daniel
On 1/9/20 2:00 PM, Daniel W. S. Almeida wrote: > Hi Shuah, thank you for you input! > > >> >> Looks like there is more code to be added to register/unregister > > Just to be clear, attach() is missing, but register / unregister belong > in the bridge driver. Is this correct? > > Right attach/detach is what I was thinking. >> >> Please add more detail here on what this driver does. What this >> driver is used for and so on. >> >> >> Samehere. Give more details on what this driver does. Add enough detail >> to help users decide why they should enable it or not. >> General practice is the following. >> >> Copyright (c) 2020 Daniel W. S. Almeida <dwlsalmeida@gmail.com> > >> Please add details about the driver similar to the .c file. > > OK. > thanks, -- Shuah
diff --git a/drivers/media/tuners/Kconfig b/drivers/media/tuners/Kconfig index e104bb7766e1..efa1591fe0ae 100644 --- a/drivers/media/tuners/Kconfig +++ b/drivers/media/tuners/Kconfig @@ -296,4 +296,11 @@ config MEDIA_TUNER_QM1D1B0004 default m if !MEDIA_SUBDRV_AUTOSELECT help Sharp QM1D1B0004 ISDB-S tuner driver. + +config MEDIA_TUNER_DVB_DUMMY_TUNER + tristate "Dummy tuner" + depends on MEDIA_SUPPORT && I2C + default m if !MEDIA_SUBDRV_AUTOSELECT + help + Dummy tuner driver for common TV standards. endmenu diff --git a/drivers/media/tuners/Makefile b/drivers/media/tuners/Makefile index 7b4f8423501e..f98de1cf2e19 100644 --- a/drivers/media/tuners/Makefile +++ b/drivers/media/tuners/Makefile @@ -44,5 +44,6 @@ obj-$(CONFIG_MEDIA_TUNER_QM1D1C0042) += qm1d1c0042.o obj-$(CONFIG_MEDIA_TUNER_QM1D1B0004) += qm1d1b0004.o obj-$(CONFIG_MEDIA_TUNER_M88RS6000T) += m88rs6000t.o obj-$(CONFIG_MEDIA_TUNER_TDA18250) += tda18250.o +obj-$(CONFIG_MEDIA_TUNER_DVB_DUMMY_TUNER) += dvb_dummy_tuner.o ccflags-y += -I$(srctree)/drivers/media/dvb-frontends diff --git a/drivers/media/tuners/dvb_dummy_tuner.c b/drivers/media/tuners/dvb_dummy_tuner.c new file mode 100644 index 000000000000..63d2e47d4739 --- /dev/null +++ b/drivers/media/tuners/dvb_dummy_tuner.c @@ -0,0 +1,153 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Driver for Dummy Frontend + * + * Written by Daniel W. S. Almeida <dwlsalmeida@gmail.com> + */ + +#include <linux/errno.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/types.h> +#include <media/dvb_frontend.h> +#include "dvb_dummy_tuner.h" + +struct dvb_dummy_tuner_dev { + struct dvb_frontend *fe; +}; + +static void dvb_dummy_tuner_release(struct dvb_frontend *fe) +{ +} + +static int dvb_dummy_tuner_init(struct dvb_frontend *fe) +{ + return 0; +} + +static int dvb_dummy_tuner_sleep(struct dvb_frontend *fe) +{ + return 0; +} + +static int dvb_dummy_tuner_suspend(struct dvb_frontend *fe) +{ + return 0; +} + +static int dvb_dummy_tuner_resume(struct dvb_frontend *fe) +{ + return 0; +} + +static int dvb_dummy_tuner_set_params(struct dvb_frontend *fe) +{ + return 0; +} + +static int dvb_dummy_tuner_set_config(struct dvb_frontend *fe, void *priv_cfg) +{ + return 0; +} + +static int dvb_dummy_tuner_get_frequency(struct dvb_frontend *fe, + u32 *frequency) +{ + return 0; +} + +static int dvb_dummy_tuner_get_bandwidth(struct dvb_frontend *fe, + u32 *bandwidth) +{ + return 0; +} + +static int dvb_dummy_tuner_get_if_frequency(struct dvb_frontend *fe, + u32 *frequency) +{ + return 0; +} + +static int dvb_dummy_tuner_get_status(struct dvb_frontend *fe, u32 *status) +{ + return 0; +} + +static const struct dvb_tuner_ops dvb_dummy_tuner_ops = { + .release = dvb_dummy_tuner_release, + .init = dvb_dummy_tuner_init, + .sleep = dvb_dummy_tuner_sleep, + .suspend = dvb_dummy_tuner_suspend, + .resume = dvb_dummy_tuner_resume, + .set_params = dvb_dummy_tuner_set_params, + .set_config = dvb_dummy_tuner_set_config, + .get_bandwidth = dvb_dummy_tuner_get_bandwidth, + .get_frequency = dvb_dummy_tuner_get_frequency, + .get_if_frequency = dvb_dummy_tuner_get_if_frequency, + .get_status = dvb_dummy_tuner_get_status, +}; + +static const struct i2c_device_id dvb_dummy_tuner_i2c_id_table[] = { + {"dvb_dummy_tuner", 0}, + {} +}; +MODULE_DEVICE_TABLE(i2c, dvb_dummy_tuner_i2c_id_table); + +static int dvb_dummy_tuner_i2c_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + int ret = 0; + struct dvb_dummy_tuner_config *config = client->dev.platform_data; + struct dvb_frontend *fe = config->fe; + struct dvb_dummy_tuner_dev *tuner_dev = NULL; + + tuner_dev = kzalloc(sizeof(*tuner_dev), GFP_KERNEL); + if (!tuner_dev) { + ret = -ENOMEM; + goto err; + } + + i2c_set_clientdata(client, tuner_dev); + tuner_dev->fe = config->fe; + + memcpy(&fe->ops.tuner_ops, + &dvb_dummy_tuner_ops, + sizeof(struct dvb_tuner_ops)); + + fe->tuner_priv = client; + + pr_debug("%s: Successfully probed %s\n", __func__, client->name); + return ret; + +err: + pr_err("%s: failed\n", __func__); + return ret; +} + +static int dvb_dummy_tuner_i2c_remove(struct i2c_client *client) +{ + struct dvb_dummy_tuner_dev *tuner_dev = i2c_get_clientdata(client); + struct dvb_frontend *fe = tuner_dev->fe; + + memset(&fe->ops.tuner_ops, 0, sizeof(struct dvb_tuner_ops)); + fe->tuner_priv = NULL; + kfree(tuner_dev); + + return 0; +} + +static struct i2c_driver dvb_dummy_tuner_i2c_driver = { + .driver = { + .name = "dvb_dummy_tuner", + .suppress_bind_attrs = true, + }, + .probe = dvb_dummy_tuner_i2c_probe, + .remove = dvb_dummy_tuner_i2c_remove, + .id_table = dvb_dummy_tuner_i2c_id_table, +}; +module_i2c_driver(dvb_dummy_tuner_i2c_driver); + +MODULE_DESCRIPTION("DVB Dummy Tuner"); +MODULE_AUTHOR("Daniel W. S. Almeida"); +MODULE_LICENSE("GPL"); diff --git a/drivers/media/tuners/dvb_dummy_tuner.h b/drivers/media/tuners/dvb_dummy_tuner.h new file mode 100644 index 000000000000..fde7628258fa --- /dev/null +++ b/drivers/media/tuners/dvb_dummy_tuner.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * DVB Dummy Tuner + * + */ +#ifndef __TUNER_DVB_DUMMY_TUNER_H__ +#define __TUNER_DVB_DUMMY_TUNER_H__ + +#include <media/dvb_frontend.h> +/** + * struct dvb_dummy_tuner_config - configuration parameters for the dummy tuner + * + * @fe: + * frontend returned by driver + */ +struct dvb_dummy_tuner_config { + struct dvb_frontend *fe; +}; + +#endif /*__TUNER_DVB_DUMMY_TUNER_H__ */