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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux