Re: [PATCH v2 0/15] [dt-bindings] [media] Add document file and driver for Sony CXD2880 DVB-T2/T tuner + demodulator

[Takiguchi, Yasunari]

Dear Mauro

Thanks for your kind review during your busy schedule.

I will confirm with your findings and discuss about them
(how to change our codes) in our team.

Regards & Thanks
Takiguchi

On 2017/06/13 23:38, Mauro Carvalho Chehab wrote:
> Hi Takiguchi-san,
> 
> Em Thu, 25 May 2017 15:15:39 +0900
> "Takiguchi, Yasunari"  escreveu:
> 
>> Hi, all
>>
>> I sent the patch series of Sony CXD2880 DVB-T2/T tuner + demodulator driver 
>> on Apr/14.
>> Are there any comments, advices and review results for them?
> 
> Usually, reviewing drivers takes more time, as one needs to reserve a
> long period of time for reviewing. Sorry for the delay.
> 
>> I'd like to get better understanding of current review status for our codes.
> 
> Just sent today a review. There are some things that need to be
> changed in order to get it into a better shape and make it easier
> to review. In particular, it should be using some Kernel internal APIs,
> instead of coming with re-implementation of existing core code. That's fine. 
> It is very unusual that the first contributions from a new Kernel
> developer to gets everything the way as it is expected mainstream ;-)
> 
> One thing that come into my mind, besides what was already commented,
> is that it seems you added an abstraction layer. We don't like such
> layers very much, as it makes harder to understand the driver, usually
> for very little benefit.
> 
> On this first review, I didn't actually try to understand what's
> going on there. As the driver doesn't contain any comments inside,
> it makes harder to understand why some things were coded using
> such approach.
> 
> Thanks,
> Mauro

Re: [PATCH v2 0/15] [dt-bindings] [media] Add document file and driver for Sony CXD2880 DVB-T2/T tuner + demodulator

[Mauro Carvalho Chehab]

Hi Takiguchi-san,

Em Thu, 25 May 2017 15:15:39 +0900
"Takiguchi, Yasunari"  escreveu:

> Hi, all
> 
> I sent the patch series of Sony CXD2880 DVB-T2/T tuner + demodulator driver 
> on Apr/14.
> Are there any comments, advices and review results for them?

Usually, reviewing drivers takes more time, as one needs to reserve a
long period of time for reviewing. Sorry for the delay.

> I'd like to get better understanding of current review status for our codes.

Just sent today a review. There are some things that need to be
changed in order to get it into a better shape and make it easier
to review. In particular, it should be using some Kernel internal APIs,
instead of coming with re-implementation of existing core code. That's fine. 
It is very unusual that the first contributions from a new Kernel
developer to gets everything the way as it is expected mainstream ;-)

One thing that come into my mind, besides what was already commented,
is that it seems you added an abstraction layer. We don't like such
layers very much, as it makes harder to understand the driver, usually
for very little benefit.

On this first review, I didn't actually try to understand what's
going on there. As the driver doesn't contain any comments inside,
it makes harder to understand why some things were coded using
such approach.

Thanks,
Mauro

Re: [PATCH v2 0/15] [dt-bindings] [media] Add document file and driver for Sony CXD2880 DVB-T2/T tuner + demodulator

[Abylay Ospan]

Dear Takiguchi,

Roger that.
Thanks for explanation !

