Message-ID: <3cce7711-3e31-2f9b-8be7-36be6ee8a19c@linaro.org>
Date:   Sat, 17 Jun 2023 10:34:40 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Stanley Chang <stanley_chang@...ltek.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Vinod Koul <vkoul@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Flavio Suligoi <f.suligoi@...m.it>,
        Douglas Anderson <dianders@...omium.org>,
        Bagas Sanjaya <bagasdotme@...il.com>,
        Matthias Kaehlcke <mka@...omium.org>,
        Ray Chi <raychi@...gle.com>, linux-phy@...ts.infradead.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH v4 2/5] phy: realtek: usb: Add driver for the Realtek SoC
 USB 2.0 PHY

On 14/06/2023 11:28, Stanley Chang wrote:
> Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
> controller. Added the driver to drive the USB 2.0 PHY transceivers.
> 
> Signed-off-by: Stanley Chang <stanley_chang@...ltek.com>
> ---
> v3 to v4 change:
>     1. Remove the parameter and non hardware properties from dts.
>        Add the compatible data included the config and parameter.
>     2. Fix the warning for checkpatch with strict.
>     3. Remove useless debug messages.
>     4. Fix the incorrect or useless "if() return" which pointer check.
>     5. Remove the phy power domain control due to we control on other
>        driver.
> v2 to v3 change:
>     1. Broken down into two patches, one for each of USB 2 & 3 PHY.
>     2. Removed parameter v1 support for simplification.
>     3. Use remove_new for driver remove callback.
> v1 to v2 change:
>     1. Move the drivers to drivers/phy/ for generic phy driver
>     2. Use the generic phy driver api to initialize phy
>     3. Use readl/writel to instead phy_read/phy_write directly.
>     4. Use read_poll_timeout() in function utmi_wait_register.
>     5. Revised some coding styles.
>     6. fix the compiler warning for kernel test robot.
> ---

...

