Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, August 27, 2019 9:36 PM
> 
> Hi Shimoda-san,
> 
> On Tue, Aug 27, 2019 at 12:05 PM Yoshihiro Shimoda
> <yoshihiro.shimoda...@renesas.com> wrote:
> > To simplify adding xhci->quirks instead of the .init_quirk()
> > function, this patch adds a new parameter "quirks" into
> > the struct xhci_plat_priv.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be>

Thank you for your review!

> > --- a/drivers/usb/host/xhci-plat.h
> > +++ b/drivers/usb/host/xhci-plat.h
> > @@ -12,10 +12,12 @@
> >
> >  struct xhci_plat_priv {
> >         const char *firmware_name;
> > +       unsigned long long quirks;
> >         void (*plat_start)(struct usb_hcd *);
> >         int (*init_quirk)(struct usb_hcd *);
> >         int (*resume_quirk)(struct usb_hcd *);
> >  };
> >
> >  #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
> > +#define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv)
> 
> Just wondering: is x->priv guaranteed to be of type struct xhci_plat_priv *,
> also in the future?
> 
>     struct xhci_hcd {
>             ...
>             unsigned long           priv[0] __aligned(sizeof(s64));

It seems so. But, I'm not sure that we can change type of the priv[0] to
struct xhci_plat_priv *, because this implementation is related to all usb_hcd 
drivers:

< Details if you are interested in :) >
--- include/linux/usb/hcd.h ---
struct usb_hcd {
        ...
        /* The HC driver's private data is stored at the end of
         * this structure.
         */
        unsigned long hcd_priv[0]
                        __attribute__ ((aligned(sizeof(s64))));
};

--- drivers/usb/host/xhci.c ---
static const struct hc_driver xhci_hc_driver = {
        ...
        .hcd_priv_size =        sizeof(struct xhci_hcd),

and

void xhci_init_driver(struct hc_driver *drv,
                      const struct xhci_driver_overrides *over)
{
        BUG_ON(!over);

        /* Copy the generic table to drv then apply the overrides */
        *drv = xhci_hc_driver;

        if (over) {
                drv->hcd_priv_size += over->extra_priv_size;

--- drivers/usb/host/xhci-plat.c ---
static const struct xhci_driver_overrides xhci_plat_overrides __initconst = {
        .extra_priv_size = sizeof(struct xhci_plat_priv),

void xhci_init_driver(struct hc_driver *drv,
                      const struct xhci_driver_overrides *over)
{
        BUG_ON(!over);

        /* Copy the generic table to drv then apply the overrides */
        *drv = xhci_hc_driver;

        if (over) {
                drv->hcd_priv_size += over->extra_priv_size;

--- drivers/usb/core/hcd.c ---
struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
                struct device *sysdev, struct device *dev, const char *bus_name,
                struct usb_hcd *primary_hcd)
{
        struct usb_hcd *hcd;

        hcd = kzalloc(sizeof(*hcd) + driver->hcd_priv_size, GFP_KERNEL);
------------------------------

Best regards,
Yoshihiro Shimoda

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
>                                 -- Linus Torvalds

Reply via email to