Re: [PATCH v3 03/12] soc: qcom: geni-se: Introduce helper API for resource initialization

[Praveen Talari]

Hi Konrad

On 1/30/2026 5:40 PM, Konrad Dybcio wrote:
> On 1/12/26 11:47 AM, Praveen Talari wrote:
> > The GENI Serial Engine drivers (I2C, SPI, and SERIAL) currently duplicate
> > code for initializing shared resources such as clocks and interconnect
> > paths.
> >
> > Introduce a new helper API, geni_se_resources_init(), to centralize this
> > initialization logic, improving modularity and simplifying the probe
> > function.
> >
> > Signed-off-by: Praveen Talari <praveen.talari@xxxxxxxxxxxxxxxx>
> > ---
> > v1 -> v2:
> >  From kernel test robot
> > - Updated proper return value for devm_pm_opp_set_clkname()
> > ---
> >   drivers/soc/qcom/qcom-geni-se.c  | 47 ++++++++++++++++++++++++++++++++
> >   include/linux/soc/qcom/geni-se.h |  6 ++++
> >   2 files changed, 53 insertions(+)
> >
> > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> > index b0542f836453..75e722cd1a94 100644
> > --- a/drivers/soc/qcom/qcom-geni-se.c
> > +++ b/drivers/soc/qcom/qcom-geni-se.c
> > @@ -19,6 +19,7 @@
> >   #include <linux/of_platform.h>
> >   #include <linux/pinctrl/consumer.h>
> >   #include <linux/platform_device.h>
> > +#include <linux/pm_opp.h>
> >   #include <linux/soc/qcom/geni-se.h>
> >   
> >   /**
> > @@ -1012,6 +1013,52 @@ int geni_icc_disable(struct geni_se *se)
> >   }
> >   EXPORT_SYMBOL_GPL(geni_icc_disable);
> >   
> > +/**
> > + * geni_se_resources_init() - Initialize resources for a GENI SE device.
> > + * @se: Pointer to the geni_se structure representing the GENI SE device.
> > + *
> > + * This function initializes various resources required by the GENI Serial Engine
> > + * (SE) device, including clock resources (core and SE clocks), interconnect
> > + * paths for communication.
> > + * It retrieves optional and mandatory clock resources, adds an OF-based
> > + * operating performance point (OPP) table, and sets up interconnect paths
> > + * with default bandwidths. The function also sets a flag (`has_opp`) to
> > + * indicate whether OPP support is available for the device.
> > + *
> > + * Return: 0 on success, or a negative errno on failure.
> > + */
> > +int geni_se_resources_init(struct geni_se *se)
> > +{
> > +	int ret;
> > +
> > +	se->core_clk = devm_clk_get_optional(se->dev, "core");
> > +	if (IS_ERR(se->core_clk))
> > +		return dev_err_probe(se->dev, PTR_ERR(se->core_clk),
> > +				     "Failed to get optional core clk\n");
> > +
> > +	se->clk = devm_clk_get(se->dev, "se");
> > +	if (IS_ERR(se->clk) && !has_acpi_companion(se->dev))
> > +		return dev_err_probe(se->dev, PTR_ERR(se->clk),
> > +				     "Failed to get SE clk\n");
> > +
> > +	ret = devm_pm_opp_set_clkname(se->dev, "se");
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_pm_opp_of_add_table(se->dev);
> > +	if (ret && ret != -ENODEV)
> > +		return dev_err_probe(se->dev, ret, "Failed to add OPP table\n");
> > +
> > +	se->has_opp = (ret == 0);
> > +
> > +	ret = geni_icc_get(se, "qup-memory");
>
> The second argument is a NOP after patch 1.. originally I think I had a
> cross-subsys patch to get rid of that, neither solution is exactly pretty..

I will drop the second argument once these changes are ported across 
UART and SPI as well.

Thanks,
Praveen Talari

> But otherwise, this looks good
>
> Konrad