2017-06-13 1:35 GMT-04:00 Takiguchi, Yasunari :
> Dear Abylay Ospan
>
> Thank you for your review and proposal.
>
> Unfortunately, we supposed it's difficult to cover CXD2841 functionality by 
> CXD2880 driver.
> CXD2880 is for mobile IC, tuner (RF) and demodulator convined IC.
> On the other hand, CXD2841 is demodulator only IC for stationary use.
> CXD2841 supports terrestrial, cable and satellite broadcasting systems.
> But CXD2880 supports terrestrial broadcasting systems only.
> And as you suggested, the driver supports SPI interface only.
>
> Regards & Thanks a lot
> Takiguchi
>
>
> On 2017/06/12 22:33, Abylay Ospan wrote:
>> Dear Takiguchi,
>>
>> I'm working on 'drivers/media/dvb-frontends/cxd2841er.c' (universal
>> demod) and 'linux/drivers/media/dvb-frontends/helene.c' (universal
>> TERR/CABLE/SAT tuner).
>>
>> How do you think - is your cxd2880 proposed driver have possibilities
>> to cover cxd2841er demod functionality too ?
>>
>> I have quickly checked your cxd2880_top.c and looks like cxd2880 works
>> over SPI (not I2C) ?Also, looks like registers map, sequences
>> is different. Am I right ?
>>
>> How do you think ?
>>
>> Thanks a lot !
>



-- 
Abylay Ospan,
NetUP Inc.
http://www.netup.tv

Re: [PATCH v2 0/15] [dt-bindings] [media] Add document file and driver for Sony CXD2880 DVB-T2/T tuner + demodulator

[Takiguchi, Yasunari]

Dear Abylay Ospan

Thank you for your review and proposal.

Unfortunately, we supposed it's difficult to cover CXD2841 functionality by 
CXD2880 driver.
CXD2880 is for mobile IC, tuner (RF) and demodulator convined IC.
On the other hand, CXD2841 is demodulator only IC for stationary use.
CXD2841 supports terrestrial, cable and satellite broadcasting systems.
But CXD2880 supports terrestrial broadcasting systems only.
And as you suggested, the driver supports SPI interface only.

Regards & Thanks a lot
Takiguchi


On 2017/06/12 22:33, Abylay Ospan wrote:
> Dear Takiguchi,
> 
> I'm working on 'drivers/media/dvb-frontends/cxd2841er.c' (universal
> demod) and 'linux/drivers/media/dvb-frontends/helene.c' (universal
> TERR/CABLE/SAT tuner).
> 
> How do you think - is your cxd2880 proposed driver have possibilities
> to cover cxd2841er demod functionality too ?
> 
> I have quickly checked your cxd2880_top.c and looks like cxd2880 works
> over SPI (not I2C) ?Also, looks like registers map, sequences
> is different. Am I right ?
> 
> How do you think ?
> 
> Thanks a lot !

Re: [PATCH v2 0/15] [dt-bindings] [media] Add document file and driver for Sony CXD2880 DVB-T2/T tuner + demodulator

[Abylay Ospan]

Dear Takiguchi,

I'm working on 'drivers/media/dvb-frontends/cxd2841er.c' (universal
demod) and 'linux/drivers/media/dvb-frontends/helene.c' (universal
TERR/CABLE/SAT tuner).

How do you think - is your cxd2880 proposed driver have possibilities
to cover cxd2841er demod functionality too ?

I have quickly checked your cxd2880_top.c and looks like cxd2880 works
over SPI (not I2C) ?Also, looks like registers map, sequences
is different. Am I right ?

How do you think ?

Thanks a lot !

2017-05-31 21:41 GMT-04:00 Takiguchi, Yasunari :
> Hi, all
>
> I sent the patch series of Sony CXD2880 DVB-T2/T tuner + demodulator driver 
> on Apr/14.
> Are there any comments, advices and review results for them?
>
> I'd like to get better understanding of current review status for our codes.
>
> Regards,
> Takiguchi



-- 
Abylay Ospan,
NetUP Inc.
http://www.netup.tv

Re: [PATCH v2 0/15] [dt-bindings] [media] Add document file and driver for Sony CXD2880 DVB-T2/T tuner + demodulator

[Takiguchi, Yasunari]

Hi, all

I sent the patch series of Sony CXD2880 DVB-T2/T tuner + demodulator driver on 
Apr/14.
Are there any comments, advices and review results for them?

I'd like to get better understanding of current review status for our codes.

Regards,
Takiguchi

Re: [PATCH v2 0/15] [dt-bindings] [media] Add document file and driver for Sony CXD2880 DVB-T2/T tuner + demodulator

