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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 11, 2010 at 3:35 PM, David Härdeman <david@xxxxxxxxxxx> wrote:
> On Thu, Nov 11, 2010 at 02:00:38PM -0200, Mauro Carvalho Chehab wrote:
...
>>> 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).
>>
>>input_dev fields need to be properly filled, but just duplicating the same
>>info on both structs and copying from one to the other doesn't seem the better
>>way. Why not just initialize rc_dev->input_dev fields directly, not duplicating
>>the data (or having a helper in-lined routine) to initialize those fields?
>
> The data is not duplicated, input_name and input_phys are passed around
> as pointers.
>
> And the reason it looks the way it does is, again, that
> multiple-input-devices-per-rc-device will be broken if drivers fiddle
> with the input_dev directly. Not to mention it's a layering violation to
> expect rc drivers to also know about the underlying input devices.
>
> (Yes, you could say that input_name, input_phys and input_id are
> layering violations as well, but they are not an equally problematic
> violation and they're a stopgap measure).
>
>>Ok, but the point is that a driver like ir-kbd-i2c (and other I2C drivers like
>>lirc-zilog - after ported to rc-core) will require several additional fields
>>that are added at rc_dev (basically, all fields that are, currently, at
>>ir_dev_props structs may be needed by an i2c driver).
>
> I don't think it's a problem. Making rc-core ir-kbd-i2c friendly is
> putting the cart before the horse, ir-kbd-i2c is but one user of rc-core
> (and I also doubt that you'll actually need to duplicate all members,
> i2c hardware is too basic to need all bells and whistles of rc-core).

And just for the record, lirc_zilog probably needs a fairly massive
overhaul, so I'd definitely not worry about it *too* much...

>>> 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).
>>
>>dvb-usb uses a large struct to device dvb devices, and, due to the
>>way it were done, every single field at RC should be inititialized,
>>per device. I don't like the way it is, but I didn't want to delay
>>the rc_core port on it, due to some discussions about how to re-structure
>>it to avoid the large amount of data duplication there. So, I just
>>added the absolute minimum fields there. IMO, we should do later
>>a large cleanup on it. Yet, it is different than ir-kdb-i2c, since
>>since the beginning, the complete IR code is exported on DVB drivers,
>>while V4L drivers use to export just a few bits of the IR code (up
>>to seven bits). So, it is not a good example.
>>
>>A good exercise would be to port lirc-zilog and see what happens.
>
> I had a quick look at lirc-zilog and I doubt it would be a good
> candidate to integrate with ir-kbd-i2c.c (I assume that's what you were
> implying?). Which code from ir-kbd-i2c would it actually be using?

On the receive side, lirc_zilog was pretty similar to lirc_i2c, which
we dropped entirely, as ir-kbd-i2c handles receive just fine for all
the relevant rx-only devices lirc_i2c worked with. So in theory,
ir-kbd-i2c might want to just grow tx support, but I think I'm more
inclined to make it a new stand-alone rx and tx capable driver.

>>> What's your suggestion?
>>
>>One idea could be to initialize rc_dev at the caller drivers, passing
>>it via platform_data for the I2C drivers.
>
> Having a subsystem mucking around in a struct embedded as part of the
> platform data of a higher level driver sounds iffy. You'll never (for
> example) be able to const'ify platform_data...
>
>>Also, instead of duplicating input_dev fields, directly initialize them
>>inside the drivers, and not at rc-core.
>
> Won't work for the reasons explained above.
>
>>I like the idea of having an inlined function (like
>>usb_fill_control_urb), to be sure that all mandatory fields are
>>initialized by the drivers.
>
> I like the idea of having a function, let's call it
> rc_register_device(), which makes sure that all mandatory fields are
> initialized by the drivers :)

rc_register_device(rc, name, phys, id); to further prevent duplicate
struct members? :)

I still really like this interface change, even if its going to cause
short-term issues for i2c devices. I think we just extend this as
needed to handle the i2c bits. That said, I haven't really looked all
that closely at how much that entails...

-- 
Jarod Wilson
jarod@xxxxxxxxxxxx
--
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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux