Re: [PATCH] Full DVB driver package for Earthsoft PT3 (ISDB-S/T) cards

[ほち <knightrider@are.ma>]

see inline

2013/11/6 Michael Krufky <mkrufky@linuxtv.org>:
> responding inline:
>
>> Mauro Carvalho Chehab asked to put tuner code as an I2C driver, under drivers/media/tuners, frontends at drivers/media/dvb-
>> However, to keep package integrity & compatibility with PT1/PT2 user apps, FE etc. are still placed in the same directory.
>
> A userspace application doesn't care  where file are places within the
> kernel tree.  You are to use standard linux-dvb api's and the
> codingstyle of the driver shall comply with that of the linux kernel's
> DVB subsystem, including the proper placement of files.

As stated in my (previous) mails, I took PT1 module as a reference.
Everything is located in single directory ...pt1/, including the frontends.
They are built as a single integrated module, earth-pt1.ko

Sure I can split the FEs out to .../dvb-frontends/, build there as a separated
(FE only) module that would be hot-linked with the main module. However
I'm afraid (& sure) this will only make people confused with complex
dependencies leading to annoying bugs... The simpler the better...

Guys I need more opinions from other people before splitting the module.
IMHO even Linus won't like this...

>> diff --git a/drivers/media/pci/pt3_dvb/Kconfig b/drivers/media/pci/pt3_dvb/Kconfig
>> new file mode 100644
>> index 0000000..f9ba00d
>> --- /dev/null
>> +++ b/drivers/media/pci/pt3_dvb/Kconfig
>> @@ -0,0 +1,12 @@
>> +config PT3_DVB
>> +       tristate "Earthsoft PT3 cards"
>> +       depends on DVB_CORE && PCI
>> +       help
>> +         Support for Earthsoft PT3 PCI-Express cards.
>> +
>> +         Since these cards have no MPEG decoder onboard, they transmit
>> +         only compressed MPEG data over the PCI bus, so you need
>> +         an external software decoder to watch TV on your computer.
>> +
>> +         Say Y or M if you own such a device and want to use it.
>
> Very few of these digital tuner boards have onboard mpeg decoders.  We
> can do without this extra information here.

ok, will change to:
These cards transmit only compressed MPEG data over the PCI bus.
You need external software decoder to watch TV on your computer.

>> diff --git a/drivers/media/pci/pt3_dvb/Makefile b/drivers/media/pci/pt3_dvb/Makefile
>> new file mode 100644
>> index 0000000..7087c90
>> --- /dev/null
>> +++ b/drivers/media/pci/pt3_dvb/Makefile
>> @@ -0,0 +1,6 @@
>> +pt3_dvb-objs := pt3.o pt3_fe.o pt3_dma.o pt3_tc.o pt3_i2c.o pt3_bus.o
>> +
>> +obj-$(CONFIG_PT3_DVB) += pt3_dvb.o
>> +
>> +ccflags-y += -Idrivers/media/dvb-core -Idrivers/media/dvb-frontends -Idrivers/media/tuners
>> +
>
> Ususally, the driver would be named 'pt3.ko' and the object file that
> handles the DVB api would be called pt3_dvb.o
>
> This isn't any rule, but the way that you've named them seems a bit
> awkward to me.  I don't require that you change this, just stating my
> awkward feeling on the matter.

FYI, there is another version of PT3 driver, named pt3_drv.ko, that
utilize character devices as the I/O. I'd rather use pt3_dvb.ko to
distinguish.

Maybe I'd like to change the dirname:
drivers/media/pci/pt3_dvb => drivers/media/pci/pt3

>> +static int lnb = 2;    /* used if not set by frontend / the value is invalid */
>> +module_param(lnb, int, 0);
>> +MODULE_PARM_DESC(lnb, "LNB level (0:OFF 1:+11V 2:+15V)");
>
> Take these above three statements out of the header file and move them
> into a .c file

OK Sir

>> +struct dvb_frontend *pt3_fe_s_attach(struct pt3_adapter *adap);
>> +struct dvb_frontend *pt3_fe_t_attach(struct pt3_adapter *adap);
>
> Please create separate headers corresponding to the .c file that
> contains the function.  Don't put them all in one, as the tuner and
> demodulator should be separate modules

Splitting the protos? Well I will consider...

> every source file and header file should include GPLv2 license headers.

Roger: not very crucial though...

>> +#define PT3_QM_INIT_DUMMY_RESET 0x0c
>
> it's nicer when these macros are defined in one place, but its not a
> requirement.  It's OK to leave it here if you really want to, but I
> suggest instead to create a _reg.h file containing all register
> #defines

Will consider...