Re: [PATCH v3 1/3] usb: typec: Separate USB Power Delivery from USB Type-C

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Apr 25, 2022 at 03:49:44PM +0300, Heikki Krogerus wrote:
> --- /dev/null
> +++ b/drivers/usb/typec/pd.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __USB_POWER_DELIVERY__
> +#define __USB_POWER_DELIVERY__
> +
> +#include <linux/kobject.h>

Why kobject.h when:

> +
> +struct pd_capabilities {
> +	struct device dev;

This is a device?

> +	struct pd *pd;
> +	enum typec_role role;
> +};
> +
> +struct pd {
> +	struct device		dev;
> +	int			id;
> +
> +	u16			revision; /* 0300H = "3.0" */

So BCD?

> +	u16			version;
> +};



> +
> +#define to_pd_capabilities(o) container_of(o, struct pd_capabilities, dev)
> +#define to_pd(o) container_of(o, struct pd, dev)
> +
> +struct pd *pd_find(const char *name);

"struct pd" is just about the shortest structure name I've seen in the
kernel so far.  How about using some more letters?  :)

> +
> +int pd_init(void);
> +void pd_exit(void);

The kobject question above goes to the code as well.  You are creating a
bunch of raw kobjects still, why?  This should all fit into the driver
model and kobjects shouldn't be needed.  Are you trying to nest too deep
in the attributes?  If so, kobjects will not work as userspace tools
will not realize they are there and are attributes at all.

Try running some tests using libudev and see the lack of attributes with
this patch, tools are not going to be able to see them at all this way.

thanks,

greg k-h