Re: [PATCH 4/5] soundwire: qcom: avoid dependency on CONFIG_SLIMBUS

[Jonathan Marek]

On 6/9/20 5:52 AM, Srinivas Kandagatla wrote:
> On 08/06/2020 21:43, Jonathan Marek wrote:
> > The driver may be used without slimbus, so don't depend on slimbus.
> >
> > Signed-off-by: Jonathan Marek <jonathan@xxxxxxxx>
> > ---
> > Â drivers/soundwire/Kconfig | 1 -
> >  drivers/soundwire/qcom.c | 5 +++++
> > Â 2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig
> > index fa2b4ab92ed9..d121cf739090 100644
> > --- a/drivers/soundwire/Kconfig
> > +++ b/drivers/soundwire/Kconfig
> > @@ -33,7 +33,6 @@ config SOUNDWIRE_INTEL
> > Â config SOUNDWIRE_QCOM
> > ÂÂÂÂÂ tristate "Qualcomm SoundWire Master driver"
> > -ÂÂÂ depends on SLIMBUS
> > ÂÂÂÂÂ depends on SND_SOC
>
> Why not move this to imply SLIMBUS this will give more flexibility!

If you mean to change it to "select SLIMBUS", I'd prefer not to, because 
this would increase code size unnecessarily in my kernel.

> > ÂÂÂÂÂ help
> > ÂÂÂÂÂÂÂ SoundWire Qualcomm Master driver.
> > diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> > index 14334442615f..ac81c64768ea 100644
> > --- a/drivers/soundwire/qcom.c
> > +++ b/drivers/soundwire/qcom.c
> > @@ -769,13 +769,18 @@ static int qcom_swrm_probe(struct 
> > platform_device *pdev)
> > ÂÂÂÂÂ if (!ctrl)
> > ÂÂÂÂÂÂÂÂÂ return -ENOMEM;
> > +#ifdef CONFIG_SLIMBUS
> > ÂÂÂÂÂ if (dev->parent->bus == &slimbus_bus) {
> > +#else
> > +ÂÂÂ if (false) {
> > +#endif
>
> May be you can do bit more cleanup here, which could endup like:
>
>
> ctrl->regmap = dev_get_regmap(dev->parent, NULL);
> if (!ctrl->regmap) {
>  ÂÂÂÂres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  ÂÂÂÂif (res) {
>  ÂÂÂÂÂÂÂ ctrl->mmio = devm_ioremap_resource(dev, res);
>  ÂÂÂÂÂÂÂ if (IS_ERR(ctrl->mmio)) {
>  ÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "No valid mem resource found\n");
>  ÂÂÂÂÂÂÂÂÂÂÂ return PTR_ERR(ctrl->mmio);
>  ÂÂÂÂÂÂÂ }
>
>  ÂÂÂÂÂÂÂ ctrl->reg_read = qcom_swrm_cpu_reg_read;
>  ÂÂÂÂÂÂÂ ctrl->reg_write = qcom_swrm_cpu_reg_write;
>  ÂÂÂÂ} else {
>  ÂÂÂÂÂÂÂ dev_err(dev, "No valid slim resource found\n");
>  ÂÂÂÂÂÂÂ return -EINVAL;
>  ÂÂÂÂ}
> } else {
>  ÂÂÂÂctrl->reg_read = qcom_swrm_ahb_reg_read;
>  ÂÂÂÂctrl->reg_write = qcom_swrm_ahb_reg_write;
> }
>
>
>
> thanks,
> srini

I don't think this is better, it feels more obfuscated, and I think its 
possible we may end up with the mmio sdw having a parent with a regmap. 
(it is not how I did things up in my upstream stack, but in downstream 
the sdw nodes are inside the "macro" codec nodes)

I understand the '#ifdef CONFIG_SLIMBUS'/'dev->parent->bus == 
&slimbus_bus' is ugly, but at least its clear what's going on. Unless 
you have a better suggestion?

> > ÂÂÂÂÂÂÂÂÂ ctrl->reg_read = qcom_swrm_ahb_reg_read;
> > ÂÂÂÂÂÂÂÂÂ ctrl->reg_write = qcom_swrm_ahb_reg_write;
> > ÂÂÂÂÂÂÂÂÂ ctrl->regmap = dev_get_regmap(dev->parent, NULL);
> > ÂÂÂÂÂÂÂÂÂ if (!ctrl->regmap)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
> > ÂÂÂÂÂ } else {
> > +
> > ÂÂÂÂÂÂÂÂÂ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > ÂÂÂÂÂÂÂÂÂ ctrl->reg_read = qcom_swrm_cpu_reg_read;