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

[Yasunari.Takiguchi]

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

[Yasunari.Takiguchi]

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
> <yasunari.takigu...@sony.com> escreveu:
> 
> > From: Yasunari Takiguchi <yasunari.takigu...@sony.com>
> >
> > 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 <yasunari.takigu...@sony.com>
> > Signed-off-by: Masayuki Yamamoto <masayuki.yamam...@sony.com>
> > Signed-off-by: Hideki Nozawa <hideki.noz...@sony.com>
> > Signed-off-by: Kota Yonezawa <kota.yonez...@sony.com>
> > Signed-off-by: Toshihiko Matsumoto <toshihiko.matsum...@sony.com>
> > Signed-off-by: Satoshi Watanabe <satoshi.c.watan...@sony.com>
> > ---
> >
> > [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 

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

[Mauro Carvalho Chehab]

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(, 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();
> + spi_message_add_tail(, );
> +
> + return spi_sync(spi, );
> +}
> +
> +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