| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5fee4ccb-1030-b698-e7ef-7f4390563a76@linaro.org>
Date: Thu, 9 Nov 2017 21:14:07 +0000
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Vinod Koul <vinod.koul@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
ALSA <alsa-devel@...a-project.org>, Mark <broonie@...nel.org>,
Takashi <tiwai@...e.de>,
Pierre <pierre-louis.bossart@...ux.intel.com>,
Sanyog Kale <sanyog.r.kale@...el.com>,
Shreyas NC <shreyas.nc@...el.com>, patches.audio@...el.com,
alan@...ux.intel.com,
Charles Keepax <ckeepax@...nsource.cirrus.com>,
Sagar Dharia <sdharia@...eaurora.org>, plai@...eaurora.org,
Sudheer Papothi <spapothi@...eaurora.org>
Subject: Re: [PATCH 02/14] soundwire: Add SoundWire bus type
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@...el.com>
> Signed-off-by: Vinod Koul <vinod.koul@...el.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?
> + ---help---
> + SoundWire is a 2-Pin interface with data and clock line ratified
> + by the MIPI Alliance. SoundWire is used for transporting data
> + typically related to audio functions. SoundWire interface is
> +#ifndef __SDW_BUS_H
> +#define __SDW_BUS_H
> +
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/acpi.h>
Do you need these headers here?
> +#include <linux/soundwire/sdw.h>
> +
> +int sdw_slave_modalias(struct sdw_slave *slave, char *buf, size_t size);
> +
> +#endif /* __SDW_BUS_H */
> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> new file mode 100644
> index 000000000000..a14d1de80afa
> --- /dev/null
> +++ b/drivers/soundwire/bus_type.c
>
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/soundwire/sdw.h>
> +#include "bus.h"
> +
> +/**
> + * sdw_get_device_id: find the matching SoundWire device id
> + *
function name should end with () - according to kernel doc.
> + * @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?
> +static const struct sdw_device_id *
> +sdw_get_device_id(struct sdw_slave *slave, struct sdw_driver *drv)
> +{
> + const struct sdw_device_id *id = drv->id_table;
> +
> + while (id && id->mfg_id) {
> + if (slave->id.mfg_id == id->mfg_id &&
> + slave->id.part_id == id->part_id) {
> + return id;
> + }
> + id++;
> + }
> +
> + return NULL;
> +}
> +
> +static int sdw_bus_match(struct device *dev, struct device_driver *ddrv)
> +{
> + struct sdw_slave *slave = dev_to_sdw_dev(dev);
> + struct sdw_driver *drv = drv_to_sdw_driver(ddrv);
> +
> + return !!sdw_get_device_id(slave, drv);
> +}
> +
> +int sdw_slave_modalias(struct sdw_slave *slave, char *buf, size_t size)
> +{
> + /* modalias is sdw:m<mfg_id>p<part_id> */
> +
> + return snprintf(buf, size, "sdw:m%04Xp%04X\n",
> + slave->id.mfg_id, slave->id.part_id);
> +}
> +
> +static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> + struct sdw_slave *slave = dev_to_sdw_dev(dev);
> + char modalias[32];
> +
> + sdw_slave_modalias(slave, modalias, sizeof(modalias));
> +
> + if (add_uevent_var(env, "MODALIAS=%s", modalias))
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +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?
> + 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.
> + 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.
> +
> + return 0;
> +}
> +
>
Powered by blists - more mailing lists