Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match tables

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Aug 22, 2023 at 12:51:04PM +0000, Biju Das wrote:
> > On Tue, Aug 22, 2023 at 02:00:05PM +0200, Geert Uytterhoeven wrote:
> > > On Tue, Aug 22, 2023 at 1:44 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Tue, Aug 22, 2023 at 09:21:19AM +0200, Geert Uytterhoeven wrote:
> > > > > On Mon, Aug 21, 2023 at 7:09 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Mon, Aug 21, 2023 at 05:40:05PM +0200, Geert Uytterhoeven wrote:
> > > > > > > On Mon, Aug 21, 2023 at 5:25 PM Andy Shevchenko
> > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > On Mon, Aug 21, 2023 at 03:27:43PM +0200, Geert Uytterhoeven
> > wrote:
> > > > > > > > > On Mon, Aug 21, 2023 at 3:04 PM Andy Shevchenko
> > > > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > > > On Sun, Aug 20, 2023 at 07:44:00PM +0100, Biju Das wrote:

...

> > > > > > > > > > For all your work likes this as I noted in the reply to
> > > > > > > > > > Guenter that the couple of the selling points here are:
> > > > > > > > > > 1) avoidance of the pointer abuse in OF table
> > > > > > > > > >    (we need that to be a valid pointer);
> > > > > > > > >
> > > > > > > > > There is no pointer abuse: both const void * (in e.g.
> > > > > > > > > of_device_id) and kernel_ulong_t (in e.g. i2c_device_id)
> > > > > > > > > can be used by drivers to store a magic cookie, being either
> > a pointer, or an integer value.
> > > > > > > > > The same is true for the various unsigned long and void *
> > "driver_data"
> > > > > > > > > fields in subsystem-specific driver structures.
> > > > > > > >
> > > > > > > > (void *)5 is the abuse of the pointer.
> > > > > > > > We carry something which is not a valid pointer from kernel
> > perspective.
> > > > > > >
> > > > > > > But the data field is not required to be a valid pointer.
> > > > > > > What kind and type of information it represents is specific to
> > the driver.
> > > > > >
> > > > > > Where to find necessary information which is not always an integer
> > constant.
> > > > > > For example, for the driver data that has callbacks it can't be
> > invalid pointer.
> > > > >
> > > > > If the driver uses it to store callbacks, of course it needs to be
> > > > > a valid pointer. But that is internal to the driver.  It is not
> > > > > that we're passing random integer values to a function that
> > > > > expects a pointer that can actually be dereferenced.
> > > > >
> > > > > > Since OF ID table structure is universal, it uses pointers.
> > > > > > Maybe you need to update it to use plain integer instead?
> > > > >
> > > > > It is fairly common in the kernel to use void * to indicate a
> > > > > driver-specific cookie, being either a real pointer or an integral
> > > > > value, that is passed verbatim.  See also e.g. the "dev" parameter
> > > > > of request_irq().
> > > >
> > > > Yes, that parameter is void * due to calling kfree(free_irq(...)).
> > > > So, that's argument for my concerns.
> > >
> > > Sorry, I don't understand this comment.
> > > (kfree(free_irq(...)) is only called in pci_free_irq()?)
> > 
> > Passing void * for a "driver cookie" makes sense due to possibility of the
> > passing it to other functions that want to have void * as your example
> > shows.
> > And that supports my idea of having void * over the unsigned long.
> 
> U-boot also uses unsigned long for .data in struct udevice_id. There may be a
> reason for it instead of void* ??

U-Boot to be honest not the best example of the code and I believe the reason
why is pure historical. unsigned long and void * are both architecture dependent,
so the one vs. another is only for the convenience here or there. The only thing
so far is to preserve the const qualifier, which you can not achieve with integer.

-- 
With Best Regards,
Andy Shevchenko