Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 13 Sep 2022 14:54:27 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Jiasheng Jiang <jiasheng@iscas.ac.cn>
Cc:     gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] USB: serial: ftdi_sio: Convert to use dev_groups
Message-ID: <YyB9g2xKIvUsv+vL@hovoldconsulting.com>
References: <20220905095120.343807-1-jiasheng@iscas.ac.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220905095120.343807-1-jiasheng@iscas.ac.cn>
Precedence: bulk

On Mon, Sep 05, 2022 at 05:51:20PM +0800, Jiasheng Jiang wrote:
> The driver core supports the ability to handle the creation and removal
> of device-specific sysfs files in a race-free manner. Moreover, it can
> guarantee the success of creation. Therefore, it should be better to
> move the code and convert to use dev_groups.
> 
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
> Changelog:
> 
> v3 -> v4:
> 
> 1. Move the code and remove the pre-definitions.

I don't know exactly you were told to do here, but do not reorganise the
entire driver just to add an attribute group. It not only makes the
patch harder to review but also messes up the git history (e.g.
git-blame) for no good reason.

> +static umode_t ftdi_sio_attr_is_visible(struct kobject *kobj,
> +					 struct attribute *attr, int idx)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct usb_serial_port *port = container_of(dev, struct usb_serial_port, dev);
> +	struct ftdi_private *priv = usb_get_serial_port_data(port);
> +	umode_t mode = attr->mode;
> +
> +	if (attr == &dev_attr_latency_timer.attr) {
> +		if (priv->chip_type == FT232BM ||
> +		    priv->chip_type == FT2232C ||
> +		    priv->chip_type == FT232RL ||
> +		    priv->chip_type == FT2232H ||
> +		    priv->chip_type == FT4232H ||
> +		    priv->chip_type == FT232H ||
> +		    priv->chip_type == FTX) {
> +			return mode;
> +		}
> +	}
> +	return 0;
> +}

This isn't equivalent with the current implementation, which used
different visibility per attribute.

> +static struct attribute *ftdi_sio_attrs[] = {
> +	&dev_attr_event_char.attr,
> +	&dev_attr_latency_timer.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ftdi_sio_group = {
> +	.attrs = ftdi_sio_attrs,
> +	.is_visible = ftdi_sio_attr_is_visible,
> +};
> +
> +static const struct attribute_group *ftdi_sio_groups[] = {
> +	&ftdi_sio_group,
> +	NULL
> +};
> +
>  static struct usb_serial_driver ftdi_sio_device = {
>  	.driver = {
>  		.owner =	THIS_MODULE,
>  		.name =		"ftdi_sio",
> +		.dev_groups =	ftdi_sio_groups,

And you should only need a forward declaration for ftdi_sio_groups for
this.

>  	},
>  	.description =		"FTDI USB Serial Device",
>  	.id_table =		id_table_combined,
> @@ -1144,9 +1279,6 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  };

Johan