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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 19 Oct 2017 09:40:06 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Vinod Koul <vinod.koul@...el.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        ALSA <alsa-devel@...a-project.org>, Mark <broonie@...nel.org>,
        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>,
        srinivas.kandagatla@...aro.org, plai@...eaurora.org,
        Sudheer Papothi <spapothi@...eaurora.org>
Subject: Re: [PATCH 02/14] soundwire: Add SoundWire bus type

On Thu, 19 Oct 2017 05:03:18 +0200,
Vinod Koul wrote:
> 
> --- /dev/null
> +++ b/drivers/soundwire/Kconfig
> @@ -0,0 +1,22 @@
> +#
> +# SoundWire subsystem configuration
> +#
> +
> +menuconfig SOUNDWIRE
> +	bool "SoundWire support"
> +	---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
> +	  optimized to integrate audio devices in mobile or mobile inspired
> +	  systems
> +
> +if SOUNDWIRE
> +
> +comment "SoundWire Devices"
> +
> +config SOUNDWIRE_BUS
> +	tristate
> +	default SOUNDWIRE
> +

Does it make sense to be tristate?
Since CONFIG_SOUNDWIRE is a bool, the above would be also only either
Y or N.  If it's Y and others select M, it'll be still Y.


> --- /dev/null
> +++ b/drivers/soundwire/bus_type.c
> +/**
> + * sdw_get_device_id: find the matching SoundWire device id
> + *
> + * @slave: SoundWire Slave device
> + * @drv: SoundWire Slave Driver

Inconsistent upper/lower letters in these two lines.

> + * 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.
> + */
> +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) {

Please indentation properly.

> +			return id;
> +		}

Superfluous braces for a single-line.

> +		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)

I'd put const to slave argument, as it won't be modified.

> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -228,6 +228,13 @@ struct hda_device_id {
>  	unsigned long driver_data;
>  };
>  
> +struct sdw_device_id {
> +	__u16 mfg_id;
> +	__u16 part_id;
> +	__u8 class_id;
> +	kernel_ulong_t driver_data;

Better to think of alignment.


> --- /dev/null
> +++ b/include/linux/soundwire/sdw.h
....
> +/**
> + * struct sdw_bus: SoundWire bus
> + *
> + * @dev: Master linux device
> + * @link_id: Link id number, can be 0 to N, unique for each Master
> + * @slaves: list of Slaves on this bus
> + * @assigned: logical addresses assigned, Index 0 (broadcast) would be unused
> + * @bus_lock: bus lock
> + */
> +struct sdw_bus {
> +	struct device *dev;
> +	unsigned int link_id;
> +	struct list_head slaves;
> +	bool assigned[SDW_MAX_DEVICES + 1];

Why not a bitmap?


thanks,

Takashi