RE: [PATCH v5 02/12] [media] cxd2880-spi: Add support for CXD2880 SPI interface
Hi, Mauro > > + u8 send_data[BURST_WRITE_MAX + 4]; > > + const u8 *write_data_top = NULL; > > + int ret = 0; > > + > > + if (!spi || !data) { > > + pr_err("invalid arg\n"); > > + return -EINVAL; > > + } > > + if (size > BURST_WRITE_MAX) { > > + pr_err("data size > WRITE_MAX\n"); > > + return -EINVAL; > > + } > > + > > + if (sub_address + size > 0x100) { > > + pr_err("out of range\n"); > > + return -EINVAL; > > + } > > It is better to use dev_err(spi->dev, ...) instead of pr_err(). 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? Takiguchi
RE: [PATCH v5 02/12] [media] cxd2880-spi: Add support for CXD2880 SPI interface
Dear Mauro I am very glad to hear your message. Being busy, thank you for taking care of fixing patches as well also. And we will improve about your below comments continuously. Regards & Thanks Takiguchi > -Original Message- > From: Mauro Carvalho Chehab [mailto:mche...@s-opensource.com] > Sent: Wednesday, March 7, 2018 7:15 PM > To: Takiguchi, Yasunari (SSS) > Cc: linux-ker...@vger.kernel.org; devicet...@vger.kernel.org; > linux-media@vger.kernel.org; tbird...@gmail.com; > frowand.l...@gmail.com; Yamamoto, Masayuki (SSS); Nozawa, Hideki (STWN); > Yonezawa, Kota (SSS); Matsumoto, Toshihiko (SSS); Watanabe, Satoshi (SSS) > Subject: Re: [PATCH v5 02/12] [media] cxd2880-spi: Add support for CXD2880 > SPI interface > > Em Thu, 18 Jan 2018 17:46:10 +0900 > escreveu: > > > From: Yasunari Takiguchi > > > > This is the SPI adapter part of the driver for the Sony CXD2880 > > DVB-T2/T tuner + demodulator. > > Thanks for the patches! > > The patch series look ok. Just a few nitpicks that could be solved later. > > I had to apply a few patches to make it build and remove some warnings > with W=1. Patches sent. > > With that, I'm applying this series. > > Regards, > Mauro > > > > > Signed-off-by: Yasunari Takiguchi > > Signed-off-by: Masayuki Yamamoto > > Signed-off-by: Hideki Nozawa > > Signed-off-by: Kota Yonezawa > > Signed-off-by: Toshihiko Matsumoto > > Signed-off-by: Satoshi Watanabe > > --- > > > > [Change list] > > Changes in V5 > >Using SPDX-License-Identifier > >drivers/media/spi/cxd2880-spi.c > > -modified typo about "ivnalid" -> "invalid" > > -modified typo about "drvier" -> "driver" > > -removed unnecessary if() > > -modified return error code > > -reduction of valiable names > > -removed unnecessary parentheses > > -changed members of struct cxd2880_ts_buf_info > > > > Changes in V4 > >drivers/media/spi/cxd2880-spi.c > > -removed Camel case > > -removed unnecessary initialization at variable declaration > > -removed unnecessary brace {} > > > > Changes in V3 > >drivers/media/spi/cxd2880-spi.c > > -adjusted of indent spaces > > -removed unnecessary cast > > -changed debugging code > > -changed timeout method > > -modified coding style of if() > > -changed hexadecimal code to lower case. > > > > Changes in V2 > >drivers/media/spi/cxd2880-spi.c > > -Modified PID filter setting. > > > > drivers/media/spi/cxd2880-spi.c | 670 > > > > 1 file changed, 670 insertions(+) > > create mode 100644 drivers/media/spi/cxd2880-spi.c > > > > diff --git a/drivers/media/spi/cxd2880-spi.c > > b/drivers/media/spi/cxd2880-spi.c new file mode 100644 index > > ..857e4c0d7a92 > > --- /dev/null > > +++ b/drivers/media/spi/cxd2880-spi.c > > @@ -0,0 +1,670 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * cxd2880-spi.c > > + * Sony CXD2880 DVB-T2/T tuner + demodulator driver > > + * SPI adapter > > + * > > + * Copyright (C) 2016, 2017, 2018 Sony Semiconductor Solutions > > +Corporation */ > > + > > +#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__ > > + > > +#include > > +#include > > + > > +#include "dvb_demux.h" > > +#include "dmxdev.h" > > +#include "dvb_frontend.h" > > +#include "cxd2880.h" > > + > > +#define CXD2880_MAX_FILTER_SIZE 32 > > +#define BURST_WRITE_MAX 128 > > +#define MAX_TRANS_PKT 300 > > + > > +struct cxd2880_ts_buf_info { > > + u8 read_ready:1; > > + u8 almost_full:1; > > + u8 almost_empty:1; > > + u8 overflow:1; > > + u8 underflow:1; > > + u16 pkt_num; > > +}; > > + > > +struct cxd2880_pid_config { > > + u8 is_enable; > > + u16 pid; > > +}; > > + > > +struct cxd2880_pid_filter_config { > > + u8 is_negative; > > + struct cxd2880_pid_config pid_config[CXD2880_MAX_FILTER_SIZE]; > > +}; > > + > > +struct cxd2880_dvb_spi { > > + struct dvb_frontend dvb_fe; > > + struct dvb_adapter adapter; > > + struct dvb_demux demux; > > + struct dmxdev dmxdev; > > + struct dmx_frontend dmx_fe; > > + struct task_struct *cxd2880_ts_read_thread; > > + struct spi_dev
Re: [PATCH v5 02/12] [media] cxd2880-spi: Add support for CXD2880 SPI interface
Em Thu, 18 Jan 2018 17:46:10 +0900 escreveu: > From: Yasunari Takiguchi > > This is the SPI adapter part of the driver for the > Sony CXD2880 DVB-T2/T tuner + demodulator. Thanks for the patches! The patch series look ok. Just a few nitpicks that could be solved later. I had to apply a few patches to make it build and remove some warnings with W=1. Patches sent. With that, I'm applying this series. Regards, Mauro > > Signed-off-by: Yasunari Takiguchi > Signed-off-by: Masayuki Yamamoto > Signed-off-by: Hideki Nozawa > Signed-off-by: Kota Yonezawa > Signed-off-by: Toshihiko Matsumoto > Signed-off-by: Satoshi Watanabe > --- > > [Change list] > Changes in V5 >Using SPDX-License-Identifier >drivers/media/spi/cxd2880-spi.c > -modified typo about "ivnalid" -> "invalid" > -modified typo about "drvier" -> "driver" > -removed unnecessary if() > -modified return error code > -reduction of valiable names > -removed unnecessary parentheses > -changed members of struct cxd2880_ts_buf_info > > Changes in V4 >drivers/media/spi/cxd2880-spi.c > -removed Camel case > -removed unnecessary initialization at variable declaration > -removed unnecessary brace {} > > Changes in V3 >drivers/media/spi/cxd2880-spi.c > -adjusted of indent spaces > -removed unnecessary cast > -changed debugging code > -changed timeout method > -modified coding style of if() > -changed hexadecimal code to lower case. > > Changes in V2 >drivers/media/spi/cxd2880-spi.c > -Modified PID filter setting. > > drivers/media/spi/cxd2880-spi.c | 670 > > 1 file changed, 670 insertions(+) > create mode 100644 drivers/media/spi/cxd2880-spi.c > > diff --git a/drivers/media/spi/cxd2880-spi.c b/drivers/media/spi/cxd2880-spi.c > new file mode 100644 > index ..857e4c0d7a92 > --- /dev/null > +++ b/drivers/media/spi/cxd2880-spi.c > @@ -0,0 +1,670 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * cxd2880-spi.c > + * Sony CXD2880 DVB-T2/T tuner + demodulator driver > + * SPI adapter > + * > + * Copyright (C) 2016, 2017, 2018 Sony Semiconductor Solutions Corporation > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__ > + > +#include > +#include > + > +#include "dvb_demux.h" > +#include "dmxdev.h" > +#include "dvb_frontend.h" > +#include "cxd2880.h" > + > +#define CXD2880_MAX_FILTER_SIZE 32 > +#define BURST_WRITE_MAX 128 > +#define MAX_TRANS_PKT 300 > + > +struct cxd2880_ts_buf_info { > + u8 read_ready:1; > + u8 almost_full:1; > + u8 almost_empty:1; > + u8 overflow:1; > + u8 underflow:1; > + u16 pkt_num; > +}; > + > +struct cxd2880_pid_config { > + u8 is_enable; > + u16 pid; > +}; > + > +struct cxd2880_pid_filter_config { > + u8 is_negative; > + struct cxd2880_pid_config pid_config[CXD2880_MAX_FILTER_SIZE]; > +}; > + > +struct cxd2880_dvb_spi { > + struct dvb_frontend dvb_fe; > + struct dvb_adapter adapter; > + struct dvb_demux demux; > + struct dmxdev dmxdev; > + struct dmx_frontend dmx_fe; > + struct task_struct *cxd2880_ts_read_thread; > + struct spi_device *spi; > + struct mutex spi_mutex; /* For SPI access exclusive control */ > + int feed_count; > + int all_pid_feed_count; > + u8 *ts_buf; > + struct cxd2880_pid_filter_config filter_config; > +}; > + > +DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr); > + > +static int cxd2880_write_spi(struct spi_device *spi, u8 *data, u32 size) > +{ > + struct spi_message msg; > + struct spi_transfer tx; > + > + if (!spi || !data) { > + pr_err("invalid arg\n"); > + return -EINVAL; > + } > + > + memset(&tx, 0, sizeof(tx)); Nitpick: instead, you could just declare tx as: struct spi_transfer tx = {}; and get rid of memset (same applies to similar code blocks). > + tx.tx_buf = data; > + tx.len = size; > + > + spi_message_init(&msg); > + spi_message_add_tail(&tx, &msg); > + > + return spi_sync(spi, &msg); > +} > + > +static int cxd2880_write_reg(struct spi_device *spi, > + u8 sub_address, const u8 *data, u32 size) > +{ > + u8 send_data[BURST_WRITE_MAX + 4]; > + const u8 *write_data_top = NULL; > + int ret = 0; > + > + if (!spi || !data) { > + pr_err("invalid arg\n"); > + return -EINVAL; > + } > + if (size > BURST_WRITE_MAX) { > + pr_err("data size > WRITE_MAX\n"); > + return -EINVAL; > + } > + > + if (sub_address + size > 0x100) { > + pr_err("out of range\n"); > + return -EINVAL; > + } It is better to use dev_err(spi->dev, ...) instead of pr_err(). > + > + send_data[0] = 0x0e; > + write_data_top = data; > + > + while (size > 0) { > + send_data[1] = sub_address; > + if (size