linux-kernel - Re: [PATCH v3 05/14] [media] cxd2880: Add tuner part of the driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 7 Sep 2017 07:26:25 -0300
From:   Mauro Carvalho Chehab <mchehab@...pensource.com>
To:     "Takiguchi, Yasunari" <Yasunari.Takiguchi@...y.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "tbird20d@...il.com" <tbird20d@...il.com>,
        "frowand.list@...il.com" <frowand.list@...il.com>,
        "Yamamoto, Masayuki" <Masayuki.Yamamoto@...y.com>,
        "Nozawa, Hideki (STWN)" <Hideki.Nozawa@...y.com>,
        "Yonezawa, Kota" <Kota.Yonezawa@...y.com>,
        "Matsumoto, Toshihiko" <Toshihiko.Matsumoto@...y.com>,
        "Watanabe, Satoshi (SSS)" <Satoshi.C.Watanabe@...y.com>
Subject: Re: [PATCH v3 05/14] [media] cxd2880: Add tuner part of the driver

Em Thu, 7 Sep 2017 19:12:57 +0900
"Takiguchi, Yasunari" <Yasunari.Takiguchi@...y.com> escreveu:

> Dear Mauro
> 
> Thanks for your review and reply.
> 
> We are going to discuss how to change our code with your comments internally.
> 
> I reply for your  2 comments,
> 
> >> [Change list]
> >> Changes in V3
> >>    drivers/media/dvb-frontends/cxd2880/cxd2880_dtv.h
> >>       -removed code relevant to ISDB-T
> > 
> > Just curiosity here: why is it removed?
> We decided to withhold the ISDB-T functionality as it contains some company proprietary code.

I'm sorry to hear. I hope that such code could be released
on some future.

> >> +	if (ret)
> >> +		return ret;
> >> +	if ((sys == CXD2880_DTV_SYS_DVBT2) && en_fef_intmtnt_ctrl) {
> >> +		data[0] = 0x01;
> >> +		data[1] = 0x01;
> >> +		data[2] = 0x01;
> >> +		data[3] = 0x01;
> >> +		data[4] = 0x01;
> >> +		data[5] = 0x01;
> >> +	} else {
> >> +		data[0] = 0x00;
> >> +		data[1] = 0x00;
> >> +		data[2] = 0x00;
> >> +		data[3] = 0x00;
> >> +		data[4] = 0x00;
> >> +		data[5] = 0x00;
> >> +	}
> > 
> > Instead, just do:
> > 
> > 	if ((sys == CXD2880_DTV_SYS_DVBT2) && en_fef_intmtnt_ctrl)
> > 		memset(data, 0x01, sizeof(data));
> > 	else
> > 		memset(data, 0x00, sizeof(data));
> > 
> >> +	ret = tnr_dmd->io->write_regs(tnr_dmd->io,
> >> +				      CXD2880_IO_TGT_SYS,
> >> +				      0xef, data, 6);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = tnr_dmd->io->write_reg(tnr_dmd->io,
> >> +				     CXD2880_IO_TGT_DMD,
> >> +				     0x00, 0x2d);
> >> +	if (ret)
> >> +		return ret;
> > 
> >> +	if ((sys == CXD2880_DTV_SYS_DVBT2) && en_fef_intmtnt_ctrl)
> >> +		data[0] = 0x00;
> >> +	else
> >> +		data[0] = 0x01;
> > 
> > Not actually needed, as the previous logic already set data[0]
> > accordingly.
> > 
> >> +	ret = tnr_dmd->io->write_reg(tnr_dmd->io,
> >> +				     CXD2880_IO_TGT_DMD,
> >> +				     0xb1, data[0]);
> 
> In this case、logic of data[0]( logic of if() ) is different from that of previous one.
> And with setting register for address 0xb1, a bug might occur in the future, 
> if our software specification (sequence) is changed.
> So we would like to keep setting value of data[0] for address 0xb1.

OK. Better to document it then, as otherwise someone might
end by sending cleanup patches that would touch it.

> 
> Thanks,
> Takiguchi



Thanks,
Mauro