On Wed, Oct 20, 2010 at 01:13:34PM -0500, Bill Gatliff wrote:
> Grant:
>
> > I'm not hugely keen on the naming approach, and I'd rather see the
> > pwm core be responsible for the namespace.
>
> The problem with that, as I see it, is that then the channel
> identifiers become dependent on the order of device initialization.
Correct, just like on all other subsystems.
> By allowing driver authors to assign names, however, users are capable
> of pinning their channel requests to specific devices and channels
> with confidence that those names will remain valid regardless of the
> sequence they compile their kernel in.
Identify by name tends to be a horrible interface in my opinion. The
lesson has been learned in the network and block layers that names
change over time and trying to lock them down to something that a user
finds 'sane' ends up in a mess of workarounds and bad assumptions in
the code.
For in-kernel users, either platform_data or notifier hooks can be used
by one device to have explicit knowledge about the device. No need to
muck about with name encoding.
In userspace, the correct solution is to expose real information about
the device (how the instance is connected to the system plus any other
identifying data) in the sysfs tree and let udev (or a trivial
replacement) decode to a stable user-friendly name.
Essentially, identifying by name attempts to expose real information
about which device it is, but the process of encoding a name is lossy
and so it fails to be robust in the long term.
>
> > Nitpick: putting the filename into the file itself I find tends to be
> > useless
>
> Agreed. Those tend to sneak in during my early development, and then
> I get so used to seeing them that I forget to take them out.
:-)
>
> >> +static int __pwm_create_sysfs(struct pwm_device *pwm);
> >
> > If you order the functions in this file correct, the forward
> > declaration should not be necessary.
>
> In theory, I agree completely with you. In this specific instance,
> however, I haven't been able to structure things in a way that
> eliminates at least one forward declaration.
Fair enough, sometimes that is necessary. I didn't look that closely.
> One possibility would be to move all the sysfs-related stuff out to a
> separate file altogether. Depending on what the class attribute
> rework ends up doing to the code, I just might do that. Of course,
> that wouldn't eliminate the forward declaration--- it would just move
> it. :)
Yeah, by changing to using class attributes, a lot of this could end
up going away.
>
> >> + for (wchan = 0; wchan < pwm->nchan; wchan++) {
> >> + dev = class_find_device(&pwm_class, NULL,
> >> + &pwm->channels[wchan],
> >> + __match_device);
> >
> > If the pwm_channel structure carried a pointer to its device
> > structure, then this lookup wouldn't be needed.
>
> This comment, combined with your other one on the class attributes
> (below) convince me that the device model isn't quite right yet. I
> will rework this until seems correct.
:-)
>
> > The above 8 functions are so tiny that they should probably be static
> > inlines in the header file.
>
> Agreed.
>
> > Rather than open coding the registration of sysfs files, I believe the
> > right thing to do is to use class attributes so that the files get
> > automatically registered for you and are immediately advertised to
> > userspace (very important for correct interaction with udev).
>
> I think I'm either still a little weak on my device model
> understanding, or things have been changing there lately and I haven't
> had the time to notice. Regardless, this code always seemed a little
> awkward--- you seem to agree, so I'll do some research and come back
> with something better.
>
> > Also, sysfs_create_group() should not be called directly when working
> > with devices.
>
> Probably necessary because I'm not doing something right elsewhere. :)
>
> >> + /* TODO: how to deal with devices that register very early? */
> >
> > Same thing we always do for bootstrapping. If it *must* be early,
> > then we do a simple direct access to get things running in platform
> > code, and then take over in the driver when it finally gets probed.
> > The driver model helps with many things, but really early stuff isn't
> > one of them.
>
> I _think_ you and I are saying the same thing: that the device model
> only works well with late-registering devices. I'm not sure I
> understand your suggestion for how to deal with must-be-early devices.
My experience has been there are very few things that cannot be
deferred to module_initcall() time if the device model is handled
well. Typically the things that fall outside of that model are core
infrastructure things like irq controllers which need to be ready to
go before many of the subsystems are initialized.
Basically I'm saying that for things that *really do* need to be setup
before the initcalls, you either don't use the device model at all, or
you figure out how to hand off control from early bootstrap code once
the driver model is set up. Kind of like the early console code.
Scary stuff.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
