RE: [PATCH v4 02/12] [media] cxd2880-spi: Add support for CXD2880 SPI interface

[Takiguchi, Yasunari]

Hi, Mauro.

> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__
> 
> It would be better to use dev_foo() debug macros instead of
> pr_foo() ones.

I got comment for this previous version patch as below
--------------------------------------------------------------------------------------
The best would be to se dev_err() & friends for printing messages, 
as they print the device's name as filled at struct device.
If you don't use, please add a define that will print the name at the logs, like:

  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

either at the begining of the driver or at some header file.

Btw, I'm noticing that you're also using dev_err() on other places of the code. 
Please standardize. OK, on a few places, you may still need to use pr_err(), 
if you need to print a message before initializing struct device, but I suspect that you can init
--------------------------------------------------------------------------------------

You pointed out here before. Because dev_foo () and pr_foo () were mixed.
We standardize with pr_foo() because the logs is outputted before getting the device structure.
Is it better to use dev_foo() where we can use it?


> > +static int cxd2880_stop_feed(struct dvb_demux_feed *feed) {
> > +	int i = 0;
> > +	int ret;
> > +	struct dvb_demux *demux = NULL;
> > +	struct cxd2880_dvb_spi *dvb_spi = NULL;
> > +
> > +	if (!feed) {
> > +		pr_err("invalid arg\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	demux = feed->demux;
> > +	if (!demux) {
> > +		pr_err("feed->demux is NULL\n");
> > +		return -EINVAL;
> > +	}
> > +	dvb_spi = demux->priv;
> > +
> > +	if (!dvb_spi->feed_count) {
> > +		pr_err("no feed is started\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (feed->pid == 0x2000) {
> > +		/*
> > +		 * Special PID case.
> > +		 * Number of 0x2000 feed request was stored
> > +		 * in dvb_spi->all_pid_feed_count.
> > +		 */
> > +		if (dvb_spi->all_pid_feed_count <= 0) {
> > +			pr_err("PID %d not found.\n", feed->pid);
> > +			return -EINVAL;
> > +		}
> > +		dvb_spi->all_pid_feed_count--;
> > +	} else {
> > +		struct cxd2880_pid_filter_config cfgtmp;
> > +
> > +		cfgtmp = dvb_spi->filter_config;
> > +
> > +		for (i = 0; i < CXD2880_MAX_FILTER_SIZE; i++) {
> > +			if (feed->pid == cfgtmp.pid_config[i].pid &&
> > +			    cfgtmp.pid_config[i].is_enable != 0) {
> > +				cfgtmp.pid_config[i].is_enable = 0;
> > +				cfgtmp.pid_config[i].pid = 0;
> > +				pr_debug("removed PID %d from #%d\n",
> > +					 feed->pid, i);
> > +				break;
> > +			}
> > +		}
> > +		dvb_spi->filter_config = cfgtmp;
> > +
> > +		if (i == CXD2880_MAX_FILTER_SIZE) {
> > +			pr_err("PID %d not found\n", feed->pid);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	ret = cxd2880_update_pid_filter(dvb_spi,
> > +					&dvb_spi->filter_config,
> > +					dvb_spi->all_pid_feed_count >
> 0);
> > +	dvb_spi->feed_count--;
> > +
> > +	if (dvb_spi->feed_count == 0) {
> > +		int ret_stop = 0;
> > +
> > +		ret_stop =
> kthread_stop(dvb_spi->cxd2880_ts_read_thread);
> > +		if (ret_stop) {
> > +			pr_err("'kthread_stop failed. (%d)\n",
> ret_stop);
> > +			ret = ret_stop;
> > +		}
> > +		kfree(dvb_spi->ts_buf);
> > +		dvb_spi->ts_buf = NULL;
> > +	}
> > +
> > +	pr_debug("stop feed ok.(count %d)\n", dvb_spi->feed_count);
> > +
> > +	return ret;
> > +}
> 
> I have the feeling that I've seen the code above before at the dvb core.
> Any reason why don't use the already-existing code at dvb_demux.c &
> friends?

The CXD2880 HW PID filter is used to decrease the amount of TS data transferred via limited bit rate SPI interface.

Thanks,
Takiguchi