Hi Catalin, Thank you for the patch. It looks good to me. > On 06/10/2024 11:04, Sherry Sun wrote: > > [收到此邮件的某些人通常不会收到来自 sherry.sun@xxxxxxx > 的电子邮件。请访问 > > > https://aka.ms/LearnAboutSenderIdentification，以了解为什么这一点很重; > 要。] > > > > This email is not from Hexagon’s Office 365 instance. Please be careful while > clicking links, opening attachments, or replying to this email. > > > > > >> -----Original Message----- > >> From: Catalin Popescu <catalin.popescu@xxxxxxxxxxxxxxxxxxxx> > >> Sent: Friday, October 4, 2024 7:36 PM > >> To: Amitkumar Karwar <amitkumar.karwar@xxxxxxx>; Neeraj Sanjay Kale > >> <neeraj.sanjaykale@xxxxxxx>; marcel@xxxxxxxxxxxx; > >> luiz.dentz@xxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; > >> conor+dt@xxxxxxxxxx; p.zabel@xxxxxxxxxxxxxx > >> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > >> linux- kernel@xxxxxxxxxxxxxxx; m.felsch@xxxxxxxxxxxxxx; bsp- > >> development.geo@xxxxxxxxxxxxxxxxxxxx; Catalin Popescu > >> <catalin.popescu@xxxxxxxxxxxxxxxxxxxx> > >> Subject: [PATCH 2/2] Bluetooth: btnxpuart: implement powerup sequence. > >> > >> NXP bluetooth chip shares power supply and reset gpio with a WLAN chip. > >> Add support for power supply and reset and enforce powerup > >> sequence: > >> 1. apply power supply > >> 2. deassert reset/powerdown > > Hi Catalin, > > > > For NXP WIFI/BT chip, WIFI and BT share the one PDn pin, which means that > both wifi and BT controller will be powered on and off at the same time. So > you control this pin in the BT driver probe and remove function will directly > break the wifi function I think. > > Hi Sherry, > > I know and that's why I'm using reset control and not gpio... Reset control are > refcounted, gpios are not. So, basically we can use one reset control shared by > bluetooth and wlan drivers, each driver can operate independently from > other as long as they implement the powerup sequence (shared supply and > reset pin) and download their specific firmware (combo firmware not used). > As long as one driver is functional, the chip supply and reset is not asserted. > So, you can look at the drivers as users of shared resources (supply and > reset). > > > > >> Signed-off-by: Catalin Popescu <catalin.popescu@xxxxxxxxxxxxxxxxxxxx> > >> --- > >> drivers/bluetooth/btnxpuart.c | 18 ++++++++++++++++++ > >> 1 file changed, 18 insertions(+) > >> > >> diff --git a/drivers/bluetooth/btnxpuart.c > >> b/drivers/bluetooth/btnxpuart.c index 4f493be763b8..e58e7ac7999f > >> 100644 > >> --- a/drivers/bluetooth/btnxpuart.c > >> +++ b/drivers/bluetooth/btnxpuart.c > >> @@ -16,6 +16,8 @@ > >> #include <linux/crc8.h> > >> #include <linux/crc32.h> > >> #include <linux/string_helpers.h> > >> +#include <linux/regulator/consumer.h> #include <linux/reset.h> > >> > >> #include <net/bluetooth/bluetooth.h> > >> #include <net/bluetooth/hci_core.h> @@ -188,6 +190,7 @@ struct > >> btnxpuart_dev { > >> > >> struct ps_data psdata; > >> struct btnxpuart_data *nxp_data; > >> + struct reset_control *pdn; > >> }; > >> > >> #define NXP_V1_FW_REQ_PKT 0xa5 > >> @@ -1458,6 +1461,7 @@ static int nxp_serdev_probe(struct > >> serdev_device > >> *serdev) { > >> struct hci_dev *hdev; > >> struct btnxpuart_dev *nxpdev; > >> + int err; > >> > >> nxpdev = devm_kzalloc(&serdev->dev, sizeof(*nxpdev), GFP_KERNEL); > >> if (!nxpdev) > >> @@ -1485,6 +1489,16 @@ static int nxp_serdev_probe(struct > >> serdev_device > >> *serdev) > >> > >> crc8_populate_msb(crc8_table, POLYNOMIAL8); > >> > >> + nxpdev->pdn = devm_reset_control_get_optional_shared(&serdev- > >>> dev, NULL); > >> + if (IS_ERR(nxpdev->pdn)) > >> + return PTR_ERR(nxpdev->pdn); > >> + > >> + err = devm_regulator_get_enable(&serdev->dev, "vcc"); > >> + if (err) { > >> + dev_err(&serdev->dev, "Failed to enable vcc regulator\n"); > >> + return err; > >> + } > >> + > > Now in NXP local repo, the PDn pin has been controlled by the > corresponding wifi PCIe/SDIO controller, so we won't add the vcc-supply and > reset-gpios properties for the BT driver, seems the code here will return err if > no vcc-supply and reset-gpios properties provided, which will break the BT > driver probe. > If you look closely, the gpio is optional and the supply will return a dummy if > none is declared in the DT. So, this change won't break anything. > > Best Regards > > Sherry > > > >> /* Initialize and register HCI device */ > >> hdev = hci_alloc_dev(); > >> if (!hdev) { > >> @@ -1492,6 +1506,8 @@ static int nxp_serdev_probe(struct > >> serdev_device > >> *serdev) > >> return -ENOMEM; > >> } > >> > >> + reset_control_deassert(nxpdev->pdn); > >> + > >> nxpdev->hdev = hdev; > >> > >> hdev->bus = HCI_UART; > >> @@ -1509,6 +1525,7 @@ static int nxp_serdev_probe(struct > >> serdev_device > >> *serdev) > >> > >> if (hci_register_dev(hdev) < 0) { > >> dev_err(&serdev->dev, "Can't register HCI device\n"); > >> + reset_control_assert(nxpdev->pdn); > >> hci_free_dev(hdev); > >> return -ENODEV; > >> } > >> @@ -1540,6 +1557,7 @@ static void nxp_serdev_remove(struct > >> serdev_device *serdev) > >> } > >> ps_cleanup(nxpdev); > >> hci_unregister_dev(hdev); > >> + reset_control_assert(nxpdev->pdn); > >> hci_free_dev(hdev); > >> } > >> > >> -- > >> 2.34.1 > >> Reviewed-by: Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx> Thanks, Neeraj
