Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion

["David Härdeman" <david@hardeman.nu>]

On Thu, 11 Nov 2010 11:54:30 -0200, Mauro Carvalho Chehab
<mchehab@infradead.org> wrote:
> The bad news is that ir-kbd-i2c also needs the stuff that are inside
> ir.props (e. g., the IR configuration logic). I wrote and just sent 2
> patches to the ML with the fix patches, against my media-tree.git,
> branch staging/for_v2.6.38. For now, only one field
> of props is used, but other fields there are likely needed for the other
> places where this driver is used, like the open/close callbacks,
> allowed_protocols, etc.
> 
> I don't like the idea of just copying all those config stuff into struct
> IR_i2c, and then at struct rc_dev,

Which is the way it is with or without my patch, either a struct
ir_dev_props or a subset of the former members of that struct are in
IR_i2c, same data. And right now you would need to have a mix between
ir_dev_props data and additional ir_dev related data in struct IR_i2c (the
rc map name is outside of ir_dev_props).

Note that the patch *removes* > 200 lines of code (without any loss of
functionality) - and that's because the interface is simplified. Simple is
good.

> and then at struct input_dev.

struct input_dev only gets input_name, input_phys and input_id from struct
rc_dev, and I did it that way because I didn't want to remove all that
information from all drivers (and fill input_dev with generic information
instead). We'll have to readdress that issue once
multi-input-devs-per-rc-dev functionality is implemented.

> It is too much data duplication for no good reason.

And you believe it's an important feature that props used to be a struct
and that you could pass a pointer (and take care of initializing rc_map)
instead of initializing a couple of members in rc_dev directly?

The use of struct ir_dev_props was a truely bizarre interface. For
example: setting the props member was optional, and a ir_input_dev struct
without a props member lacks a driver_type submember. So all codepaths need
to know what the default driver_type is when props is not set. Not to
mention the number of oops reports that linux-media has already seen,
caused by code which didn't check ->props before dereferencing it. That's
of course a bug in code, but it's a bug caused by a non-intuitive
interface.

The new struct is much more straightforward, and your worries about
additional pain caused by not having a struct ir_dev_props did not
materialize in any of the changes I did (see for example
drivers/media/dvb/dvb-usb/dib0700_devices.c which had similar requirements
to struct IR_i2c).

> So, I think we should re-think about your patch 6/7.

What's your suggestion?

> Comments?

Unsurprisingly, I disagree on the whole re-think part :)

-- 
David Härdeman