Hi Andy,
Thanks for the feedback.
> Subject: Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer
> for data in the match tables
>
> On Sun, Aug 20, 2023 at 07:44:00PM +0100, Biju Das wrote:
> > Convert enum->pointer for data in the match tables, so that
> > device_get_match_data() can do match against OF/ACPI/I2C tables, once
> > i2c bus type match support added to it and it returns NULL for non-match.
> >
> > Therefore it is better to convert enum->pointer for data match and
> > extend match support for both ID and OF tables by using
> > i2c_get_match_data() by adding struct rt1711h_chip_info with did
> > variable and replacing did->info in struct rt1711h_chip. Later patches
> > will add more hw differences to struct rt1711h_chip_info and avoid
> checking did for HW differences.
>
> ...
>
> > +struct rt1711h_chip_info {
> > + u16 did;
> > +};
> > +
> > struct rt1711h_chip {
> > struct tcpci_data data;
> > struct tcpci *tcpci;
> > struct device *dev;
> > struct regulator *vbus;
> > bool src_en;
> > - u16 did;
> > + const struct rt1711h_chip_info *info;
>
> Have you run pahole? I believe now you wasting a few more bytes (besides
> the pointer requirement) due to (mis)placing a new member.
>
> > };
Just tried pahole for the first time.
$ pahole -C rt1711h_chip drivers/usb/typec/tcpm/tcpci_rt1711h.o
struct rt1711h_chip {
struct tcpci_data data; /* 0 72 */
/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
struct tcpci * tcpci; /* 72 8 */
struct device * dev; /* 80 8 */
struct regulator * vbus; /* 88 8 */
bool src_en; /* 96 1 */
/* XXX 7 bytes hole, try to pack */
const struct rt1711h_chip_info * info; /* 104 8 */
/* size: 112, cachelines: 2, members: 6 */
/* sum members: 105, holes: 1, sum holes: 7 */
/* last cacheline: 48 bytes */
};
biju@biju-VirtualBox:~/linux-next-test$ pahole -C rt1711h_chip_info drivers/usb/typec/tcpm/tcpci_rt1711h.o
struct rt1711h_chip_info {
u16 did; /* 0 2 */
/* XXX 2 bytes hole, try to pack */
u32 rxdz_sel; /* 4 4 */
unsigned int enable_pd30_extended_message:1; /* 8: 0 4 */
/* size: 12, cachelines: 1, members: 3 */
/* sum members: 6, holes: 1, sum holes: 2 */
/* sum bitfield members: 1 bits (0 bytes) */
/* bit_padding: 31 bits */
/* last cacheline: 12 bytes */
};
Currently size is 12 bytes, it can be reduced to 8 by adding bool.
biju@biju-VirtualBox:~/linux-next-test$ pahole -C rt1711h_chip_info drivers/usb/typec/tcpm/tcpci_rt1711h.o
struct rt1711h_chip_info {
u32 rxdz_sel; /* 0 4 */
u16 did; /* 4 2 */
bool enable_pd30_extended_message; /* 6 1 */
/* size: 8, cachelines: 1, members: 3 */
/* padding: 1 */
/* last cacheline: 8 bytes */
};
Cheers,
Biju
> ...
>
> 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);
> 2) preservation of the const qualifier (despite kernel_ulong_t
> being used in the middle).
>
> With that added I believe you can sell this much more easier.
>
> --
> With Best Regards,
> Andy Shevchenko
>
