Re: [PATCH v5 7/7] tpm: create TPM 2.0 devices using own device class

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Tue, Nov 04, 2014 at 11:14:33AM -0700, Jason Gunthorpe wrote:
> On Tue, Nov 04, 2014 at 01:47:34PM +0200, Jarkko Sakkinen wrote:
> 
> > > > I used the class 'tpm' only for TPM 2.x because I didn't want to
> > > > break the binary compatibility for TPM 1.x anyway. In ideal situtation
> > > > both would be character devices inside the class 'tpm' and there would
> > > > be sysfs attribute such as 'family' to mark the protocol to be used.
> > > 
> > > You can create the class without moving away from miscdev...
> > > 
> > > Not having the device creates way to much difference that has to be
> > > supported, way too messy.
> > 
> > I have to admit that I'm not quite following here but I assume that
> > you restated this below in more verbose way and this is basically the
> > same argument :)
> 
> I mean, if we have a patch that does:
> 
> struct tpm_chip {
>    struct device cdev;  // the class device
>    struct device *pdev; // the 'platform' device chip is bound too
> 
>    struct device *dev = pdev; // Temporary Compatability
> [+ device_register/etc/etc]
> 
> Then both cdev and pdev should always be valid. We should not have cdev
> be valid for TPM2 and invalid for TPM1, that is just a big mess.

As a first patch I'll do a patch that does dev -> pdev rename and
nothing else. IMHO it's clean and easy to review if no other changes
are contained. One reason for this is obviously that I want to use
cdev for struct cdev not for the class device.

> Just this change is perfectly safe, it creates the /sys/class/tpm/tpm0
> directory, but doesn't change anything already existing
> 
> Then, a patch: go through and replace all uses of chip->dev with &chip->cdev in
> 90% of cases
> 
> Another patch: use chip->pdev for the handfull of the rest, and drop
> dev entirely
> 
> Then a patch: Drop misc_register entirely, no compat stuff. Explain
> clearly the resulting sysfs changes, CC the various people who monitor
> the sysfs API, act on any feedback. I'm hoping it is an OK change.
> [ If it is not OK then we can talk about using it only for TPM2 or
>   whatever ]
> 
> > I think the current variable name "dev" is miss-leading. The
> > use of "pdev" would document better the appropriate use for that
> > field (i.e. for the most cases DON'T use it).
> 
> Yes, see above :)
> 
> > I think that would be a mess. The way things are done in this v5
> > patch set is actually quite coherent and it does not break backwards
> > compatibility because the "proper" device hierarchy is only used for
> > TPM 2.0 devices.
> 
> I mean, just do something like this:
> 
> if (chip->id == 0)
>    chip->cdev.devt = MKDEV(MISC_MAJOR, TPM_MINOR)
> else
>    chip->cdev.devt = [.. dynamic alloc_chrddev_region ..]
> 
> Very simple, keeps the major/minor for TPM0, moves the 'dev' file to
> the new location in sysfs.
> 
> I can't find another example of this in the kernel, so I don't know
> what the thinking is..
> 
> Jason