> +/* updated disconnect level at page0 */
> +static void update_dc_disconnect_level_at_page0(struct rtk_phy *rtk_phy,
> +						struct phy_parameter *phy_parameter, bool update)
> +{
> +	struct phy_cfg *phy_cfg;
> +	struct phy_reg *phy_reg;
> +	struct phy_data *phy_data_page;
> +	struct phy_data *phy_data;
> +	u8 addr, data;
> +	int offset = 4;
> +	s32 dc_disconnect_mask;
> +	int i;
> +
> +	phy_cfg = rtk_phy->phy_cfg;
> +	phy_reg = &phy_parameter->phy_reg;
> +
> +	/* Set page 0 */
> +	phy_data_page = phy_cfg->page0;
> +	rtk_phy_set_page(phy_reg, 0);
> +
> +	i = page_addr_to_array_index(PAGE0_0XE4);
> +	phy_data = phy_data_page + i;
> +	if (!phy_data->addr) {
> +		phy_data->addr = PAGE0_0XE4;
> +		phy_data->data = rtk_phy_read(phy_reg, PAGE0_0XE4);
> +	}
> +
> +	addr = phy_data->addr;
> +	data = phy_data->data;
> +	dc_disconnect_mask = phy_cfg->dc_disconnect_mask;
> +
> +	if (update)
> +		data = __updated_dc_disconnect_level_page0_0xe4(phy_cfg, phy_parameter, data);
> +	else
> +		data = (data & ~(dc_disconnect_mask << offset)) |
> +			(DEFAULT_DC_DISCONNECTION_VALUE << offset);
> +
> +	if (rtk_phy_write(phy_reg, addr, data))
> +		dev_err(rtk_phy->dev,
> +			"[%s:%d] Error page1 addr=0x%x value=0x%x\n",
> +			__func__, __LINE__,
> +			addr, data);

Is addr a kernel address or any memory (not SFR) address? If so, you
cannot print it.

> +}
> +
> +static u8 __updated_dc_disconnect_level_page1_0xe2(struct phy_cfg *phy_cfg,
> +						   struct phy_parameter *phy_parameter, u8 data)
> +{
> +	u8 val;
> +	s32 __val;
> +	s32 dc_disconnect_mask = phy_cfg->dc_disconnect_mask;
> +
> +	if (phy_cfg->check_efuse_version == CHECK_EFUSE_V1) {
> +		__val = (s32)(data & dc_disconnect_mask)
> +			    + phy_parameter->efuse_usb_dc_dis
> +			    + phy_parameter->disconnection_compensate;
> +	} else { /* for CHECK_EFUSE_V2 or no efuse */
> +		if (phy_parameter->efuse_usb_dc_dis)
> +			__val = (s32)(phy_parameter->efuse_usb_dc_dis +
> +				    phy_parameter->disconnection_compensate);
> +		else
> +			__val = (s32)((data & dc_disconnect_mask) +
> +				    phy_parameter->disconnection_compensate);
> +	}
> +
> +	if (__val > dc_disconnect_mask)
> +		__val = dc_disconnect_mask;
> +	else if (__val < 0)
> +		__val = 0;
> +
> +	val = (data & (~dc_disconnect_mask)) | (__val & dc_disconnect_mask);
> +
> +	return val;
> +}
> +
> +/* updated disconnect level at page1 */
> +static void update_dc_disconnect_level_at_page1(struct rtk_phy *rtk_phy,
> +						struct phy_parameter *phy_parameter, bool update)
> +{
> +	struct phy_cfg *phy_cfg;
> +	struct phy_data *phy_data_page;
> +	struct phy_data *phy_data;
> +	struct phy_reg *phy_reg;
> +	u8 addr, data;
> +	s32 dc_disconnect_mask;
> +	int i;
> +
> +	phy_cfg = rtk_phy->phy_cfg;
> +	phy_reg = &phy_parameter->phy_reg;
> +
> +	/* Set page 1 */
> +	phy_data_page = phy_cfg->page1;
> +	rtk_phy_set_page(phy_reg, 1);
> +
> +	i = page_addr_to_array_index(PAGE1_0XE2);
> +	phy_data = phy_data_page + i;
> +	if (!phy_data->addr) {
> +		phy_data->addr = PAGE1_0XE2;
> +		phy_data->data = rtk_phy_read(phy_reg, PAGE1_0XE2);
> +	}
> +
> +	addr = phy_data->addr;
> +	data = phy_data->data;
> +	dc_disconnect_mask = phy_cfg->dc_disconnect_mask;
> +
> +	if (update)
> +		data = __updated_dc_disconnect_level_page1_0xe2(phy_cfg, phy_parameter, data);
> +	else
> +		data = (data & ~dc_disconnect_mask) | DEFAULT_DC_DISCONNECTION_VALUE;
> +
> +	if (rtk_phy_write(phy_reg, addr, data))
> +		dev_err(rtk_phy->dev,
> +			"[%s:%d] Error page1 addr=0x%x value=0x%x\n",
> +			__func__, __LINE__,
> +			addr, data);

Ditto and in all other places.

> +}
> +
> +static void update_dc_disconnect_level(struct rtk_phy *rtk_phy,
> +				       struct phy_parameter *phy_parameter, bool update)
> +{
> +	struct phy_cfg *phy_cfg = rtk_phy->phy_cfg;
> +
> +	if (phy_cfg->usb_dc_disconnect_at_page0)
> +		update_dc_disconnect_level_at_page0(rtk_phy, phy_parameter, update);
> +	else
> +		update_dc_disconnect_level_at_page1(rtk_phy, phy_parameter, update);
> +}
> +
> +static u8 __update_dc_driving_page0_0xe4(struct phy_cfg *phy_cfg,
> +					 struct phy_parameter *phy_parameter, u8 data)
> +{
> +	s32 driving_compensate = phy_parameter->driving_compensate;
> +	s32 dc_driving_mask = phy_cfg->dc_driving_mask;
> +	s32 __val;
> +	u8 val;

Two variables with the same name. No, it is not readable code.

...

> +static void update_dc_driving_level(struct rtk_phy *rtk_phy,
> +				    struct phy_parameter *phy_parameter)
> +{
> +	struct phy_cfg *phy_cfg;
> +	struct phy_reg *phy_reg;
> +

...
> +
> +static const struct phy_ops ops = {
> +	.init		= rtk_phy_init,
> +	.exit		= rtk_phy_exit,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static void rtk_phy_toggle(struct usb_phy *usb2_phy, bool connect, int port)
> +{
> +	int index = port;
> +	struct rtk_phy *rtk_phy = NULL;
> +
> +	rtk_phy = dev_get_drvdata(usb2_phy->dev);
> +
> +	if (index > rtk_phy->num_phy) {
> +		pr_err("%s %d ERROR! port=%d > num_phy=%d\n",

dev_err

> +		       __func__, __LINE__, index, rtk_phy->num_phy);

all these func and LINE point to poor code quality and poor debugging
practices. These are added dugin development, not for production code,
because error message should be obvious. Your usage of pr_err, func,
LINE and some unprecise messages suggests this is not ready.

Fix all your error messages to be meaningful.

> +		return
> +	}
> +
> +	do_rtk_phy_toggle(rtk_phy, index, connect);
> +}
> +
> +static int rtk_phy_notify_port_status(struct usb_phy *x, int port,
> +				      u16 portstatus, u16 portchange)
> +{
> +	bool connect = false;
> +
> +	pr_debug("%s port=%d portstatus=0x%x portchange=0x%x\n",
> +		 __func__, port, (int)portstatus, (int)portchange);
> +	if (portstatus & USB_PORT_STAT_CONNECTION)
> +		connect = true;
> +
> +	if (portchange & USB_PORT_STAT_C_CONNECTION)
> +		rtk_phy_toggle(x, connect, port);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_DEBUG_FS
> +static struct dentry *create_phy_debug_root(void)
> +{
> +	struct dentry *phy_debug_root;
> +
> +	phy_debug_root = debugfs_lookup("phy", usb_debug_root);
> +	if (!phy_debug_root)
> +		phy_debug_root = debugfs_create_dir("phy", usb_debug_root);
> +
> +	return phy_debug_root;
> +}
> +
> +static int rtk_usb2_parameter_show(struct seq_file *s, void *unused)
> +{
> +	struct rtk_phy *rtk_phy = s->private;
> +	struct phy_cfg *phy_cfg;
> +	int i, index;
> +
> +	phy_cfg = rtk_phy->phy_cfg;
> +
> +	seq_puts(s, "Property:\n");
> +	seq_printf(s, "  check_efuse: %s\n",
> +		   phy_cfg->check_efuse ? "Enable" : "Disable");
> +	seq_printf(s, "  check_efuse_version: %d\n",
> +		   phy_cfg->check_efuse_version);
> +	seq_printf(s, "  efuse_dc_driving_rate: %d\n",
> +		   phy_cfg->efuse_dc_driving_rate);
> +	seq_printf(s, "  dc_driving_mask: 0x%x\n",
> +		   phy_cfg->dc_driving_mask);
> +	seq_printf(s, "  efuse_dc_disconnect_rate: %d\n",
> +		   phy_cfg->efuse_dc_disconnect_rate);
> +	seq_printf(s, "  dc_disconnect_mask: 0x%x\n",
> +		   phy_cfg->dc_disconnect_mask);
> +	seq_printf(s, "  usb_dc_disconnect_at_page0: %s\n",
> +		   phy_cfg->usb_dc_disconnect_at_page0 ? "true" : "false");
> +	seq_printf(s, "  do_toggle: %s\n",
> +		   phy_cfg->do_toggle ? "Enable" : "Disable");
> +	seq_printf(s, "  do_toggle_driving: %s\n",
> +		   phy_cfg->do_toggle_driving ? "Enable" : "Disable");
> +	seq_printf(s, "  driving_updated_for_dev_dis: 0x%x\n",
> +		   phy_cfg->driving_updated_for_dev_dis);
> +	seq_printf(s, "  use_default_parameter: %s\n",
> +		   phy_cfg->use_default_parameter ? "Enable" : "Disable");
> +	seq_printf(s, "  is_double_sensitivity_mode: %s\n",
> +		   phy_cfg->is_double_sensitivity_mode ? "Enable" : "Disable");
> +
> +	for (index = 0; index < rtk_phy->num_phy; index++) {
> +		struct phy_parameter *phy_parameter;
> +		struct phy_reg *phy_reg;
> +		struct phy_data *phy_data_page;
> +
> +		phy_parameter =  &((struct phy_parameter *)rtk_phy->phy_parameter)[index];
> +		phy_reg = &phy_parameter->phy_reg;
> +
> +		seq_printf(s, "PHY %d:\n", index);
> +
> +		seq_puts(s, "Page 0:\n");
> +		/* Set page 0 */
> +		phy_data_page = phy_cfg->page0;
> +		rtk_phy_set_page(phy_reg, 0);
> +
> +		for (i = 0; i < phy_cfg->page0_size; i++) {
> +			struct phy_data *phy_data = phy_data_page + i;
> +			u8 addr = array_index_to_page_addr(i);
> +			u8 data = phy_data->data;
> +			u8 value = rtk_phy_read(phy_reg, addr);
> +
> +			if (phy_data->addr)
> +				seq_printf(s, "  Page 0: addr=0x%x data=0x%02x ==> read value=0x%02x\n",
> +					   addr, data, value);
> +			else
> +				seq_printf(s, "  Page 0: addr=0x%x data=none ==> read value=0x%02x\n",
> +					   addr, value);
> +		}
> +
> +		seq_puts(s, "Page 1:\n");
> +		/* Set page 1 */
> +		phy_data_page = phy_cfg->page1;
> +		rtk_phy_set_page(phy_reg, 1);
> +
> +		for (i = 0; i < phy_cfg->page1_size; i++) {
> +			struct phy_data *phy_data = phy_data_page + i;
> +			u8 addr = array_index_to_page_addr(i);
> +			u8 data = phy_data->data;
> +			u8 value = rtk_phy_read(phy_reg, addr);
> +
> +			if (phy_data->addr)
> +				seq_printf(s, "  Page 1: addr=0x%x data=0x%02x ==> read value=0x%02x\n",
> +					   addr, data, value);
> +			else
> +				seq_printf(s, "  Page 1: addr=0x%x data=none ==> read value=0x%02x\n",
> +					   addr, value);
> +		}
> +
> +		if (phy_cfg->page2_size == 0)
> +			goto out;
> +
> +		seq_puts(s, "Page 2:\n");
> +		/* Set page 2 */
> +		phy_data_page = phy_cfg->page2;
> +		rtk_phy_set_page(phy_reg, 2);
> +
> +		for (i = 0; i < phy_cfg->page2_size; i++) {
> +			struct phy_data *phy_data = phy_data_page + i;
> +			u8 addr = array_index_to_page_addr(i);
> +			u8 data = phy_data->data;
> +			u8 value = rtk_phy_read(phy_reg, addr);
> +
> +			if (phy_data->addr)
> +				seq_printf(s, "  Page 2: addr=0x%x data=0x%02x ==> read value=0x%02x\n",
> +					   addr, data, value);
> +			else
> +				seq_printf(s, "  Page 2: addr=0x%x data=none ==> read value=0x%02x\n",
> +					   addr, value);
> +		}
> +
> +out:
> +		seq_puts(s, "PHY Property:\n");
> +		seq_printf(s, "  efuse_usb_dc_cal: %d\n",
> +			   (int)phy_parameter->efuse_usb_dc_cal);
> +		seq_printf(s, "  efuse_usb_dc_dis: %d\n",
> +			   (int)phy_parameter->efuse_usb_dc_dis);
> +		seq_printf(s, "  inverse_hstx_sync_clock: %s\n",
> +			   phy_parameter->inverse_hstx_sync_clock ? "Enable" : "Disable");
> +		seq_printf(s, "  driving_level: %d\n",
> +			   phy_parameter->driving_level);
> +		seq_printf(s, "  driving_compensate: %d\n",
> +			   phy_parameter->driving_compensate);
> +		seq_printf(s, "  disconnection_compensate: %d\n",
> +			   phy_parameter->disconnection_compensate);
> +	}
> +
> +	return 0;
> +}
> +
> +static int rtk_usb2_parameter_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, rtk_usb2_parameter_show, inode->i_private);
> +}
> +
> +static const struct file_operations rtk_usb2_parameter_fops = {
> +	.open			= rtk_usb2_parameter_open,
> +	.read			= seq_read,
> +	.llseek			= seq_lseek,
> +	.release		= single_release,
> +};
> +
> +static int __get_parameter_at_page(struct seq_file *s,
> +				   struct rtk_phy *rtk_phy,
> +				   struct phy_data *phy_data_page,
> +				   const char *phy_page, const char *phy_addr)
> +{
> +	struct phy_data *phy_data;
> +	u32 addr;
> +	int i, ret;
> +
> +	ret = kstrtouint(phy_addr, 16, &addr);
> +	if (ret < 0) {
> +		pr_err("%s::kstrtouint() failed\n", __func__);
> +		return -EINVAL;
> +	}
> +	i = page_addr_to_array_index(addr);
> +	phy_data = (phy_data_page + i);
> +
> +	if (phy_data->addr)
> +		seq_printf(s, "Now Parameter %s addr 0x%02x = 0x%02x\n",
> +			   phy_page, phy_data->addr, phy_data->data);
> +	else
> +		seq_printf(s, "Now Parameter %s addr 0x%02x is default\n",
> +			   phy_page, addr);
> +
> +	return 0;
> +}
> +
> +static int __set_parameter_at_page(struct rtk_phy *rtk_phy,
> +				   struct phy_reg *phy_reg, struct phy_parameter *phy_parameter,
> +				   struct phy_data *phy_data_page,
> +				   const char *phy_page, const char *phy_addr,
> +				   const char *phy_value)
> +{
> +	struct phy_data *phy_data;
> +	u32 addr, value;
> +	int i, ret;
> +
> +	ret = kstrtouint(phy_addr, 16, &addr);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	ret = kstrtouint(phy_value, 16, &value);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	i = page_addr_to_array_index(addr);
> +	phy_data = (phy_data_page + i);
> +
> +	if (phy_data->addr) {
> +		phy_data->data = value;
> +	} else {
> +		phy_data->addr = addr;
> +		phy_data->data = value;
> +	}
> +
> +	if (rtk_phy_write(phy_reg, addr, value))
> +		dev_err(rtk_phy->dev,
> +			"[%s:%d] Error: addr=0x%02x value=0x%02x\n",
> +			__func__, __LINE__, addr, value);
> +
> +	return 0;
> +}
> +
> +static int rtk_usb2_set_parameter_show(struct seq_file *s, void *unused)
> +{
> +	const struct file *file = s->file;
> +	const char *file_name = file_dentry(file)->d_iname;
> +	struct dentry *p_dentry = file_dentry(file)->d_parent;
> +	const char *dir_name = p_dentry->d_iname;
> +	struct dentry *pp_dentry = p_dentry->d_parent;
> +	const char *phy_dir_name = pp_dentry->d_iname;
> +	struct rtk_phy *rtk_phy = s->private;
> +	struct phy_cfg *phy_cfg;
> +	struct phy_parameter *phy_parameter = NULL;
> +	int ret = 0;
> +	int index;
> +
> +	for (index = 0; index < rtk_phy->num_phy; index++) {
> +		size_t sz = 30;
> +		char name[30] = {0};
> +
> +		snprintf(name, sz, "phy%d", index);
> +		if (strncmp(name, phy_dir_name, strlen(name)) == 0) {
> +			phy_parameter = &((struct phy_parameter *)rtk_phy->phy_parameter)[index];
> +			break;
> +		}
> +	}
> +
> +	if (!phy_parameter)
> +		return -EINVAL;
> +
> +	phy_cfg = rtk_phy->phy_cfg;
> +
> +	if (strcmp("page0", dir_name) == 0)
> +		ret = __get_parameter_at_page(s, rtk_phy, phy_cfg->page0,
> +					      dir_name, file_name);
> +	else if (strcmp("page1", dir_name) == 0)
> +		ret = __get_parameter_at_page(s, rtk_phy, phy_cfg->page1,
> +					      dir_name, file_name);
> +	else if (strcmp("page2", dir_name) == 0)
> +		ret = __get_parameter_at_page(s, rtk_phy, phy_cfg->page2,
> +					      dir_name, file_name);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	seq_puts(s, "Set phy parameter by following command\n");
> +	seq_printf(s, "echo \"value\" > %s/%s/%s\n",
> +		   phy_dir_name, dir_name, file_name);
> +
> +	return 0;
> +}
> +
> +static int rtk_usb2_set_parameter_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, rtk_usb2_set_parameter_show, inode->i_private);
> +}
> +
> +static ssize_t rtk_usb2_set_parameter_write(struct file *file,
> +					    const char __user *ubuf, size_t count,
> +					    loff_t *ppos)
> +{
> +	const char *file_name = file_dentry(file)->d_iname;
> +	struct dentry *p_dentry = file_dentry(file)->d_parent;
> +	const char *dir_name = p_dentry->d_iname;
> +	struct dentry *pp_dentry = p_dentry->d_parent;
> +	const char *phy_dir_name = pp_dentry->d_iname;
> +	struct seq_file *s = file->private_data;
> +	struct rtk_phy *rtk_phy = s->private;
> +	struct phy_reg *phy_reg;
> +	struct phy_cfg *phy_cfg;
> +	struct phy_parameter *phy_parameter = NULL;
> +	int ret = 0;
> +	char buffer[40] = {0};
> +	int index;
> +
> +	if (copy_from_user(&buffer, ubuf,
> +			   min_t(size_t, sizeof(buffer) - 1, count)))
> +		return -EFAULT;
> +
> +	for (index = 0; index < rtk_phy->num_phy; index++) {
> +		size_t sz = 30;
> +		char name[30] = {0};
> +
> +		snprintf(name, sz, "phy%d", index);
> +		if (strncmp(name, phy_dir_name, strlen(name)) == 0) {
> +			phy_parameter = &((struct phy_parameter *)rtk_phy->phy_parameter)[index];
> +			break;
> +		}
> +	}
> +
> +	if (!phy_parameter)
> +		return -EINVAL;
> +
> +	phy_cfg = rtk_phy->phy_cfg;
> +	phy_reg = &phy_parameter->phy_reg;
> +
> +	if (strcmp("page0", dir_name) == 0) {
> +		rtk_phy_set_page(phy_reg, 0);
> +		ret = __set_parameter_at_page(rtk_phy, phy_reg, phy_parameter,
> +					      phy_cfg->page0, dir_name, file_name, buffer);
> +	} else if (strcmp("page1", dir_name) == 0) {
> +		rtk_phy_set_page(phy_reg, 1);
> +		ret = __set_parameter_at_page(rtk_phy, phy_reg, phy_parameter,
> +					      phy_cfg->page1, dir_name, file_name, buffer);
> +	} else if (strcmp("page2", dir_name) == 0) {
> +		rtk_phy_set_page(phy_reg, 2);
> +		ret = __set_parameter_at_page(rtk_phy, phy_reg, phy_parameter,
> +					      phy_cfg->page2, dir_name, file_name, buffer);
> +	}
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static const struct file_operations rtk_usb2_set_parameter_fops = {
> +	.open			= rtk_usb2_set_parameter_open,
> +	.write			= rtk_usb2_set_parameter_write,

NAK. You just created user interface via debugfs. You cannot. Reading
for some debug is okay, but configuring device via undocumented debugfs
is a source of troubles.

Drop all writes to debugfs.

...

> +
> +static int parse_phy_data(struct rtk_phy *rtk_phy)
> +{
> +	struct device *dev = rtk_phy->dev;
> +	struct device_node *node;

By convention:
s/node/np/

> +	struct phy_cfg *phy_cfg;
> +	struct phy_parameter *phy_parameter;
> +	int ret = 0;
> +	int index;
> +
> +	node = dev->of_node;

Keep it in variable definition.

> +	phy_cfg = rtk_phy->phy_cfg;
> +
> +	rtk_phy->phy_parameter = devm_kzalloc(dev, sizeof(struct phy_parameter) *
> +						rtk_phy->num_phy, GFP_KERNEL);
> +	if (!rtk_phy->phy_parameter)
> +		return -ENOMEM;
> +
> +	for (index = 0; index < rtk_phy->num_phy; index++) {
> +		phy_parameter = &((struct phy_parameter *)rtk_phy->phy_parameter)[index];
> +
> +		phy_parameter->phy_reg.reg_wrap_vstatus = of_iomap(dev->of_node, 0);
> +		phy_parameter->phy_reg.reg_gusb2phyacc0 = of_iomap(dev->of_node, 1) + index;
> +		phy_parameter->phy_reg.vstatus_index = index;
> +
> +		if (of_property_read_bool(node, "realtek,inverse-hstx-sync-clock"))
> +			phy_parameter->inverse_hstx_sync_clock = true;
> +		else
> +			phy_parameter->inverse_hstx_sync_clock = false;
> +
> +		if (of_property_read_u32_index(node, "realtek,driving-level",
> +					       index, &phy_parameter->driving_level))
> +			phy_parameter->driving_level = DEFAULT_DC_DRIVING_VALUE;
> +
> +		if (of_property_read_u32_index(node, "realtek,driving-compensate",
> +					       index, &phy_parameter->driving_compensate))
> +			phy_parameter->driving_compensate = 0;
> +
> +		if (of_property_read_u32_index(node, "realtek,disconnection-compensate",
> +					       index, &phy_parameter->disconnection_compensate))
> +			phy_parameter->disconnection_compensate = 0;
> +
> +		get_phy_data_by_efuse(rtk_phy, phy_parameter, index);
> +
> +		update_dc_driving_level(rtk_phy, phy_parameter);
> +
> +		update_hs_clk_select(rtk_phy, phy_parameter);
> +	}
> +
> +	return ret;
> +}
> +
> +static int rtk_usb2phy_probe(struct platform_device *pdev)
> +{
> +	struct rtk_phy *rtk_phy;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node;
> +	struct phy *generic_phy;
> +	struct phy_provider *phy_provider;
> +	const struct phy_cfg *phy_cfg;
> +	int ret = 0;
> +
> +	phy_cfg = of_device_get_match_data(dev);
> +	if (!phy_cfg) {
> +		dev_err(dev, "phy config are not assigned!\n");
> +		return -EINVAL;
> +	}
> +
> +	rtk_phy = devm_kzalloc(dev, sizeof(*rtk_phy), GFP_KERNEL);
> +	if (!rtk_phy)
> +		return -ENOMEM;
> +
> +	rtk_phy->dev			= &pdev->dev;
> +	rtk_phy->phy.dev		= rtk_phy->dev;
> +	rtk_phy->phy.label		= "rtk-usb2phy";
> +	rtk_phy->phy.notify_port_status = rtk_phy_notify_port_status;
> +
> +	rtk_phy->phy_cfg = devm_kzalloc(dev, sizeof(*phy_cfg), GFP_KERNEL);
> +
> +	memcpy(rtk_phy->phy_cfg, phy_cfg, sizeof(*phy_cfg));
> +
> +	node = dev->of_node;

Drop it. Useless assignment.

> +
> +	if (of_device_is_compatible(node, "realtek,rtd1395-usb2phy-2port"))

No, customize variant with driver_data. Don't embed compatibles in the code.


> +		rtk_phy->num_phy = 2;
> +	else
> +		rtk_phy->num_phy = 1;
> +
> +	ret = parse_phy_data(rtk_phy);
> +	if (ret)
> +		goto err;
> +
> +	platform_set_drvdata(pdev, rtk_phy);
> +
> +	generic_phy = devm_phy_create(rtk_phy->dev, NULL, &ops);
> +	if (IS_ERR(generic_phy))
> +		return PTR_ERR(generic_phy);
> +
> +	phy_set_drvdata(generic_phy, rtk_phy);
> +
> +	phy_provider = devm_of_phy_provider_register(rtk_phy->dev,
> +						     of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider))
> +		return PTR_ERR(phy_provider);
> +
> +	ret = usb_add_phy_dev(&rtk_phy->phy);
> +	if (ret)
> +		goto err;
> +
> +	create_debug_files(rtk_phy);
> +
> +err:
> +	dev_dbg(dev, "Probe RTK USB 2.0 PHY (ret=%d)\n", ret);

NAK. I made it pretty clear last time.


This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

> +
> +	return ret;
> +}
> +
> +static void rtk_usb2phy_remove(struct platform_device *pdev)
> +{
> +	struct rtk_phy *rtk_phy = platform_get_drvdata(pdev);
> +
> +	remove_debug_files(rtk_phy);
> +
> +	usb_remove_phy(&rtk_phy->phy);
> +}

...

> +
> +static const struct of_device_id usbphy_rtk_dt_match[] = {
> +	{ .compatible = "realtek,rtd1295-usb2phy", .data = &rtd1295_phy_cfg },
> +	{ .compatible = "realtek,rtd1312c-usb2phy", .data = &rtd1312c_phy_cfg },
> +	{ .compatible = "realtek,rtd1315e-usb2phy", .data = &rtd1315e_phy_cfg },
> +	{ .compatible = "realtek,rtd1319-usb2phy", .data = &rtd1319_phy_cfg },
> +	{ .compatible = "realtek,rtd1319d-usb2phy", .data = &rtd1319d_phy_cfg },
> +	{ .compatible = "realtek,rtd1395-usb2phy", .data = &rtd1395_phy_cfg },
> +	{ .compatible = "realtek,rtd1395-usb2phy-2port", .data = &rtd1395_phy_cfg },
> +	{ .compatible = "realtek,rtd1619-usb2phy", .data = &rtd1619_phy_cfg },
> +	{ .compatible = "realtek,rtd1619b-usb2phy", .data = &rtd1619b_phy_cfg },
> +	{ .compatible = "realtek,usb2phy", .data = &rtk_phy_cfg },

This is now even more suprising. Drop "realtek,usb2phy"

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, usbphy_rtk_dt_match);
> +
> +static struct platform_driver rtk_usb2phy_driver = {
> +	.probe		= rtk_usb2phy_probe,
> +	.remove_new	= rtk_usb2phy_remove,
> +	.driver		= {
> +		.name	= "rtk-usb2phy",
> +		.owner	= THIS_MODULE,

??? Didn't you base your driver on some really, really ancient code
(like 5 years old)? If so, please don't.

Run coccicenelle/coccicheck, smatch and sparse, to avoid common mistakes.

Best regards,
Krzysztof

Powered by blists - more mailing lists