usb5744 supports SMBus Configuration and it may be configured via the
SMBus slave interface during the hub’s start-up configuration stage.
To program it introduce i2c initialization hook and set usb5744 platform
data with function having required smbus initialization sequence. Core
driver uses i2c-bus phandle (added in commit '02be19e914b8 dt-bindings:
usb: Add support for Microchip usb5744 hub controller') to get i2c client
device and then calls usb5744 i2c default initialization sequence.
Apart from the USB command attach, prevent the hub from suspend.
when the “USB Attach with SMBus (0xAA56)” command is issued to the hub,
the hub is getting enumerated and then it puts in a suspend mode.
This causes the hub to NAK any SMBus access made by the SMBus Master
during this period and not able to see the hub's slave address while
running the "i2c probe" command.
Prevent the MCU from the putting the HUB in suspend mode through
register write. The BYPASS_UDC_SUSPEND bit (Bit 3) of the RuntimeFlags2
register at address 0x411D controls this aspect of the hub. The
BYPASS_UDC_SUSPEND bit in register 0x411Dh must be set to ensure that the
MCU is always enabled and ready to respond to SMBus runtime commands.
This register needs to be written before the USB attach command is issued.
The byte sequence is as follows:
Slave addr: 0x2d 00 00 05 00 01 41 1D 08
Slave addr: 0x2d 99 37 00
Slave addr: 0x2d AA 56 00
In addition to SMBus programming sequence also update post reset
delay as without it there is a failure on first SMBus write.
i2c 2-002d: error -ENXIO: BYPASS_UDC_SUSPEND bit configuration failed
Signed-off-by: Radhey Shyam Pandey <[email protected]>
---
---
drivers/usb/misc/onboard_usb_dev.c | 46 ++++++++++++++++++++++++++++++
drivers/usb/misc/onboard_usb_dev.h | 8 +++++-
2 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
index f2bcc1a8b95f..5621c1273a12 100644
--- a/drivers/usb/misc/onboard_usb_dev.c
+++ b/drivers/usb/misc/onboard_usb_dev.c
@@ -98,6 +98,7 @@ static int onboard_dev_power_on(struct onboard_dev *onboard_dev)
fsleep(onboard_dev->pdata->reset_us);
gpiod_set_value_cansleep(onboard_dev->reset_gpio, 0);
+ fsleep(onboard_dev->pdata->reset_us);
onboard_dev->is_powered_on = true;
@@ -296,10 +297,34 @@ static void onboard_dev_attach_usb_driver(struct work_struct *work)
pr_err("Failed to attach USB driver: %pe\n", ERR_PTR(err));
}
+int onboard_dev_5744_i2c_init(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ int ret;
+
+ char wr_buf[7] = {0x00, 0x05, 0x00, 0x01, 0x41, 0x1D, 0x08};
+
+ ret = i2c_smbus_write_block_data(client, 0, sizeof(wr_buf), wr_buf);
+ if (ret)
+ return dev_err_probe(dev, ret, "BYPASS_UDC_SUSPEND bit configuration failed\n");
+
+ ret = i2c_smbus_write_word_data(client, 0x99, htons(0x3700));
+ if (ret)
+ return dev_err_probe(dev, ret, "Configuration Register Access Command failed\n");
+
+ /* Send SMBus command to boot hub. */
+ ret = i2c_smbus_write_word_data(client, 0xAA, htons(0x5600));
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "USB Attach with SMBus command failed\n");
+
+ return ret;
+}
+
static int onboard_dev_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct onboard_dev *onboard_dev;
+ struct device_node *i2c_node;
int err;
onboard_dev = devm_kzalloc(dev, sizeof(*onboard_dev), GFP_KERNEL);
@@ -339,6 +364,23 @@ static int onboard_dev_probe(struct platform_device *pdev)
if (err)
return err;
+ i2c_node = of_parse_phandle(pdev->dev.of_node, "i2c-bus", 0);
+ if (i2c_node) {
+ struct i2c_client *client;
+
+ client = of_find_i2c_device_by_node(i2c_node);
+ of_node_put(i2c_node);
+
+ if (!client) {
+ err = -EPROBE_DEFER;
+ goto err_dev_power_off;
+ }
+ err = onboard_dev->pdata->onboard_dev_i2c_init(client);
+ put_device(&client->dev);
+ if (err < 0)
+ goto err_dev_power_off;
+ }
+
/*
* The USB driver might have been detached from the USB devices by
* onboard_dev_remove() (e.g. through an 'unbind' by userspace),
@@ -350,6 +392,10 @@ static int onboard_dev_probe(struct platform_device *pdev)
schedule_work(&attach_usb_driver_work);
return 0;
+
+err_dev_power_off:
+ onboard_dev_power_off(onboard_dev);
+ return err;
}
static void onboard_dev_remove(struct platform_device *pdev)
diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h
index fbba549c0f47..17311ea7bacd 100644
--- a/drivers/usb/misc/onboard_usb_dev.h
+++ b/drivers/usb/misc/onboard_usb_dev.h
@@ -6,6 +6,8 @@
#ifndef _USB_MISC_ONBOARD_USB_DEV_H
#define _USB_MISC_ONBOARD_USB_DEV_H
+#include <linux/i2c.h>
+
#define MAX_SUPPLIES 2
struct onboard_dev_pdata {
@@ -13,6 +15,7 @@ struct onboard_dev_pdata {
unsigned int num_supplies; /* number of supplies */
const char * const supply_names[MAX_SUPPLIES];
bool is_hub;
+ int (*onboard_dev_i2c_init)(struct i2c_client *client);
};
static const struct onboard_dev_pdata microchip_usb424_data = {
@@ -22,11 +25,14 @@ static const struct onboard_dev_pdata microchip_usb424_data = {
.is_hub = true,
};
+int onboard_dev_5744_i2c_init(struct i2c_client *client);
+
static const struct onboard_dev_pdata microchip_usb5744_data = {
- .reset_us = 0,
+ .reset_us = 10000,
.num_supplies = 2,
.supply_names = { "vdd", "vdd2" },
.is_hub = true,
+ .onboard_dev_i2c_init = onboard_dev_5744_i2c_init,
};
static const struct onboard_dev_pdata realtek_rts5411_data = {
--
2.34.1
On Thu, Jun 06, 2024 at 05:58:03PM +0530, Radhey Shyam Pandey wrote:
> PATCH] usb: misc: add Microchip usb5744 SMBus programming support
usb: misc: onboard_usb_dev: ...
>
> usb5744 supports SMBus Configuration and it may be configured via the
> SMBus slave interface during the hub’s start-up configuration stage.
>
> To program it introduce i2c initialization hook and set usb5744 platform
> data with function having required smbus initialization sequence. Core
> driver uses i2c-bus phandle (added in commit '02be19e914b8 dt-bindings:
> usb: Add support for Microchip usb5744 hub controller') to get i2c client
> device and then calls usb5744 i2c default initialization sequence.
>
> Apart from the USB command attach, prevent the hub from suspend.
> when the “USB Attach with SMBus (0xAA56)” command is issued to the hub,
> the hub is getting enumerated and then it puts in a suspend mode.
> This causes the hub to NAK any SMBus access made by the SMBus Master
> during this period and not able to see the hub's slave address while
> running the "i2c probe" command.
>
> Prevent the MCU from the putting the HUB in suspend mode through
> register write. The BYPASS_UDC_SUSPEND bit (Bit 3) of the RuntimeFlags2
> register at address 0x411D controls this aspect of the hub. The
> BYPASS_UDC_SUSPEND bit in register 0x411Dh must be set to ensure that the
> MCU is always enabled and ready to respond to SMBus runtime commands.
> This register needs to be written before the USB attach command is issued.
>
> The byte sequence is as follows:
> Slave addr: 0x2d 00 00 05 00 01 41 1D 08
> Slave addr: 0x2d 99 37 00
> Slave addr: 0x2d AA 56 00
>
> In addition to SMBus programming sequence also update post reset
> delay as without it there is a failure on first SMBus write.
> i2c 2-002d: error -ENXIO: BYPASS_UDC_SUSPEND bit configuration failed
>
> Signed-off-by: Radhey Shyam Pandey <[email protected]>
> ---
> ---
> drivers/usb/misc/onboard_usb_dev.c | 46 ++++++++++++++++++++++++++++++
> drivers/usb/misc/onboard_usb_dev.h | 8 +++++-
> 2 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> index f2bcc1a8b95f..5621c1273a12 100644
> --- a/drivers/usb/misc/onboard_usb_dev.c
> +++ b/drivers/usb/misc/onboard_usb_dev.c
> @@ -98,6 +98,7 @@ static int onboard_dev_power_on(struct onboard_dev *onboard_dev)
>
> fsleep(onboard_dev->pdata->reset_us);
> gpiod_set_value_cansleep(onboard_dev->reset_gpio, 0);
> + fsleep(onboard_dev->pdata->reset_us);
This also impacts devices that don't require a delay, plus requirements for
this delay are not necessarily the same as the reset delay.
Better add a dedicated field like 'power_on_delay_us'.
>
> onboard_dev->is_powered_on = true;
>
> @@ -296,10 +297,34 @@ static void onboard_dev_attach_usb_driver(struct work_struct *work)
> pr_err("Failed to attach USB driver: %pe\n", ERR_PTR(err));
> }
>
> +int onboard_dev_5744_i2c_init(struct i2c_client *client)
static int
We probably want to move hardware specific code to a dedicated file
as there is added more, but I for now it's ok to have it in the main
driver.
> +{
> + struct device *dev = &client->dev;
> + int ret;
> +
> + char wr_buf[7] = {0x00, 0x05, 0x00, 0x01, 0x41, 0x1D, 0x08};
Please use constants for the different bits instead of magic values. I know
the magic values are explained in the commit message, but that's something
people have to dig up.
> +
> + ret = i2c_smbus_write_block_data(client, 0, sizeof(wr_buf), wr_buf);
> + if (ret)
> + return dev_err_probe(dev, ret, "BYPASS_UDC_SUSPEND bit configuration failed\n");
> +
> + ret = i2c_smbus_write_word_data(client, 0x99, htons(0x3700));
ditto, no magic values please
> + if (ret)
> + return dev_err_probe(dev, ret, "Configuration Register Access Command failed\n");
> +
> + /* Send SMBus command to boot hub. */
> + ret = i2c_smbus_write_word_data(client, 0xAA, htons(0x5600));
ditto
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "USB Attach with SMBus command failed\n");
> +
> + return ret;
return 0;
> +}
> +
> static int onboard_dev_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct onboard_dev *onboard_dev;
> + struct device_node *i2c_node;
> int err;
>
> onboard_dev = devm_kzalloc(dev, sizeof(*onboard_dev), GFP_KERNEL);
> @@ -339,6 +364,23 @@ static int onboard_dev_probe(struct platform_device *pdev)
> if (err)
> return err;
>
> + i2c_node = of_parse_phandle(pdev->dev.of_node, "i2c-bus", 0);
> + if (i2c_node) {
> + struct i2c_client *client;
> +
> + client = of_find_i2c_device_by_node(i2c_node);
> + of_node_put(i2c_node);
> +
> + if (!client) {
> + err = -EPROBE_DEFER;
> + goto err_dev_power_off;
nit: err_power_off
> + }
> + err = onboard_dev->pdata->onboard_dev_i2c_init(client);
> + put_device(&client->dev);
> + if (err < 0)
> + goto err_dev_power_off;
> + }
> +
> /*
> * The USB driver might have been detached from the USB devices by
> * onboard_dev_remove() (e.g. through an 'unbind' by userspace),
> @@ -350,6 +392,10 @@ static int onboard_dev_probe(struct platform_device *pdev)
> schedule_work(&attach_usb_driver_work);
>
> return 0;
> +
> +err_dev_power_off:
> + onboard_dev_power_off(onboard_dev);
> + return err;
> }
>
> static void onboard_dev_remove(struct platform_device *pdev)
> diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h
> index fbba549c0f47..17311ea7bacd 100644
> --- a/drivers/usb/misc/onboard_usb_dev.h
> +++ b/drivers/usb/misc/onboard_usb_dev.h
> @@ -6,6 +6,8 @@
> #ifndef _USB_MISC_ONBOARD_USB_DEV_H
> #define _USB_MISC_ONBOARD_USB_DEV_H
>
> +#include <linux/i2c.h>
> +
> #define MAX_SUPPLIES 2
>
> struct onboard_dev_pdata {
> @@ -13,6 +15,7 @@ struct onboard_dev_pdata {
> unsigned int num_supplies; /* number of supplies */
> const char * const supply_names[MAX_SUPPLIES];
> bool is_hub;
> + int (*onboard_dev_i2c_init)(struct i2c_client *client);
> };
>
> static const struct onboard_dev_pdata microchip_usb424_data = {
> @@ -22,11 +25,14 @@ static const struct onboard_dev_pdata microchip_usb424_data = {
> .is_hub = true,
> };
>
> +int onboard_dev_5744_i2c_init(struct i2c_client *client);
> +
> static const struct onboard_dev_pdata microchip_usb5744_data = {
> - .reset_us = 0,
> + .reset_us = 10000,
That's one reason why I don't think it's a good idea to use 'reset_us'
twice. In this case the total delay would go from formerly 0ms to 20ms,
when a delay of 10ms after the reset should be sufficient.
> .num_supplies = 2,
> .supply_names = { "vdd", "vdd2" },
> .is_hub = true,
> + .onboard_dev_i2c_init = onboard_dev_5744_i2c_init,
> };
>
> static const struct onboard_dev_pdata realtek_rts5411_data = {
> --
> 2.34.1
>
Hi Radhey,
kernel test robot noticed the following build errors:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus linus/master v6.10-rc2 next-20240607]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Radhey-Shyam-Pandey/usb-misc-add-Microchip-usb5744-SMBus-programming-support/20240606-203028
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/1717676883-2876611-1-git-send-email-radhey.shyam.pandey%40amd.com
patch subject: [PATCH] usb: misc: add Microchip usb5744 SMBus programming support
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240607/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240607/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> aarch64-linux-ld: drivers/usb/misc/onboard_usb_dev_pdevs.o:(.rodata+0x1498): undefined reference to `onboard_dev_5744_i2c_init'
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Radhey,
kernel test robot noticed the following build errors:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus linus/master v6.10-rc2 next-20240607]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Radhey-Shyam-Pandey/usb-misc-add-Microchip-usb5744-SMBus-programming-support/20240606-203028
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/1717676883-2876611-1-git-send-email-radhey.shyam.pandey%40amd.com
patch subject: [PATCH] usb: misc: add Microchip usb5744 SMBus programming support
config: arm-defconfig (https://download.01.org/0day-ci/archive/20240607/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240607/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> ld.lld: error: undefined symbol: onboard_dev_5744_i2c_init
>>> referenced by onboard_usb_dev_pdevs.c
>>> drivers/usb/misc/onboard_usb_dev_pdevs.o:(microchip_usb5744_data) in archive vmlinux.a
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Radhey,
kernel test robot noticed the following build warnings:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.10-rc2 next-20240607]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Radhey-Shyam-Pandey/usb-misc-add-Microchip-usb5744-SMBus-programming-support/20240606-203028
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/1717676883-2876611-1-git-send-email-radhey.shyam.pandey%40amd.com
patch subject: [PATCH] usb: misc: add Microchip usb5744 SMBus programming support
config: i386-randconfig-063-20240607 (https://download.01.org/0day-ci/archive/20240607/[email protected]/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240607/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
>> drivers/usb/misc/onboard_usb_dev.c:311:55: sparse: sparse: incorrect type in argument 3 (different base types) @@ expected unsigned short [usertype] value @@ got restricted __be16 [usertype] @@
drivers/usb/misc/onboard_usb_dev.c:311:55: sparse: expected unsigned short [usertype] value
drivers/usb/misc/onboard_usb_dev.c:311:55: sparse: got restricted __be16 [usertype]
drivers/usb/misc/onboard_usb_dev.c:316:55: sparse: sparse: incorrect type in argument 3 (different base types) @@ expected unsigned short [usertype] value @@ got restricted __be16 [usertype] @@
drivers/usb/misc/onboard_usb_dev.c:316:55: sparse: expected unsigned short [usertype] value
drivers/usb/misc/onboard_usb_dev.c:316:55: sparse: got restricted __be16 [usertype]
drivers/usb/misc/onboard_usb_dev.c: note: in included file (through include/linux/mmzone.h, include/linux/gfp.h, include/linux/xarray.h, ...):
include/linux/page-flags.h:240:46: sparse: sparse: self-comparison always evaluates to false
include/linux/page-flags.h:240:46: sparse: sparse: self-comparison always evaluates to false
drivers/usb/misc/onboard_usb_dev.c: note: in included file (through include/linux/mutex.h, include/linux/notifier.h, include/linux/clk.h):
include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
vim +311 drivers/usb/misc/onboard_usb_dev.c
299
300 int onboard_dev_5744_i2c_init(struct i2c_client *client)
301 {
302 struct device *dev = &client->dev;
303 int ret;
304
305 char wr_buf[7] = {0x00, 0x05, 0x00, 0x01, 0x41, 0x1D, 0x08};
306
307 ret = i2c_smbus_write_block_data(client, 0, sizeof(wr_buf), wr_buf);
308 if (ret)
309 return dev_err_probe(dev, ret, "BYPASS_UDC_SUSPEND bit configuration failed\n");
310
> 311 ret = i2c_smbus_write_word_data(client, 0x99, htons(0x3700));
312 if (ret)
313 return dev_err_probe(dev, ret, "Configuration Register Access Command failed\n");
314
315 /* Send SMBus command to boot hub. */
316 ret = i2c_smbus_write_word_data(client, 0xAA, htons(0x5600));
317 if (ret < 0)
318 return dev_err_probe(dev, ret, "USB Attach with SMBus command failed\n");
319
320 return ret;
321 }
322
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
> -----Original Message-----
> From: Matthias Kaehlcke <[email protected]>
> Sent: Thursday, June 6, 2024 11:54 PM
> To: Pandey, Radhey Shyam <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Simek, Michal
> <[email protected]>; [email protected]; linux-
> [email protected]; git (AMD-Xilinx) <[email protected]>
> Subject: Re: [PATCH] usb: misc: add Microchip usb5744 SMBus programming
> support
>
> On Thu, Jun 06, 2024 at 05:58:03PM +0530, Radhey Shyam Pandey wrote:
>
> > PATCH] usb: misc: add Microchip usb5744 SMBus programming support
>
> usb: misc: onboard_usb_dev: ...
Sure , will update in next version.
>
> >
> > usb5744 supports SMBus Configuration and it may be configured via the
> > SMBus slave interface during the hub’s start-up configuration stage.
> >
> > To program it introduce i2c initialization hook and set usb5744
> > platform data with function having required smbus initialization
> > sequence. Core driver uses i2c-bus phandle (added in commit
> '02be19e914b8 dt-bindings:
> > usb: Add support for Microchip usb5744 hub controller') to get i2c
> > client device and then calls usb5744 i2c default initialization sequence.
> >
> > Apart from the USB command attach, prevent the hub from suspend.
> > when the “USB Attach with SMBus (0xAA56)” command is issued to the
> > hub, the hub is getting enumerated and then it puts in a suspend mode.
> > This causes the hub to NAK any SMBus access made by the SMBus Master
> > during this period and not able to see the hub's slave address while
> > running the "i2c probe" command.
> >
> > Prevent the MCU from the putting the HUB in suspend mode through
> > register write. The BYPASS_UDC_SUSPEND bit (Bit 3) of the
> > RuntimeFlags2 register at address 0x411D controls this aspect of the
> > hub. The BYPASS_UDC_SUSPEND bit in register 0x411Dh must be set to
> > ensure that the MCU is always enabled and ready to respond to SMBus
> runtime commands.
> > This register needs to be written before the USB attach command is issued.
> >
> > The byte sequence is as follows:
> > Slave addr: 0x2d 00 00 05 00 01 41 1D 08
> > Slave addr: 0x2d 99 37 00
> > Slave addr: 0x2d AA 56 00
> >
> > In addition to SMBus programming sequence also update post reset delay
> > as without it there is a failure on first SMBus write.
> > i2c 2-002d: error -ENXIO: BYPASS_UDC_SUSPEND bit configuration failed
> >
> > Signed-off-by: Radhey Shyam Pandey <[email protected]>
> > ---
> > ---
> > drivers/usb/misc/onboard_usb_dev.c | 46
> > ++++++++++++++++++++++++++++++ drivers/usb/misc/onboard_usb_dev.h
> |
> > 8 +++++-
> > 2 files changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/misc/onboard_usb_dev.c
> > b/drivers/usb/misc/onboard_usb_dev.c
> > index f2bcc1a8b95f..5621c1273a12 100644
> > --- a/drivers/usb/misc/onboard_usb_dev.c
> > +++ b/drivers/usb/misc/onboard_usb_dev.c
> > @@ -98,6 +98,7 @@ static int onboard_dev_power_on(struct
> onboard_dev
> > *onboard_dev)
> >
> > fsleep(onboard_dev->pdata->reset_us);
> > gpiod_set_value_cansleep(onboard_dev->reset_gpio, 0);
> > + fsleep(onboard_dev->pdata->reset_us);
>
> This also impacts devices that don't require a delay, plus requirements for
> this delay are not necessarily the same as the reset delay.
>
> Better add a dedicated field like 'power_on_delay_us'.
It's a nice suggestion. I will create a separate preparatory patch to
introduce 'power_on_delay_us' and then set its value in this usb5744
SMBus programming support patch.
>
> >
> > onboard_dev->is_powered_on = true;
> >
> > @@ -296,10 +297,34 @@ static void
> onboard_dev_attach_usb_driver(struct work_struct *work)
> > pr_err("Failed to attach USB driver: %pe\n", ERR_PTR(err)); }
> >
> > +int onboard_dev_5744_i2c_init(struct i2c_client *client)
>
> static int
>
> We probably want to move hardware specific code to a dedicated file as
> there is added more, but I for now it's ok to have it in the main driver.
To make onboard_dev_5744_i2c_init as static we need to move
microchip_usb5744_data and onboard_dev_match to onboard_usb_dev.c
but then i noticed onboard_dev_match is also being used in
onboard_usb_dev_pdevs.c source file.
>
> > +{
> > + struct device *dev = &client->dev;
> > + int ret;
> > +
> > + char wr_buf[7] = {0x00, 0x05, 0x00, 0x01, 0x41, 0x1D, 0x08};
>
> Please use constants for the different bits instead of magic values. I know the
> magic values are explained in the commit message, but that's something
> people have to dig up.
Sure, will use defines for these values.
>
> > +
> > + ret = i2c_smbus_write_block_data(client, 0, sizeof(wr_buf), wr_buf);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "BYPASS_UDC_SUSPEND bit
> > +configuration failed\n");
> > +
> > + ret = i2c_smbus_write_word_data(client, 0x99, htons(0x3700));
>
> ditto, no magic values please
Yes, will fix it in next version.
>
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Configuration Register
> Access
> > +Command failed\n");
> > +
> > + /* Send SMBus command to boot hub. */
> > + ret = i2c_smbus_write_word_data(client, 0xAA, htons(0x5600));
>
> ditto
Sure, will fix it.
>
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret, "USB Attach with SMBus
> command
> > +failed\n");
> > +
> > + return ret;
>
> return 0;
> > +}
> > +
> > static int onboard_dev_probe(struct platform_device *pdev) {
> > struct device *dev = &pdev->dev;
> > struct onboard_dev *onboard_dev;
> > + struct device_node *i2c_node;
> > int err;
> >
> > onboard_dev = devm_kzalloc(dev, sizeof(*onboard_dev),
> GFP_KERNEL);
> > @@ -339,6 +364,23 @@ static int onboard_dev_probe(struct
> platform_device *pdev)
> > if (err)
> > return err;
> >
> > + i2c_node = of_parse_phandle(pdev->dev.of_node, "i2c-bus", 0);
> > + if (i2c_node) {
> > + struct i2c_client *client;
> > +
> > + client = of_find_i2c_device_by_node(i2c_node);
> > + of_node_put(i2c_node);
> > +
> > + if (!client) {
> > + err = -EPROBE_DEFER;
> > + goto err_dev_power_off;
>
> nit: err_power_off
Sure, will fix it.
>
> > + }
> > + err = onboard_dev->pdata->onboard_dev_i2c_init(client);
> > + put_device(&client->dev);
> > + if (err < 0)
> > + goto err_dev_power_off;
> > + }
> > +
> > /*
> > * The USB driver might have been detached from the USB devices by
> > * onboard_dev_remove() (e.g. through an 'unbind' by userspace),
> @@
> > -350,6 +392,10 @@ static int onboard_dev_probe(struct platform_device
> *pdev)
> > schedule_work(&attach_usb_driver_work);
> >
> > return 0;
> > +
> > +err_dev_power_off:
> > + onboard_dev_power_off(onboard_dev);
> > + return err;
> > }
> >
> > static void onboard_dev_remove(struct platform_device *pdev) diff
> > --git a/drivers/usb/misc/onboard_usb_dev.h
> > b/drivers/usb/misc/onboard_usb_dev.h
> > index fbba549c0f47..17311ea7bacd 100644
> > --- a/drivers/usb/misc/onboard_usb_dev.h
> > +++ b/drivers/usb/misc/onboard_usb_dev.h
> > @@ -6,6 +6,8 @@
> > #ifndef _USB_MISC_ONBOARD_USB_DEV_H
> > #define _USB_MISC_ONBOARD_USB_DEV_H
> >
> > +#include <linux/i2c.h>
> > +
> > #define MAX_SUPPLIES 2
> >
> > struct onboard_dev_pdata {
> > @@ -13,6 +15,7 @@ struct onboard_dev_pdata {
> > unsigned int num_supplies; /* number of supplies */
> > const char * const supply_names[MAX_SUPPLIES];
> > bool is_hub;
> > + int (*onboard_dev_i2c_init)(struct i2c_client *client);
> > };
> >
> > static const struct onboard_dev_pdata microchip_usb424_data = { @@
> > -22,11 +25,14 @@ static const struct onboard_dev_pdata
> microchip_usb424_data = {
> > .is_hub = true,
> > };
> >
> > +int onboard_dev_5744_i2c_init(struct i2c_client *client);
> > +
> > static const struct onboard_dev_pdata microchip_usb5744_data = {
> > - .reset_us = 0,
> > + .reset_us = 10000,
>
> That's one reason why I don't think it's a good idea to use 'reset_us'
> twice. In this case the total delay would go from formerly 0ms to 20ms, when
> a delay of 10ms after the reset should be sufficient.
Agree , will introduce 'power_on_delay_us' and it should address it.
>
> > .num_supplies = 2,
> > .supply_names = { "vdd", "vdd2" },
> > .is_hub = true,
> > + .onboard_dev_i2c_init = onboard_dev_5744_i2c_init,
> > };
> >
> > static const struct onboard_dev_pdata realtek_rts5411_data = {
> > --
> > 2.34.1
> >