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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

On 10/11/17 04:59, Vinod Koul wrote:
> On Thu, Nov 09, 2017 at 09:14:07PM +0000, Srinivas Kandagatla wrote:
>>
>>
>> On 19/10/17 04:03, Vinod Koul wrote:
>>> This adds the base SoundWire bus type, bus and driver registration.
>>> along with changes to module device table for new SoundWire
>>> device type.
>>>
>>> Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
>>> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
>>> ---
>>
>>> +++ b/drivers/soundwire/Kconfig
>>> @@ -0,0 +1,22 @@
>>> +#
>>> +# SoundWire subsystem configuration
>>> +#
>>> +
>>> +menuconfig SOUNDWIRE
>>> +	bool "SoundWire support"
>>
>> Any reason why this subsystem can not be build as module?
> 
> This is not subsystem symbol but the menu. The SOUNDWIRE_BUS can be module.
I Noticed that.

Are you able to be build SOUNDWIRE_BUS as moudles?

I think the issue is that SOUNDWIRE_BUS default is set to SOUNDWIRE
This would never allow SOUNDWIRE_BUS to set as module if SOUNDWIRE is bool.

May be that is the issue.

config SOUNDWIRE_BUS
         tristate
         default SOUNDWIRE
         select REGMAP_SOUNDWIRE

> 
>>> +/**
>>> + * sdw_get_device_id: find the matching SoundWire device id
>>> + *
>> function name should end with () - according to kernel doc.
> 
> ah thanks for pointing will add
> 
>>
>>> + * @slave: SoundWire Slave device
>>> + * @drv: SoundWire Slave Driver
>>> + *
>>> + * The match is done by comparing the mfg_id and part_id from the
>>> + * struct sdw_device_id. class_id is unused, as it is a placeholder
>>> + * in MIPI Spec.
>>> + */
>>
>> BTW, This is a static private function, why are we adding kernel doc for
>> this?
> 
> the match is an important routine and helps people understand the logic
> hence documentation. More doc is better right :)
> 
I agree, more doc is better.

>>> +struct bus_type sdw_bus_type = {
>>> +	.name = "soundwire",
>>> +	.match = sdw_bus_match,
>>> +	.uevent = sdw_uevent,
>>> +};
>>> +EXPORT_SYMBOL(sdw_bus_type);
>>> +
>>> +static int sdw_drv_probe(struct device *dev)
>>> +{
>>> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
>>> +	struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
>>> +	const struct sdw_device_id *id;
>>> +	int ret;
>>> +
>>> +	id = sdw_get_device_id(slave, drv);
>>
>> By this time we must have already matched dev and driver by the ID,
>> shouldn't it be just slave->id  here?
> 
> I don't think so we do not have slave->id, we pass the id in probe as an
> argument
> 
Which probe function are you referening too ?

Not sure I get it, Only way to get to this probe is that id_table from 
driver matches slave id which is done as part of sdw_bus_match().
So the id should be valid and calling sdw_get_device_id() is redundant here?

>>> +	if (!id)
>>> +		return -ENODEV;
>>> +
>>> +	/*
>>> +	 * attach to power domain but don't turn on (last arg)
>>> +	 */
>>> +	ret = dev_pm_domain_attach(dev, false);
>>> +	if (ret) {
>> Shouldn't it just handle the EPROBE_DEFER case and ignore it for other
>> errors.
> 
> why should we ignore other errors and continue?
> 

If you are making power domain as mandatory for all the devices, then it 
makes sense to err out. But not all the devices might have pm domains 
associated, so continuing on other errors makes sense.. All of the bus 
drivers in the kernel do that ex: ./drivers/base/platform.c,
./drivers/mmc/core/sdio_bus.c, ./drivers/spi/spi.c...


>>
>>
>>> +		dev_err(dev, "Failed to attach PM domain: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = drv->probe(slave, id);
>>> +	if (ret) {
>>> +		dev_err(dev, "Probe of %s failed: %d\n", drv->name, ret);
>>> +		return ret;
>>> +	}
>>
>>
>> What happens if the slave driver is built as module and loaded after the
>> slave device is attached to the bus. How does the slave driver get updated
>> status in this case?
>>
>> We have similar usecase in slimbus too.
> 
> So we create devices based on firmware description, then the Slave may
> report as present and we mark it as present. Once a driver is loaded, the
> driver is probed here, the slave->status clearly tells the driver that slave
> has already reported present.

Yep, that solution makes sense, Looks like I can do the same for slimbus 
too.


>