* [RFC PATCH v1 0/2] fw_devlink overlay fix
From: Saravana Kannan @ 2024-04-08 23:13 UTC (permalink / raw)
To: Herve Codina, Geert Uytterhoeven
Cc: Saravana Kannan, Rob Herring, kernel-team, linux-kernel, imx,
linux-arm-kernel, linux-i2c, devicetree, linux-spi, linux-acpi
Don't bother review this patch. It needs to be refactored better once I
know it works.
Geert and Herve,
This patch serious should hopefully fix both of your use cases. Can you
please check to make sure the device links created to/from the overlay
devices are to/from the right ones?
I've only compile tested it. If I made some obvious mistake, feel free
to fix it and give it a shot.
Cc: Rob Herring <robh@kernel.org>
Thanks,
Saravana
Saravana Kannan (2):
Revert "treewide: Fix probing of devices in DT overlays"
of: dynamic: Fix probing of overlay devices
drivers/base/core.c | 10 ++++++++++
drivers/bus/imx-weim.c | 6 ------
drivers/i2c/i2c-core-of.c | 5 -----
drivers/of/dynamic.c | 9 ++++++++-
drivers/of/platform.c | 5 -----
drivers/spi/spi.c | 5 -----
include/linux/fwnode.h | 2 ++
7 files changed, 20 insertions(+), 22 deletions(-)
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply
* [RFC PATCH v1 1/2] Revert "treewide: Fix probing of devices in DT overlays"
From: Saravana Kannan @ 2024-04-08 23:13 UTC (permalink / raw)
To: Herve Codina, Geert Uytterhoeven
Cc: Saravana Kannan, Rob Herring, kernel-team, linux-kernel, imx,
linux-arm-kernel, linux-i2c, devicetree, linux-spi, linux-acpi
In-Reply-To: <20240408231310.325451-1-saravanak@google.com>
This reverts commit 1a50d9403fb90cbe4dea0ec9fd0351d2ecbd8924.
---
drivers/bus/imx-weim.c | 6 ------
drivers/i2c/i2c-core-of.c | 5 -----
drivers/of/dynamic.c | 1 -
drivers/of/platform.c | 5 -----
drivers/spi/spi.c | 5 -----
5 files changed, 22 deletions(-)
diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 837bf9d51c6e..caaf887e0ccc 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -331,12 +331,6 @@ static int of_weim_notify(struct notifier_block *nb, unsigned long action,
"Failed to setup timing for '%pOF'\n", rd->dn);
if (!of_node_check_flag(rd->dn, OF_POPULATED)) {
- /*
- * Clear the flag before adding the device so that
- * fw_devlink doesn't skip adding consumers to this
- * device.
- */
- rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
if (!of_platform_device_create(rd->dn, NULL, &pdev->dev)) {
dev_err(&pdev->dev,
"Failed to create child device '%pOF'\n",
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index a6c407d36800..a250921bbce0 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -178,11 +178,6 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
return NOTIFY_OK;
}
- /*
- * Clear the flag before adding the device so that fw_devlink
- * doesn't skip adding consumers to this device.
- */
- rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
client = of_i2c_register_device(adap, rd->dn);
if (IS_ERR(client)) {
dev_err(&adap->dev, "failed to create client for '%pOF'\n",
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 4d57a4e34105..19a1a38554f2 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -224,7 +224,6 @@ static void __of_attach_node(struct device_node *np)
np->sibling = np->parent->child;
np->parent->child = np;
of_node_clear_flag(np, OF_DETACHED);
- np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
raw_spin_unlock_irqrestore(&devtree_lock, flags);
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 389d4ea6bfc1..efd861fa254f 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -743,11 +743,6 @@ static int of_platform_notify(struct notifier_block *nb,
if (of_node_check_flag(rd->dn, OF_POPULATED))
return NOTIFY_OK;
- /*
- * Clear the flag before adding the device so that fw_devlink
- * doesn't skip adding consumers to this device.
- */
- rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
/* pdev_parent may be NULL when no bus platform device */
pdev_parent = of_find_device_by_node(rd->dn->parent);
pdev = of_platform_device_create(rd->dn, NULL,
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index ff75838c1b5d..17cd417f7681 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4761,11 +4761,6 @@ static int of_spi_notify(struct notifier_block *nb, unsigned long action,
return NOTIFY_OK;
}
- /*
- * Clear the flag before adding the device so that fw_devlink
- * doesn't skip adding consumers to this device.
- */
- rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
spi = of_register_spi_device(ctlr, rd->dn);
put_device(&ctlr->dev);
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related
* [RFC PATCH v1 2/2] of: dynamic: Fix probing of overlay devices
From: Saravana Kannan @ 2024-04-08 23:13 UTC (permalink / raw)
To: Herve Codina, Geert Uytterhoeven
Cc: Saravana Kannan, Rob Herring, kernel-team, linux-kernel, imx,
linux-arm-kernel, linux-i2c, devicetree, linux-spi, linux-acpi
In-Reply-To: <20240408231310.325451-1-saravanak@google.com>
Get fw_devlink to work well with overlay devices.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/base/core.c | 10 ++++++++++
drivers/of/dynamic.c | 8 ++++++++
include/linux/fwnode.h | 2 ++
3 files changed, 20 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5f4e03336e68..d856f9c5d601 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -237,6 +237,16 @@ static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,
__fw_devlink_pickup_dangling_consumers(child, new_sup);
}
+
+void fw_devlink_pickup_dangling_consumers(struct fwnode_handle *child,
+ struct fwnode_handle *parent)
+{
+ mutex_lock(&fwnode_link_lock);
+ __fw_devlink_pickup_dangling_consumers(child, parent);
+ __fw_devlink_link_to_consumers(parent->dev);
+ mutex_unlock(&fwnode_link_lock);
+}
+
static DEFINE_MUTEX(device_links_lock);
DEFINE_STATIC_SRCU(device_links_srcu);
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 19a1a38554f2..0a936f46820e 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -237,6 +237,7 @@ static void __of_attach_node(struct device_node *np)
int of_attach_node(struct device_node *np)
{
struct of_reconfig_data rd;
+ struct fwnode_handle *fwnode, *parent;
memset(&rd, 0, sizeof(rd));
rd.dn = np;
@@ -246,6 +247,13 @@ int of_attach_node(struct device_node *np)
mutex_unlock(&of_mutex);
of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, &rd);
+ fwnode = of_fwnode_handle(np);
+ fwnode_for_each_parent_node(fwnode, parent)
+ if (parent->dev) {
+ fw_devlink_pickup_dangling_consumers(fwnode, parent);
+ fwnode_handle_put(parent);
+ break;
+ }
return 0;
}
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 0d79070c5a70..4b3f697a90e8 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -220,6 +220,8 @@ int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup,
u8 flags);
void fwnode_links_purge(struct fwnode_handle *fwnode);
void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode);
+void fw_devlink_pickup_dangling_consumers(struct fwnode_handle *child,
+ struct fwnode_handle *parent);
bool fw_devlink_is_strict(void);
#endif
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related
* Re: [PATCH 2/2] of: property: fw_devlink: Fix links to supplier when created from phandles
From: Saravana Kannan @ 2024-04-08 23:17 UTC (permalink / raw)
To: Herve Codina
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Rob Herring, Frank Rowand,
Shawn Guo, Wolfram Sang, Mark Brown, Geert Uytterhoeven,
Rob Herring, linux-kernel, devicetree, Luca Ceresoli,
Thomas Petazzoni, stable, Android Kernel Team
In-Reply-To: <20240408164021.6f13bf66@bootlin.com>
On Mon, Apr 8, 2024 at 7:40 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi Sarava,
>
> On Fri, 22 Mar 2024 19:00:03 -0700
> Saravana Kannan <saravanak@google.com> wrote:
>
> > On Thu, Mar 21, 2024 at 4:59 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > >
> > > Hi Saravana,
> > >
> > > On Mon, 4 Mar 2024 23:14:13 -0800
> > > Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > ...
> > > >
> > > > Thanks for the example. Let me think about this a bit on how we could
> > > > fix this and get back to you.
> > > >
> > > > Please do ping me if I don't get back in a week or two.
> > > >
> > >
> > > This is my ping.
> > > Do you move forward ?
> >
> > Thanks for the ping. I thought about it a bit. I think the right fix
> > it to undo the overlay fix I had suggested to Geert and then make the
> > overlay code call __fw_devlink_pickup_dangling_consumers() on the
> > parent device of the top level overlay nodes that get added that don't
> > have a device created for them.
> >
> > I'll try to wrap up a patch for this on Monday. But if you want to
> > take a shot at this, that's ok too.
> >
>
> I didn't see anything on this topic. Maybe I missed the related modifications.
> Did you move forward on that patch ?
Give this a shot and let me know please.
https://lore.kernel.org/lkml/20240408231310.325451-1-saravanak@google.com/T/#m40e641cb2b1c0cf5ad1af1021f2daca63faeb427
-Saravana
^ permalink raw reply
* RE: [PATCH v2 3/6] firmware: arm_scmi: add initial support for i.MX BBM protocol
From: Peng Fan @ 2024-04-08 23:35 UTC (permalink / raw)
To: Cristian Marussi, Peng Fan (OSS)
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Sudeep Holla, devicetree@vger.kernel.org, imx@lists.linux.dev,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <ZhQxu8LVNKf5QDLm@pluto>
> Subject: Re: [PATCH v2 3/6] firmware: arm_scmi: add initial support for i.MX
> BBM protocol
>
> On Fri, Apr 05, 2024 at 08:39:25PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The i.MX BBM protocol is for managing i.MX BBM module which provides
> > RTC and BUTTON feature.
> >
.....
> > +#include "notify.h"
> > +
> > +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x10000
> > +
>
> I appreciate that you added versioning but I think a bit of documentation
> about what the protocol and its comamnds purpose is still lacking, as asked
> by Sudeep previously
Sorry for missing the previous comment. Will add some comments in the file.
>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%
> 2Flore.kernel.org%2Flinux-arm-
> kernel%2FZeGtoJ7ztSe8Kg8R%40bogus%2F%23t&data=05%7C02%7Cpeng.fa
> n%40nxp.com%7C37b12c01b51f4329e9e308dc57f66153%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C638481962901820964%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> WwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=d2XRugSYyiFuUnE5R2Oz6p
> xmXBaPC9lZ%2Bb%2FcMBuXeKo%3D&reserved=0
>
> > +enum scmi_imx_bbm_protocol_cmd {
> > + IMX_BBM_GPR_SET = 0x3,
....
> > + cfg->flags = 0;
> > + cfg->value_low = lower_32_bits(sec);
> > + cfg->value_high = upper_32_bits(sec);
>
> Sorry I may have not been clear on this, but when I mentioned lower/upper
> helpers I did not mean that they solved ALSO the endianity problem, so I
> suppose that after having chunked your 64bits value in 2, you still want to
> transmit it as 2 LE quantity....this is generally the expectation for SCMI
> payloads...in this case any available documentation about the expected
> command layout would have helped...
Got it , will fix in v3.
>
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > +
> > + ph->xops->xfer_put(ph, t);
> > +
> > + return ret;
> > +}
> > +
> > +static int scmi_imx_bbm_rtc_time_get(const struct scmi_protocol_handle
> *ph,
> > + u32 rtc_id, u64 *value)
> > +{
> > + struct scmi_imx_bbm_info *pi = ph->get_priv(ph);
> > + struct scmi_imx_bbm_get_time *cfg;
> > + struct scmi_xfer *t;
> > + int ret;
> > +
> > + if (rtc_id >= pi->nr_rtc)
> > + return -EINVAL;
> > +
> > + ret = ph->xops->xfer_get_init(ph, IMX_BBM_RTC_TIME_GET,
> sizeof(*cfg),
> > + sizeof(u64), &t);
> > + if (ret)
> > + return ret;
> > +
> > + cfg = t->tx.buf;
> > + cfg->id = cpu_to_le32(rtc_id);
> > + cfg->flags = 0;
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret)
> > + *value = get_unaligned_le64(t->rx.buf);
> > +
> > + ph->xops->xfer_put(ph, t);
> > +
> > + return ret;
> > +}
> > +
> > +static int scmi_imx_bbm_rtc_alarm_set(const struct scmi_protocol_handle
> *ph,
> > + u32 rtc_id, u64 sec)
> > +{
> > + struct scmi_imx_bbm_info *pi = ph->get_priv(ph);
> > + struct scmi_imx_bbm_alarm_time *cfg;
> > + struct scmi_xfer *t;
> > + int ret;
> > +
> > + if (rtc_id >= pi->nr_rtc)
> > + return -EINVAL;
> > +
> > + ret = ph->xops->xfer_get_init(ph, IMX_BBM_RTC_ALARM_SET,
> sizeof(*cfg), 0, &t);
> > + if (ret)
> > + return ret;
> > +
> > + cfg = t->tx.buf;
> > + cfg->id = cpu_to_le32(rtc_id);
> > + cfg->flags = SCMI_IMX_BBM_RTC_ALARM_ENABLE_FLAG;
> > + cfg->value_low = lower_32_bits(sec);
> > + cfg->value_high = upper_32_bits(sec);
>
> Same.
Fix in V3.
Thanks,
Peng
>
> Thanks,
> Cristian
^ permalink raw reply
* RE: [PATCH v7 0/7] Coresight for Kernel panic and watchdog reset
From: Linu Cherian @ 2024-04-09 0:10 UTC (permalink / raw)
To: Linu Cherian, suzuki.poulose@arm.com, mike.leach@linaro.org,
james.clark@arm.com, leo.yan@linaro.org
Cc: linux-arm-kernel@lists.infradead.org, coresight@lists.linaro.org,
linux-kernel@vger.kernel.org, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
devicetree@vger.kernel.org, Sunil Kovvuri Goutham, George Cherian
In-Reply-To: <20240307033625.325058-1-lcherian@marvell.com>
Hi Suzuki/James,
> -----Original Message-----
> From: Linu Cherian <lcherian@marvell.com>
> Sent: Thursday, March 7, 2024 9:06 AM
> To: suzuki.poulose@arm.com; mike.leach@linaro.org; james.clark@arm.com;
> leo.yan@linaro.org
> Cc: linux-arm-kernel@lists.infradead.org; coresight@lists.linaro.org; linux-
> kernel@vger.kernel.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
> devicetree@vger.kernel.org; Sunil Kovvuri Goutham
> <sgoutham@marvell.com>; George Cherian <gcherian@marvell.com>; Linu
> Cherian <lcherian@marvell.com>
> Subject: [PATCH v7 0/7] Coresight for Kernel panic and watchdog reset
>
> This patch series is rebased on v6.8-rc4 from coresisght tree,[1], since latest
> changes are dependent on coresight_get/set_mode APIs.
>
Do you have any feedback on this version ?
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/5] ARM: dts: aspeed: greatlakes: correct Mellanox multi-host property
From: Andrew Jeffery @ 2024-04-09 0:24 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, devicetree, linux-arm-kernel,
linux-aspeed, linux-kernel
In-Reply-To: <20240405064624.18997-1-krzysztof.kozlowski@linaro.org>
On Fri, 2024-04-05 at 08:46 +0200, Krzysztof Kozlowski wrote:
> "mlx,multi-host" is using incorrect vendor prefix and is not documented.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
Thanks, I've applied these to my tree to be picked up by Joel.
Andrew
^ permalink raw reply
* Re: [PATCH v20 2/9] usb: dwc3: core: Access XHCI address space temporarily to read port info
From: Thinh Nguyen @ 2024-04-09 0:28 UTC (permalink / raw)
To: Krishna Kurapati
Cc: Krzysztof Kozlowski, Rob Herring, Bjorn Andersson, Wesley Cheng,
Konrad Dybcio, Greg Kroah-Hartman, Conor Dooley, Thinh Nguyen,
Felipe Balbi, Johan Hovold, devicetree@vger.kernel.org,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, quic_ppratap@quicinc.com,
quic_jackp@quicinc.com, Bjorn Andersson
In-Reply-To: <20240408132925.1880571-3-quic_kriskura@quicinc.com>
On Mon, Apr 08, 2024, Krishna Kurapati wrote:
> All DWC3 Multi Port controllers that exist today only support host mode.
> Temporarily map XHCI address space for host-only controllers and parse
> XHCI Extended Capabilities registers to read number of usb2 ports and
> usb3 ports present on multiport controller. Each USB Port is at least HS
> capable.
>
> The port info for usb2 and usb3 phy are identified as num_usb2_ports
> and num_usb3_ports. The intention is as follows:
>
> Wherever we need to perform phy operations like:
>
> LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS()
> {
> phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
> phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
> }
>
> If number of usb2 ports is 3, loop can go from index 0-2 for
> usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure,
> if the first 2 ports are SS capable or some other ports like (2 and 3)
> are SS capable. So instead, num_usb2_ports is used to loop around all
> phy's (both hs and ss) for performing phy operations. If any
> usb3_generic_phy turns out to be NULL, phy operation just bails out.
> num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up
> phy's as we need to know how many SS capable ports are there for this.
>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Reviewed-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
> drivers/usb/dwc3/core.c | 61 +++++++++++++++++++++++++++++++++++++++++
> drivers/usb/dwc3/core.h | 5 ++++
> 2 files changed, 66 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 31684cdaaae3..ddab30531f8a 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -39,6 +39,7 @@
> #include "io.h"
>
> #include "debug.h"
> +#include "../host/xhci-ext-caps.h"
>
> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */
>
> @@ -1881,10 +1882,56 @@ static int dwc3_get_clocks(struct dwc3 *dwc)
> return 0;
> }
>
> +static int dwc3_get_num_ports(struct dwc3 *dwc)
> +{
> + void __iomem *base;
> + u8 major_revision;
> + u32 offset;
> + u32 val;
> +
> + /*
> + * Remap xHCI address space to access XHCI ext cap regs since it is
> + * needed to get information on number of ports present.
> + */
> + base = ioremap(dwc->xhci_resources[0].start,
> + resource_size(&dwc->xhci_resources[0]));
> + if (!base)
> + return -ENOMEM;
> +
> + offset = 0;
> + do {
> + offset = xhci_find_next_ext_cap(base, offset,
> + XHCI_EXT_CAPS_PROTOCOL);
> + if (!offset)
> + break;
> +
> + val = readl(base + offset);
> + major_revision = XHCI_EXT_PORT_MAJOR(val);
> +
> + val = readl(base + offset + 0x08);
> + if (major_revision == 0x03) {
> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(val);
> + } else if (major_revision <= 0x02) {
> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(val);
> + } else {
> + dev_warn(dwc->dev, "unrecognized port major revision %d\n",
> + major_revision);
> + }
> + } while (1);
> +
> + dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n",
> + dwc->num_usb2_ports, dwc->num_usb3_ports);
> +
> + iounmap(base);
> +
> + return 0;
> +}
> +
> static int dwc3_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct resource *res, dwc_res;
> + unsigned int hw_mode;
> void __iomem *regs;
> struct dwc3 *dwc;
> int ret;
> @@ -1968,6 +2015,20 @@ static int dwc3_probe(struct platform_device *pdev)
> goto err_disable_clks;
> }
>
> + /*
> + * Currently only DWC3 controllers that are host-only capable
> + * can have more than one port.
> + */
> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
> + ret = dwc3_get_num_ports(dwc);
> + if (ret)
> + goto err_disable_clks;
> + } else {
> + dwc->num_usb2_ports = 1;
> + dwc->num_usb3_ports = 1;
> + }
> +
> spin_lock_init(&dwc->lock);
> mutex_init(&dwc->mutex);
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 7e80dd3d466b..341e4c73cb2e 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1039,6 +1039,8 @@ struct dwc3_scratchpad_array {
> * @usb3_phy: pointer to USB3 PHY
> * @usb2_generic_phy: pointer to USB2 PHY
> * @usb3_generic_phy: pointer to USB3 PHY
> + * @num_usb2_ports: number of USB2 ports
> + * @num_usb3_ports: number of USB3 ports
> * @phys_ready: flag to indicate that PHYs are ready
> * @ulpi: pointer to ulpi interface
> * @ulpi_ready: flag to indicate that ULPI is initialized
> @@ -1187,6 +1189,9 @@ struct dwc3 {
> struct phy *usb2_generic_phy;
> struct phy *usb3_generic_phy;
>
> + u8 num_usb2_ports;
> + u8 num_usb3_ports;
> +
> bool phys_ready;
>
> struct ulpi *ulpi;
> --
> 2.34.1
>
Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Thanks,
Thinh
^ permalink raw reply
* Re: [PATCH v20 4/9] usb: dwc3: core: Refactor PHY logic to support Multiport Controller
From: Thinh Nguyen @ 2024-04-09 1:11 UTC (permalink / raw)
To: Krishna Kurapati
Cc: Krzysztof Kozlowski, Rob Herring, Bjorn Andersson, Wesley Cheng,
Konrad Dybcio, Greg Kroah-Hartman, Conor Dooley, Thinh Nguyen,
Felipe Balbi, Johan Hovold, devicetree@vger.kernel.org,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, quic_ppratap@quicinc.com,
quic_jackp@quicinc.com, Johan Hovold
In-Reply-To: <20240408132925.1880571-5-quic_kriskura@quicinc.com>
On Mon, Apr 08, 2024, Krishna Kurapati wrote:
> Currently the DWC3 driver supports only single port controller
> which requires at least one HS PHY and at most one SS PHY.
>
> But the DWC3 USB controller can be connected to multiple ports and
> each port can have their own PHYs. Each port of the multiport
> controller can either be HS+SS capable or HS only capable
> Proper quantification of them is required to modify GUSB2PHYCFG
> and GUSB3PIPECTL registers appropriately.
>
> Add support for detecting, obtaining and configuring PHYs supported
> by a multiport controller. Limit support to multiport controllers
> with up to four ports for now (e.g. as needed for SC8280XP).
>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> drivers/usb/dwc3/core.c | 251 ++++++++++++++++++++++++++++------------
> drivers/usb/dwc3/core.h | 14 ++-
> drivers/usb/dwc3/drd.c | 15 ++-
> 3 files changed, 193 insertions(+), 87 deletions(-)
>
<snip>
> @@ -1937,6 +2020,10 @@ static int dwc3_get_num_ports(struct dwc3 *dwc)
>
> iounmap(base);
>
> + if (dwc->num_usb2_ports > DWC3_MAX_PORTS ||
> + dwc->num_usb3_ports > DWC3_MAX_PORTS)
> + return -ENOMEM;
This should be -EINVAL.
> +
> return 0;
> }
<snip>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 341e4c73cb2e..df2e111aa848 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -33,6 +33,12 @@
>
> #include <linux/power_supply.h>
>
> +/*
> + * Maximum number of ports currently supported for multiport
> + * controllers.
This macro here is being used per USB2 vs USB3 ports rather than USB2 +
USB3, unlike the xHCI MAXPORTS. You can clarify in the comment and
rename the macro to avoid any confusion. You can also create 2 separate
macros for number of USB2 and USB3 ports even if they share the same
value.
As noted[*], we support have different max number of usb2 ports vs usb3
ports. I would suggest splitting the macros.
[*] https://lore.kernel.org/linux-usb/20230801013031.ft3zpoatiyfegmh6@synopsys.com/
> + */
> +#define DWC3_MAX_PORTS 4
> +
>
But it's not a big issue whether you decided to push a new version or a
create a separate patch for the comments above. Here's my Ack:
Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Thanks,
Thinh
^ permalink raw reply
* Re: [PATCH v20 6/9] usb: dwc3: qcom: Add helper function to request wakeup interrupts
From: Thinh Nguyen @ 2024-04-09 1:12 UTC (permalink / raw)
To: Krishna Kurapati
Cc: Krzysztof Kozlowski, Rob Herring, Bjorn Andersson, Wesley Cheng,
Konrad Dybcio, Greg Kroah-Hartman, Conor Dooley, Thinh Nguyen,
Felipe Balbi, Johan Hovold, devicetree@vger.kernel.org,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, quic_ppratap@quicinc.com,
quic_jackp@quicinc.com, Johan Hovold
In-Reply-To: <20240408132925.1880571-7-quic_kriskura@quicinc.com>
On Mon, Apr 08, 2024, Krishna Kurapati wrote:
> The logic for requesting interrupts is duplicated for each interrupt. In
> the upcoming patches that introduces support for multiport, it would be
> better to clean up the duplication before reading mulitport related
> interrupts.
>
> Refactor interrupt setup call by adding a new helper function for
> requesting the wakeup interrupts. To simplify implementation, make
> the display name same as the interrupt name expected in DT.
>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 53 ++++++++++++++++--------------------
> 1 file changed, 24 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index f6b2fab49d5e..cae5dab8fcfc 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -501,6 +501,22 @@ static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom)
> PIPE_UTMI_CLK_DIS);
> }
>
> +static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq,
> + const char *name)
> +{
> + int ret;
> +
> + /* Keep wakeup interrupts disabled until suspend */
> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
> + qcom_dwc3_resume_irq,
> + IRQF_ONESHOT | IRQF_NO_AUTOEN,
> + name, qcom);
> + if (ret)
> + dev_err(qcom->dev, "failed to request irq %s: %d\n", name, ret);
> +
> + return ret;
> +}
> +
> static int dwc3_qcom_setup_irq(struct platform_device *pdev)
> {
> struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
> @@ -509,54 +525,33 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
>
> irq = platform_get_irq_byname_optional(pdev, "qusb2_phy");
> if (irq > 0) {
> - /* Keep wakeup interrupts disabled until suspend */
> - ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
> - qcom_dwc3_resume_irq,
> - IRQF_ONESHOT | IRQF_NO_AUTOEN,
> - "qcom_dwc3 QUSB2", qcom);
> - if (ret) {
> - dev_err(qcom->dev, "qusb2_phy_irq failed: %d\n", ret);
> + ret = dwc3_qcom_request_irq(qcom, irq, "qusb2_phy");
> + if (ret)
> return ret;
> - }
> qcom->qusb2_phy_irq = irq;
> }
>
> irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_irq");
> if (irq > 0) {
> - ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
> - qcom_dwc3_resume_irq,
> - IRQF_ONESHOT | IRQF_NO_AUTOEN,
> - "qcom_dwc3 DP_HS", qcom);
> - if (ret) {
> - dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret);
> + ret = dwc3_qcom_request_irq(qcom, irq, "dp_hs_phy_irq");
> + if (ret)
> return ret;
> - }
> qcom->dp_hs_phy_irq = irq;
> }
>
> irq = platform_get_irq_byname_optional(pdev, "dm_hs_phy_irq");
> if (irq > 0) {
> - ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
> - qcom_dwc3_resume_irq,
> - IRQF_ONESHOT | IRQF_NO_AUTOEN,
> - "qcom_dwc3 DM_HS", qcom);
> - if (ret) {
> - dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret);
> + ret = dwc3_qcom_request_irq(qcom, irq, "dm_hs_phy_irq");
> + if (ret)
> return ret;
> - }
> qcom->dm_hs_phy_irq = irq;
> }
>
> irq = platform_get_irq_byname_optional(pdev, "ss_phy_irq");
> if (irq > 0) {
> - ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
> - qcom_dwc3_resume_irq,
> - IRQF_ONESHOT | IRQF_NO_AUTOEN,
> - "qcom_dwc3 SS", qcom);
> - if (ret) {
> - dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret);
> + ret = dwc3_qcom_request_irq(qcom, irq, "ss_phy_irq");
> + if (ret)
> return ret;
> - }
> qcom->ss_phy_irq = irq;
> }
>
> --
> 2.34.1
>
Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Thanks,
Thinh
^ permalink raw reply
* Re: [PATCH v20 7/9] usb: dwc3: qcom: Refactor IRQ handling in glue driver
From: Thinh Nguyen @ 2024-04-09 1:14 UTC (permalink / raw)
To: Krishna Kurapati
Cc: Krzysztof Kozlowski, Rob Herring, Bjorn Andersson, Wesley Cheng,
Konrad Dybcio, Greg Kroah-Hartman, Conor Dooley, Thinh Nguyen,
Felipe Balbi, Johan Hovold, devicetree@vger.kernel.org,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, quic_ppratap@quicinc.com,
quic_jackp@quicinc.com, Johan Hovold
In-Reply-To: <20240408132925.1880571-8-quic_kriskura@quicinc.com>
On Mon, Apr 08, 2024, Krishna Kurapati wrote:
> On multiport supported controllers, each port has its own DP/DM
> and SS (if super speed capable) interrupts. As per the bindings,
> their interrupt names differ from standard ones having "_x" added
> as suffix (x indicates port number). Identify from the interrupt
> names whether the controller is a multiport controller or not.
> Refactor dwc3_qcom_setup_irq() call to parse multiport interrupts
> along with non-multiport ones accordingly..
>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 137 ++++++++++++++++++++++++++---------
> 1 file changed, 103 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index cae5dab8fcfc..35eb338514bc 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -52,6 +52,13 @@
> #define APPS_USB_AVG_BW 0
> #define APPS_USB_PEAK_BW MBps_to_icc(40)
>
> +struct dwc3_qcom_port {
> + int qusb2_phy_irq;
> + int dp_hs_phy_irq;
> + int dm_hs_phy_irq;
> + int ss_phy_irq;
> +};
> +
> struct dwc3_qcom {
> struct device *dev;
> void __iomem *qscratch_base;
> @@ -59,11 +66,8 @@ struct dwc3_qcom {
> struct clk **clks;
> int num_clocks;
> struct reset_control *resets;
> -
> - int qusb2_phy_irq;
> - int dp_hs_phy_irq;
> - int dm_hs_phy_irq;
> - int ss_phy_irq;
> + struct dwc3_qcom_port ports[DWC3_MAX_PORTS];
> + u8 num_ports;
> enum usb_device_speed usb2_speed;
>
> struct extcon_dev *edev;
> @@ -354,24 +358,24 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)
>
> static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
> {
> - dwc3_qcom_disable_wakeup_irq(qcom->qusb2_phy_irq);
> + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].qusb2_phy_irq);
>
> if (qcom->usb2_speed == USB_SPEED_LOW) {
> - dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
> + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq);
> } else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
> (qcom->usb2_speed == USB_SPEED_FULL)) {
> - dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
> + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq);
> } else {
> - dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
> - dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
> + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq);
> + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq);
> }
>
> - dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq);
> + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].ss_phy_irq);
> }
>
> static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
> {
> - dwc3_qcom_enable_wakeup_irq(qcom->qusb2_phy_irq, 0);
> + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].qusb2_phy_irq, 0);
>
> /*
> * Configure DP/DM line interrupts based on the USB2 device attached to
> @@ -383,20 +387,20 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
> */
>
> if (qcom->usb2_speed == USB_SPEED_LOW) {
> - dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq,
> - IRQ_TYPE_EDGE_FALLING);
> + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq,
> + IRQ_TYPE_EDGE_FALLING);
> } else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
> (qcom->usb2_speed == USB_SPEED_FULL)) {
> - dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq,
> - IRQ_TYPE_EDGE_FALLING);
> + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq,
> + IRQ_TYPE_EDGE_FALLING);
> } else {
> - dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq,
> - IRQ_TYPE_EDGE_RISING);
> - dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq,
> - IRQ_TYPE_EDGE_RISING);
> + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq,
> + IRQ_TYPE_EDGE_RISING);
> + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq,
> + IRQ_TYPE_EDGE_RISING);
> }
>
> - dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0);
> + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].ss_phy_irq, 0);
> }
>
> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> @@ -517,42 +521,107 @@ static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq,
> return ret;
> }
>
> -static int dwc3_qcom_setup_irq(struct platform_device *pdev)
> +static int dwc3_qcom_setup_port_irq(struct platform_device *pdev, int port_index, bool is_multiport)
> {
> struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
> + const char *irq_name;
> int irq;
> int ret;
>
> - irq = platform_get_irq_byname_optional(pdev, "qusb2_phy");
> + if (is_multiport)
> + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_%d", port_index + 1);
> + else
> + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_irq");
> + if (!irq_name)
> + return -ENOMEM;
> +
> + irq = platform_get_irq_byname_optional(pdev, irq_name);
> if (irq > 0) {
> - ret = dwc3_qcom_request_irq(qcom, irq, "qusb2_phy");
> + ret = dwc3_qcom_request_irq(qcom, irq, irq_name);
> if (ret)
> return ret;
> - qcom->qusb2_phy_irq = irq;
> + qcom->ports[port_index].dp_hs_phy_irq = irq;
> }
>
> - irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_irq");
> + if (is_multiport)
> + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dm_hs_phy_%d", port_index + 1);
> + else
> + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dm_hs_phy_irq");
> + if (!irq_name)
> + return -ENOMEM;
> +
> + irq = platform_get_irq_byname_optional(pdev, irq_name);
> if (irq > 0) {
> - ret = dwc3_qcom_request_irq(qcom, irq, "dp_hs_phy_irq");
> + ret = dwc3_qcom_request_irq(qcom, irq, irq_name);
> if (ret)
> return ret;
> - qcom->dp_hs_phy_irq = irq;
> + qcom->ports[port_index].dm_hs_phy_irq = irq;
> }
>
> - irq = platform_get_irq_byname_optional(pdev, "dm_hs_phy_irq");
> + if (is_multiport)
> + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ss_phy_%d", port_index + 1);
> + else
> + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ss_phy_irq");
> + if (!irq_name)
> + return -ENOMEM;
> +
> + irq = platform_get_irq_byname_optional(pdev, irq_name);
> if (irq > 0) {
> - ret = dwc3_qcom_request_irq(qcom, irq, "dm_hs_phy_irq");
> + ret = dwc3_qcom_request_irq(qcom, irq, irq_name);
> if (ret)
> return ret;
> - qcom->dm_hs_phy_irq = irq;
> + qcom->ports[port_index].ss_phy_irq = irq;
> }
>
> - irq = platform_get_irq_byname_optional(pdev, "ss_phy_irq");
> + if (is_multiport)
> + return 0;
> +
> + irq = platform_get_irq_byname_optional(pdev, "qusb2_phy");
> if (irq > 0) {
> - ret = dwc3_qcom_request_irq(qcom, irq, "ss_phy_irq");
> + ret = dwc3_qcom_request_irq(qcom, irq, "qusb2_phy");
> + if (ret)
> + return ret;
> + qcom->ports[port_index].qusb2_phy_irq = irq;
> + }
> +
> + return 0;
> +}
> +
> +static int dwc3_qcom_find_num_ports(struct platform_device *pdev)
> +{
> + char irq_name[14];
> + int port_num;
> + int irq;
> +
> + irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_1");
> + if (irq <= 0)
> + return 1;
> +
> + for (port_num = 2; port_num <= DWC3_MAX_PORTS; port_num++) {
> + sprintf(irq_name, "dp_hs_phy_%d", port_num);
> +
> + irq = platform_get_irq_byname_optional(pdev, irq_name);
> + if (irq <= 0)
> + return port_num - 1;
> + }
> +
> + return DWC3_MAX_PORTS;
> +}
> +
> +static int dwc3_qcom_setup_irq(struct platform_device *pdev)
> +{
> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
> + bool is_multiport;
> + int ret;
> + int i;
> +
> + qcom->num_ports = dwc3_qcom_find_num_ports(pdev);
> + is_multiport = (qcom->num_ports > 1);
> +
> + for (i = 0; i < qcom->num_ports; i++) {
> + ret = dwc3_qcom_setup_port_irq(pdev, i, is_multiport);
> if (ret)
> return ret;
> - qcom->ss_phy_irq = irq;
> }
>
> return 0;
> --
> 2.34.1
>
Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Thanks,
Thinh
^ permalink raw reply
* Re: [PATCH v20 8/9] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport
From: Thinh Nguyen @ 2024-04-09 1:16 UTC (permalink / raw)
To: Krishna Kurapati
Cc: Krzysztof Kozlowski, Rob Herring, Bjorn Andersson, Wesley Cheng,
Konrad Dybcio, Greg Kroah-Hartman, Conor Dooley, Thinh Nguyen,
Felipe Balbi, Johan Hovold, devicetree@vger.kernel.org,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, quic_ppratap@quicinc.com,
quic_jackp@quicinc.com, Johan Hovold
In-Reply-To: <20240408132925.1880571-9-quic_kriskura@quicinc.com>
On Mon, Apr 08, 2024, Krishna Kurapati wrote:
> DWC3 Qcom wrapper currently supports only wakeup configuration
> for single port controllers. Read speed of each port connected
> to the controller and enable wakeup for each of them accordingly.
>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 71 +++++++++++++++++++++---------------
> 1 file changed, 41 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 35eb338514bc..12182e0f8f45 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -57,6 +57,7 @@ struct dwc3_qcom_port {
> int dp_hs_phy_irq;
> int dm_hs_phy_irq;
> int ss_phy_irq;
> + enum usb_device_speed usb2_speed;
> };
>
> struct dwc3_qcom {
> @@ -68,7 +69,6 @@ struct dwc3_qcom {
> struct reset_control *resets;
> struct dwc3_qcom_port ports[DWC3_MAX_PORTS];
> u8 num_ports;
> - enum usb_device_speed usb2_speed;
>
> struct extcon_dev *edev;
> struct extcon_dev *host_edev;
> @@ -307,7 +307,7 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
> return dwc->xhci;
> }
>
> -static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> +static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, int port_index)
> {
> struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> struct usb_device *udev;
> @@ -318,14 +318,8 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> */
> hcd = platform_get_drvdata(dwc->xhci);
>
> - /*
> - * It is possible to query the speed of all children of
> - * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code
> - * currently supports only 1 port per controller. So
> - * this is sufficient.
> - */
> #ifdef CONFIG_USB
> - udev = usb_hub_find_child(hcd->self.root_hub, 1);
> + udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1);
> #else
> udev = NULL;
> #endif
> @@ -356,26 +350,26 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)
> disable_irq_nosync(irq);
> }
>
> -static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
> +static void dwc3_qcom_disable_port_interrupts(struct dwc3_qcom_port *port)
> {
> - dwc3_qcom_disable_wakeup_irq(qcom->ports[0].qusb2_phy_irq);
> + dwc3_qcom_disable_wakeup_irq(port->qusb2_phy_irq);
>
> - if (qcom->usb2_speed == USB_SPEED_LOW) {
> - dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq);
> - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
> - (qcom->usb2_speed == USB_SPEED_FULL)) {
> - dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq);
> + if (port->usb2_speed == USB_SPEED_LOW) {
> + dwc3_qcom_disable_wakeup_irq(port->dm_hs_phy_irq);
> + } else if ((port->usb2_speed == USB_SPEED_HIGH) ||
> + (port->usb2_speed == USB_SPEED_FULL)) {
> + dwc3_qcom_disable_wakeup_irq(port->dp_hs_phy_irq);
> } else {
> - dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq);
> - dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq);
> + dwc3_qcom_disable_wakeup_irq(port->dp_hs_phy_irq);
> + dwc3_qcom_disable_wakeup_irq(port->dm_hs_phy_irq);
> }
>
> - dwc3_qcom_disable_wakeup_irq(qcom->ports[0].ss_phy_irq);
> + dwc3_qcom_disable_wakeup_irq(port->ss_phy_irq);
> }
>
> -static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
> +static void dwc3_qcom_enable_port_interrupts(struct dwc3_qcom_port *port)
> {
> - dwc3_qcom_enable_wakeup_irq(qcom->ports[0].qusb2_phy_irq, 0);
> + dwc3_qcom_enable_wakeup_irq(port->qusb2_phy_irq, 0);
>
> /*
> * Configure DP/DM line interrupts based on the USB2 device attached to
> @@ -386,21 +380,37 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
> * DP and DM lines as rising edge to detect HS/HS/LS device connect scenario.
> */
>
> - if (qcom->usb2_speed == USB_SPEED_LOW) {
> - dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq,
> + if (port->usb2_speed == USB_SPEED_LOW) {
> + dwc3_qcom_enable_wakeup_irq(port->dm_hs_phy_irq,
> IRQ_TYPE_EDGE_FALLING);
> - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
> - (qcom->usb2_speed == USB_SPEED_FULL)) {
> - dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq,
> + } else if ((port->usb2_speed == USB_SPEED_HIGH) ||
> + (port->usb2_speed == USB_SPEED_FULL)) {
> + dwc3_qcom_enable_wakeup_irq(port->dp_hs_phy_irq,
> IRQ_TYPE_EDGE_FALLING);
> } else {
> - dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq,
> + dwc3_qcom_enable_wakeup_irq(port->dp_hs_phy_irq,
> IRQ_TYPE_EDGE_RISING);
> - dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq,
> + dwc3_qcom_enable_wakeup_irq(port->dm_hs_phy_irq,
> IRQ_TYPE_EDGE_RISING);
> }
>
> - dwc3_qcom_enable_wakeup_irq(qcom->ports[0].ss_phy_irq, 0);
> + dwc3_qcom_enable_wakeup_irq(port->ss_phy_irq, 0);
> +}
> +
> +static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
> +{
> + int i;
> +
> + for (i = 0; i < qcom->num_ports; i++)
> + dwc3_qcom_disable_port_interrupts(&qcom->ports[i]);
> +}
> +
> +static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
> +{
> + int i;
> +
> + for (i = 0; i < qcom->num_ports; i++)
> + dwc3_qcom_enable_port_interrupts(&qcom->ports[i]);
> }
>
> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> @@ -427,7 +437,8 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> * freezable workqueue.
> */
> if (dwc3_qcom_is_host(qcom) && wakeup) {
> - qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
> + for (i = 0; i < qcom->num_ports; i++)
> + qcom->ports[i].usb2_speed = dwc3_qcom_read_usb2_speed(qcom, i);
> dwc3_qcom_enable_interrupts(qcom);
> }
>
> --
> 2.34.1
>
Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Thanks,
Thinh
^ permalink raw reply
* Re: [PATCH v20 9/9] usb: dwc3: qcom: Add multiport suspend/resume support for wrapper
From: Thinh Nguyen @ 2024-04-09 1:20 UTC (permalink / raw)
To: Krishna Kurapati
Cc: Krzysztof Kozlowski, Rob Herring, Bjorn Andersson, Wesley Cheng,
Konrad Dybcio, Greg Kroah-Hartman, Conor Dooley, Thinh Nguyen,
Felipe Balbi, Johan Hovold, devicetree@vger.kernel.org,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, quic_ppratap@quicinc.com,
quic_jackp@quicinc.com, Johan Hovold
In-Reply-To: <20240408132925.1880571-10-quic_kriskura@quicinc.com>
On Mon, Apr 08, 2024, Krishna Kurapati wrote:
> Power event IRQ is used for wakeup in cases:
> a) where the controller is super speed capable and missing an
> ss_phy interrupt.
> b) where the GIC is not capable of detecting DP/DM hs phy irq's.
>
> Power event IRQ stat register indicates whether high speed phy
> entered and exited L2 successfully during suspend and resume.
> Indicate the same for all ports of multiport.
>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 12182e0f8f45..d3e6d5d5e8bf 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -52,6 +52,13 @@
> #define APPS_USB_AVG_BW 0
> #define APPS_USB_PEAK_BW MBps_to_icc(40)
>
> +static const u32 pwr_evnt_irq_stat_reg[DWC3_MAX_PORTS] = {
DWC3_MAX_PORTS is not qcom specific. For qcom specific max ports, please
create your own macro here.
> + 0x58,
> + 0x1dc,
> + 0x228,
> + 0x238,
> +};
> +
> struct dwc3_qcom_port {
> int qusb2_phy_irq;
> int dp_hs_phy_irq;
> @@ -421,9 +428,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> if (qcom->is_suspended)
> return 0;
>
> - val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
> - if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> - dev_err(qcom->dev, "HS-PHY not in L2\n");
> + for (i = 0; i < qcom->num_ports; i++) {
> + val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg[i]);
> + if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> + dev_err(qcom->dev, "port-%d HS-PHY not in L2\n", i + 1);
> + }
>
> for (i = qcom->num_clocks - 1; i >= 0; i--)
> clk_disable_unprepare(qcom->clks[i]);
> @@ -472,8 +481,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
> dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
>
> /* Clear existing events from PHY related to L2 in/out */
> - dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG,
> - PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
> + for (i = 0; i < qcom->num_ports; i++) {
> + dwc3_qcom_setbits(qcom->qscratch_base,
> + pwr_evnt_irq_stat_reg[i],
> + PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
> + }
>
> qcom->is_suspended = false;
>
> --
> 2.34.1
>
Whether or not you decided to create v21:
Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Thanks,
Thinh
^ permalink raw reply
* Re: [PATCH v1] rtc: pcf2127: configurable power management function
From: Rob Herring @ 2024-04-09 1:29 UTC (permalink / raw)
To: Markus Burri
Cc: linux-kernel, devicetree, Krzysztof Kozlowski, Alexandre Belloni,
Conor Dooley, linux-rtc
In-Reply-To: <20240408054926.3994-1-markus.burri@mt.com>
On Mon, 08 Apr 2024 07:49:26 +0200, Markus Burri wrote:
> In the PCF2131 power management the battery switch-over function is
> disabled by default.
> After a power cycle the rtc clock is wrong because of that.
> Add a device-tree property to configure the power management function
> and enable battery switch-over.
>
> Signed-off-by: Markus Burri <markus.burri@mt.com>
> ---
> .../devicetree/bindings/rtc/nxp,pcf2127.yaml | 3 +++
> drivers/rtc/rtc-pcf2127.c | 22 +++++++++++++++++++
> 2 files changed, 25 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/rtc/nxp,pcf2127.yaml: pwrmng-function: missing type definition
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240408054926.3994-1-markus.burri@mt.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: mtd: amlogic,meson-nand: support fields for boot ROM code
From: Rob Herring @ 2024-04-09 1:29 UTC (permalink / raw)
To: Arseniy Krasnov
Cc: kernel, Krzysztof Kozlowski, Rob Herring, Neil Armstrong,
devicetree, linux-kernel, oxffffaa, Martin Blumenstingl,
Conor Dooley, Miquel Raynal, linux-arm-kernel, Kevin Hilman,
Richard Weinberger, linux-mtd, Vignesh Raghavendra, Jerome Brunet,
linux-amlogic
In-Reply-To: <20240408085931.456337-2-avkrasnov@salutedevices.com>
On Mon, 08 Apr 2024 11:59:30 +0300, Arseniy Krasnov wrote:
> Boot ROM code on Meson requires that some pages on NAND must be written
> in special mode: "short" ECC mode where each block is 384 bytes and
> scrambling mode is on. Such pages located with the specified interval
> within specified offset. Both interval and offset are located in the
> device tree and used by driver if 'nand-is-boot-medium' is set for
> NAND chip.
>
> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
> ---
> .../bindings/mtd/amlogic,meson-nand.yaml | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
./Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml:84:61: [warning] too few spaces after comma (commas)
./Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml:85:61: [warning] too few spaces after comma (commas)
dtschema/dtc warnings/errors:
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240408085931.456337-2-avkrasnov@salutedevices.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply
* Re: [PATCH v2] ASoC: dt-bindings: imx-audio-spdif: convert to YAML
From: Shengjiu Wang @ 2024-04-09 1:37 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Shengjiu Wang, lgirdwood, broonie, robh+dt,
krzysztof.kozlowski+dt, conor+dt, linux-sound, devicetree,
linux-kernel, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel
In-Reply-To: <69ab2bd9-401a-42ff-90fa-6600dd071722@linaro.org>
On Mon, Apr 8, 2024 at 11:06 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/04/2024 10:01, Shengjiu Wang wrote:
> >>> +
> >>> +anyOf:
> >>> + - required:
> >>> + - spdif-in
> >>> + - required:
> >>> + - spdif-out
> >>> + - required:
> >>> + - spdif-out
> >>> + - spdif-in
> >>
> >> Do you need the last required block?
> >
> > Yes, one of them or both are required.
>
> And? It's already there: that's the meaning of any. It is not oneOf...
> Before answering please test your changes and ideas. I pointed issue
> here and you responded just to close my comment. That does not make me
> happy, just wastes my time.
Maybe I didn't express clearly.
we need at least one of them (spdif-in, spdif-out) in the node. which means
that we need to select "spdif-in", or "spdif-out", or "spdif-in and
spdif-out".
So my understanding is that need to use "anyOf", if it is wrong, please let
me know.
>
> >
> >>
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> + - |
> >>> + sound {
> >>
> >> That's a random change...
So I can use "sound-spdif", right?
best regards
wang shengjiu
> >>
> >> Instead of sending two patches per day, please carefully address the
> >> feedback.
> >
> > In v1 you suggest to change it to spdif? but spdif may conflict
> > with the fsl,spdif.yaml. so which name I should use?
>
> I don't understand where is the conflict. That's a different binding.
>
>
^ permalink raw reply
* Re: [RFC PATCH v1 2/2] of: dynamic: Fix probing of overlay devices
From: Saravana Kannan @ 2024-04-09 1:40 UTC (permalink / raw)
To: Herve Codina, Geert Uytterhoeven
Cc: Rob Herring, kernel-team, linux-kernel, imx, linux-arm-kernel,
linux-i2c, devicetree, linux-spi, linux-acpi
In-Reply-To: <20240408231310.325451-3-saravanak@google.com>
On Mon, Apr 8, 2024 at 4:13 PM Saravana Kannan <saravanak@google.com> wrote:
>
> Get fw_devlink to work well with overlay devices.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> drivers/base/core.c | 10 ++++++++++
> drivers/of/dynamic.c | 8 ++++++++
> include/linux/fwnode.h | 2 ++
> 3 files changed, 20 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 5f4e03336e68..d856f9c5d601 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -237,6 +237,16 @@ static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,
> __fw_devlink_pickup_dangling_consumers(child, new_sup);
> }
>
> +
> +void fw_devlink_pickup_dangling_consumers(struct fwnode_handle *child,
> + struct fwnode_handle *parent)
> +{
> + mutex_lock(&fwnode_link_lock);
> + __fw_devlink_pickup_dangling_consumers(child, parent);
> + __fw_devlink_link_to_consumers(parent->dev);
> + mutex_unlock(&fwnode_link_lock);
> +}
> +
> static DEFINE_MUTEX(device_links_lock);
> DEFINE_STATIC_SRCU(device_links_srcu);
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 19a1a38554f2..0a936f46820e 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -237,6 +237,7 @@ static void __of_attach_node(struct device_node *np)
> int of_attach_node(struct device_node *np)
> {
> struct of_reconfig_data rd;
> + struct fwnode_handle *fwnode, *parent;
>
> memset(&rd, 0, sizeof(rd));
> rd.dn = np;
> @@ -246,6 +247,13 @@ int of_attach_node(struct device_node *np)
> mutex_unlock(&of_mutex);
>
> of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, &rd);
> + fwnode = of_fwnode_handle(np);
> + fwnode_for_each_parent_node(fwnode, parent)
> + if (parent->dev) {
> + fw_devlink_pickup_dangling_consumers(fwnode, parent);
> + fwnode_handle_put(parent);
> + break;
> + }
I'm clearly calling this in the wrong location. Please move this logic
to __of_changeset_entry_notify() and for the case
OF_RECONFIG_ATTACH_NODE. Haven't fully thought through the DETACH
case, but it should work correctly for that case too. If not, I'll
take care of that next.
-Saravana
>
> return 0;
> }
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 0d79070c5a70..4b3f697a90e8 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -220,6 +220,8 @@ int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup,
> u8 flags);
> void fwnode_links_purge(struct fwnode_handle *fwnode);
> void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode);
> +void fw_devlink_pickup_dangling_consumers(struct fwnode_handle *child,
> + struct fwnode_handle *parent);
> bool fw_devlink_is_strict(void);
>
> #endif
> --
> 2.44.0.478.gd926399ef9-goog
>
^ permalink raw reply
* [PATCH v4 1/2] dt-bindings: phy: mediatek,mt7988-xfi-tphy: add new bindings
From: Daniel Golle @ 2024-04-09 2:23 UTC (permalink / raw)
To: Vinod Koul, Bc-bocun Chen, Steven Liu, John Crispin, Chunfeng Yun,
Kishon Vijay Abraham I, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno,
Daniel Golle, Qingfang Deng, SkyLake Huang, Philipp Zabel,
linux-arm-kernel, linux-mediatek, linux-phy, devicetree,
linux-kernel, netdev
Add bindings for the MediaTek XFI Ethernet SerDes T-PHY found in the
MediaTek MT7988 SoC which can operate at various interfaces modes:
via USXGMII PCS:
* USXGMII
* 10GBase-R
* 5GBase-R
via LynxI SGMII PCS:
* 2500Base-X
* 1000Base-X
* Cisco SGMII (MAC side)
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v4: no changes
v3: Add reference to MediaTek-internal "pextp" name, better explain reset as
well as 10GBase-R tuning work-around.
v2: unify filename and compatible as requested
.../phy/mediatek,mt7988-xfi-tphy.yaml | 80 +++++++++++++++++++
1 file changed, 80 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/mediatek,mt7988-xfi-tphy.yaml
diff --git a/Documentation/devicetree/bindings/phy/mediatek,mt7988-xfi-tphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,mt7988-xfi-tphy.yaml
new file mode 100644
index 00000000000000..cfb3ca97f87c68
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/mediatek,mt7988-xfi-tphy.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/mediatek,mt7988-xfi-tphy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek MT7988 XFI T-PHY
+
+maintainers:
+ - Daniel Golle <daniel@makrotopia.org>
+
+description:
+ The MediaTek XFI SerDes T-PHY provides the physical SerDes lanes
+ used by the (10G/5G) USXGMII PCS and (1G/2.5G) LynxI PCS found in
+ MediaTek's 10G-capabale MT7988 SoC.
+ In MediaTek's SDK sources, this unit is referred to as "pextp".
+
+properties:
+ compatible:
+ const: mediatek,mt7988-xfi-tphy
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: XFI PHY clock
+ - description: XFI register clock
+
+ clock-names:
+ items:
+ - const: xfipll
+ - const: topxtal
+
+ resets:
+ items:
+ - description: Reset controller corresponding to the phy instance.
+
+ mediatek,usxgmii-performance-errata:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description:
+ One instance of the T-PHY on MT7988 suffers from a performance
+ problem in 10GBase-R mode which needs a work-around in the driver.
+ This flag enables a work-around ajusting an analog phy setting and
+ is required for XFI Port0 of the MT7988 SoC to be in compliance with
+ the SFP specification.
+
+ "#phy-cells":
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - resets
+ - "#phy-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/mediatek,mt7988-clk.h>
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ phy@11f20000 {
+ compatible = "mediatek,mt7988-xfi-tphy";
+ reg = <0 0x11f20000 0 0x10000>;
+ clocks = <&xfi_pll CLK_XFIPLL_PLL_EN>,
+ <&topckgen CLK_TOP_XFI_PHY_0_XTAL_SEL>;
+ clock-names = "xfipll", "topxtal";
+ resets = <&watchdog 14>;
+ mediatek,usxgmii-performance-errata;
+ #phy-cells = <0>;
+ };
+ };
+
+...
--
2.44.0
^ permalink raw reply related
* [PATCH v4 2/2] phy: add driver for MediaTek XFI T-PHY
From: Daniel Golle @ 2024-04-09 2:24 UTC (permalink / raw)
To: Vinod Koul, Bc-bocun Chen, Steven Liu, John Crispin, Chunfeng Yun,
Kishon Vijay Abraham I, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno,
Daniel Golle, Qingfang Deng, SkyLake Huang, Philipp Zabel,
linux-arm-kernel, linux-mediatek, linux-phy, devicetree,
linux-kernel, netdev
In-Reply-To: <da5498096f71a40ca1eac4124b7bb601c82396fb.1712625857.git.daniel@makrotopia.org>
Add driver for MediaTek's XFI T-PHY which can be found in the MT7988
SoC. The XFI T-PHY is a 10 Gigabit/s Ethernet SerDes PHY with muxes on
the internal side to be used with either USXGMII PCS or LynxI PCS,
depending on the selected PHY interface mode.
The PHY can operates only in PHY_MODE_ETHERNET, the submode is one of
PHY_INTERFACE_MODE_* corresponding to the supported modes:
* USXGMII \
* 10GBase-R }- USXGMII PCS - XGDM \
* 5GBase-R / \
}- Ethernet MAC
* 2500Base-X \ /
* 1000Base-X }- LynxI PCS - GDM /
* Cisco SGMII (MAC side) /
I chose the name XFI T-PHY because names of functions dealing with the
phy in the vendor driver are prefixed "xfi_pextp_".
The register space used by the phy is called "pextp" in the vendor
sources, which could be read as "_P_CI _ex_press _T_-_P_hy", and that
is quite misleading as this phy isn't used for anything related to
PCIe, so I wanted to find a better name.
XFI is still somehow related (as in: you would find the relevant
places using grep in the vendor driver when looking for that) and the
term seemed to at least somehow be aligned with the function of that
phy: Dealing with (up to) 10 Gbit/s Ethernet serialized differential
signals.
In order to work-around a performance issue present on the first of
two XFI T-PHYs found in MT7988, special tuning is applied which can
be selected by adding the 'mediatek,usxgmii-performance-errata'
property to the device tree node, similar to how the vendor driver is
doing that too.
There is no documentation for most registers used for the
analog/tuning part, however, most of the registers have been partially
reverse-engineered from MediaTek's SDK implementation (see links, an
opaque sequence of 32-bit register writes) and descriptions for all
relevant digital registers and bits such as resets and muxes have been
supplied by MediaTek.
Link: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/b72d6cba92bf9e29fb035c03052fa1e86664a25b/21.02/files/target/linux/mediatek/files-5.4/drivers/net/ethernet/mediatek/mtk_sgmii.c
Link: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/dec96a1d9b82cdcda4a56453fd0b453d4cab4b85/21.02/files/target/linux/mediatek/files-5.4/drivers/net/ethernet/mediatek/mtk_eth_soc.c
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
v4:
Functional changes:
* Include fix for SGMII, found in vendor SDK[1].
Non-functional/cosmetic changes:
* Separate allocation and assignment of bools in mtk_xfi_tphy_setup().
* Remove unnecessary parentheses.
* Explain naming in commit message.
* Drop unnecessary dependencies from Kconfig.
* Provide URL references to source of magic values.
* Add kernel-doc comments.
[1]: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/b72d6cba92bf9e29fb035c03052fa1e86664a25b
v3: no changes
v2:
* use IO helpers from mtk-io.h instead of rolling my own
* use devm_clk_bulk_get()
* yse devm_platform_ioremap_resource()
* unify name and description everywhere
* invert bool is_xgmii into bool use_lynxi_pcs and add comments
describing the meaning of each of the stack variables
* not much we can do about remaining magic values unless MTK provides
definitions for them
MAINTAINERS | 1 +
drivers/phy/mediatek/Kconfig | 11 +
drivers/phy/mediatek/Makefile | 1 +
drivers/phy/mediatek/phy-mtk-xfi-tphy.c | 451 ++++++++++++++++++++++++
4 files changed, 464 insertions(+)
create mode 100644 drivers/phy/mediatek/phy-mtk-xfi-tphy.c
diff --git a/MAINTAINERS b/MAINTAINERS
index a4924fda31829b..98b4b391d224f9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13870,6 +13870,7 @@ L: netdev@vger.kernel.org
S: Maintained
F: drivers/net/phy/mediatek-ge-soc.c
F: drivers/net/phy/mediatek-ge.c
+F: drivers/phy/mediatek/phy-mtk-xfi-tphy.c
MEDIATEK I2C CONTROLLER DRIVER
M: Qii Wang <qii.wang@mediatek.com>
diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig
index 3849b7c87d2872..60e00057e8bc71 100644
--- a/drivers/phy/mediatek/Kconfig
+++ b/drivers/phy/mediatek/Kconfig
@@ -13,6 +13,17 @@ config PHY_MTK_PCIE
callback for PCIe GEN3 port, it supports software efuse
initialization.
+config PHY_MTK_XFI_TPHY
+ tristate "MediaTek 10GE SerDes XFI T-PHY driver"
+ depends on ARCH_MEDIATEK || COMPILE_TEST
+ depends on OF
+ select GENERIC_PHY
+ help
+ Say 'Y' here to add support for MediaTek XFI T-PHY driver.
+ The driver provides access to the Ethernet SerDes T-PHY supporting
+ 1GE and 2.5GE modes via the LynxI PCS, and 5GE and 10GE modes
+ via the USXGMII PCS found in MediaTek SoCs with 10G Ethernet.
+
config PHY_MTK_TPHY
tristate "MediaTek T-PHY Driver"
depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile
index f6e24a47e08153..1b8088df71e845 100644
--- a/drivers/phy/mediatek/Makefile
+++ b/drivers/phy/mediatek/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PHY_MTK_PCIE) += phy-mtk-pcie.o
obj-$(CONFIG_PHY_MTK_TPHY) += phy-mtk-tphy.o
obj-$(CONFIG_PHY_MTK_UFS) += phy-mtk-ufs.o
obj-$(CONFIG_PHY_MTK_XSPHY) += phy-mtk-xsphy.o
+obj-$(CONFIG_PHY_MTK_XFI_TPHY) += phy-mtk-xfi-tphy.o
phy-mtk-hdmi-drv-y := phy-mtk-hdmi.o
phy-mtk-hdmi-drv-y += phy-mtk-hdmi-mt2701.o
diff --git a/drivers/phy/mediatek/phy-mtk-xfi-tphy.c b/drivers/phy/mediatek/phy-mtk-xfi-tphy.c
new file mode 100644
index 00000000000000..3e3af7f7a3fdf5
--- /dev/null
+++ b/drivers/phy/mediatek/phy-mtk-xfi-tphy.c
@@ -0,0 +1,451 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * MediaTek 10GE SerDes XFI T-PHY driver
+ *
+ * Copyright (c) 2024 Daniel Golle <daniel@makrotopia.org>
+ * Bc-bocun Chen <bc-bocun.chen@mediatek.com>
+ * based on mtk_usxgmii.c and mtk_sgmii.c found in MediaTek's SDK (GPL-2.0)
+ * Copyright (c) 2022 MediaTek Inc.
+ * Author: Henry Yen <henry.yen@mediatek.com>
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/reset.h>
+#include <linux/phy.h>
+#include <linux/phy/phy.h>
+
+#include "phy-mtk-io.h"
+
+#define MTK_XFI_TPHY_NUM_CLOCKS 2
+
+#define REG_DIG_GLB_70 0x0070
+#define XTP_PCS_RX_EQ_IN_PROGRESS(x) FIELD_PREP(GENMASK(25, 24), (x))
+#define XTP_PCS_MODE_MASK GENMASK(17, 16)
+#define XTP_PCS_MODE(x) FIELD_PREP(GENMASK(17, 16), (x))
+#define XTP_PCS_RST_B BIT(15)
+#define XTP_FRC_PCS_RST_B BIT(14)
+#define XTP_PCS_PWD_SYNC_MASK GENMASK(13, 12)
+#define XTP_PCS_PWD_SYNC(x) FIELD_PREP(XTP_PCS_PWD_SYNC_MASK, (x))
+#define XTP_PCS_PWD_ASYNC_MASK GENMASK(11, 10)
+#define XTP_PCS_PWD_ASYNC(x) FIELD_PREP(XTP_PCS_PWD_ASYNC_MASK, (x))
+#define XTP_FRC_PCS_PWD_ASYNC BIT(8)
+#define XTP_PCS_UPDT BIT(4)
+#define XTP_PCS_IN_FR_RG BIT(0)
+
+#define REG_DIG_GLB_F4 0x00f4
+#define XFI_DPHY_PCS_SEL BIT(0)
+#define XFI_DPHY_PCS_SEL_SGMII FIELD_PREP(XFI_DPHY_PCS_SEL, 1)
+#define XFI_DPHY_PCS_SEL_USXGMII FIELD_PREP(XFI_DPHY_PCS_SEL, 0)
+#define XFI_DPHY_AD_SGDT_FRC_EN BIT(5)
+
+#define REG_DIG_LN_TRX_40 0x3040
+#define XTP_LN_FRC_TX_DATA_EN BIT(29)
+#define XTP_LN_TX_DATA_EN BIT(28)
+
+#define REG_DIG_LN_TRX_B0 0x30b0
+#define XTP_LN_FRC_TX_MACCK_EN BIT(5)
+#define XTP_LN_TX_MACCK_EN BIT(4)
+
+#define REG_ANA_GLB_D0 0x90d0
+#define XTP_GLB_USXGMII_SEL_MASK GENMASK(3, 1)
+#define XTP_GLB_USXGMII_SEL(x) FIELD_PREP(GENMASK(3, 1), (x))
+#define XTP_GLB_USXGMII_EN BIT(0)
+
+/**
+ * struct mtk_xfi_tphy - run-time data of the XFI phy instance
+ * @base: IO memory area to access phy registers.
+ * @dev: Kernel device used to output prefixed debug info.
+ * @reset: Reset control corresponding to the phy instance.
+ * @clocks: All clocks required for the phy to operate.
+ * @da_war: Enables work-around for 10GBase-R mode.
+ */
+struct mtk_xfi_tphy {
+ void __iomem *base;
+ struct device *dev;
+ struct reset_control *reset;
+ struct clk_bulk_data clocks[MTK_XFI_TPHY_NUM_CLOCKS];
+ bool da_war;
+};
+
+/**
+ * mtk_xfi_tphy_setup() - Setup phy for specified interface mode.
+ * @xfi_tphy: XFI phy instance.
+ * @interface: Ethernet interface mode
+ *
+ * The setup function is the condensed result of combining the 5 functions which
+ * setup the phy in MediaTek's GPL licensed public SDK sources. They can be found
+ * in mtk_sgmii.c[1] as well as mtk_usxgmii.c[2].
+ *
+ * Many magic values have been replaced by register and bit definitions, however,
+ * that has not been possible in all cases. While the vendor driver uses a
+ * sequence of 32-bit writes, here we try to only modify the actually required
+ * bits.
+ *
+ * [1]: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/b72d6cba92bf9e29fb035c03052fa1e86664a25b/21.02/files/target/linux/mediatek/files-5.4/drivers/net/ethernet/mediatek/mtk_sgmii.c
+ *
+ * [2]: https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/dec96a1d9b82cdcda4a56453fd0b453d4cab4b85/21.02/files/target/linux/mediatek/files-5.4/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+ */
+static void mtk_xfi_tphy_setup(struct mtk_xfi_tphy *xfi_tphy,
+ phy_interface_t interface)
+{
+ bool is_1g, is_2p5g, is_5g, is_10g, da_war, use_lynxi_pcs;
+
+ /* shorthands for specific clock speeds depending on interface mode */
+ is_1g = interface == PHY_INTERFACE_MODE_1000BASEX ||
+ interface == PHY_INTERFACE_MODE_SGMII;
+ is_2p5g = interface == PHY_INTERFACE_MODE_2500BASEX;
+ is_5g = interface == PHY_INTERFACE_MODE_5GBASER;
+ is_10g = interface == PHY_INTERFACE_MODE_10GBASER ||
+ interface == PHY_INTERFACE_MODE_USXGMII;
+
+ /* Is overriding 10GBase-R tuning value required? */
+ da_war = xfi_tphy->da_war && (interface == PHY_INTERFACE_MODE_10GBASER);
+
+ /* configure input mux to either
+ * - USXGMII PCS (64b/66b coding) for 5G/10G
+ * - LynxI PCS (8b/10b coding) for 1G/2.5G
+ */
+ use_lynxi_pcs = is_1g || is_2p5g;
+
+ dev_dbg(xfi_tphy->dev, "setting up for mode %s\n", phy_modes(interface));
+
+ /* Setup PLL setting */
+ mtk_phy_update_bits(xfi_tphy->base + 0x9024, 0x100000, is_10g ? 0x0 : 0x100000);
+ mtk_phy_update_bits(xfi_tphy->base + 0x2020, 0x202000, is_5g ? 0x202000 : 0x0);
+ mtk_phy_update_bits(xfi_tphy->base + 0x2030, 0x500, is_1g ? 0x0 : 0x500);
+ mtk_phy_update_bits(xfi_tphy->base + 0x2034, 0xa00, is_1g ? 0x0 : 0xa00);
+ mtk_phy_update_bits(xfi_tphy->base + 0x2040, 0x340000, is_1g ? 0x200000 : 0x140000);
+
+ /* Setup RXFE BW setting */
+ mtk_phy_update_bits(xfi_tphy->base + 0x50f0, 0xc10, is_1g ? 0x410 : is_5g ? 0x800 : 0x400);
+ mtk_phy_update_bits(xfi_tphy->base + 0x50e0, 0x4000, is_5g ? 0x0 : 0x4000);
+
+ /* Setup RX CDR setting */
+ mtk_phy_update_bits(xfi_tphy->base + 0x506c, 0x30000, is_5g ? 0x0 : 0x30000);
+ mtk_phy_update_bits(xfi_tphy->base + 0x5070, 0x670000, is_5g ? 0x620000 : 0x50000);
+ mtk_phy_update_bits(xfi_tphy->base + 0x5074, 0x180000, is_5g ? 0x180000 : 0x0);
+ mtk_phy_update_bits(xfi_tphy->base + 0x5078, 0xf000400, is_5g ? 0x8000000 :
+ 0x7000400);
+ mtk_phy_update_bits(xfi_tphy->base + 0x507c, 0x5000500, is_5g ? 0x4000400 :
+ 0x1000100);
+ mtk_phy_update_bits(xfi_tphy->base + 0x5080, 0x1410, is_1g ? 0x400 : is_5g ? 0x1010 : 0x0);
+ mtk_phy_update_bits(xfi_tphy->base + 0x5084, 0x30300, is_1g ? 0x30300 :
+ is_5g ? 0x30100 :
+ 0x100);
+ mtk_phy_update_bits(xfi_tphy->base + 0x5088, 0x60200, is_1g ? 0x20200 :
+ is_5g ? 0x40000 :
+ 0x20000);
+
+ /* Setting RXFE adaptation range setting */
+ mtk_phy_update_bits(xfi_tphy->base + 0x50e4, 0xc0000, is_5g ? 0x0 : 0xc0000);
+ mtk_phy_update_bits(xfi_tphy->base + 0x50e8, 0x40000, is_5g ? 0x0 : 0x40000);
+ mtk_phy_update_bits(xfi_tphy->base + 0x50ec, 0xa00, is_1g ? 0x200 : 0x800);
+ mtk_phy_update_bits(xfi_tphy->base + 0x50a8, 0xee0000, is_5g ? 0x800000 :
+ 0x6e0000);
+ mtk_phy_update_bits(xfi_tphy->base + 0x6004, 0x190000, is_5g ? 0x0 : 0x190000);
+
+ if (is_10g)
+ writel(0x01423342, xfi_tphy->base + 0x00f8);
+ else if (is_5g)
+ writel(0x00a132a1, xfi_tphy->base + 0x00f8);
+ else if (is_2p5g)
+ writel(0x009c329c, xfi_tphy->base + 0x00f8);
+ else
+ writel(0x00fa32fa, xfi_tphy->base + 0x00f8);
+
+ /* Force SGDT_OUT off and select PCS */
+ mtk_phy_update_bits(xfi_tphy->base + REG_DIG_GLB_F4,
+ XFI_DPHY_AD_SGDT_FRC_EN | XFI_DPHY_PCS_SEL,
+ XFI_DPHY_AD_SGDT_FRC_EN |
+ (use_lynxi_pcs ? XFI_DPHY_PCS_SEL_SGMII :
+ XFI_DPHY_PCS_SEL_USXGMII));
+
+ /* Force GLB_CKDET_OUT */
+ mtk_phy_set_bits(xfi_tphy->base + 0x0030, 0xc00);
+
+ /* Force AEQ on */
+ writel(XTP_PCS_RX_EQ_IN_PROGRESS(2) | XTP_PCS_PWD_SYNC(2) | XTP_PCS_PWD_ASYNC(2),
+ xfi_tphy->base + REG_DIG_GLB_70);
+
+ usleep_range(1, 5);
+ writel(XTP_LN_FRC_TX_DATA_EN, xfi_tphy->base + REG_DIG_LN_TRX_40);
+
+ /* Setup TX DA default value */
+ mtk_phy_update_bits(xfi_tphy->base + 0x30b0, 0x30, 0x20);
+ writel(0x00008a01, xfi_tphy->base + 0x3028);
+ writel(0x0000a884, xfi_tphy->base + 0x302c);
+ writel(0x00083002, xfi_tphy->base + 0x3024);
+
+ /* Setup RG default value */
+ if (use_lynxi_pcs) {
+ writel(0x00011110, xfi_tphy->base + 0x3010);
+ writel(0x40704000, xfi_tphy->base + 0x3048);
+ } else {
+ writel(0x00022220, xfi_tphy->base + 0x3010);
+ writel(0x0f020a01, xfi_tphy->base + 0x5064);
+ writel(0x06100600, xfi_tphy->base + 0x50b4);
+ if (interface == PHY_INTERFACE_MODE_USXGMII)
+ writel(0x40704000, xfi_tphy->base + 0x3048);
+ else
+ writel(0x47684100, xfi_tphy->base + 0x3048);
+ }
+
+ if (is_1g)
+ writel(0x0000c000, xfi_tphy->base + 0x3064);
+
+ /* Setup RX EQ initial value */
+ mtk_phy_update_bits(xfi_tphy->base + 0x3050, 0xa8000000,
+ (interface != PHY_INTERFACE_MODE_10GBASER) ? 0xa8000000 : 0x0);
+ mtk_phy_update_bits(xfi_tphy->base + 0x3054, 0xaa,
+ (interface != PHY_INTERFACE_MODE_10GBASER) ? 0xaa : 0x0);
+
+ if (!use_lynxi_pcs)
+ writel(0x00000f00, xfi_tphy->base + 0x306c);
+ else if (is_2p5g)
+ writel(0x22000f00, xfi_tphy->base + 0x306c);
+ else
+ writel(0x20200f00, xfi_tphy->base + 0x306c);
+
+ mtk_phy_update_bits(xfi_tphy->base + 0xa008, 0x10000, da_war ? 0x10000 : 0x0);
+
+ mtk_phy_update_bits(xfi_tphy->base + 0xa060, 0x50000, use_lynxi_pcs ? 0x50000 : 0x40000);
+
+ /* Setup PHYA speed */
+ mtk_phy_update_bits(xfi_tphy->base + REG_ANA_GLB_D0,
+ XTP_GLB_USXGMII_SEL_MASK | XTP_GLB_USXGMII_EN,
+ is_10g ? XTP_GLB_USXGMII_SEL(0) :
+ is_5g ? XTP_GLB_USXGMII_SEL(1) :
+ is_2p5g ? XTP_GLB_USXGMII_SEL(2) :
+ XTP_GLB_USXGMII_SEL(3));
+ mtk_phy_set_bits(xfi_tphy->base + REG_ANA_GLB_D0, XTP_GLB_USXGMII_EN);
+
+ /* Release reset */
+ mtk_phy_set_bits(xfi_tphy->base + REG_DIG_GLB_70,
+ XTP_PCS_RST_B | XTP_FRC_PCS_RST_B);
+ usleep_range(150, 500);
+
+ /* Switch to P0 */
+ mtk_phy_update_bits(xfi_tphy->base + REG_DIG_GLB_70,
+ XTP_PCS_IN_FR_RG |
+ XTP_FRC_PCS_PWD_ASYNC |
+ XTP_PCS_PWD_ASYNC_MASK |
+ XTP_PCS_PWD_SYNC_MASK |
+ XTP_PCS_UPDT,
+ XTP_PCS_IN_FR_RG |
+ XTP_FRC_PCS_PWD_ASYNC |
+ XTP_PCS_UPDT);
+ usleep_range(1, 5);
+
+ mtk_phy_clear_bits(xfi_tphy->base + REG_DIG_GLB_70, XTP_PCS_UPDT);
+ usleep_range(15, 50);
+
+ if (use_lynxi_pcs) {
+ /* Switch to Gen2 */
+ mtk_phy_update_bits(xfi_tphy->base + REG_DIG_GLB_70,
+ XTP_PCS_MODE_MASK | XTP_PCS_UPDT,
+ XTP_PCS_MODE(1) | XTP_PCS_UPDT);
+ } else {
+ /* Switch to Gen3 */
+ mtk_phy_update_bits(xfi_tphy->base + REG_DIG_GLB_70,
+ XTP_PCS_MODE_MASK | XTP_PCS_UPDT,
+ XTP_PCS_MODE(2) | XTP_PCS_UPDT);
+ }
+ usleep_range(1, 5);
+
+ mtk_phy_clear_bits(xfi_tphy->base + REG_DIG_GLB_70, XTP_PCS_UPDT);
+
+ usleep_range(100, 500);
+
+ /* Enable MAC CK */
+ mtk_phy_set_bits(xfi_tphy->base + REG_DIG_LN_TRX_B0, XTP_LN_TX_MACCK_EN);
+ mtk_phy_clear_bits(xfi_tphy->base + REG_DIG_GLB_F4, XFI_DPHY_AD_SGDT_FRC_EN);
+
+ /* Enable TX data */
+ mtk_phy_set_bits(xfi_tphy->base + REG_DIG_LN_TRX_40,
+ XTP_LN_FRC_TX_DATA_EN | XTP_LN_TX_DATA_EN);
+ usleep_range(400, 1000);
+}
+
+/**
+ * mtk_xfi_tphy_set_mode() - Setup phy for specified interface mode.
+ *
+ * @phy: Phy instance.
+ * @mode: Only PHY_MODE_ETHERNET is supported.
+ * @submode: An Ethernet interface mode.
+ *
+ * Validate selected mode and call function mtk_xfi_tphy_setup().
+ *
+ * Return:
+ * * %0 - OK
+ * * %-EINVAL - invalid mode
+ */
+static int mtk_xfi_tphy_set_mode(struct phy *phy, enum phy_mode mode, int
+ submode)
+{
+ struct mtk_xfi_tphy *xfi_tphy = phy_get_drvdata(phy);
+
+ if (mode != PHY_MODE_ETHERNET)
+ return -EINVAL;
+
+ switch (submode) {
+ case PHY_INTERFACE_MODE_1000BASEX:
+ case PHY_INTERFACE_MODE_2500BASEX:
+ case PHY_INTERFACE_MODE_SGMII:
+ case PHY_INTERFACE_MODE_5GBASER:
+ case PHY_INTERFACE_MODE_10GBASER:
+ case PHY_INTERFACE_MODE_USXGMII:
+ mtk_xfi_tphy_setup(xfi_tphy, submode);
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+/**
+ * mtk_xfi_tphy_reset() - Reset the phy.
+ *
+ * @phy: Phy instance.
+ *
+ * Reset the phy using the external reset controller.
+ *
+ * Return:
+ * %0 - OK
+ */
+static int mtk_xfi_tphy_reset(struct phy *phy)
+{
+ struct mtk_xfi_tphy *xfi_tphy = phy_get_drvdata(phy);
+
+ reset_control_assert(xfi_tphy->reset);
+ usleep_range(100, 500);
+ reset_control_deassert(xfi_tphy->reset);
+ usleep_range(1, 10);
+
+ return 0;
+}
+
+/**
+ * mtk_xfi_tphy_power_on() - Power-on the phy.
+ *
+ * @phy: Phy instance.
+ *
+ * Prepare and enable all clocks required for the phy to operate.
+ *
+ * Return:
+ * See clk_bulk_prepare_enable().
+ */
+static int mtk_xfi_tphy_power_on(struct phy *phy)
+{
+ struct mtk_xfi_tphy *xfi_tphy = phy_get_drvdata(phy);
+
+ return clk_bulk_prepare_enable(MTK_XFI_TPHY_NUM_CLOCKS, xfi_tphy->clocks);
+}
+
+/**
+ * mtk_xfi_tphy_power_off() - Power-off the phy.
+ *
+ * @phy: Phy instance.
+ *
+ * Disable and unprepare all clocks previously enabled.
+ *
+ * Return:
+ * See clk_bulk_prepare_disable().
+ */
+static int mtk_xfi_tphy_power_off(struct phy *phy)
+{
+ struct mtk_xfi_tphy *xfi_tphy = phy_get_drvdata(phy);
+
+ clk_bulk_disable_unprepare(MTK_XFI_TPHY_NUM_CLOCKS, xfi_tphy->clocks);
+
+ return 0;
+}
+
+static const struct phy_ops mtk_xfi_tphy_ops = {
+ .power_on = mtk_xfi_tphy_power_on,
+ .power_off = mtk_xfi_tphy_power_off,
+ .set_mode = mtk_xfi_tphy_set_mode,
+ .reset = mtk_xfi_tphy_reset,
+ .owner = THIS_MODULE,
+};
+
+/**
+ * mtk_xfi_tphy_probe() - Probe phy instance from Device Tree.
+ * @pdev: Matching platform device.
+ *
+ * The probe function gets IO resource, clocks, reset controller and
+ * whether the DA work-around for 10GBase-R is required from Device Tree and
+ * allocates memory for holding that information in a struct mtk_xfi_tphy.
+ *
+ * Return:
+ * * %0 - OK
+ * * %-ENODEV - Missing associated Device Tree node (should never happen).
+ * * %-ENOMEM - Out of memory.
+ * * Any error value which devm_platform_ioremap_resource(),
+ * devm_clk_bulk_get(), devm_reset_control_get_exclusive(),
+ * devm_phy_create() or devm_of_phy_provider_register() may return.
+ */
+static int mtk_xfi_tphy_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct phy_provider *phy_provider;
+ struct mtk_xfi_tphy *xfi_tphy;
+ struct phy *phy;
+ int ret;
+
+ if (!np)
+ return -ENODEV;
+
+ xfi_tphy = devm_kzalloc(&pdev->dev, sizeof(*xfi_tphy), GFP_KERNEL);
+ if (!xfi_tphy)
+ return -ENOMEM;
+
+ xfi_tphy->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(xfi_tphy->base))
+ return PTR_ERR(xfi_tphy->base);
+
+ xfi_tphy->dev = &pdev->dev;
+ xfi_tphy->clocks[0].id = "topxtal";
+ xfi_tphy->clocks[1].id = "xfipll";
+ ret = devm_clk_bulk_get(&pdev->dev, MTK_XFI_TPHY_NUM_CLOCKS, xfi_tphy->clocks);
+ if (ret)
+ return ret;
+
+ xfi_tphy->reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+ if (IS_ERR(xfi_tphy->reset))
+ return PTR_ERR(xfi_tphy->reset);
+
+ xfi_tphy->da_war = of_property_read_bool(np, "mediatek,usxgmii-performance-errata");
+
+ phy = devm_phy_create(&pdev->dev, NULL, &mtk_xfi_tphy_ops);
+ if (IS_ERR(phy))
+ return PTR_ERR(phy);
+
+ phy_set_drvdata(phy, xfi_tphy);
+ phy_provider = devm_of_phy_provider_register(&pdev->dev, of_phy_simple_xlate);
+
+ return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id mtk_xfi_tphy_match[] = {
+ { .compatible = "mediatek,mt7988-xfi-tphy", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mtk_xfi_tphy_match);
+
+static struct platform_driver mtk_xfi_tphy_driver = {
+ .probe = mtk_xfi_tphy_probe,
+ .driver = {
+ .name = "mtk-xfi-tphy",
+ .of_match_table = mtk_xfi_tphy_match,
+ },
+};
+module_platform_driver(mtk_xfi_tphy_driver);
+
+MODULE_DESCRIPTION("MediaTek 10GE SerDes XFI T-PHY driver");
+MODULE_AUTHOR("Daniel Golle <daniel@makrotopia.org>");
+MODULE_AUTHOR("Bc-bocun Chen <bc-bocun.chen@mediatek.com>");
+MODULE_LICENSE("GPL");
--
2.44.0
^ permalink raw reply related
* Re: [PATCH v2 0/2] Add support for AD4000 series
From: David Lechner @ 2024-04-09 2:54 UTC (permalink / raw)
To: Marcelo Schmitt
Cc: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt, marcelo.schmitt1, linux-iio, devicetree, linux-kernel
In-Reply-To: <cover.1712585500.git.marcelo.schmitt@analog.com>
On Mon, Apr 8, 2024 at 9:31 AM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> This is more like an RFC patch set since configuration read/write is currently
> buggy.
>
> Change log v1 -> v2:
> - Took device tree provided by David.
> - Dropped ABI additions in favor of device tree properties.
> - Set differential IIO channel subtype for differential ADCs.
> - Set scan_type shift bits to mask out correct real bits from buffer.
> - Added __aligned(8) to buffer timestamp.
> - Used union to reduce buffer memory usage for 16-bit devices.
> - Used SPI transfer functions rather than SPI message.
> - Used c99 style structure initialization.
> - Used iio_device_claim_direct_scoped().
> - Removed unneeded pointer casts.
> - Added other power supplies (VDD and VIO).
>
> Link to v1: https://lore.kernel.org/linux-iio/cover.1711131830.git.marcelo.schmitt@analog.com/
>
> Additional topics:
>
> - Why there is no different handling for the different SPI wiring modes?
> It looks like there is no need for different handling of "4-wire" and "3-wire"
> modes.
> If in "4-wire" (dt default mode), SDI is connected to SPI controller CS and
> CNV is active high. We can activate the CNV GPIO then let the SPI controller
> bring CS (connected to SDI) down when starting the transfer.
> If in "3-wire" (dt single mode), if we have a CNV (active low) GPIO we activate
> it and then proceed with with the transfer. If controller CS is connected to
> CNV it works the same way.
> I'm thinking it's better if we can support these devices in similar way
> other SPI ADCs are supported. Does that make sense?
In the AD7944 driver, I handled the "3-wire" mode separately because
the sample conversion is triggered on the rising edge of the CNV line.
In "4-wire" mode, since we have a GPIO connected to CNV, we can just
toggle the GPIO from low to high, wait for the conversion time
(t_CONV) and then read the sample (SPI xfer) then toggle the CNV line
low again. In 3-wire mode, the CS line is connected to the CNV pin, so
in order to get an up-to-date sample, we need to to toggle the CS line
from low to high to trigger a conversion (spi xfer with no data, only
delay), then wait for the conversion time, then read the sample (2nd
spi xfer). So in "4-wire" mode, the CS line is getting toggled once
per sample, but in "3-wire" mode, it is getting toggled twice per
sample. I didn't add support for "3-wire" mode where CNV is connected
to GPIO because we can't get max sample rate that way and it is
unusual to not have CS connected to something. But if we do that here,
the timing has to be different from 4-wire mode in order to not get
stale data.
> To me, the "3-wire" mode with controller CS to ADC CNV is what most resembles
> conventional SPI. The only important distinction is that the
> controller must be able to keep ADC SDI line high during conversions.
> Although, while the spi-engine implementation provided to me can keep SDI up
> during conversions, I'm not sure its a thing all SPI controllers can do.
> I tried a raspberry pi 4 some time ago and it was leaving the SDI line low if
> no tx buffer was provided. Even with a tx full of 1s the controller would
> bring SDI down between each 8 bits of transfer.
This is a good point. It sounds like additional bindings are needed to
describe the various wiring cases of the SDI line.
It sounds like possibilities are:
1. SDI is hard-wired high -> can't write to registers, CNV is
connected to SPI controller CS, chip is in "3-wire" mode. Currently
adi,spi-mode="single"
2. SDI is connected to SDO of another chip, SDI of last chip is
hard-wired low -> can't write to registers, CNV is connected to SPI
controller CS, chips are in daisy chain mode. Currently
adi,spi-mode="chain"
3. SDI is connected to SPI controller CS -> can't write registers,
chip can operate in 4-wire mode with CNV connected to GPIO, Currently
adi,spi-mode omitted.
4. SDI is connected to SPI controller SDO -> can write registers, and
support all writing modes (3-wire, 4-wire, daisy chain) as long as SPI
controller SDO line can be kept high or low at the appropriate time.
Currently not handled.
5. There could be a pin mux that switches between the one of the first
three and the 4th option (needed to avoid the issue with SPI
controller not being able to place the SDI pin in the correct state
during conversion trigger as described above).
On AD7944, the proposed adi,spi-mode property was sufficient to
describe what was wired to the SDI pin because we only had the first 3
options (the AD7944 doesn't have SPI registers to write to).
Also see related comments in my reply to the DT bindings patch.
(From the complete bindings point of view, we should probably also
consider the possibility of variations of 1. and 2. where CS of the
SPI controller is not wired and CNV is connected to a GPIO - this can
be determined by the combination of the adi,spi-mode property and the
presence or absence of the cnv-gpios property.)
> Anyway, single-shot and buffered reads work with the spi-engine controller
> with ADC in "3-wire"/single mode with controller CS line connected to ADC CNV
> pin which is how I've been testing it.
Technically, yes data can be captured in "3-wire" mode with a single
CS toggle, but then the data is stale and doesn't correspond to the
soft timestamp because it is reading the data from the previous
conversion triggered by the last SPI xfer, whenever that was. Since it
is trivial to avoid this by adding the extra CS/CNV toggle I describe
above, I don't see any reason not to.
But the way the driver is written now, it is actually only supporting
the unnamed wiring option 4 from above, so now I understand the
confusion about 3-wire vs. 4-wire mode in that context.
>
> - Why did not make vref regulator optional?
> Other SAR ADCs I've seen needed a voltage reference otherwise they simply
> could not provide any reasonable readings. Isn't it preferable to fail rather
> than having a device that can't provide reliable data?
In the device tree bindings, making vref-supply required makes sense
since there is no internal reference. In the driver, as discussed in
V1, it will fail if vref-supply in regulator_get_voltage() if
vref-supply is missing and we use devm_regulator_get() instead of
devm_regulator_get_optional(). So leaving it as-is is fine. We have a
plan to clean this up later anyway.
>
> - Why did not split into AD and ADAQ patches?
> The main difference between AD and ADAQ is the amplifier in front of the ADC.
> If only supporting AD, we could probably avoid the scale table since it would
> only have two possible values per ADC. But then the handling of span compression
> scale would need refactoring to be in the scale table when adding ADAQ.
> I'm not excited to implement something knowing it will need rework in the
> following patch. Will do if required.
If it isn't that much work, it seems worth it to me. If the driver
work is too much, maybe just split the DT patch?
>
> - Span compression and offset.
> For non-differential ADCs, enabling the span compression requires an input offset.
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/AD4000-4004-4008.pdf
> page 18
> and
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> page 19
> I updated the _offset attribute for those ADCs according to span compression
> being enabled or not. Is it okay to have an attribute update cause an update to
> another one?
> Maybe also make the span compression a dt property and have it fixed after probe?
This doesn't sound like something that belongs in DT since it doesn't
depend on the physical properties of what is wired to the input.
But the fact that offset should not be read until after scale is set
sounds like a quirk that would be worth documenting in some
chip-specific docs.
>
> - Configuration register
> Despite it doing single-shot and buffered captures, read and writes to the
> configuration register are currently buggy. It is as if the register was
> "floating". I tried setting up buffers like ad7768-1, adxl355_core, bma220_spi,
> bma400_core, and mcp3911.
If the ADC CNV pin is connected to a GPIO and the ADC SDI pin is
connected to SDO of the SPI controller, then nothing is connected to
CS of the SPI controller, so that might be the problem.
>
>
> Thanks,
> Marcelo
>
> Marcelo Schmitt (2):
> dt-bindings: iio: adc: Add AD4000
> iio: adc: Add support for AD4000
>
> .../bindings/iio/adc/adi,ad4000.yaml | 201 ++++++
> MAINTAINERS | 8 +
> drivers/iio/adc/Kconfig | 12 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ad4000.c | 649 ++++++++++++++++++
> 5 files changed, 871 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> create mode 100644 drivers/iio/adc/ad4000.c
>
> --
> 2.43.0
>
>
^ permalink raw reply
* Re: [PATCH v3 2/2] media: i2c: Add GC05A2 image sensor driver
From: Zhi Mao (毛智) @ 2024-04-09 2:56 UTC (permalink / raw)
To: mchehab@kernel.org, sakari.ailus@linux.intel.com,
robh+dt@kernel.org, kieran.bingham@ideasonboard.com,
krzysztof.kozlowski+dt@linaro.org
Cc: heiko@sntech.de, gerald.loacker@wolfvision.net,
linux-kernel@vger.kernel.org, yunkec@chromium.org,
linux-mediatek@lists.infradead.org, dan.scally@ideasonboard.com,
linux-media@vger.kernel.org,
Shengnan Wang (王圣男), hdegoede@redhat.com,
linus.walleij@linaro.org, andy.shevchenko@gmail.com,
Yaya Chang (張雅清), bingbu.cao@intel.com,
jacopo.mondi@ideasonboard.com, jernej.skrabec@gmail.com,
devicetree@vger.kernel.org, conor+dt@kernel.org,
tomi.valkeinen@ideasonboard.com, 10572168@qq.com,
hverkuil-cisco@xs4all.nl, linux-arm-kernel@lists.infradead.org,
Project_Global_Chro.me_Upstream_Group@mediatek.com,
matthias.bgg@gmail.com, laurent.pinchart@ideasonboard.com,
angelogioacchino.delregno@collabora.com, macromorgan@hotmail.com
In-Reply-To: <171258299292.1623123.5498855680488410646@ping.linuxembedded.co.uk>
On Mon, 2024-04-08 at 14:29 +0100, Kieran Bingham wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Quoting Zhi Mao (毛智) (2024-04-08 12:50:21)
> > Hi Kieran,
> >
> > Thanks for your review this patch.
> >
> > It seems that there are some difficult for us(Mediatek) to explain
> > these register setting comments.
> > As these settings are released by GC sensor vendor, and we have not
> > detailed datasheet described them.
> > And even if send the letter to ask sensor vendor, I am afraid there
> may
> > be not a clear response.
> >
> > Can we just focus on the driver code function and control flow
> part?
> >
>
> As I said - You can take my comments with a pinch of salt ... but I
> wanted to know your position on it ;-)
>
I agree with your opinion and have sent mail to vendor for these
comments.
If I got the feedback, I will sync information with you.
> --
> Kieran
>
> > On Sun, 2024-04-07 at 10:08 +0100, Kieran Bingham wrote:
> > >
> > > External email : Please do not click links or open attachments
> until
> > > you have verified the sender or the content.
> > > Hello,
> > >
> > > Thanks for helping extending the kernels sensor driver support.
> > >
> > > My comments below can likely be taken with a pinch of salt, as
> they
> > > are
> > > mostly around the tabled register values ... but we have many
> drivers
> > > which are binary blobs of sensor register values and I think it
> would
> > > be
> > > far more beneficial to clean these up where possible...
> > >
> > > So the first question is ... Can we ?
> > >
> > >
> > >
> > > Quoting Zhi Mao (2024-04-03 04:38:25)
> > > > Add a V4L2 sub-device driver for Galaxycore GC05A2 image
> sensor.
> > > >
> > > > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> > > > ---
> > > > drivers/media/i2c/Kconfig | 10 +
> > > > drivers/media/i2c/Makefile | 1 +
> > > > drivers/media/i2c/gc05a2.c | 1383
> > > ++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 1394 insertions(+)
> > > > create mode 100644 drivers/media/i2c/gc05a2.c
> > > >
> > > > diff --git a/drivers/media/i2c/Kconfig
> b/drivers/media/i2c/Kconfig
> > > > index 56f276b920ab..97993bf160f9 100644
> > > > --- a/drivers/media/i2c/Kconfig
> > > > +++ b/drivers/media/i2c/Kconfig
> > > > @@ -70,6 +70,16 @@ config VIDEO_GC0308
> > > > To compile this driver as a module, choose M here:
> the
> > > > module will be called gc0308.
> > > >
> > > > +config VIDEO_GC05A2
> > > > + tristate "GalaxyCore gc05a2 sensor support"
> > > > + select V4L2_CCI_I2C
> > > > + help
> > > > + This is a Video4Linux2 sensor driver for the
> GalaxyCore
> > > gc05a2
> > > > + camera.
> > > > +
> > > > + To compile this driver as a module, choose M here:
> the
> > > > + module will be called gc05a2.
> > > > +
> > > > config VIDEO_GC2145
> > > > select V4L2_CCI_I2C
> > > > tristate "GalaxyCore GC2145 sensor support"
> > > > diff --git a/drivers/media/i2c/Makefile
> > > b/drivers/media/i2c/Makefile
> > > > index dfbe6448b549..8ed6faf0f854 100644
> > > > --- a/drivers/media/i2c/Makefile
> > > > +++ b/drivers/media/i2c/Makefile
> > > > @@ -38,6 +38,7 @@ obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
> > > > obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
> > > > obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
> > > > obj-$(CONFIG_VIDEO_GC0308) += gc0308.o
> > > > +obj-$(CONFIG_VIDEO_GC05A2) += gc05a2.o
> > > > obj-$(CONFIG_VIDEO_GC2145) += gc2145.o
> > > > obj-$(CONFIG_VIDEO_HI556) += hi556.o
> > > > obj-$(CONFIG_VIDEO_HI846) += hi846.o
> > > > diff --git a/drivers/media/i2c/gc05a2.c
> > > b/drivers/media/i2c/gc05a2.c
> > > > new file mode 100644
> > > > index 000000000000..461d33055a3b
> > > > --- /dev/null
> > > > +++ b/drivers/media/i2c/gc05a2.c
> > > > @@ -0,0 +1,1383 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Driver for GalaxyCore gc05a2 image sensor
> > > > + *
> > > > + * Copyright 2024 MediaTek
> > > > + *
> > > > + * Zhi Mao <zhi.mao@mediatek.com>
> > > > + */
> > > > +#include <linux/array_size.h>
> > > > +#include <linux/bits.h>
> > > > +#include <linux/clk.h>
> > > > +#include <linux/container_of.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/gpio/consumer.h>
> > > > +#include <linux/math64.h>
> > > > +#include <linux/mod_devicetable.h>
> > > > +#include <linux/pm_runtime.h>
> > > > +#include <linux/property.h>
> > > > +#include <linux/regulator/consumer.h>
> > > > +#include <linux/types.h>
> > > > +#include <linux/units.h>
> > > > +
> > > > +#include <media/v4l2-cci.h>
> > > > +#include <media/v4l2-ctrls.h>
> > > > +#include <media/v4l2-event.h>
> > > > +#include <media/v4l2-fwnode.h>
> > > > +#include <media/v4l2-subdev.h>
> > > > +
> > > > +#define GC05A2_REG_TEST_PATTERN_EN CCI_REG8(0x008c)
> > > > +#define GC05A2_REG_TEST_PATTERN_IDX CCI_REG8(0x008d)
> > > > +#define GC05A2_TEST_PATTERN_EN 0x01
> > > > +
> > > > +#define GC05A2_STREAMING_REG CCI_REG8(0x0100)
> > > > +
> > > > +#define GC05A2_FLIP_REG CCI_REG8(0x0101)
> > > > +#define GC05A2_FLIP_H_MASK BIT(0)
> > > > +#define GC05A2_FLIP_V_MASK BIT(1)
> > > > +
> > > > +#define GC05A2_EXP_REG CCI_REG16(0x0202)
> > > > +#define GC05A2_EXP_MARGIN 16
> > > > +#define GC05A2_EXP_MIN 4
> > > > +#define GC05A2_EXP_STEP 1
> > > > +
> > > > +#define GC05A2_AGAIN_REG CCI_REG16(0x0204)
> > > > +#define GC05A2_AGAIN_MIN 1024
> > > > +#define GC05A2_AGAIN_MAX (1024 * 16)
> > > > +#define GC05A2_AGAIN_STEP 1
> > > > +
> > > > +#define GC05A2_FRAME_LENGTH_REG CCI_REG16(0x0340)
> > > > +#define GC05A2_VTS_MAX 0xffff
> > > > +
> > > > +#define GC05A2_REG_CHIP_ID CCI_REG16(0x03f0)
> > > > +#define GC05A2_CHIP_ID 0x05a2
> > > > +
> > > > +#define GC05A2_NATIVE_WIDTH 2592
> > > > +#define GC05A2_NATIVE_HEIGHT 1944
> > > > +
> > > > +#define GC05A2_DEFAULT_CLK_FREQ (24 * HZ_PER_MHZ)
> > > > +#define GC05A2_MBUS_CODE MEDIA_BUS_FMT_SGRBG10_1X10
> > > > +#define GC05A2_DATA_LANES 2
> > > > +#define GC05A2_RGB_DEPTH 10
> > > > +#define GC05A2_SLEEP_US (2 * USEC_PER_MSEC)
> > > > +
> > > > +static const char *const gc05a2_test_pattern_menu[] = {
> > > > + "No Pattern", "Fade_to_gray_Color Bar", "Color Bar",
> > > > + "PN9", "Horizental_gradient", "Checkboard
> > > Pattern",
> > > > + "Slant", "Resolution", "Solid Black",
> > > > + "Solid White",
> > > > +};
> > > > +
> > > > +static const s64 gc05a2_link_freq_menu_items[] = {
> > > > + (448 * HZ_PER_MHZ),
> > > > + (224 * HZ_PER_MHZ),
> > > > +};
> > > > +
> > > > +static const char *const gc05a2_supply_name[] = {
> > > > + "avdd",
> > > > + "dvdd",
> > > > + "dovdd",
> > > > +};
> > > > +
> > > > +struct gc05a2 {
> > > > + struct device *dev;
> > > > + struct v4l2_subdev sd;
> > > > + struct media_pad pad;
> > > > +
> > > > + struct clk *xclk;
> > > > + struct regulator_bulk_data
> > > supplies[ARRAY_SIZE(gc05a2_supply_name)];
> > > > + struct gpio_desc *reset_gpio;
> > > > +
> > > > + struct v4l2_ctrl_handler ctrls;
> > > > + struct v4l2_ctrl *pixel_rate;
> > > > + struct v4l2_ctrl *link_freq;
> > > > + struct v4l2_ctrl *exposure;
> > > > + struct v4l2_ctrl *vblank;
> > > > + struct v4l2_ctrl *hblank;
> > > > + struct v4l2_ctrl *hflip;
> > > > + struct v4l2_ctrl *vflip;
> > > > +
> > > > + struct regmap *regmap;
> > > > + unsigned long link_freq_bitmap;
> > > > +
> > > > + /* True if the device has been identified */
> > > > + bool identified;
> > > > + const struct gc05a2_mode *cur_mode;
> > > > +};
> > > > +
> > > > +struct gc05a2_reg_list {
> > > > + u32 num_of_regs;
> > > > + const struct cci_reg_sequence *regs;
> > > > +};
> > > > +
> > > > +static const struct cci_reg_sequence mode_2592x1944[] = {
> > > > + /* system */
> > > > + { CCI_REG8(0x0135), 0x01 },
> > > > +
> > > > + /* pre_setting */
> > > > + { CCI_REG8(0x0084), 0x21 },
> > > > + { CCI_REG8(0x0d05), 0xcc },
> > > > + { CCI_REG8(0x0218), 0x00 },
> > > > + { CCI_REG8(0x005e), 0x48 },
> > > > + { CCI_REG8(0x0d06), 0x01 },
> > > > + { CCI_REG8(0x0007), 0x16 },
> > > > + { CCI_REG8(0x0101), 0x00 },
> > > > +
> > > > + /* analog */
> > > > + { CCI_REG8(0x0342), 0x07 },
> > > > + { CCI_REG8(0x0343), 0x28 },
> > > > + { CCI_REG8(0x0220), 0x07 },
> > > > + { CCI_REG8(0x0221), 0xd0 },
> > > > + { CCI_REG8(0x0202), 0x07 },
> > > > + { CCI_REG8(0x0203), 0x32 },
> > > > + { CCI_REG8(0x0340), 0x07 },
> > > > + { CCI_REG8(0x0341), 0xf0 },
> > > > + { CCI_REG8(0x0219), 0x00 },
> > > > + { CCI_REG8(0x0346), 0x00 },
> > > > + { CCI_REG8(0x0347), 0x04 },
> > > > + { CCI_REG8(0x0d14), 0x00 },
> > > > + { CCI_REG8(0x0d13), 0x05 },
> > > > + { CCI_REG8(0x0d16), 0x05 },
> > > > + { CCI_REG8(0x0d15), 0x1d },
> > > > + { CCI_REG8(0x00c0), 0x0a },
> > > > + { CCI_REG8(0x00c1), 0x30 },
> > > > + { CCI_REG8(0x034a), 0x07 },
> > > > + { CCI_REG8(0x034b), 0xa8 },
> > > > + { CCI_REG8(0x0e0a), 0x00 },
> > > > + { CCI_REG8(0x0e0b), 0x00 },
> > > > + { CCI_REG8(0x0e0e), 0x03 },
> > > > + { CCI_REG8(0x0e0f), 0x00 },
> > > > + { CCI_REG8(0x0e06), 0x0a },
> > > > + { CCI_REG8(0x0e23), 0x15 },
> > > > + { CCI_REG8(0x0e24), 0x15 },
> > > > + { CCI_REG8(0x0e2a), 0x10 },
> > > > + { CCI_REG8(0x0e2b), 0x10 },
> > > > + { CCI_REG8(0x0e17), 0x49 },
> > > > + { CCI_REG8(0x0e1b), 0x1c },
> > > > + { CCI_REG8(0x0e3a), 0x36 },
> > > > + { CCI_REG8(0x0d11), 0x84 },
> > > > + { CCI_REG8(0x0e52), 0x14 },
> > > > + { CCI_REG8(0x000b), 0x10 },
> > > > + { CCI_REG8(0x0008), 0x08 },
> > > > + { CCI_REG8(0x0223), 0x17 },
> > > > + { CCI_REG8(0x0d27), 0x39 },
> > > > + { CCI_REG8(0x0d22), 0x00 },
> > > > + { CCI_REG8(0x03f6), 0x0d },
> > > > + { CCI_REG8(0x0d04), 0x07 },
> > > > + { CCI_REG8(0x03f3), 0x72 },
> > > > + { CCI_REG8(0x03f4), 0xb8 },
> > > > + { CCI_REG8(0x03f5), 0xbc },
> > > > + { CCI_REG8(0x0d02), 0x73 },
> > > > +
> > > > + /* auto load start */
> > > > + { CCI_REG8(0x00cb), 0x00 },
> > > > +
> > > > + /* OUT 2592*1944 */
> > > > + { CCI_REG8(0x0350), 0x01 },
> > > > + { CCI_REG8(0x0353), 0x00 },
> > > > + { CCI_REG8(0x0354), 0x08 },
> > >
> > > > + { CCI_REG8(0x034c), 0x0a },
> > > > + { CCI_REG8(0x034d), 0x20 },
> > >
> > > Should/Could this be
> > > { CCI_REG16(0x034c), 2592 }, /* Width */
> > >
> > >
> > > > + { CCI_REG8(0x021f), 0x14 },
> > > > +
> > > > + /* MIPI */
> > > > + { CCI_REG8(0x0107), 0x05 },
> > > > + { CCI_REG8(0x0117), 0x01 },
> > > > + { CCI_REG8(0x0d81), 0x00 },
> > > > + { CCI_REG8(0x0d84), 0x0c },
> > > > + { CCI_REG8(0x0d85), 0xa8 },
> > > > + { CCI_REG8(0x0d86), 0x06 },
> > > > + { CCI_REG8(0x0d87), 0x55 },
> > > > + { CCI_REG8(0x0db3), 0x06 },
> > > > + { CCI_REG8(0x0db4), 0x08 },
> > > > + { CCI_REG8(0x0db5), 0x1e },
> > > > + { CCI_REG8(0x0db6), 0x02 },
> > > > + { CCI_REG8(0x0db8), 0x12 },
> > > > + { CCI_REG8(0x0db9), 0x0a },
> > > > + { CCI_REG8(0x0d93), 0x06 },
> > > > + { CCI_REG8(0x0d94), 0x09 },
> > > > + { CCI_REG8(0x0d95), 0x0d },
> > > > + { CCI_REG8(0x0d99), 0x0b },
> > > > + { CCI_REG8(0x0084), 0x01 },
> > > > +
> > > > + /* OUT */
> > > > + { CCI_REG8(0x0110), 0x01 },
> > > > +};
> > > > +
> > > > +static const struct cci_reg_sequence mode_1280x720[] = {
> > > > + /* system */
> > > > + { CCI_REG8(0x0135), 0x05 },
> > >
> > > In 2592x1944 this is 0x01. Do you have a datasheet? Can you
> explain
> > > why
> > > they are different? Can you add register definitions that have
> names
> > > to
> > > make this more maintainable or extendable in the future?
> > >
> > > There's discussion in the recent series improving the IMX258
> which
> > > makes
> > > me wonder if we should try harder to have sensor drivers with
> clearer
> > > definitions.
> > >
> > >
> > > > +
> > > > + /*pre_setting*/
> > >
> > > /* pre_setting */ ?
> > >
> > > > + { CCI_REG8(0x0084), 0x21 },
> > > > + { CCI_REG8(0x0d05), 0xcc },
> > > > + { CCI_REG8(0x0218), 0x80 },
> > > > + { CCI_REG8(0x005e), 0x49 },
> > > > + { CCI_REG8(0x0d06), 0x81 },
> > > > + { CCI_REG8(0x0007), 0x16 },
> > > > + { CCI_REG8(0x0101), 0x00 },
> > >
> > > In 2592x1944, only register 0x0218 differs. Why? What is that?
> Can it
> > > be
> > > broken out to a function that applies the correct configuration
> at
> > > startuup based on a parameter instead of duplicating this table
> set?
> > >
> > > > +
> > > > + /* analog */
> > > > + { CCI_REG8(0x0342), 0x07 },
> > > > + { CCI_REG8(0x0343), 0x10 },
> > > > + { CCI_REG8(0x0220), 0x07 },
> > > > + { CCI_REG8(0x0221), 0xd0 },
> > > > + { CCI_REG8(0x0202), 0x03 },
> > > > + { CCI_REG8(0x0203), 0x32 },
> > > > + { CCI_REG8(0x0340), 0x04 },
> > > > + { CCI_REG8(0x0341), 0x08 },
> > > > + { CCI_REG8(0x0219), 0x00 },
> > > > + { CCI_REG8(0x0346), 0x01 },
> > > > + { CCI_REG8(0x0347), 0x00 },
> > > > + { CCI_REG8(0x0d14), 0x00 },
> > > > + { CCI_REG8(0x0d13), 0x05 },
> > > > + { CCI_REG8(0x0d16), 0x05 },
> > > > + { CCI_REG8(0x0d15), 0x1d },
> > > > + { CCI_REG8(0x00c0), 0x0a },
> > > > + { CCI_REG8(0x00c1), 0x30 },
> > > > + { CCI_REG8(0x034a), 0x05 },
> > > > + { CCI_REG8(0x034b), 0xb0 },
> > > > + { CCI_REG8(0x0e0a), 0x00 },
> > > > + { CCI_REG8(0x0e0b), 0x00 },
> > > > + { CCI_REG8(0x0e0e), 0x03 },
> > > > + { CCI_REG8(0x0e0f), 0x00 },
> > > > + { CCI_REG8(0x0e06), 0x0a },
> > > > + { CCI_REG8(0x0e23), 0x15 },
> > > > + { CCI_REG8(0x0e24), 0x15 },
> > > > + { CCI_REG8(0x0e2a), 0x10 },
> > > > + { CCI_REG8(0x0e2b), 0x10 },
> > > > + { CCI_REG8(0x0e17), 0x49 },
> > > > + { CCI_REG8(0x0e1b), 0x1c },
> > > > + { CCI_REG8(0x0e3a), 0x36 },
> > > > + { CCI_REG8(0x0d11), 0x84 },
> > > > + { CCI_REG8(0x0e52), 0x14 },
> > > > + { CCI_REG8(0x000b), 0x0e },
> > > > + { CCI_REG8(0x0008), 0x03 },
> > > > + { CCI_REG8(0x0223), 0x16 },
> > > > + { CCI_REG8(0x0d27), 0x39 },
> > > > + { CCI_REG8(0x0d22), 0x00 },
> > > > + { CCI_REG8(0x03f6), 0x0d },
> > > > + { CCI_REG8(0x0d04), 0x07 },
> > > > + { CCI_REG8(0x03f3), 0x72 },
> > > > + { CCI_REG8(0x03f4), 0xb8 },
> > > > + { CCI_REG8(0x03f5), 0xbc },
> > > > + { CCI_REG8(0x0d02), 0x73 },
> > > > +
> > >
> > > Are any of those able to be broken out to named register to be
> more
> > > clear in their intent?
> > >
> > > > + /* auto load start */
> > > > + { CCI_REG8(0x00cb), 0xfc },
> > > > +
> > >
> > > Why is this auto load start so different to the other modes 'auto
> > > load
> > > start'? What do the bits refer to ?
> > >
> > > > + /* OUT 1280x720 */
> > > > + { CCI_REG8(0x0350), 0x01 },
> > > > + { CCI_REG8(0x0353), 0x00 },
> > > > + { CCI_REG8(0x0354), 0x0c },
> > >
> > > > + { CCI_REG8(0x034c), 0x05 },
> > > > + { CCI_REG8(0x034d), 0x00 },
> > >
> > > Should/Could this be
> > > { CCI_REG16(0x034c), 1280 },
> > >
> > > Are there any other register settings that would make more sense
> to
> > > be
> > > in decimal units that match their actual context?
> > >
> > >
> > > > + { CCI_REG8(0x021f), 0x14 },
> > >
> > > I don't see a setting for 720/0x2d0. Do these registers only set
> the
> > > width?
> > >
> > > > +
> > > > + /* MIPI */
> > > > + { CCI_REG8(0x0107), 0x05 },
> > > > + { CCI_REG8(0x0117), 0x01 },
> > > > + { CCI_REG8(0x0d81), 0x00 },
> > > > + { CCI_REG8(0x0d84), 0x06 },
> > > > + { CCI_REG8(0x0d85), 0x40 },
> > > > + { CCI_REG8(0x0d86), 0x03 },
> > > > + { CCI_REG8(0x0d87), 0x21 },
> > > > + { CCI_REG8(0x0db3), 0x03 },
> > > > + { CCI_REG8(0x0db4), 0x04 },
> > > > + { CCI_REG8(0x0db5), 0x0d },
> > > > + { CCI_REG8(0x0db6), 0x01 },
> > > > + { CCI_REG8(0x0db8), 0x04 },
> > > > + { CCI_REG8(0x0db9), 0x06 },
> > > > + { CCI_REG8(0x0d93), 0x03 },
> > > > + { CCI_REG8(0x0d94), 0x04 },
> > > > + { CCI_REG8(0x0d95), 0x05 },
> > > > + { CCI_REG8(0x0d99), 0x06 },
> > > > + { CCI_REG8(0x0084), 0x01 },
> > > > +
> > > > + /* OUT */
> > >
> > > Out where? What is out?
> > >
> > > > + { CCI_REG8(0x0110), 0x01 },
> > > > +};
> > > > +
> > > > +static const struct cci_reg_sequence mode_table_common[] = {
> > > > + { GC05A2_STREAMING_REG, 0x00 },
> > > > + /* system */
> > > > + { CCI_REG8(0x0315), 0xd4 },
> > > > + { CCI_REG8(0x0d06), 0x01 },
> > > > + { CCI_REG8(0x0a70), 0x80 },
> > > > + { CCI_REG8(0x031a), 0x00 },
> > > > + { CCI_REG8(0x0314), 0x00 },
> > > > + { CCI_REG8(0x0130), 0x08 },
> > > > + { CCI_REG8(0x0132), 0x01 },
> > > > + { CCI_REG8(0x0136), 0x38 },
> > > > + { CCI_REG8(0x0137), 0x03 },
> > > > + { CCI_REG8(0x0134), 0x5b },
> > > > + { CCI_REG8(0x031c), 0xe0 },
> > > > + { CCI_REG8(0x0d82), 0x14 },
> > > > + { CCI_REG8(0x0dd1), 0x56 },
> > > > +
> > > > + /* gate_mode */
> > > > + { CCI_REG8(0x0af4), 0x01 },
> > > > + { CCI_REG8(0x0002), 0x10 },
> > > > + { CCI_REG8(0x00c3), 0x34 },
> > > > +
> > > > + /* auto load start */
> > >
> > > The previous 'auto load start' referenced 0x00cb ?
> > >
> > > > + { CCI_REG8(0x00c4), 0x00 },
> > > > + { CCI_REG8(0x00c5), 0x01 },
> > > > + { CCI_REG8(0x0af6), 0x00 },
> > > > + { CCI_REG8(0x0ba0), 0x17 },
> > > > + { CCI_REG8(0x0ba1), 0x00 },
> > > > + { CCI_REG8(0x0ba2), 0x00 },
> > > > + { CCI_REG8(0x0ba3), 0x00 },
> > > > + { CCI_REG8(0x0ba4), 0x03 },
> > > > + { CCI_REG8(0x0ba5), 0x00 },
> > > > + { CCI_REG8(0x0ba6), 0x00 },
> > > > + { CCI_REG8(0x0ba7), 0x00 },
> > > > + { CCI_REG8(0x0ba8), 0x40 },
> > > > + { CCI_REG8(0x0ba9), 0x00 },
> > > > + { CCI_REG8(0x0baa), 0x00 },
> > > > + { CCI_REG8(0x0bab), 0x00 },
> > > > + { CCI_REG8(0x0bac), 0x40 },
> > > > + { CCI_REG8(0x0bad), 0x00 },
> > > > + { CCI_REG8(0x0bae), 0x00 },
> > > > + { CCI_REG8(0x0baf), 0x00 },
> > > > + { CCI_REG8(0x0bb0), 0x02 },
> > > > + { CCI_REG8(0x0bb1), 0x00 },
> > > > + { CCI_REG8(0x0bb2), 0x00 },
> > > > + { CCI_REG8(0x0bb3), 0x00 },
> > > > + { CCI_REG8(0x0bb8), 0x02 },
> > > > + { CCI_REG8(0x0bb9), 0x00 },
> > > > + { CCI_REG8(0x0bba), 0x00 },
> > > > + { CCI_REG8(0x0bbb), 0x00 },
> > > > + { CCI_REG8(0x0a70), 0x80 },
> > > > + { CCI_REG8(0x0a71), 0x00 },
> > > > + { CCI_REG8(0x0a72), 0x00 },
> > > > + { CCI_REG8(0x0a66), 0x00 },
> > > > + { CCI_REG8(0x0a67), 0x80 },
> > > > + { CCI_REG8(0x0a4d), 0x4e },
> > > > + { CCI_REG8(0x0a50), 0x00 },
> > > > + { CCI_REG8(0x0a4f), 0x0c },
> > > > + { CCI_REG8(0x0a66), 0x00 },
> > > > + { CCI_REG8(0x00ca), 0x00 },
> > > > + { CCI_REG8(0x00cc), 0x00 },
> > > > + { CCI_REG8(0x00cd), 0x00 },
> > > > + { CCI_REG8(0x0aa1), 0x00 },
> > > > + { CCI_REG8(0x0aa2), 0xe0 },
> > > > + { CCI_REG8(0x0aa3), 0x00 },
> > > > + { CCI_REG8(0x0aa4), 0x40 },
> > > > + { CCI_REG8(0x0a90), 0x03 },
> > > > + { CCI_REG8(0x0a91), 0x0e },
> > > > + { CCI_REG8(0x0a94), 0x80 },
> > > > +
> > > > + /* standby */
> > > > + { CCI_REG8(0x0af6), 0x20 },
> > > > + { CCI_REG8(0x0b00), 0x91 },
> > > > + { CCI_REG8(0x0b01), 0x17 },
> > > > + { CCI_REG8(0x0b02), 0x01 },
> > > > + { CCI_REG8(0x0b03), 0x00 },
> > > > + { CCI_REG8(0x0b04), 0x01 },
> > > > + { CCI_REG8(0x0b05), 0x17 },
> > > > + { CCI_REG8(0x0b06), 0x01 },
> > > > + { CCI_REG8(0x0b07), 0x00 },
> > > > + { CCI_REG8(0x0ae9), 0x01 },
> > > > + { CCI_REG8(0x0aea), 0x02 },
> > > > + { CCI_REG8(0x0ae8), 0x53 },
> > > > + { CCI_REG8(0x0ae8), 0x43 },
> > > > +
> > > > + /* gain_partition */
> > > > + { CCI_REG8(0x0af6), 0x30 },
> > > > + { CCI_REG8(0x0b00), 0x08 },
> > > > + { CCI_REG8(0x0b01), 0x0f },
> > > > + { CCI_REG8(0x0b02), 0x00 },
> > > > + { CCI_REG8(0x0b04), 0x1c },
> > > > + { CCI_REG8(0x0b05), 0x24 },
> > > > + { CCI_REG8(0x0b06), 0x00 },
> > > > + { CCI_REG8(0x0b08), 0x30 },
> > > > + { CCI_REG8(0x0b09), 0x40 },
> > > > + { CCI_REG8(0x0b0a), 0x00 },
> > > > + { CCI_REG8(0x0b0c), 0x0e },
> > > > + { CCI_REG8(0x0b0d), 0x2a },
> > > > + { CCI_REG8(0x0b0e), 0x00 },
> > > > + { CCI_REG8(0x0b10), 0x0e },
> > > > + { CCI_REG8(0x0b11), 0x2b },
> > > > + { CCI_REG8(0x0b12), 0x00 },
> > > > + { CCI_REG8(0x0b14), 0x0e },
> > > > + { CCI_REG8(0x0b15), 0x23 },
> > > > + { CCI_REG8(0x0b16), 0x00 },
> > > > + { CCI_REG8(0x0b18), 0x0e },
> > > > + { CCI_REG8(0x0b19), 0x24 },
> > > > + { CCI_REG8(0x0b1a), 0x00 },
> > > > + { CCI_REG8(0x0b1c), 0x0c },
> > > > + { CCI_REG8(0x0b1d), 0x0c },
> > > > + { CCI_REG8(0x0b1e), 0x00 },
> > > > + { CCI_REG8(0x0b20), 0x03 },
> > > > + { CCI_REG8(0x0b21), 0x03 },
> > > > + { CCI_REG8(0x0b22), 0x00 },
> > > > + { CCI_REG8(0x0b24), 0x0e },
> > > > + { CCI_REG8(0x0b25), 0x0e },
> > > > + { CCI_REG8(0x0b26), 0x00 },
> > > > + { CCI_REG8(0x0b28), 0x03 },
> > > > + { CCI_REG8(0x0b29), 0x03 },
> > > > + { CCI_REG8(0x0b2a), 0x00 },
> > > > + { CCI_REG8(0x0b2c), 0x12 },
> > > > + { CCI_REG8(0x0b2d), 0x12 },
> > > > + { CCI_REG8(0x0b2e), 0x00 },
> > > > + { CCI_REG8(0x0b30), 0x08 },
> > > > + { CCI_REG8(0x0b31), 0x08 },
> > > > + { CCI_REG8(0x0b32), 0x00 },
> > > > + { CCI_REG8(0x0b34), 0x14 },
> > > > + { CCI_REG8(0x0b35), 0x14 },
> > > > + { CCI_REG8(0x0b36), 0x00 },
> > > > + { CCI_REG8(0x0b38), 0x10 },
> > > > + { CCI_REG8(0x0b39), 0x10 },
> > > > + { CCI_REG8(0x0b3a), 0x00 },
> > > > + { CCI_REG8(0x0b3c), 0x16 },
> > > > + { CCI_REG8(0x0b3d), 0x16 },
> > > > + { CCI_REG8(0x0b3e), 0x00 },
> > > > + { CCI_REG8(0x0b40), 0x10 },
> > > > + { CCI_REG8(0x0b41), 0x10 },
> > > > + { CCI_REG8(0x0b42), 0x00 },
> > > > + { CCI_REG8(0x0b44), 0x19 },
> > > > + { CCI_REG8(0x0b45), 0x19 },
> > > > + { CCI_REG8(0x0b46), 0x00 },
> > > > + { CCI_REG8(0x0b48), 0x16 },
> > > > + { CCI_REG8(0x0b49), 0x16 },
> > > > + { CCI_REG8(0x0b4a), 0x00 },
> > > > + { CCI_REG8(0x0b4c), 0x19 },
> > > > + { CCI_REG8(0x0b4d), 0x19 },
> > > > + { CCI_REG8(0x0b4e), 0x00 },
> > > > + { CCI_REG8(0x0b50), 0x16 },
> > > > + { CCI_REG8(0x0b51), 0x16 },
> > > > + { CCI_REG8(0x0b52), 0x00 },
> > > > + { CCI_REG8(0x0b80), 0x01 },
> > > > + { CCI_REG8(0x0b81), 0x00 },
> > > > + { CCI_REG8(0x0b82), 0x00 },
> > > > + { CCI_REG8(0x0b84), 0x00 },
> > > > + { CCI_REG8(0x0b85), 0x00 },
> > > > + { CCI_REG8(0x0b86), 0x00 },
> > > > + { CCI_REG8(0x0b88), 0x01 },
> > > > + { CCI_REG8(0x0b89), 0x6a },
> > > > + { CCI_REG8(0x0b8a), 0x00 },
> > > > + { CCI_REG8(0x0b8c), 0x00 },
> > > > + { CCI_REG8(0x0b8d), 0x01 },
> > > > + { CCI_REG8(0x0b8e), 0x00 },
> > > > + { CCI_REG8(0x0b90), 0x01 },
> > > > + { CCI_REG8(0x0b91), 0xf6 },
> > > > + { CCI_REG8(0x0b92), 0x00 },
> > > > + { CCI_REG8(0x0b94), 0x00 },
> > > > + { CCI_REG8(0x0b95), 0x02 },
> > > > + { CCI_REG8(0x0b96), 0x00 },
> > > > + { CCI_REG8(0x0b98), 0x02 },
> > > > + { CCI_REG8(0x0b99), 0xc4 },
> > > > + { CCI_REG8(0x0b9a), 0x00 },
> > > > + { CCI_REG8(0x0b9c), 0x00 },
> > > > + { CCI_REG8(0x0b9d), 0x03 },
> > > > + { CCI_REG8(0x0b9e), 0x00 },
> > > > + { CCI_REG8(0x0ba0), 0x03 },
> > > > + { CCI_REG8(0x0ba1), 0xd8 },
> > > > + { CCI_REG8(0x0ba2), 0x00 },
> > > > + { CCI_REG8(0x0ba4), 0x00 },
> > > > + { CCI_REG8(0x0ba5), 0x04 },
> > > > + { CCI_REG8(0x0ba6), 0x00 },
> > > > + { CCI_REG8(0x0ba8), 0x05 },
> > > > + { CCI_REG8(0x0ba9), 0x4d },
> > > > + { CCI_REG8(0x0baa), 0x00 },
> > > > + { CCI_REG8(0x0bac), 0x00 },
> > > > + { CCI_REG8(0x0bad), 0x05 },
> > > > + { CCI_REG8(0x0bae), 0x00 },
> > > > + { CCI_REG8(0x0bb0), 0x07 },
> > > > + { CCI_REG8(0x0bb1), 0x3e },
> > > > + { CCI_REG8(0x0bb2), 0x00 },
> > > > + { CCI_REG8(0x0bb4), 0x00 },
> > > > + { CCI_REG8(0x0bb5), 0x06 },
> > > > + { CCI_REG8(0x0bb6), 0x00 },
> > > > + { CCI_REG8(0x0bb8), 0x0a },
> > > > + { CCI_REG8(0x0bb9), 0x1a },
> > > > + { CCI_REG8(0x0bba), 0x00 },
> > > > + { CCI_REG8(0x0bbc), 0x09 },
> > > > + { CCI_REG8(0x0bbd), 0x36 },
> > > > + { CCI_REG8(0x0bbe), 0x00 },
> > > > + { CCI_REG8(0x0bc0), 0x0e },
> > > > + { CCI_REG8(0x0bc1), 0x66 },
> > > > + { CCI_REG8(0x0bc2), 0x00 },
> > > > + { CCI_REG8(0x0bc4), 0x10 },
> > > > + { CCI_REG8(0x0bc5), 0x06 },
> > > > + { CCI_REG8(0x0bc6), 0x00 },
> > > > + { CCI_REG8(0x02c1), 0xe0 },
> > > > + { CCI_REG8(0x0207), 0x04 },
> > > > + { CCI_REG8(0x02c2), 0x10 },
> > > > + { CCI_REG8(0x02c3), 0x74 },
> > > > + { CCI_REG8(0x02c5), 0x09 },
> > > > + { CCI_REG8(0x02c1), 0xe0 },
> > > > + { CCI_REG8(0x0207), 0x04 },
> > > > + { CCI_REG8(0x02c2), 0x10 },
> > > > + { CCI_REG8(0x02c5), 0x09 },
> > > > + { CCI_REG8(0x02c1), 0xe0 },
> > > > + { CCI_REG8(0x0207), 0x04 },
> > > > + { CCI_REG8(0x02c2), 0x10 },
> > > > + { CCI_REG8(0x02c5), 0x09 },
> > > > +
> > > > + /* auto load CH_GAIN */
> > > > + { CCI_REG8(0x0aa1), 0x15 },
> > > > + { CCI_REG8(0x0aa2), 0x50 },
> > > > + { CCI_REG8(0x0aa3), 0x00 },
> > > > + { CCI_REG8(0x0aa4), 0x09 },
> > > > + { CCI_REG8(0x0a90), 0x25 },
> > > > + { CCI_REG8(0x0a91), 0x0e },
> > > > + { CCI_REG8(0x0a94), 0x80 },
> > > > +
> > > > + /* ISP */
> > > > + { CCI_REG8(0x0050), 0x00 },
> > > > + { CCI_REG8(0x0089), 0x83 },
> > > > + { CCI_REG8(0x005a), 0x40 },
> > > > + { CCI_REG8(0x00c3), 0x35 },
> > > > + { CCI_REG8(0x00c4), 0x80 },
> > > > + { CCI_REG8(0x0080), 0x10 },
> > > > + { CCI_REG8(0x0040), 0x12 },
> > > > + { CCI_REG8(0x0053), 0x0a },
> > > > + { CCI_REG8(0x0054), 0x44 },
> > > > + { CCI_REG8(0x0055), 0x32 },
> > > > + { CCI_REG8(0x0058), 0x89 },
> > > > + { CCI_REG8(0x004a), 0x03 },
> > > > + { CCI_REG8(0x0048), 0xf0 },
> > > > + { CCI_REG8(0x0049), 0x0f },
> > > > + { CCI_REG8(0x0041), 0x20 },
> > > > + { CCI_REG8(0x0043), 0x0a },
> > > > + { CCI_REG8(0x009d), 0x08 },
> > > > + { CCI_REG8(0x0236), 0x40 },
> > > > +
> > > > + /* gain */
> > >
> > > Is the gain configurable? Is this analogue gain? digital gain? or
> > > colour
> > > balanace gains ?
> > >
> > >
> > > > + { CCI_REG8(0x0204), 0x04 },
> > > > + { CCI_REG8(0x0205), 0x00 },
> > > > + { CCI_REG8(0x02b3), 0x00 },
> > > > + { CCI_REG8(0x02b4), 0x00 },
> > > > + { CCI_REG8(0x009e), 0x01 },
> > > > + { CCI_REG8(0x009f), 0x94 },
> > > > +
> > > > + /* auto load REG */
> > > > + { CCI_REG8(0x0aa1), 0x10 },
> > > > + { CCI_REG8(0x0aa2), 0xf8 },
> > > > + { CCI_REG8(0x0aa3), 0x00 },
> > > > + { CCI_REG8(0x0aa4), 0x1f },
> > > > + { CCI_REG8(0x0a90), 0x11 },
> > > > + { CCI_REG8(0x0a91), 0x0e },
> > > > + { CCI_REG8(0x0a94), 0x80 },
> > > > + { CCI_REG8(0x03fe), 0x00 },
> > > > + { CCI_REG8(0x0a90), 0x00 },
> > > > + { CCI_REG8(0x0a70), 0x00 },
> > > > + { CCI_REG8(0x0a67), 0x00 },
> > > > + { CCI_REG8(0x0af4), 0x29 },
> > > > +
> > > > + /* DPHY */
> > > > + { CCI_REG8(0x0d80), 0x07 },
> > > > + { CCI_REG8(0x0dd3), 0x18 },
> > > > +
> > > > + /* CISCTL_Reset */
> > > > + { CCI_REG8(0x031c), 0x80 },
> > > > + { CCI_REG8(0x03fe), 0x30 },
> > > > + { CCI_REG8(0x0d17), 0x06 },
> > > > + { CCI_REG8(0x03fe), 0x00 },
> > > > + { CCI_REG8(0x0d17), 0x00 },
> > > > + { CCI_REG8(0x031c), 0x93 },
> > > > + { CCI_REG8(0x03fe), 0x00 },
> > > > + { CCI_REG8(0x031c), 0x80 },
> > > > + { CCI_REG8(0x03fe), 0x30 },
> > > > + { CCI_REG8(0x0d17), 0x06 },
> > > > + { CCI_REG8(0x03fe), 0x00 },
> > > > + { CCI_REG8(0x0d17), 0x00 },
> > > > + { CCI_REG8(0x031c), 0x93 },
> > > > +};
> > > > +
> > > > +struct gc05a2_mode {
> > > > + u32 width;
> > > > + u32 height;
> > > > + const struct gc05a2_reg_list reg_list;
> > > > +
> > > > + u32 hts; /* Horizontal timining size */
> > > > + u32 vts_def; /* Default vertical timining size */
> > > > + u32 vts_min; /* Min vertical timining size */
> > > > +};
> > > > +
> > > > +/* Declare modes in order, from biggest to smallest height. */
> > > > +static const struct gc05a2_mode gc05a2_modes[] = {
> > > > + {
> > > > + /* 2592*1944@30fps */
> > > > + .width = GC05A2_NATIVE_WIDTH,
> > > > + .height = GC05A2_NATIVE_HEIGHT,
> > > > + .reg_list = {
> > > > + .num_of_regs =
> ARRAY_SIZE(mode_2592x1944),
> > > > + .regs = mode_2592x1944,
> > > > + },
> > > > + .hts = 3664,
> > > > + .vts_def = 2032,
> > > > + .vts_min = 2032,
> > > > + },
> > > > + {
> > > > + /* 1280*720@60fps */
> > > > + .width = 1280,
> > > > + .height = 720,
> > > > + .reg_list = {
> > > > + .num_of_regs =
> ARRAY_SIZE(mode_1280x720),
> > > > + .regs = mode_1280x720,
> > > > + },
> > > > + .hts = 3616,
> > > > + .vts_def = 1032,
> > > > + .vts_min = 1032,
> > > > + },
> > > > +};
> > > > +
> > > > +static inline struct gc05a2 *to_gc05a2(struct v4l2_subdev *sd)
> > > > +{
> > > > + return container_of(sd, struct gc05a2, sd);
> > > > +}
> > > > +
> > > > +static int gc05a2_power_on(struct device *dev)
> > > > +{
> > > > + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > > + struct gc05a2 *gc05a2 = to_gc05a2(sd);
> > > > + int ret;
> > > > +
> > > > + ret =
> regulator_bulk_enable(ARRAY_SIZE(gc05a2_supply_name),
> > > > + gc05a2->supplies);
> > > > + if (ret < 0) {
> > > > + dev_err(gc05a2->dev, "failed to enable
> regulators:
> > > %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = clk_prepare_enable(gc05a2->xclk);
> > > > + if (ret < 0) {
> > > >
> > >
> + regulator_bulk_disable(ARRAY_SIZE(gc05a2_supply_name)
> > > ,
> > > > + gc05a2->supplies);
> > > > + dev_err(gc05a2->dev, "clk prepare enable
> > > failed\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + fsleep(GC05A2_SLEEP_US);
> > > > +
> > > > + gpiod_set_value_cansleep(gc05a2->reset_gpio, 0);
> > > > + fsleep(GC05A2_SLEEP_US);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int gc05a2_power_off(struct device *dev)
> > > > +{
> > > > + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > > + struct gc05a2 *gc05a2 = to_gc05a2(sd);
> > > > +
> > > > + clk_disable_unprepare(gc05a2->xclk);
> > > > + gpiod_set_value_cansleep(gc05a2->reset_gpio, 1);
> > > > + regulator_bulk_disable(ARRAY_SIZE(gc05a2_supply_name),
> > > > + gc05a2->supplies);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int gc05a2_enum_mbus_code(struct v4l2_subdev *sd,
> > > > + struct v4l2_subdev_state
> > > *sd_state,
> > > > + struct
> v4l2_subdev_mbus_code_enum
> > > *code)
> > > > +{
> > > > + if (code->index > 0)
> > > > + return -EINVAL;
> > > > +
> > > > + code->code = GC05A2_MBUS_CODE;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int gc05a2_enum_frame_size(struct v4l2_subdev *subdev,
> > > > + struct v4l2_subdev_state
> > > *sd_state,
> > > > + struct
> > > v4l2_subdev_frame_size_enum *fse)
> > > > +{
> > > > + if (fse->code != GC05A2_MBUS_CODE)
> > > > + return -EINVAL;
> > > > +
> > > > + if (fse->index >= ARRAY_SIZE(gc05a2_modes))
> > > > + return -EINVAL;
> > > > +
> > > > + fse->min_width = gc05a2_modes[fse->index].width;
> > > > + fse->max_width = gc05a2_modes[fse->index].width;
> > > > + fse->min_height = gc05a2_modes[fse->index].height;
> > > > + fse->max_height = gc05a2_modes[fse->index].height;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int gc05a2_update_cur_mode_controls(struct gc05a2
> *gc05a2,
> > > > + const struct
> gc05a2_mode
> > > *mode)
> > > > +{
> > > > + s64 exposure_max, h_blank;
> > > > + int ret;
> > > > +
> > > > + ret = __v4l2_ctrl_modify_range(gc05a2->vblank,
> > > > + mode->vts_min - mode-
> > > >height,
> > > > + GC05A2_VTS_MAX - mode-
> > > >height, 1,
> > > > + mode->vts_def - mode-
> > > >height);
> > > > + if (ret) {
> > > > + dev_err(gc05a2->dev, "VB ctrl range update
> > > failed\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + h_blank = mode->hts - mode->width;
> > > > + ret = __v4l2_ctrl_modify_range(gc05a2->hblank, h_blank,
> > > h_blank, 1,
> > > > + h_blank);
> > > > + if (ret) {
> > > > + dev_err(gc05a2->dev, "HB ctrl range update
> > > failed\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + exposure_max = mode->vts_def - GC05A2_EXP_MARGIN;
> > > > + ret = __v4l2_ctrl_modify_range(gc05a2->exposure,
> > > GC05A2_EXP_MIN,
> > > > + exposure_max,
> > > GC05A2_EXP_STEP,
> > > > + exposure_max);
> > > > + if (ret) {
> > > > + dev_err(gc05a2->dev, "exposure ctrl range
> update
> > > failed\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void gc05a2_update_pad_format(struct gc05a2 *gc08a3,
> > > > + const struct gc05a2_mode
> > > *mode,
> > > > + struct v4l2_mbus_framefmt
> > > *fmt)
> > > > +{
> > > > + fmt->width = mode->width;
> > > > + fmt->height = mode->height;
> > > > + fmt->code = GC05A2_MBUS_CODE;
> > > > + fmt->field = V4L2_FIELD_NONE;
> > > > + fmt->colorspace = V4L2_COLORSPACE_RAW;
> > > > + fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt-
> > > >colorspace);
> > > > + fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > > + fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> > > > +}
> > > > +
> > > > +static int gc05a2_set_format(struct v4l2_subdev *sd,
> > > > + struct v4l2_subdev_state *state,
> > > > + struct v4l2_subdev_format *fmt)
> > > > +{
> > > > + struct gc05a2 *gc05a2 = to_gc05a2(sd);
> > > > + struct v4l2_mbus_framefmt *mbus_fmt;
> > > > + struct v4l2_rect *crop;
> > > > + const struct gc05a2_mode *mode;
> > > > +
> > > > + mode = v4l2_find_nearest_size(gc05a2_modes,
> > > ARRAY_SIZE(gc05a2_modes),
> > > > + width, height, fmt-
> > > >format.width,
> > > > + fmt->format.height);
> > > > +
> > > > + /* update crop info to subdev state */
> > > > + crop = v4l2_subdev_state_get_crop(state, 0);
> > > > + crop->width = mode->width;
> > > > + crop->height = mode->height;
> > > > +
> > > > + /* update fmt info to subdev state */
> > > > + gc05a2_update_pad_format(gc05a2, mode, &fmt->format);
> > > > + mbus_fmt = v4l2_subdev_state_get_format(state, 0);
> > > > + *mbus_fmt = fmt->format;
> > > > +
> > > > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> > > > + return 0;
> > > > + gc05a2->cur_mode = mode;
> > > > + gc05a2_update_cur_mode_controls(gc05a2, mode);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int gc05a2_get_selection(struct v4l2_subdev *sd,
> > > > + struct v4l2_subdev_state
> *state,
> > > > + struct v4l2_subdev_selection
> *sel)
> > > > +{
> > > > + switch (sel->target) {
> > > > + case V4L2_SEL_TGT_CROP_DEFAULT:
> > > > + case V4L2_SEL_TGT_CROP:
> > > > + sel->r = *v4l2_subdev_state_get_crop(state, 0);
> > > > + break;
> > > > + case V4L2_SEL_TGT_CROP_BOUNDS:
> > > > + sel->r.top = 0;
> > > > + sel->r.left = 0;
> > > > + sel->r.width = GC05A2_NATIVE_WIDTH;
> > > > + sel->r.height = GC05A2_NATIVE_HEIGHT;
> > > > + break;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int gc05a2_init_state(struct v4l2_subdev *sd,
> > > > + struct v4l2_subdev_state *state)
> > > > +{
> > > > + struct v4l2_subdev_format fmt = {
> > > > + .which = V4L2_SUBDEV_FORMAT_TRY,
> > > > + .pad = 0,
> > > > + .format = {
> > > > + .code = GC05A2_MBUS_CODE,
> > > > + .width = gc05a2_modes[0].width,
> > > > + .height = gc05a2_modes[0].height,
> > > > + },
> > > > + };
> > > > +
> > > > + gc05a2_set_format(sd, state, &fmt);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int gc05a2_set_ctrl_hflip(struct gc05a2 *gc05a2, u32
> > > ctrl_val)
> > > > +{
> > > > + int ret;
> > > > + u64 val;
> > > > +
> > > > + ret = cci_read(gc05a2->regmap, GC05A2_FLIP_REG, &val,
> > > NULL);
> > > > + if (ret) {
> > > > + dev_err(gc05a2->dev, "read hflip register
> failed:
> > > %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return cci_update_bits(gc05a2->regmap, GC05A2_FLIP_REG,
> > > > + GC05A2_FLIP_H_MASK,
> > > > + ctrl_val ? GC05A2_FLIP_H_MASK :
> 0,
> > > NULL);
> > > > +}
> > > > +
> > > > +static int gc05a2_set_ctrl_vflip(struct gc05a2 *gc05a2, u32
> > > ctrl_val)
> > > > +{
> > > > + int ret;
> > > > + u64 val;
> > > > +
> > > > + ret = cci_read(gc05a2->regmap, GC05A2_FLIP_REG, &val,
> > > NULL);
> > > > + if (ret) {
> > > > + dev_err(gc05a2->dev, "read vflip register
> failed:
> > > %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return cci_update_bits(gc05a2->regmap, GC05A2_FLIP_REG,
> > > > + GC05A2_FLIP_V_MASK,
> > > > + ctrl_val ? GC05A2_FLIP_V_MASK :
> 0,
> > > NULL);
> > > > +}
> > > > +
> > > > +static int gc05a2_test_pattern(struct gc05a2 *gc05a2, u32
> > > pattern_menu)
> > > > +{
> > > > + u32 pattern;
> > > > + int ret;
> > > > +
> > > > + if (pattern_menu) {
> > > > + switch (pattern_menu) {
> > > > + case 1:
> > > > + case 2:
> > > > + case 3:
> > > > + case 4:
> > > > + case 5:
> > > > + case 6:
> > > > + case 7:
> > > > + pattern = pattern_menu << 4;
> > > > + break;
> > > > +
> > > > + case 8:
> > > > + pattern = 0;
> > > > + break;
> > > > +
> > > > + case 9:
> > > > + pattern = 4;
> > > > + break;
> > > > +
> > > > + default:
> > > > + pattern = 0x00;
> > > > + break;
> > > > + }
> > >
> > > This is fairly terse. Can we add comments, or definitions for the
> > > types
> > > or such so that the above is easier to interpret?
> > >
> > > > +
> > > > + ret = cci_write(gc05a2->regmap,
> > > GC05A2_REG_TEST_PATTERN_IDX,
> > > > + pattern, NULL);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + return cci_write(gc05a2->regmap,
> > > GC05A2_REG_TEST_PATTERN_EN,
> > > > + GC05A2_TEST_PATTERN_EN, NULL);
> > > > + } else {
> > > > + return cci_write(gc05a2->regmap,
> > > GC05A2_REG_TEST_PATTERN_EN,
> > > > + 0x00, NULL);
> > > > + }
> > > > +}
> > > > +
> > > > +static int gc05a2_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > +{
> > > > + struct gc05a2 *gc05a2 =
> > > > + container_of(ctrl->handler, struct gc05a2,
> ctrls);
> > > > + int ret = 0;
> > > > + s64 exposure_max;
> > > > + struct v4l2_subdev_state *state;
> > > > + const struct v4l2_mbus_framefmt *format;
> > > > +
> > > > + state = v4l2_subdev_get_locked_active_state(&gc05a2-
> >sd);
> > > > + format = v4l2_subdev_state_get_format(state, 0);
> > > > +
> > > > + if (ctrl->id == V4L2_CID_VBLANK) {
> > > > + /* Update max exposure while meeting expected
> > > vblanking */
> > > > + exposure_max = format->height + ctrl->val -
> > > GC05A2_EXP_MARGIN;
> > > > + __v4l2_ctrl_modify_range(gc05a2->exposure,
> > > > + gc05a2->exposure-
> >minimum,
> > > > + exposure_max, gc05a2-
> > > >exposure->step,
> > > > + exposure_max);
> > > > + }
> > > > +
> > > > + /*
> > > > + * Applying V4L2 control value only happens
> > > > + * when power is on for streaming.
> > > > + */
> > > > + if (!pm_runtime_get_if_active(gc05a2->dev))
> > > > + return 0;
> > > > +
> > > > + switch (ctrl->id) {
> > > > + case V4L2_CID_EXPOSURE:
> > > > + ret = cci_write(gc05a2->regmap, GC05A2_EXP_REG,
> > > > + ctrl->val, NULL);
> > > > + break;
> > > > +
> > > > + case V4L2_CID_ANALOGUE_GAIN:
> > > > + ret = cci_write(gc05a2->regmap,
> GC05A2_AGAIN_REG,
> > > > + ctrl->val, NULL);
> > > > + break;
> > > > +
> > > > + case V4L2_CID_VBLANK:
> > > > + ret = cci_write(gc05a2->regmap,
> > > GC05A2_FRAME_LENGTH_REG,
> > > > + gc05a2->cur_mode->height +
> ctrl-
> > > >val, NULL);
> > > > + break;
> > > > +
> > > > + case V4L2_CID_HFLIP:
> > > > + ret = gc05a2_set_ctrl_hflip(gc05a2, ctrl->val);
> > > > + break;
> > > > +
> > > > + case V4L2_CID_VFLIP:
> > > > + ret = gc05a2_set_ctrl_vflip(gc05a2, ctrl->val);
> > > > + break;
> > > > +
> > > > + case V4L2_CID_TEST_PATTERN:
> > > > + ret = gc05a2_test_pattern(gc05a2, ctrl->val);
> > > > + break;
> > > > +
> > > > + default:
> > > > + break;
> > > > + }
> > > > +
> > > > + pm_runtime_put(gc05a2->dev);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static const struct v4l2_ctrl_ops gc05a2_ctrl_ops = {
> > > > + .s_ctrl = gc05a2_set_ctrl,
> > > > +};
> > > > +
> > > > +static int gc05a2_identify_module(struct gc05a2 *gc05a2)
> > > > +{
> > > > + u64 val;
> > > > + int ret;
> > > > +
> > > > + if (gc05a2->identified)
> > > > + return 0;
> > > > +
> > > > + ret = cci_read(gc05a2->regmap, GC05A2_REG_CHIP_ID,
> &val,
> > > NULL);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + if (val != GC05A2_CHIP_ID) {
> > > > + dev_err(gc05a2->dev, "chip id mismatch:
> > > 0x%x!=0x%llx",
> > > > + GC05A2_CHIP_ID, val);
> > > > + return -ENXIO;
> > > > + }
> > > > +
> > > > + gc05a2->identified = true;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int gc05a2_start_streaming(struct gc05a2 *gc05a2)
> > > > +{
> > > > + const struct gc05a2_mode *mode;
> > > > + const struct gc05a2_reg_list *reg_list;
> > > > + int ret;
> > > > +
> > > > + ret = pm_runtime_resume_and_get(gc05a2->dev);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + ret = gc05a2_identify_module(gc05a2);
> > > > + if (ret)
> > > > + goto err_rpm_put;
> > > > +
> > > > + ret = cci_multi_reg_write(gc05a2->regmap,
> > > > + mode_table_common,
> > > >
> + ARRAY_SIZE(mode_table_common),
> > > NULL);
> > > > + if (ret)
> > > > + goto err_rpm_put;
> > > > +
> > > > + mode = gc05a2->cur_mode;
> > > > + reg_list = &mode->reg_list;
> > > > +
> > > > + ret = cci_multi_reg_write(gc05a2->regmap,
> > > > + reg_list->regs, reg_list-
> > > >num_of_regs, NULL);
> > > > + if (ret < 0)
> > > > + goto err_rpm_put;
> > > > +
> > > > + ret = __v4l2_ctrl_handler_setup(&gc05a2->ctrls);
> > > > + if (ret < 0) {
> > > > + dev_err(gc05a2->dev, "could not sync v4l2
> > > controls\n");
> > > > + goto err_rpm_put;
> > > > + }
> > > > +
> > > > + ret = cci_write(gc05a2->regmap, GC05A2_STREAMING_REG,
> 1,
> > > NULL);
> > > > + if (ret < 0) {
> > > > + dev_err(gc05a2->dev, "write STREAMING_REG
> failed:
> > > %d\n", ret);
> > > > + goto err_rpm_put;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +
> > > > +err_rpm_put:
> > > > + pm_runtime_put(gc05a2->dev);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int gc05a2_stop_streaming(struct gc05a2 *gc05a2)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + ret = cci_write(gc05a2->regmap, GC05A2_STREAMING_REG,
> 0,
> > > NULL);
> > > > + if (ret < 0)
> > > > + dev_err(gc05a2->dev, "could not sent stop
> streaming
> > > %d\n", ret);
> > > > +
> > > > + pm_runtime_put(gc05a2->dev);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int gc05a2_s_stream(struct v4l2_subdev *subdev, int
> enable)
> > > > +{
> > > > + struct gc05a2 *gc05a2 = to_gc05a2(subdev);
> > > > + struct v4l2_subdev_state *state;
> > > > + int ret;
> > > > +
> > > > + state = v4l2_subdev_lock_and_get_active_state(subdev);
> > > > +
> > > > + if (enable)
> > > > + ret = gc05a2_start_streaming(gc05a2);
> > > > + else
> > > > + ret = gc05a2_stop_streaming(gc05a2);
> > > > +
> > > > + v4l2_subdev_unlock_state(state);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static const struct v4l2_subdev_video_ops gc05a2_video_ops = {
> > > > + .s_stream = gc05a2_s_stream,
> > > > +};
> > > > +
> > > > +static const struct v4l2_subdev_pad_ops gc05a2_subdev_pad_ops
> = {
> > > > + .enum_mbus_code = gc05a2_enum_mbus_code,
> > > > + .enum_frame_size = gc05a2_enum_frame_size,
> > > > + .get_fmt = v4l2_subdev_get_fmt,
> > > > + .set_fmt = gc05a2_set_format,
> > > > + .get_selection = gc05a2_get_selection,
> > > > +};
> > > > +
> > > > +static const struct v4l2_subdev_core_ops gc05a2_core_ops = {
> > > > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > > > + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > > > +};
> > > > +
> > > > +static const struct v4l2_subdev_ops gc05a2_subdev_ops = {
> > > > + .core = &gc05a2_core_ops,
> > > > + .video = &gc05a2_video_ops,
> > > > + .pad = &gc05a2_subdev_pad_ops,
> > > > +};
> > > > +
> > > > +static const struct v4l2_subdev_internal_ops
> gc05a2_internal_ops =
> > > {
> > > > + .init_state = gc05a2_init_state,
> > > > +};
> > > > +
> > > > +static int gc05a2_get_regulators(struct device *dev, struct
> gc05a2
> > > *gc05a2)
> > > > +{
> > > > + unsigned int i;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(gc05a2_supply_name); i++)
> > > > + gc05a2->supplies[i].supply =
> gc05a2_supply_name[i];
> > > > +
> > > > + return devm_regulator_bulk_get(dev,
> > > ARRAY_SIZE(gc05a2_supply_name),
> > > > + gc05a2->supplies);
> > > > +}
> > > > +
> > > > +static int gc05a2_parse_fwnode(struct gc05a2 *gc05a2)
> > > > +{
> > > > + struct fwnode_handle *endpoint;
> > > > + struct v4l2_fwnode_endpoint bus_cfg = {
> > > > + .bus_type = V4L2_MBUS_CSI2_DPHY,
> > > > + };
> > > > + int ret;
> > > > + struct device *dev = gc05a2->dev;
> > > > +
> > > > + endpoint =
> > > >
> + fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0,
> > > 0,
> > > >
> > >
> + FWNODE_GRAPH_ENDPOINT
> > > _NEXT);
> > > > + if (!endpoint) {
> > > > + dev_err(dev, "endpoint node not found\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint,
> &bus_cfg);
> > > > + if (ret) {
> > > > + dev_err(dev, "parsing endpoint node failed\n");
> > > > + goto done;
> > > > + }
> > > > +
> > > > + ret = v4l2_link_freq_to_bitmap(dev,
> > > bus_cfg.link_frequencies,
> > > >
> > >
> + bus_cfg.nr_of_link_frequencies
> > > ,
> > > >
> + gc05a2_link_freq_menu_items,
> > > >
> > >
> + ARRAY_SIZE(gc05a2_link_freq_me
> > > nu_items),
> > > > + &gc05a2-
> >link_freq_bitmap);
> > > > + if (ret)
> > > > + goto done;
> > > > +
> > > > +done:
> > > > + v4l2_fwnode_endpoint_free(&bus_cfg);
> > > > + fwnode_handle_put(endpoint);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static u64 gc05a2_to_pixel_rate(u32 f_index)
> > > > +{
> > > > + u64 pixel_rate =
> > > > + gc05a2_link_freq_menu_items[f_index] * 2 *
> > > GC05A2_DATA_LANES;
> > > > +
> > > > + return div_u64(pixel_rate, GC05A2_RGB_DEPTH);
> > > > +}
> > > > +
> > > > +static int gc05a2_init_controls(struct gc05a2 *gc05a2)
> > > > +{
> > > > + struct i2c_client *client =
> v4l2_get_subdevdata(&gc05a2-
> > > >sd);
> > > > + const struct gc05a2_mode *mode = &gc05a2_modes[0];
> > > > + const struct v4l2_ctrl_ops *ops = &gc05a2_ctrl_ops;
> > > > + struct v4l2_fwnode_device_properties props;
> > > > + struct v4l2_ctrl_handler *ctrl_hdlr;
> > > > + s64 exposure_max, h_blank;
> > > > + int ret;
> > > > +
> > > > + ctrl_hdlr = &gc05a2->ctrls;
> > > > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 9);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + gc05a2->hflip = v4l2_ctrl_new_std(ctrl_hdlr,
> > > &gc05a2_ctrl_ops,
> > > > + V4L2_CID_HFLIP, 0, 1,
> 1,
> > > 0);
> > > > + gc05a2->vflip = v4l2_ctrl_new_std(ctrl_hdlr,
> > > &gc05a2_ctrl_ops,
> > > > + V4L2_CID_VFLIP, 0, 1,
> 1,
> > > 0);
> > > > + v4l2_ctrl_cluster(2, &gc05a2->hflip);
> > > > +
> > > > + gc05a2->link_freq =
> > > > + v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > > > + &gc05a2_ctrl_ops,
> > > > + V4L2_CID_LINK_FREQ,
> > > >
> > >
> + ARRAY_SIZE(gc05a2_link_freq_menu_items
> > > ) - 1,
> > > > + 0,
> > > > + gc05a2_link_freq_menu_items);
> > > > + if (gc05a2->link_freq)
> > > > + gc05a2->link_freq->flags |=
> > > V4L2_CTRL_FLAG_READ_ONLY;
> > > > +
> > > > + gc05a2->pixel_rate =
> > > > + v4l2_ctrl_new_std(ctrl_hdlr,
> > > > + &gc05a2_ctrl_ops,
> > > > + V4L2_CID_PIXEL_RATE, 0,
> > > > + gc05a2_to_pixel_rate(0),
> > > > + 1,
> > > > + gc05a2_to_pixel_rate(0));
> > > > +
> > > > + gc05a2->vblank =
> > > > + v4l2_ctrl_new_std(ctrl_hdlr,
> > > > + &gc05a2_ctrl_ops,
> > > V4L2_CID_VBLANK,
> > > > + mode->vts_min - mode->height,
> > > > + GC05A2_VTS_MAX - mode-
> >height, 1,
> > > > + mode->vts_def - mode-
> >height);
> > > > +
> > > > + h_blank = mode->hts - mode->width;
> > > > + gc05a2->hblank = v4l2_ctrl_new_std(ctrl_hdlr,
> > > &gc05a2_ctrl_ops,
> > > > + V4L2_CID_HBLANK,
> > > h_blank, h_blank, 1,
> > > > + h_blank);
> > > > + if (gc05a2->hblank)
> > > > + gc05a2->hblank->flags |=
> V4L2_CTRL_FLAG_READ_ONLY;
> > > > +
> > > > + v4l2_ctrl_new_std(ctrl_hdlr, &gc05a2_ctrl_ops,
> > > > + V4L2_CID_ANALOGUE_GAIN,
> GC05A2_AGAIN_MIN,
> > > > + GC05A2_AGAIN_MAX, GC05A2_AGAIN_STEP,
> > > > + GC05A2_AGAIN_MIN);
> > > > +
> > > > + exposure_max = mode->vts_def - GC05A2_EXP_MARGIN;
> > > > + gc05a2->exposure = v4l2_ctrl_new_std(ctrl_hdlr,
> > > &gc05a2_ctrl_ops,
> > > > + V4L2_CID_EXPOSURE,
> > > GC05A2_EXP_MIN,
> > > > + exposure_max,
> > > GC05A2_EXP_STEP,
> > > > + exposure_max);
> > > > +
> > > > + v4l2_ctrl_new_std_menu_items(ctrl_hdlr,
> &gc05a2_ctrl_ops,
> > > > + V4L2_CID_TEST_PATTERN,
> > > >
> > >
> + ARRAY_SIZE(gc05a2_test_pattern_m
> > > enu) - 1,
> > > > + 0, 0,
> > > gc05a2_test_pattern_menu);
> > > > +
> > > > + /* register properties to fwnode (e.g. rotation,
> > > orientation) */
> > > > + ret = v4l2_fwnode_device_parse(&client->dev, &props);
> > > > + if (ret)
> > > > + goto error_ctrls;
> > > > +
> > > > + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, ops,
> > > &props);
> > > > + if (ret)
> > > > + goto error_ctrls;
> > > > +
> > > > + if (ctrl_hdlr->error) {
> > > > + ret = ctrl_hdlr->error;
> > > > + goto error_ctrls;
> > > > + }
> > > > +
> > > > + gc05a2->sd.ctrl_handler = ctrl_hdlr;
> > > > +
> > > > + return 0;
> > > > +
> > > > +error_ctrls:
> > > > + v4l2_ctrl_handler_free(ctrl_hdlr);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int gc05a2_probe(struct i2c_client *client)
> > > > +{
> > > > + struct device *dev = &client->dev;
> > > > + struct gc05a2 *gc05a2;
> > > > + int ret;
> > > > +
> > > > + gc05a2 = devm_kzalloc(dev, sizeof(*gc05a2),
> GFP_KERNEL);
> > > > + if (!gc05a2)
> > > > + return -ENOMEM;
> > > > +
> > > > + gc05a2->dev = dev;
> > > > +
> > > > + ret = gc05a2_parse_fwnode(gc05a2);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + gc05a2->regmap = devm_cci_regmap_init_i2c(client, 16);
> > > > + if (IS_ERR(gc05a2->regmap))
> > > > + return dev_err_probe(dev, PTR_ERR(gc05a2-
> >regmap),
> > > > + "failed to init CCI\n");
> > > > +
> > > > + gc05a2->xclk = devm_clk_get(dev, NULL);
> > > > + if (IS_ERR(gc05a2->xclk))
> > > > + return dev_err_probe(dev, PTR_ERR(gc05a2-
> >xclk),
> > > > + "failed to get xclk\n");
> > > > +
> > > > + ret = clk_set_rate(gc05a2->xclk,
> GC05A2_DEFAULT_CLK_FREQ);
> > > > + if (ret)
> > > > + return dev_err_probe(dev, ret,
> > > > + "failed to set xclk
> > > frequency\n");
> > > > +
> > > > + ret = gc05a2_get_regulators(dev, gc05a2);
> > > > + if (ret < 0)
> > > > + return dev_err_probe(dev, ret,
> > > > + "failed to get
> regulators\n");
> > > > +
> > > > + gc05a2->reset_gpio = devm_gpiod_get(dev, "reset",
> > > GPIOD_OUT_LOW);
> > > > + if (IS_ERR(gc05a2->reset_gpio))
> > > > + return dev_err_probe(dev, PTR_ERR(gc05a2-
> > > >reset_gpio),
> > > > + "failed to get gpio\n");
> > > > +
> > > > + v4l2_i2c_subdev_init(&gc05a2->sd, client,
> > > &gc05a2_subdev_ops);
> > > > + gc05a2->sd.internal_ops = &gc05a2_internal_ops;
> > > > + gc05a2->cur_mode = &gc05a2_modes[0];
> > > > +
> > > > + ret = gc05a2_init_controls(gc05a2);
> > > > + if (ret)
> > > > + return dev_err_probe(dev, ret,
> > > > + "failed to init
> controls\n");
> > > > +
> > > > + gc05a2->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > > > + V4L2_SUBDEV_FL_HAS_EVENTS;
> > > > + gc05a2->pad.flags = MEDIA_PAD_FL_SOURCE;
> > > > + gc05a2->sd.dev = &client->dev;
> > > > + gc05a2->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > > > +
> > > > + ret = media_entity_pads_init(&gc05a2->sd.entity, 1,
> > > &gc05a2->pad);
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "could not register media
> entity\n");
> > > > + goto err_v4l2_ctrl_handler_free;
> > > > + }
> > > > +
> > > > + gc05a2->sd.state_lock = gc05a2->ctrls.lock;
> > > > + ret = v4l2_subdev_init_finalize(&gc05a2->sd);
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "v4l2 subdev init error: %d\n",
> ret);
> > > > + goto err_media_entity_cleanup;
> > > > + }
> > > > +
> > > > + pm_runtime_set_active(gc05a2->dev);
> > > > + pm_runtime_enable(gc05a2->dev);
> > > > + pm_runtime_set_autosuspend_delay(gc05a2->dev, 1000);
> > > > + pm_runtime_use_autosuspend(gc05a2->dev);
> > > > + pm_runtime_idle(gc05a2->dev);
> > > > +
> > > > + ret = v4l2_async_register_subdev_sensor(&gc05a2->sd);
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "could not register v4l2
> device\n");
> > > > + goto err_rpm;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +
> > > > +err_rpm:
> > > > + pm_runtime_disable(gc05a2->dev);
> > > > + v4l2_subdev_cleanup(&gc05a2->sd);
> > > > +
> > > > +err_media_entity_cleanup:
> > > > + media_entity_cleanup(&gc05a2->sd.entity);
> > > > +
> > > > +err_v4l2_ctrl_handler_free:
> > > > + v4l2_ctrl_handler_free(&gc05a2->ctrls);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static void gc05a2_remove(struct i2c_client *client)
> > > > +{
> > > > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > + struct gc05a2 *gc05a2 = to_gc05a2(sd);
> > > > +
> > > > + v4l2_async_unregister_subdev(&gc05a2->sd);
> > > > + v4l2_subdev_cleanup(sd);
> > > > + media_entity_cleanup(&gc05a2->sd.entity);
> > > > + v4l2_ctrl_handler_free(&gc05a2->ctrls);
> > > > +
> > > > + pm_runtime_disable(&client->dev);
> > > > + if (!pm_runtime_status_suspended(&client->dev))
> > > > + gc05a2_power_off(gc05a2->dev);
> > > > + pm_runtime_set_suspended(&client->dev);
> > > > +}
> > > > +
> > > > +static const struct of_device_id gc05a2_of_match[] = {
> > > > + { .compatible = "galaxycore,gc05a2" },
> > > > + {}
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, gc05a2_of_match);
> > > > +
> > > > +static DEFINE_RUNTIME_DEV_PM_OPS(gc05a2_pm_ops,
> > > > + gc05a2_power_off,
> > > > + gc05a2_power_on,
> > > > + NULL);
> > > > +
> > > > +static struct i2c_driver gc05a2_i2c_driver = {
> > > > + .driver = {
> > > > + .of_match_table = gc05a2_of_match,
> > > > + .pm = pm_ptr(&gc05a2_pm_ops),
> > > > + .name = "gc05a2",
> > > > + },
> > > > + .probe = gc05a2_probe,
> > > > + .remove = gc05a2_remove,
> > > > +};
> > > > +module_i2c_driver(gc05a2_i2c_driver);
> > > > +
> > > > +MODULE_DESCRIPTION("GalaxyCore gc05a2 Camera driver");
> > > > +MODULE_AUTHOR("Zhi Mao <zhi.mao@mediatek.com>");
> > > > +MODULE_LICENSE("GPL");
> > > > --
> > > > 2.25.1
> > > >
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000
From: David Lechner @ 2024-04-09 2:57 UTC (permalink / raw)
To: Marcelo Schmitt
Cc: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt, marcelo.schmitt1, linux-iio, devicetree, linux-kernel
In-Reply-To: <7c877c865f0b7da28d9f1f177b3b2692b0ae20b9.1712585500.git.marcelo.schmitt@analog.com>
On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> Add device tree documentation for AD4000 family of ADC devices.
>
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
>
Suggested-by: David Lechner <dlechner@baylibre.com>
(if you still use mostly my suggestions in the end)
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
> .../bindings/iio/adc/adi,ad4000.yaml | 201 ++++++++++++++++++
> MAINTAINERS | 7 +
> 2 files changed, 208 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> new file mode 100644
> index 000000000000..ca06afb5149e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> @@ -0,0 +1,201 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD4000 and similar Analog to Digital Converters
> +
> +maintainers:
> + - Marcelo Schmitt <marcelo.schmitt@analog.com>
> +
> +description: |
> + Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
> + Specifications can be found at:
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> +
> +$ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - adi,ad4000
> + - adi,ad4001
> + - adi,ad4002
> + - adi,ad4003
> + - adi,ad4004
> + - adi,ad4005
> + - adi,ad4006
> + - adi,ad4007
> + - adi,ad4008
> + - adi,ad4010
> + - adi,ad4011
> + - adi,ad4020
> + - adi,ad4021
> + - adi,ad4022
> + - adi,adaq4001
> + - adi,adaq4003
> +
> + reg:
> + maxItems: 1
> +
> + spi-max-frequency:
> + maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
> +
> + spi-cpha: true
> +
> + adi,spi-mode:
> + $ref: /schemas/types.yaml#/definitions/string
> + enum: [ single, chain ]
It sounds like there are more possible wiring configurations for these
chips that I thought when suggesting reusing this binding from AD7944
so we probably need more options here. (see my reply to the cover
letter for the complete context of these remarks)
We identified A) an additional wiring configuration where SDI of the
ADC chip is wired to SDO of the SPI controller and B) a potential need
to pin mux between wiring modes to work around SPI controller
limitations perhaps we could omit the adi,spi-mode property and just
use the standard pinctrl properties.
pinctrl-names:
description: |
Names for possible ways the SDI line of the controller is wired.
* default: The SDI line of the ADC is connected to the SDO line of the
SPI controller. CNV line of the ADC is connected to CS of the SPI
controller.
* single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
definition of 3-wire mode is NOT at all related to the standard
spi-3wire property!) In this mode, SDI is tied to VIO, and the CNV line
can be connected to the CS line of the SPI controller (typical) or to a
GPIO, in which case the CS line of the controller is unused. The SDO
line of the SPI controller is not connected.
* multi: The datasheet calls this "4-wire mode" and is used when multiple
chips are connected in parallel. In this mode, the ADC SDI line is tied
to the CS line on the SPI controller and the CNV line is connected to
a GPIO. The SDO line of the SPI controller is not connected.
* chain: The datasheet calls this "chain mode". This mode is used to save
on wiring when multiple ADCs are used. In this mode, the SDI line of
one chip is tied to the SDO of the next chip in the chain and the SDI of
the last chip in the chain is tied to GND. Only the first chip in the
chain is connected to the SPI bus. The CNV line of all chips are tied
together. The CS line of the SPI controller can be used as the CNV line
only if it is active high.
If one name is specified, it is assumed the chip is hard-wired in this
configuration.
If two names are specified, it is assumed that a pinmux can switch between
the two wiring configurations. The first is the default mode for reading
and writing registers on the chip and the second is the mode for reading
the conversion data from the chip.
oneOf:
- items:
- enum:
- default
- single
- multi
- chain
- items:
- const: default
- enum:
- single
- multi
- chain
pinctrl-0:
maxItems: 1
pinctrl-1:
maxItems: 1
> + description: |
> + This property indicates the SPI wiring configuration.
> +
> + When this property is omitted, it is assumed that the device is using what
> + the datasheet calls "4-wire mode". This is the conventional SPI mode used
> + when there are multiple devices on the same bus. In this mode, the CNV
> + line is used to initiate the conversion and the SDI line is connected to
> + CS on the SPI controller.
> +
> + When this property is present, it indicates that the device is using one
> + of the following alternative wiring configurations:
> +
> + * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
> + definition of 3-wire mode is NOT at all related to the standard
> + spi-3wire property!) This mode is often used when the ADC is the only
> + device on the bus. In this mode, SDI is tied to VIO, and the CNV line
> + can be connected to the CS line of the SPI controller or to a GPIO, in
> + which case the CS line of the controller is unused.
> + * chain: The datasheet calls this "chain mode". This mode is used to save
> + on wiring when multiple ADCs are used. In this mode, the SDI line of
> + one chip is tied to the SDO of the next chip in the chain and the SDI of
> + the last chip in the chain is tied to GND. Only the first chip in the
> + chain is connected to the SPI bus. The CNV line of all chips are tied
> + together. The CS line of the SPI controller can be used as the CNV line
> + only if it is active high.
> +
> + '#daisy-chained-devices': true
> +
> + vdd-supply:
> + description: A 1.8V supply that powers the chip (VDD).
> +
> + vio-supply:
> + description:
> + A 1.8V to 5.5V supply for the digital inputs and outputs (VIO).
> +
> + ref-supply:
> + description:
> + A 2.5 to 5V supply for the external reference voltage (REF).
> +
> + cnv-gpios:
> + description:
> + The Convert Input (CNV). This input has multiple functions. It initiates
> + the conversions and selects the SPI mode of the device (chain or CS). In
> + 'single' mode, this property is omitted if the CNV pin is connected to the
> + CS line of the SPI controller. If 'single' mode is selected and this GPIO
> + is provided, it must be active low.
Since the conversion is triggered on the low to high transition of
CNV, I think it only makes sense to have it active high and not active
low.
> + maxItems: 1
> +
> + adi,high-z-input:
> + type: boolean
> + description:
> + High-Z mode allows the amplifier and RC filter in front of the ADC to be
> + chosen based on the signal bandwidth of interest, rather than the settling
> + requirements of the switched capacitor SAR ADC inputs.
> +
> + adi,gain-milli:
> + description: |
> + The hardware gain applied to the ADC input (in milli units).
> + The gain provided by the ADC input scaler is defined by the hardware
> + connections between chip pins OUT+, R1K-, R1K1-, R1K+, R1K1+, and OUT-.
> + If not present, default to 1000 (no actual gain applied).
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [454, 909, 1000, 1900]
> + default: 1000
Same suggestion as in V1 - we should make it clear that this property
only applies to ADAQ chips (in the description and also a -if: for the
bindings validator). Also, looking at the datasheet, it looks like
there are a lot more pins on the ADAQ chips, so I think there are more
properties missing here.
Some trivial ones:
vs-pos-supply (VS+ pin, 0 to 11V supply) and vs-neg-supply (VS- pin,
-11 to 0V supply)
pd-amp-gpios (active low) and pd-ref-gpios (active low) for optional
runtime power management.
Also the datasheet says the ADAQ chips supports "Single-ended to
differential conversion". So it seems like we might need some extra
properties to describe that case (a flag for indicating single-ended
wiring and an optional voltage supply to describe what is connected to
the negative input if it isn't tied to GND)
^ permalink raw reply
* Re: [PATCH v2 2/2] iio: adc: Add support for AD4000
From: David Lechner @ 2024-04-09 3:05 UTC (permalink / raw)
To: Marcelo Schmitt
Cc: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt, marcelo.schmitt1, linux-iio, devicetree, linux-kernel
In-Reply-To: <1d95d7d023dad69b894a2d0e7b0bad9d569ae382.1712585500.git.marcelo.schmitt@analog.com>
On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> Add support for AD4000 family of low noise, low power, high speed,
> successive aproximation register (SAR) ADCs.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
> MAINTAINERS | 1 +
> drivers/iio/adc/Kconfig | 12 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ad4000.c | 649 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 663 insertions(+)
> create mode 100644 drivers/iio/adc/ad4000.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5dfe118a5dd3..86aa96115f5a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1165,6 +1165,7 @@ L: linux-iio@vger.kernel.org
> S: Supported
> W: https://ez.analog.com/linux-software-drivers
> F: Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> +F: drivers/iio/adc/ad4000.c
>
> ANALOG DEVICES INC AD4130 DRIVER
> M: Cosmin Tanislav <cosmin.tanislav@analog.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 8db68b80b391..9c9d13d4b74f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -21,6 +21,18 @@ config AD_SIGMA_DELTA
> select IIO_BUFFER
> select IIO_TRIGGERED_BUFFER
>
> +config AD4000
> + tristate "Analog Devices AD4000 ADC Driver"
> + depends on SPI
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + Say yes here to build support for Analog Devices AD4000 high speed
> + SPI analog to digital converters (ADC).
> +
> + To compile this driver as a module, choose M here: the module will be
> + called ad4000.
> +
> config AD4130
> tristate "Analog Device AD4130 ADC Driver"
> depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index edb32ce2af02..aa52068d864b 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -6,6 +6,7 @@
> # When adding new entries keep the list in alphabetical order
> obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
> obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> +obj-$(CONFIG_AD4000) += ad4000.o
> obj-$(CONFIG_AD4130) += ad4130.o
> obj-$(CONFIG_AD7091R) += ad7091r-base.o
> obj-$(CONFIG_AD7091R5) += ad7091r5.o
> diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
> new file mode 100644
> index 000000000000..7997d9d98743
> --- /dev/null
> +++ b/drivers/iio/adc/ad4000.c
> @@ -0,0 +1,649 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD4000 SPI ADC driver
> + *
> + * Copyright 2024 Analog Devices Inc.
> + */
> +#include <asm/unaligned.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +#include <linux/units.h>
> +#include <linux/util_macros.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#define AD400X_READ_COMMAND 0x54
> +#define AD400X_WRITE_COMMAND 0x14
> +
> +/* AD4000 Configuration Register programmable bits */
> +#define AD4000_STATUS BIT(4) /* Status bits output */
> +#define AD4000_SPAN_COMP BIT(3) /* Input span compression */
> +#define AD4000_HIGHZ BIT(2) /* High impedance mode */
> +#define AD4000_TURBO BIT(1) /* Turbo mode */
Usually bits of the same register share a similar prefix, e.g.
AD4000_CFG_TURBO, AD4000_CFG_HIGHZ, etc.
> +
> +#define AD4000_TQUIET2_NS 60
> +
> +#define AD4000_18BIT_MSK GENMASK(31, 14)
> +#define AD4000_20BIT_MSK GENMASK(31, 12)
> +
> +#define AD4000_DIFF_CHANNEL(_sign, _real_bits) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .differential = 1, \
> + .channel = 0, \
> + .channel2 = 1, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
> + .scan_type = { \
> + .sign = _sign, \
> + .realbits = _real_bits, \
> + .storagebits = _real_bits > 16 ? 32 : 16, \
> + .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
> + .endianness = IIO_BE, \
> + }, \
> + } \
> +
> +#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = 0, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
> + .scan_type = { \
> + .sign = _sign, \
> + .realbits = _real_bits, \
> + .storagebits = _real_bits > 16 ? 32 : 16, \
> + .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
> + .endianness = IIO_BE, \
> + }, \
> + } \
It looks like all differential chips are signed and all
pseduo-differential chips are unsigned, so I don't think we need the
_sign parameter in these macros.
I also still have doubts about using IIO_BE and 8-bit xfers when it
comes to adding support later to achieve max sample rate with a SPI
offload. For example to get 2MSPS with an 18-bit chip, it will require
an approx 33% faster SPI clock than the actual slowest clock possible
because it will have to read 6 extra bits per sample. I didn't check
the specs, but this may not even be physically possible without
exceeding the datasheet max SPI clock rate. Also errors could be
reduced if we could actually use the slowest allowable SPI clock rate.
Furthermore, the offload hardware would have to be capable of adding
an extra byte per sample for 18 and 20-bit chips when piping the data
to DMA in order to get the 32-bit alignment in the buffer required by
IIO scan_type and the natural alignment requirements of IIO buffers in
general.
> +
> +enum ad4000_ids {
> + ID_AD4000,
> + ID_AD4001,
> + ID_AD4002,
> + ID_AD4003,
> + ID_AD4004,
> + ID_AD4005,
> + ID_AD4006,
> + ID_AD4007,
> + ID_AD4008,
> + ID_AD4010,
> + ID_AD4011,
> + ID_AD4020,
> + ID_AD4021,
> + ID_AD4022,
> + ID_ADAQ4001,
> + ID_ADAQ4003,
> +};
> +
> +struct ad4000_chip_info {
> + const char *dev_name;
> + struct iio_chan_spec chan_spec;
> +};
> +
> +static const struct ad4000_chip_info ad4000_chips[] = {
> + [ID_AD4000] = {
> + .dev_name = "ad4000",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> + },
> + [ID_AD4001] = {
> + .dev_name = "ad4001",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> + },
> + [ID_AD4002] = {
> + .dev_name = "ad4002",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> + },
> + [ID_AD4003] = {
> + .dev_name = "ad4003",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> + },
> + [ID_AD4004] = {
> + .dev_name = "ad4004",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> + },
> + [ID_AD4005] = {
> + .dev_name = "ad4005",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> + },
> + [ID_AD4006] = {
> + .dev_name = "ad4006",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> + },
> + [ID_AD4007] = {
> + .dev_name = "ad4007",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> + },
> + [ID_AD4008] = {
> + .dev_name = "ad4008",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> + },
> + [ID_AD4010] = {
> + .dev_name = "ad4010",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> + },
> + [ID_AD4011] = {
> + .dev_name = "ad4011",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> + },
> + [ID_AD4020] = {
> + .dev_name = "ad4020",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> + },
> + [ID_AD4021] = {
> + .dev_name = "ad4021",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> + },
> + [ID_AD4022] = {
> + .dev_name = "ad4022",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> + },
> + [ID_ADAQ4001] = {
> + .dev_name = "adaq4001",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> + },
> + [ID_ADAQ4003] = {
> + .dev_name = "adaq4003",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> + },
> +};
> +
> +enum ad4000_gains {
> + AD4000_0454_GAIN = 0,
> + AD4000_0909_GAIN = 1,
> + AD4000_1_GAIN = 2,
AD4000_1000_GAIN would be more consistent with the others.
> + AD4000_1900_GAIN = 3,
> + AD4000_GAIN_LEN
> +};
> +
> +/*
> + * Gains stored and computed as fractions to avoid introducing rounding errors.
> + */
> +static const int ad4000_gains_frac[AD4000_GAIN_LEN][2] = {
> + [AD4000_0454_GAIN] = { 227, 500 },
> + [AD4000_0909_GAIN] = { 909, 1000 },
> + [AD4000_1_GAIN] = { 1, 1 },
> + [AD4000_1900_GAIN] = { 19, 10 },
> +};
Why not just store the numerator in milli units and always use 1000
for the denominator? It seems like it would simplify the code and make
it easier to read and understand. Also, these values are coming from
the adi,gain-milli property already, so we could avoid the enum and
the lookup table entirely and simplify things even more.
> +
> +struct ad4000_state {
> + struct spi_device *spi;
> + struct gpio_desc *cnv_gpio;
> + int vref;
> + bool status_bits;
> + bool span_comp;
> + bool turbo_mode;
> + bool high_z_mode;
> +
> + enum ad4000_gains pin_gain;
> + int scale_tbl[AD4000_GAIN_LEN][2][2];
> +
> + /*
> + * DMA (thus cache coherency maintenance) requires the
> + * transfer buffers to live in their own cache lines.
> + */
> + struct {
> + union {
> + u16 sample_buf16;
> + u32 sample_buf32;
Technically, these are holding big-endian data, so __be16 and __be32
would be more correct.
> + } data;
> + s64 timestamp __aligned(8);
> + } scan;
> + __be16 tx_buf __aligned(IIO_DMA_MINALIGN);
> + __be16 rx_buf;
> +};
scan.data is used as SPI rx_buf so __aligned(IIO_DMA_MINALIGN); needs
to be moved to the scan field.
> +
> +static void ad4000_fill_scale_tbl(struct ad4000_state *st, int scale_bits,
> + const struct ad4000_chip_info *chip)
> +{
> + int diff = chip->chan_spec.differential;
> + int val, val2, tmp0, tmp1, i;
> + u64 tmp2;
> +
> + val2 = scale_bits;
> + for (i = 0; i < AD4000_GAIN_LEN; i++) {
Only one gain is selected by the devicetree, so why do we need to do
this for all 4 gains?
> + val = st->vref / 1000;
> + /* Multiply by MILLI here to avoid losing precision */
> + val = mult_frac(val, ad4000_gains_frac[i][1] * MILLI,
> + ad4000_gains_frac[i][0]);
> + /* Would multiply by NANO here but we already multiplied by MILLI */
> + tmp2 = shift_right((u64)val * MICRO, val2);
> + tmp0 = (int)div_s64_rem(tmp2, NANO, &tmp1);
> + /* Store scale for when span compression is disabled */
> + st->scale_tbl[i][0][0] = tmp0; /* Integer part */
> + st->scale_tbl[i][0][1] = abs(tmp1); /* Fractional part */
> + /* Store scale for when span compression is enabled */
> + st->scale_tbl[i][1][0] = tmp0;
> + if (diff)
> + st->scale_tbl[i][1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
> + else
> + st->scale_tbl[i][1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
> + }
> +}
> +
> +static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
> +{
> + put_unaligned_be16(AD400X_WRITE_COMMAND << BITS_PER_BYTE | val,
> + &st->tx_buf);
> + return spi_write(st->spi, &st->tx_buf, 2);
> +}
> +
> +static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
> +{
> + struct spi_transfer t[] = {
> + {
> + .tx_buf = &st->tx_buf,
> + .rx_buf = &st->rx_buf,
> + .len = 2,
> + },
> + };
> + int ret;
> +
> + put_unaligned_be16(AD400X_READ_COMMAND << BITS_PER_BYTE, &st->tx_buf);
> + ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> + if (ret < 0)
> + return ret;
> +
> + *val = get_unaligned_be16(&st->rx_buf);
> +
> + return ret;
> +}
> +
It would be very helpful to have comments here explaining the exact
expected wiring configuration and signal timing here since there are
so many possibilities for this chip.
> +static int ad4000_read_sample(struct ad4000_state *st,
> + const struct iio_chan_spec *chan)
> +{
> + struct spi_transfer t[] = {
Don't really need [] here since there is only one xfer.
> + {
> + .rx_buf = &st->scan.data,
> + .len = BITS_TO_BYTES(chan->scan_type.storagebits),
> + .delay = {
> + .value = AD4000_TQUIET2_NS,
> + .unit = SPI_DELAY_UNIT_NSECS,
> + },
> + },
> + };
> + int ret;
> +
> + ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int ad4000_single_conversion(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan, int *val)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> + u32 sample;
> + int ret;
> +
> + if (st->cnv_gpio)
> + gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_HIGH);
It would make more sense and be less redundant to move the gpio code
into ad4000_read_sample().
Also, gpiod_set_value_cansleep() checks for NULL, so the if () is redundant.
> +
> + ret = ad4000_read_sample(st, chan);
> + if (ret)
> + return ret;
> +
> + if (st->cnv_gpio)
> + gpiod_set_value_cansleep(st->cnv_gpio, GPIOD_OUT_LOW);
> +
> + if (chan->scan_type.storagebits > 16)
> + sample = get_unaligned_be32(&st->scan.data);
> + else
> + sample = get_unaligned_be16(&st->scan.data);
data is aligned, so be32/16_to_cpu() should be fine. Also, Jonathan
will suggest to use &st->scan.data.sample_b32/16 here too. :-)
> +
> + switch (chan->scan_type.realbits) {
> + case 16:
> + break;
> + case 18:
> + sample = FIELD_GET(AD4000_18BIT_MSK, sample);
> + break;
> + case 20:
> + sample = FIELD_GET(AD4000_20BIT_MSK, sample);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (chan->scan_type.sign == 's')
> + *val = sign_extend32(sample, chan->scan_type.realbits - 1);
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int ad4000_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long info)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> + return ad4000_single_conversion(indio_dev, chan, val);
> + unreachable();
> + case IIO_CHAN_INFO_SCALE:
> + *val = st->scale_tbl[st->pin_gain][st->span_comp][0];
> + *val2 = st->scale_tbl[st->pin_gain][st->span_comp][1];
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_OFFSET:
> + *val = 0;
> + if (st->span_comp)
> + *val = mult_frac(st->vref / 1000, 1, 10);
> +
> + return IIO_VAL_INT;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ad4000_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long info)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SCALE:
> + *vals = (int *)st->scale_tbl[st->pin_gain];
> + *length = 2 * 2;
> + *type = IIO_VAL_INT_PLUS_NANO;
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> + return -EINVAL;
not reachable because of default, so can be left out
> +}
> +
> +static int ad4000_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val, int val2,
> + long mask)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> + unsigned int reg_val;
> + bool span_comp_en;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + ret = ad4000_read_reg(st, ®_val);
> + if (ret < 0)
> + return ret;
> +
> + span_comp_en = (val2 == st->scale_tbl[st->pin_gain][1][1]);
> + reg_val &= ~AD4000_SPAN_COMP;
> + reg_val |= FIELD_PREP(AD4000_SPAN_COMP, span_comp_en);
> +
> + ret = ad4000_write_reg(st, reg_val);
> + if (ret < 0)
> + return ret;
> +
> + st->span_comp = span_comp_en;
> + return 0;
> + }
> + unreachable();
Can bring out the return 0 to avoid unreachable.
> + default:
> + break;
Can return -EINVAL to avoid break;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static irqreturn_t ad4000_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct ad4000_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (st->cnv_gpio)
> + gpiod_set_value(st->cnv_gpio, GPIOD_OUT_HIGH);
> +
> + ret = ad4000_read_sample(st, &indio_dev->channels[0]);
> + if (ret < 0)
> + goto err_out;
> +
> + if (st->cnv_gpio)
> + gpiod_set_value(st->cnv_gpio, GPIOD_OUT_LOW);
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
> + iio_get_time_ns(indio_dev));
> +
> +err_out:
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info ad4000_info = {
> + .read_raw = &ad4000_read_raw,
> + .read_avail = &ad4000_read_avail,
> + .write_raw = &ad4000_write_raw,
> + .write_raw_get_fmt = &ad4000_write_raw_get_fmt,
> +};
> +
> +static void ad4000_config(struct ad4000_state *st)
> +{
> + unsigned int reg_val;
> + int ret;
> +
> + reg_val = FIELD_PREP(AD4000_TURBO, 1);
Since the driver in it's current state can get anywhere near the max
sample rate of ~1MSPS, I don't think it makes sense to enable turbo at
this point.
> +
> + if (device_property_present(&st->spi->dev, "adi,high-z-input"))
> + reg_val |= FIELD_PREP(AD4000_HIGHZ, 1);
> +
> + /*
> + * The ADC SDI pin might be connected to controller CS line in which
> + * case the write might fail. This, however, does not prevent the device
> + * from functioning even though in a configuration other than the
> + * requested one.
> + */
> + ret = ad4000_write_reg(st, reg_val);
> + if (ret < 0)
> + dev_dbg(&st->spi->dev, "Failed to config device\n");
If writing fails because there is no CS line wired up, we won't get an
error returned here. The SPI controller has no way of knowing this
happened, so it can only assume the write was successful and return 0.
So this should return ret.
Ideally, the devicetree should tell us if CS is wired up or not.
> +}
> +
> +static void ad4000_regulator_disable(void *reg)
> +{
> + regulator_disable(reg);
> +}
> +
> +static int ad4000_probe(struct spi_device *spi)
> +{
> + const struct ad4000_chip_info *chip;
> + struct regulator *vref_reg;
> + struct iio_dev *indio_dev;
> + struct ad4000_state *st;
> + int ret;
We need a check somewhere in here to make sure that adi,spi-mode is in
a supported configuration. E.g. chain mode is not currently
implemented.
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + chip = spi_get_device_match_data(spi);
> + if (!chip)
> + return -EINVAL;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> +
> + ret = devm_regulator_get_enable(&spi->dev, "vdd");
> + if (ret)
> + return dev_err_probe(&spi->dev, ret, "Failed to enable VDD supply\n");
> +
> + ret = devm_regulator_get_enable(&spi->dev, "vio");
> + if (ret)
> + return dev_err_probe(&spi->dev, ret, "Failed to enable VIO supply\n");
> +
> + vref_reg = devm_regulator_get(&spi->dev, "ref");
> + if (IS_ERR(vref_reg))
> + return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
> + "Failed to get vref regulator\n");
> +
> + ret = regulator_enable(vref_reg);
> + if (ret < 0)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to enable voltage regulator\n");
> +
> + ret = devm_add_action_or_reset(&spi->dev, ad4000_regulator_disable, vref_reg);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to add regulator disable action\n");
> +
> + st->vref = regulator_get_voltage(vref_reg);
> + if (st->vref < 0)
> + return dev_err_probe(&spi->dev, st->vref, "Failed to get vref\n");
> +
> + st->cnv_gpio = devm_gpiod_get_optional(&spi->dev, "cnv", GPIOD_OUT_HIGH);
> + if (IS_ERR(st->cnv_gpio)) {
> + if (PTR_ERR(st->cnv_gpio) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
EPROBE_DEFER check is not needed with dev_err_probe();, it already does that.
> +
> + return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio),
> + "Failed to get CNV GPIO");
> + }
> +
> + ad4000_config(st);
> +
> + indio_dev->name = chip->dev_name;
> + indio_dev->info = &ad4000_info;
> + indio_dev->channels = &chip->chan_spec;
> + indio_dev->num_channels = 1;
> +
> + st->pin_gain = AD4000_1_GAIN;
> + if (device_property_present(&spi->dev, "adi,gain-milli")) {
> + u32 val;
Should it be an error if adi,gain-milli is set on non-adaq chips?
> +
> + ret = device_property_read_u32(&spi->dev, "adi,gain-milli", &val);
> + if (ret)
> + return ret;
> +
> + switch (val) {
> + case 454:
> + st->pin_gain = AD4000_0454_GAIN;
> + break;
> + case 909:
> + st->pin_gain = AD4000_0909_GAIN;
> + break;
> + case 1000:
> + st->pin_gain = AD4000_1_GAIN;
> + break;
> + case 1900:
> + st->pin_gain = AD4000_1900_GAIN;
> + break;
> + default:
> + return dev_err_probe(&spi->dev, -EINVAL,
> + "Invalid firmware provided gain\n");
Could help debugging if val is included in the error message.
> + }
> + }
> +
> + /*
> + * ADCs that output twos complement code have one less bit to express
> + * voltage magnitude.
> + */
> + if (chip->chan_spec.scan_type.sign == 's')
> + ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits - 1,
> + chip);
> + else
> + ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits,
> + chip);
> +
> + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> + &iio_pollfunc_store_time,
> + &ad4000_trigger_handler, NULL);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct spi_device_id ad4000_id[] = {
> + { "ad4000", (kernel_ulong_t)&ad4000_chips[ID_AD4000] },
> + { "ad4001", (kernel_ulong_t)&ad4000_chips[ID_AD4001] },
> + { "ad4002", (kernel_ulong_t)&ad4000_chips[ID_AD4002] },
> + { "ad4003", (kernel_ulong_t)&ad4000_chips[ID_AD4003] },
> + { "ad4004", (kernel_ulong_t)&ad4000_chips[ID_AD4004] },
> + { "ad4005", (kernel_ulong_t)&ad4000_chips[ID_AD4005] },
> + { "ad4006", (kernel_ulong_t)&ad4000_chips[ID_AD4006] },
> + { "ad4007", (kernel_ulong_t)&ad4000_chips[ID_AD4007] },
> + { "ad4008", (kernel_ulong_t)&ad4000_chips[ID_AD4008] },
> + { "ad4010", (kernel_ulong_t)&ad4000_chips[ID_AD4010] },
> + { "ad4011", (kernel_ulong_t)&ad4000_chips[ID_AD4011] },
> + { "ad4020", (kernel_ulong_t)&ad4000_chips[ID_AD4020] },
> + { "ad4021", (kernel_ulong_t)&ad4000_chips[ID_AD4021] },
> + { "ad4022", (kernel_ulong_t)&ad4000_chips[ID_AD4022] },
> + { "adaq4001", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4001] },
> + { "adaq4003", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4003] },
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, ad4000_id);
> +
> +static const struct of_device_id ad4000_of_match[] = {
> + { .compatible = "adi,ad4000", .data = &ad4000_chips[ID_AD4000] },
> + { .compatible = "adi,ad4001", .data = &ad4000_chips[ID_AD4001] },
> + { .compatible = "adi,ad4002", .data = &ad4000_chips[ID_AD4002] },
> + { .compatible = "adi,ad4003", .data = &ad4000_chips[ID_AD4003] },
> + { .compatible = "adi,ad4004", .data = &ad4000_chips[ID_AD4004] },
> + { .compatible = "adi,ad4005", .data = &ad4000_chips[ID_AD4005] },
> + { .compatible = "adi,ad4006", .data = &ad4000_chips[ID_AD4006] },
> + { .compatible = "adi,ad4007", .data = &ad4000_chips[ID_AD4007] },
> + { .compatible = "adi,ad4008", .data = &ad4000_chips[ID_AD4008] },
> + { .compatible = "adi,ad4010", .data = &ad4000_chips[ID_AD4010] },
> + { .compatible = "adi,ad4011", .data = &ad4000_chips[ID_AD4011] },
> + { .compatible = "adi,ad4020", .data = &ad4000_chips[ID_AD4020] },
> + { .compatible = "adi,ad4021", .data = &ad4000_chips[ID_AD4021] },
> + { .compatible = "adi,ad4022", .data = &ad4000_chips[ID_AD4022] },
> + { .compatible = "adi,adaq4001", .data = &ad4000_chips[ID_ADAQ4001] },
> + { .compatible = "adi,adaq4003", .data = &ad4000_chips[ID_ADAQ4003] },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad4000_of_match);
> +
> +static struct spi_driver ad4000_driver = {
> + .driver = {
> + .name = "ad4000",
> + .of_match_table = ad4000_of_match,
> + },
> + .probe = ad4000_probe,
> + .id_table = ad4000_id,
> +};
> +module_spi_driver(ad4000_driver);
> +
> +MODULE_AUTHOR("Mircea Caprioru <mircea.caprioru@analog.com>");
> +MODULE_AUTHOR("Marcelo Schmitt <marcelo.schmitt@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices AD4000 ADC driver");
> +MODULE_LICENSE("GPL");
> --
> 2.43.0
>
>
^ permalink raw reply
* Re: [PATCH 1/1] arm64: dts: rockchip: enable onboard spi flash for rock-3a
From: Chukun Pan @ 2024-04-09 3:28 UTC (permalink / raw)
To: jonas
Cc: amadeus, conor+dt, devicetree, heiko, krzk+dt, linux-arm-kernel,
linux-kernel, linux-rockchip, robh
In-Reply-To: <fb57172e-2517-498c-9412-76f62929d24a@kwiboo.se>
> I think the on-board spi nor flash support 104/108/133 mhz
> so 50 mhz is wrong in any case.
Thanks, I tested 104MHz on linux and it works fine. As for
133 MHz, only MX25U12835FZNI-08G supports it. And I didn't
find it support 108MHz in the datasheet of MX25U12835F.
Thanks,
Chukun
^ permalink raw reply
* Re: [PATCH v1 2/3] dt-bindings: arm: mediatek: mmsys: Add OF graph support for board path
From: Chen-Yu Tsai @ 2024-04-09 3:39 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: chunkuang.hu, robh, krzysztof.kozlowski+dt, conor+dt, p.zabel,
airlied, daniel, maarten.lankhorst, mripard, tzimmermann,
matthias.bgg, shawn.sung, yu-chang.lee, ck.hu, jitao.shi,
devicetree, linux-kernel, dri-devel, linux-mediatek,
linux-arm-kernel, kernel
In-Reply-To: <5e385c2e-6509-4347-96a5-4606b32d20ff@collabora.com>
On Mon, Apr 8, 2024 at 6:16 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 08/04/24 05:20, Chen-Yu Tsai ha scritto:
> > On Thu, Apr 4, 2024 at 4:16 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> >>
> >> Document OF graph on MMSYS/VDOSYS: this supports up to three DDP paths
> >> per HW instance (so potentially up to six displays for multi-vdo SoCs).
> >>
> >> The MMSYS or VDOSYS is always the first component in the DDP pipeline,
> >> so it only supports an output port with multiple endpoints - where each
> >> endpoint defines the starting point for one of the (currently three)
> >> possible hardware paths.
> >>
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >> ---
> >> .../bindings/arm/mediatek/mediatek,mmsys.yaml | 23 +++++++++++++++++++
> >> 1 file changed, 23 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
> >> index b3c6888c1457..90758bb5bcb1 100644
> >> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
> >> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
> >> @@ -93,6 +93,29 @@ properties:
> >> '#reset-cells':
> >> const: 1
> >>
> >> + port:
> >> + $ref: /schemas/graph.yaml#/properties/port
> >> + description:
> >> + Output port node. This port connects the MMSYS/VDOSYS output to
> >> + the first component of one display pipeline, for example one of
> >> + the available OVL or RDMA blocks.
> >> + Some MediaTek SoCs support up to three display outputs per MMSYS.
> >> + properties:
> >> + endpoint@0:
> >> + $ref: /schemas/graph.yaml#/properties/endpoint
> >> + description: Output to the primary display pipeline
> >> +
> >> + endpoint@1:
> >> + $ref: /schemas/graph.yaml#/properties/endpoint
> >> + description: Output to the secondary display pipeline
> >> +
> >> + endpoint@2:
> >> + $ref: /schemas/graph.yaml#/properties/endpoint
> >> + description: Output to the tertiary display pipeline
> >> +
> >> + required:
> >> + - endpoint@0
> >> +
> >
> > Technically the mmsys device serves as an glue layer for the display
> > pipeline, providing things like clock control and signal routing; the
> > device itself is not part of the pipeline, and probably shouldn't be
> > part of the graph?
> >
>
> That is (only) partially true: in the case of older SoCs, the MMSYS can only
> connect to a single first IP of the pipeline, but in the case of newer ones,
> and especially (but not limited to) MT8195 onwards having multiple instances
> of VDOSYS, that really becomes part of the pipeline.
>
> This is not because of the possible different first IP in the pipeline, but
> because of support for dual-interface (DSI and DP) that, in even newer SoCs,
> can be done with cross-mmsys (cross-vdosys, actually...) as some of those do
> have the two in different VDOs.
>
> So yes, this can be done without the graph in MMSYS *in this precise moment in
> time*, but we'll anyway end up adding it sooner than later - and I'm doing this
> right now, instead of later, because it's also simplifying the implementation
> so like that I'm "catching two birds with one stone" :-)
I see. Thanks for sorting it out. We had something similar on Allwinner
platforms but it was never as complex or flexible as this.
ChenYu
> Cheers,
> Angelo
>
> > ChenYu
> >
> >> required:
> >> - compatible
> >> - reg
> >> --
> >> 2.44.0
> >>
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox