Re: [PATCH v2 02/14] soundwire: Add SoundWire bus type

[Vinod Koul]

On Thu, Nov 16, 2017 at 05:24:37PM +0000, Mark Brown wrote:
> On Thu, Nov 16, 2017 at 10:32:21PM +0530, Vinod Koul wrote:
> > On Thu, Nov 16, 2017 at 04:05:17PM +0000, Srinivas Kandagatla wrote:
> 
> > > >+static const struct sdw_device_id *
> > > >+sdw_get_device_id(struct sdw_slave *slave, struct sdw_driver *drv)
> > > Indentation looks Odd here,
> 
> > not really, the return type in preceding line is a perfect way to define
> > things...
> 
> Yeah, that's fairly normal - just ugly long.  You could also do it by
> splitting the arguments over two lines.

IMO that makes it look even worse :(

static const struct sdw_device_id *sdw_get_device_id(
					struct sdw_slave *slave,
					struct sdw_driver *drv)

current one seemed lesser evil :)

> 
> > > >+	ret = dev_pm_domain_attach(dev, false);
> > > >+	if (ret) {
> 
> > > I think we discussed this in v1, but erring out here means that all the
> > > devices need to have pm domain  attached, which might not be true all the
> > > time.
> 
> > oh yes, sorry i missed this one. will fix
> 
> Looking at this pattern here I'm wondering if this should be folded into
> the domain code, everything attaching a domain is going to need to take
> care of this at the minute.

Yes, but then only busses call this, handful of users and some like platform
core have bit different handling, so it may not work for them

-- 
~Vinod