RE: [PATCH] ASoC: rt712: add GPIOs for LED control

["Shuming [范書銘]" <shumingf@realtek.com>]

> > +	dev_dbg(&rt712->slave->dev, "%s: gpio_pin=%d\n", __func__,
> > +gpio_pin);
> > +
> > +	ret = pm_runtime_resume(rt712->component->dev);
> > +	if (ret < 0 && ret != -EACCES)
> > +		return ret;
> 
> This pm_runtime_resume() usage can race with autosuspend, if the
> autosuspend timer is about to expire it could do so before we actually update
> the register values so the writes won't actually happen.  It's very unlikely to
> actually happen but the risk is there.

Thanks for review. 
I will use pm_runtime_resume_and_get() instead.

> > +	switch (gpio_pin) {
> > +	case 3:
> > +		break;
> > +	case 4:
> > +		rt712_sdca_index_update_bits(rt712, RT712_VENDOR_HDA_CTL,
> > +			RT712_HDA_LEGACY_CONFIG_CTL0, 0x000c, 0x0004);
> 
> This doesn't do any locking with other regmap users and gpiolib won't hold a
> lock for us - since rt712_sdca_index_update_bits() is a read/modify/write
> cycle this could lead to corruption.

Sure. May I add a lock in the callback instead of only locking rt712_sdca_index_update_bits()?
I will send v2 afterward.

> > +static void rt712_sdca_gpio_init(struct rt712_sdca_priv *rt712) {
> > +	int ret;
> > +
> > +	rt712->gpio_chip = rt712_sdca_template_chip;
> > +	rt712->gpio_chip.parent = &rt712->slave->dev;
> > +	rt712->gpio_chip.fwnode = dev_fwnode(&rt712->slave->dev);
> > +
> > +	ret = devm_gpiochip_add_data(&rt712->slave->dev, &rt712->gpio_chip,
> rt712);
> > +	if (ret != 0)
> > +		dev_err(&rt712->slave->dev, "Failed to add GPIOs: %d\n", ret); }
> 
> The gpiolib usage either needs to be conditional on gpiolib or we need the
> driver to depend on it.  Either is fine.

Add the 'IS_ENABLED(CONFIG_GPIOLIB)' check before calling rt712_sdca_gpio_init().

> >
> > +	/* initialize GPIOs */
> > +	rt712_sdca_gpio_init(rt712);
> > +
> >  	if (!rt712->first_hw_init)
> >  		return 0;
> >
> 
> This will happen every time the card binds and uses devm, I'd expect it to
> happen when the SoundWire device binds rather than during ASoC level
> initialisation.  We don't want to continually register and unregister the GPIOs.

Will move to sdw_driver.probe().

> > @@ -271,7 +276,6 @@ enum {
> >  	RT712_CLK_FREQ_22_5792MHZ = 4,
> >  };
> >
> > -
> >  int rt712_sdca_io_init(struct device *dev, struct sdw_slave *slave);
> > int rt712_sdca_init(struct device *dev, struct regmap *regmap,
> >  			struct regmap *mbq_regmap, struct sdw_slave *slave);
> 
> Unrelated whitespace change.

Wil fix