[Takiguchi, Yasunari]

Hi, all

I sent the patch series of Sony CXD2880 DVB-T2/T tuner + demodulator driver on 
Apr/14.
Are there any comments, advices and review results for them?

I'd like to get better understanding of current review status for our codes.

Regards,
Takiguchi

Re: [PATCH v2 0/15] [dt-bindings] [media] Add document file and driver for Sony CXD2880 DVB-T2/T tuner + demodulator

[Takiguchi, Yasunari]

On 2017/04/14 10:50, Takiguchi, Yasunari wrote:
> From: Yasunari Takiguchi 
> 
> Hi,
> 
> This is the patch series (version 2) of Sony CXD2880 DVB-T2/T tuner + 
> demodulator driver.
> The driver supports DVB-API and interfaces through SPI.
> 
> We have tested the driver on Raspberry Pi 3 and got picture and sound from a 
> media player.
> 
> Thanks,
> Takiguchi
> ---
>  Documentation/devicetree/bindings/media/spi/sony-cxd2880.txt|   14 
> ++
>  drivers/media/spi/cxd2880-spi.c | 728 
> 
>  drivers/media/dvb-frontends/cxd2880/cxd2880.h   |   46 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_common.c|   84 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_common.h|   86 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_io.c|   68 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_io.h|   62 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_stdlib.h|   35 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_stopwatch_port.c|   71 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_math.c  |   89 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_math.h  |   40 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_devio_spi.c |  147 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_devio_spi.h |   40 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_spi.h   |   51 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_spi_device.c|  130 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_spi_device.h|   45 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_dtv.h   |   50 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd.c| 3925 
> 
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd.h|  395 ++
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_driver_version.h |   29 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_mon.c|  207 ++
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_mon.h|   52 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_integ.c |   99 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_integ.h |   44 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_top.c   | 1550 
> 
>  drivers/media/dvb-frontends/cxd2880/cxd2880_dvbt.h  |   91 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt.c   | 1072 
> +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt.h   |   62 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_integ_dvbt.c|  197 ++
>  drivers/media/dvb-frontends/cxd2880/cxd2880_integ_dvbt.h|   58 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt_mon.c   | 1190 
> +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt_mon.h   |  106 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_dvbt2.h |  402 
> 
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt2.c  | 1309 
> ++
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt2.h  |   82 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_integ_dvbt2.c   |  311 
> +++
>  drivers/media/dvb-frontends/cxd2880/cxd2880_integ_dvbt2.h   |   64 +
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt2_mon.c  | 2523 
> 
>  drivers/media/dvb-frontends/cxd2880/cxd2880_tnrdmd_dvbt2_mon.h  |  170 ++
>  drivers/media/dvb-frontends/Makefile|1 +
>  drivers/media/dvb-frontends/cxd2880/Makefile|   21 
> +
>  drivers/media/spi/Makefile  |5 
> +
>  drivers/media/dvb-frontends/Kconfig |2 ++
>  drivers/media/dvb-frontends/cxd2880/Kconfig |6 
> ++
>  drivers/media/spi/Kconfig   |   14 
> ++
>  MAINTAINERS |9 
> +
> 
>  46 files changed, 15782 insertions(+)
> 
>  create mode 100644 
> Documentation/devicetree/bindings/media/spi/sony-cxd2880.txt
>  create mode 100644 drivers/media/spi/cxd2880-spi.c
>  create mode 100644 drivers/media/dvb-frontends/cxd2880/cxd2880.h
>  create mode 100644 drivers/media/dvb-frontends/cxd2880/cxd2880_common.c
>  create mode 100644 drivers/media/dvb-frontends/cxd2880/cxd2880_common.h
>  create mode 100644 drivers/media/dvb-frontends/cxd2880/cxd2880_io.c
>  create mode 100644 drivers/media/dvb-frontends/cxd2880/cxd2880_io.h
>  create mode 100644 drivers/media/dvb-frontends/cxd2880/cxd2880_stdlib.h
>  create mode 100644 
>