On Mon, 21 Oct 2019, Srinivas Kandagatla wrote: > Thanks Lee for taking time to review. > > I agree with most of the style related comments, will fix them in next > version. For others I have replied it inline. [...] > > > +static int wcd934x_bring_up(struct wcd934x_data *wcd) > > > +{ > > > + struct regmap *rm = wcd->regmap; > > > > It's much more common to use 'regmap' or 'map'. > > Only reason to make it short here is to save some lines! > If you prefer regmap, I will add that in next version! I doubt it'd save many (any?) lines. [...] > > > +static int wcd934x_slim_status(struct slim_device *sdev, > > > + enum slim_device_status status) > > > +{ > > > + struct device *dev = &sdev->dev; > > > + struct wcd934x_data *wcd; > > > + int ret; > > > > This is semantically odd! Why are you doing most of the > > initialisation and bring-up in 'status' and not 'probe'. Seems > > broken to me. > > SLIMBus device will not be in a state to communicate before enumeration (at > probe), so all the device initialization is done in status callback where it > is ready for communication. Why do we need the device to be up *before* calling probe? > This is same with SoundWire Bus as well! [...] > > > + struct device *dev; > > > + struct clk *extclk; > > > + struct regmap *regmap; > > > + struct slim_device *sdev; > > > > You don't need 'sdev' and 'dev'. > > slim_device instance (sdev) is required by audio codec driver to allocate > stream runtime. You can extrapolate one from the other. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel