Hi,
Mauro Carvalho Chehab wrote:
> Mauro Carvalho Chehab wrote:
> > Oliver Endriss wrote:
> > As usual, after receiving your fixes for the above, I'll review the driver
> > manually.
> >
>
> OK, I'm enclosing here the diff file to allow comments.
Hard to read. I hope I did not miss an important comment.
> I found just a few issues, please see bellow. Yet, I decided to keep the
> entire diff, as more people may want to review and comment.Ok, it should be fixed.
>
>...
> > +#define COMMAND_TIMEOUT_WORKAROUND
>
> Why do you need the above define? There are several places at the code where
> this is used. Maybe it should also be a module parameter, like the change done
> with one_adapter.
No, it is what it tells: a workaround.
If you undef COMMAND_TIMEOUT_WORKAROUND, command timeout errors will
happen after some time. The workaround will be removed when the problem
has been identified and fixed.
> > ...
> > +#ifdef I2C_ADAP_CLASS_TV_DIGITAL
> > + adap->class = I2C_ADAP_CLASS_TV_DIGITAL | I2C_CLASS_TV_ANALOG;
> > +#else
> > + adap->class = I2C_CLASS_TV_ANALOG;
> > +#endif
>
> This got renamed to I2C_CLASS_TV_DIGITAL.
ACK, should be fixed.
Apparently some people spend their time renaming constants. :-)
> ...
> > + SCListEntry += 1;
>
> Instead:
> SCListEntry++;
>
> ...
>
> > + PASCListEntry += sizeof(struct HW_SCATTER_GATHER_ELEMENT);
> > +
> > +#if NUM_SCATTER_GATHER_ENTRIES > 1
> > + for (j = 0; j < NUM_SCATTER_GATHER_ENTRIES - 1; j += 1) {
>
> j++, instead of j +=1
>
> ...
>
> > + SCListEntry->Address = of;
> > + SCListEntry->Length = OVERFLOW_BUFFER_SIZE;
> > + SCListEntry += 1;
> > + PASCListEntry +=
> > + sizeof(struct HW_SCATTER_GATHER_ELEMENT);
> > + }
> > +#endif
>
> The number of Scatter/Gather entries is 8. Even if you use
>
> #define NUM_SCATTER_GATHER_ENTRIES 1, the above code won't cause any harm (except maybe for
> a compiler warning - that seems healthy in this case).
>
> So, I think you can just remove the #if test.
>
>
> While here: the better is to write the loop increment as j++, instead of j+= 1:
> for (j = 0; j < NUM_SCATTER_GATHER_ENTRIES - 1; j++) {
>
> The same kind of bad coding style also happens on other parts of the code.
> I'll avoid being redundant on this.
>
> ...
>
> > +/****************************************************************************/
> > +/* PCI Subsystem ID *********************************************************/
> > +/****************************************************************************/
> > +
> > +#define NGENE_ID(_subvend, _subdev, _driverdata) { \
> > + .vendor = NGENE_VID, .device = NGENE_PID, \
> > + .subvendor = _subvend, .subdevice = _subdev, \
> > + .driver_data = (unsigned long) &_driverdata }
> > +
> > +/****************************************************************************/
> > +
> > +static const struct pci_device_id ngene_id_tbl[] __devinitdata = {
> > + NGENE_ID(0x18c3, 0xabc3, ngene_info_cineS2),
> > + NGENE_ID(0x18c3, 0xabc4, ngene_info_cineS2),
> > + NGENE_ID(0x18c3, 0xdb01, ngene_info_satixs2),
> > + {0}
>
> just {} is enough.
Mauro, I did not write the code, and I think a developer has the right
to choose the implementation he likes. It is hard enough that we have to
respect, what checkpatch.pl forces us to do.
Matthias spent a considerable amount of time to reformat the code for
that questionable kernel coding style. But now it is enough, let's
focus on bugs.
Sorry, I will not change any of these non-issues, and I will not sign a
changeset which replaces 'x += 1' by 'x++' (or something like that).
If you think it is essential, prepare a changeset by yourself.
CU
Oliver
--
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/
Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/
----------------------------------------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
