| Message ID | 1508382211-3154-3-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Thu, 19 Oct 2017 09:40:06 +0200, Takashi Iwai wrote: > > 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. I found a later patch selecting SOUNDWIRE_BUS. So just drop this "default" line, and always let others selecting the bus. Takashi
On Thu, Oct 19, 2017 at 09:40:06AM +0200, Takashi Iwai wrote: > On Thu, 19 Oct 2017 05:03:18 +0200, > Vinod Koul wrote: > > + > > +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. hmmm good point. I think would make sense to make SOUNDWIRE as tristate too, just like SOUND :) > > + * 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. thanks for spotting, will fix > > + * 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. what do you advise? if (slave->id.mfg_id == id->mfg_id && slave->id.part_id == id->part_id) { would mean below one is at same indent. Some people use: if (slave->id.mfg_id == id->mfg_id && slave->id.part_id == id->part_id) { Is it Documented anywhere... > > > + return id; > > + } > > Superfluous braces for a single-line. That bit was intentional. Yes it is not required but given that if condition was falling to two lines, I wanted to help readability by adding these. I can remove them.. > > > + 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. right... > > > --- 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. sorry not quite clear, do you mind elaborating which ones to align? > > > > --- /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? That a very good suggestion, it will help in other things too :)
On Fri, 20 Oct 2017 07:11:01 +0200, Vinod Koul wrote: > > On Thu, Oct 19, 2017 at 09:40:06AM +0200, Takashi Iwai wrote: > > On Thu, 19 Oct 2017 05:03:18 +0200, > > Vinod Koul wrote: > > > + > > > +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. > > hmmm good point. I think would make sense to make SOUNDWIRE as tristate too, > just like SOUND :) It's one option. Another would be to simply drop the "default" line. > > > + * 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. > > what do you advise? > > if (slave->id.mfg_id == id->mfg_id && > slave->id.part_id == id->part_id) { > > would mean below one is at same indent. Some people use: > > if (slave->id.mfg_id == id->mfg_id && > slave->id.part_id == id->part_id) { > > Is it Documented anywhere... This is a matter of taste. The latter is the way Emacs or indent does as default. > > > --- 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. > > sorry not quite clear, do you mind elaborating which ones to align? kernel_ulong_t may be aligned to 4 or 8 bytes, depending on architecture, so there can be a hole between class_id and driver_data. It's not an ABI, so we don't have to care too much, but it's still something exposed, hence better to be conscious about alignment. Takashi
On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h > new file mode 100644 > index 000000000000..2683c6798b95 > --- /dev/null > +++ b/drivers/soundwire/bus.h > @@ -0,0 +1,62 @@ > +/* > + * This file is provided under a dual BSD/GPLv2 license. When using or > + * redistributing this file, you may do so under either license. > + * > + * GPL LICENSE SUMMARY > + * > + * Copyright(c) 2015-17 Intel Corporation. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of version 2 of the GNU General Public License as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + * > + * BSD LICENSE > + * > + * Copyright(c) 2015-17 Intel Corporation. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * > + * * Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * * Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in > + * the documentation and/or other materials provided with the > + * distribution. > + * * Neither the name of Intel Corporation nor the names of its > + * contributors may be used to endorse or promote products derived > + * from this software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + * > + */ > + > +#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> > +#include <linux/soundwire/sdw.h> > + > +int sdw_slave_modalias(struct sdw_slave *slave, char *buf, size_t size); > + > +#endif /* __SDW_BUS_H */ When your header boiler-plate text is bigger than the content, please reconsider it. Are you sure you need this .h file? Why?
On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > 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 > @@ -0,0 +1,229 @@ > +/* > + * This file is provided under a dual BSD/GPLv2 license. When using or > + * redistributing this file, you may do so under either license. > + * > + * GPL LICENSE SUMMARY > + * > + * Copyright(c) 2015-17 Intel Corporation. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of version 2 of the GNU General Public License as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + * > + * BSD LICENSE > + * > + * Copyright(c) 2015-17 Intel Corporation. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * > + * * Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * * Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in > + * the documentation and/or other materials provided with the > + * distribution. > + * * Neither the name of Intel Corporation nor the names of its > + * contributors may be used to endorse or promote products derived > + * from this software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. Are you _sure_ that code that interacts with the driver core can have a dual-license here? Have you explained to lawyers what you are doing here (wrapping gpl-only symbols with non-gpl-only exports)? And why dual license something that will only ever work on Linux? And finally, put a real SPDX header up there so that people don't have to parse that horrid amount of text to try to determine exactly what that license is. > + * > + */ > + > +#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 > + * > + * @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. > + */ > +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); EXPORT_SYMBOL_GPL()? No release callback? Who frees the device? > + > +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); > + if (!id) > + return -ENODEV; > + > + /* > + * attach to power domain but don't turn on (last arg) > + */ > + ret = dev_pm_domain_attach(dev, false); > + if (ret) { > + 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; > + } > + > + return 0; > +} > + > +static int sdw_drv_remove(struct device *dev) > +{ > + struct sdw_slave *slave = dev_to_sdw_dev(dev); > + struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); > + > + if (drv->remove) > + drv->remove(slave); No catching the error value? > + > + dev_pm_domain_detach(dev, false); > + > + return 0; > +} > + > +static void sdw_drv_shutdown(struct device *dev) > +{ > + struct sdw_slave *slave = dev_to_sdw_dev(dev); > + struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); > + > + if (drv->shutdown) > + drv->shutdown(slave); > +} > + > +/** > + * __sdw_register_driver - register a SoundWire Slave driver > + * > + * @drv: driver to register > + * @owner: owning module/driver > + * > + * Return: zero on success, else a negative error code. > + */ > +int __sdw_register_driver(struct sdw_driver *drv, struct module *owner) > +{ > + drv->driver.bus = &sdw_bus_type; > + > + if (!drv->probe) { > + pr_err("driver %s didn't provide SDW probe routine\n", > + drv->name); > + return -EINVAL; > + } > + > + drv->driver.owner = owner; > + drv->driver.probe = sdw_drv_probe; > + > + if (drv->remove) > + drv->driver.remove = sdw_drv_remove; > + > + if (drv->shutdown) > + drv->driver.shutdown = sdw_drv_shutdown; > + > + return driver_register(&drv->driver); > +} > +EXPORT_SYMBOL(__sdw_register_driver); > + > +/* > + * sdw_unregister_driver: unregisters the SoundWire Slave driver > + * > + * @drv: driver to unregister > + */ > +void sdw_unregister_driver(struct sdw_driver *drv) > +{ > + driver_unregister(&drv->driver); > +} > +EXPORT_SYMBOL(sdw_unregister_driver); EXPORT_SYMBOL_GPL()? Same for all the exports here (I have to ask, especially given what you are wrapping...) thanks, greg k-h
On Fri, Oct 20, 2017 at 08:59:42AM +0200, Takashi Iwai wrote: > On Fri, 20 Oct 2017 07:11:01 +0200, > Vinod Koul wrote: > > > > On Thu, Oct 19, 2017 at 09:40:06AM +0200, Takashi Iwai wrote: > > > On Thu, 19 Oct 2017 05:03:18 +0200, > > > Vinod Koul wrote: > > > > + > > > > +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. > > > > hmmm good point. I think would make sense to make SOUNDWIRE as tristate too, > > just like SOUND :) > > It's one option. Another would be to simply drop the "default" line. Okay good suggestion > > > > + * 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. > > > > what do you advise? > > > > if (slave->id.mfg_id == id->mfg_id && > > slave->id.part_id == id->part_id) { > > > > would mean below one is at same indent. Some people use: > > > > if (slave->id.mfg_id == id->mfg_id && > > slave->id.part_id == id->part_id) { > > > > Is it Documented anywhere... > > This is a matter of taste. The latter is the way Emacs or indent does > as default. okay as vi user I will try to do above :) > > > > > --- 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. > > > > sorry not quite clear, do you mind elaborating which ones to align? > > kernel_ulong_t may be aligned to 4 or 8 bytes, depending on > architecture, so there can be a hole between class_id and driver_data. > It's not an ABI, so we don't have to care too much, but it's still > something exposed, hence better to be conscious about alignment. ah :) is that why hda is unsigned long :) Btw doesnt that cause compat issues, should we not do something like u64 here?
On Fri, 20 Oct 2017 17:46:49 +0200, Vinod Koul wrote: > > On Fri, Oct 20, 2017 at 08:59:42AM +0200, Takashi Iwai wrote: > > On Fri, 20 Oct 2017 07:11:01 +0200, > > Vinod Koul wrote: > > > > > > On Thu, Oct 19, 2017 at 09:40:06AM +0200, Takashi Iwai wrote: > > > > On Thu, 19 Oct 2017 05:03:18 +0200, > > > > Vinod Koul wrote: > > > > > + > > > > > +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. > > > > > > hmmm good point. I think would make sense to make SOUNDWIRE as tristate too, > > > just like SOUND :) > > > > It's one option. Another would be to simply drop the "default" line. > > Okay good suggestion > > > > > > + * 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. > > > > > > what do you advise? > > > > > > if (slave->id.mfg_id == id->mfg_id && > > > slave->id.part_id == id->part_id) { > > > > > > would mean below one is at same indent. Some people use: > > > > > > if (slave->id.mfg_id == id->mfg_id && > > > slave->id.part_id == id->part_id) { > > > > > > Is it Documented anywhere... > > > > This is a matter of taste. The latter is the way Emacs or indent does > > as default. > > okay as vi user I will try to do above :) > > > > > > > > --- 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. > > > > > > sorry not quite clear, do you mind elaborating which ones to align? > > > > kernel_ulong_t may be aligned to 4 or 8 bytes, depending on > > architecture, so there can be a hole between class_id and driver_data. > > It's not an ABI, so we don't have to care too much, but it's still > > something exposed, hence better to be conscious about alignment. > > ah :) is that why hda is unsigned long :) Btw doesnt that cause compat > issues, should we not do something like u64 here? Oh, don't take the HD-audio case as a good reference, it's a bad guy ;) In the case of hda, the definition isn't really exposed. The alignment doesn't matter whether it's unsigned long or kernel_ulong_t. It's a generic issue when you define some struct and expose it. In a safer side, you can put the enough pad bytes so that the long field is aligned in 8 bytes. Or use packed struct. Or you can just ignore and let it be so, but aware of the possible holes in your code. Takashi
On Fri, Oct 20, 2017 at 12:41:20PM +0200, Greg Kroah-Hartman wrote: > On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > > diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h > > new file mode 100644 > > index 000000000000..2683c6798b95 > > --- /dev/null > > +++ b/drivers/soundwire/bus.h > > @@ -0,0 +1,62 @@ > > +/* > > + * This file is provided under a dual BSD/GPLv2 license. When using or > > + * redistributing this file, you may do so under either license. > > + * > > + * GPL LICENSE SUMMARY > > + * > > + * Copyright(c) 2015-17 Intel Corporation. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of version 2 of the GNU General Public License as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, but > > + * WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * General Public License for more details. > > + * > > + * BSD LICENSE > > + * > > + * Copyright(c) 2015-17 Intel Corporation. > > + * > > + * Redistribution and use in source and binary forms, with or without > > + * modification, are permitted provided that the following conditions > > + * are met: > > + * > > + * * Redistributions of source code must retain the above copyright > > + * notice, this list of conditions and the following disclaimer. > > + * * Redistributions in binary form must reproduce the above copyright > > + * notice, this list of conditions and the following disclaimer in > > + * the documentation and/or other materials provided with the > > + * distribution. > > + * * Neither the name of Intel Corporation nor the names of its > > + * contributors may be used to endorse or promote products derived > > + * from this software without specific prior written permission. > > + * > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > + * > > + */ > > + > > +#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> > > +#include <linux/soundwire/sdw.h> > > + > > +int sdw_slave_modalias(struct sdw_slave *slave, char *buf, size_t size); > > + > > +#endif /* __SDW_BUS_H */ > > When your header boiler-plate text is bigger than the content, please > reconsider it. Are you sure you need this .h file? Why? At the start of the series it does indeed look so, but not at the end of the series. Pls see [1] for this file at the end of this series :) The header is required so that we can share sdw_slave_modalias() code and not do it a two places in c files. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/soundwire.git/tree/drivers/soundwire/bus.h?h=topic/patch_v1
On Fri, Oct 20, 2017 at 12:45:28PM +0200, Greg Kroah-Hartman wrote: > On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > > +/* > > + * This file is provided under a dual BSD/GPLv2 license. When using or > > + * redistributing this file, you may do so under either license. > > + * > > + * GPL LICENSE SUMMARY > > + * > > + * Copyright(c) 2015-17 Intel Corporation. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of version 2 of the GNU General Public License as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, but > > + * WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * General Public License for more details. > > + * > > + * BSD LICENSE > > + * > > + * Copyright(c) 2015-17 Intel Corporation. > > + * > > + * Redistribution and use in source and binary forms, with or without > > + * modification, are permitted provided that the following conditions > > + * are met: > > + * > > + * * Redistributions of source code must retain the above copyright > > + * notice, this list of conditions and the following disclaimer. > > + * * Redistributions in binary form must reproduce the above copyright > > + * notice, this list of conditions and the following disclaimer in > > + * the documentation and/or other materials provided with the > > + * distribution. > > + * * Neither the name of Intel Corporation nor the names of its > > + * contributors may be used to endorse or promote products derived > > + * from this software without specific prior written permission. > > + * > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > Are you _sure_ that code that interacts with the driver core can have a > dual-license here? Have you explained to lawyers what you are doing > here (wrapping gpl-only symbols with non-gpl-only exports)? Sorry, the intention is not to wrap gpl symbols for non-gpl-only exports. > And why dual license something that will only ever work on Linux? We have non Linux users (mostly RTOS folks) which we would like to support with as much as common code. > And finally, put a real SPDX header up there so that people don't have > to parse that horrid amount of text to try to determine exactly what > that license is. Sorry for confusion, For the record we are trying to do Dual GPL v2/ BSD 3 clause here. Can you give me example of SPDX use. I will be gald to use that > > +struct bus_type sdw_bus_type = { > > + .name = "soundwire", > > + .match = sdw_bus_match, > > + .uevent = sdw_uevent, > > +}; > > +EXPORT_SYMBOL(sdw_bus_type); > > EXPORT_SYMBOL_GPL()? This can be EXPORT_SYMBOL_GPL as non Linux users wont have this. But then would it be to okay to have a module with some symbols _GPL and some non _GPL (the SoundWire protocol ones would need to be non GPL) > > No release callback? Who frees the device? Sorry to have missed that, will add that > > +static int sdw_drv_remove(struct device *dev) > > +{ > > + struct sdw_slave *slave = dev_to_sdw_dev(dev); > > + struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); > > + > > + if (drv->remove) > > + drv->remove(slave); > > No catching the error value? will add > > +/* > > + * sdw_unregister_driver: unregisters the SoundWire Slave driver > > + * > > + * @drv: driver to unregister > > + */ > > +void sdw_unregister_driver(struct sdw_driver *drv) > > +{ > > + driver_unregister(&drv->driver); > > +} > > +EXPORT_SYMBOL(sdw_unregister_driver); > > EXPORT_SYMBOL_GPL()? Same for all the exports here (I have to ask, > especially given what you are wrapping...)
On Fri, Oct 20, 2017 at 12:45 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: >> 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 >> @@ -0,0 +1,229 @@ >> +/* >> + * This file is provided under a dual BSD/GPLv2 license. When using or >> + * redistributing this file, you may do so under either license. >> + * >> + * GPL LICENSE SUMMARY >> + * >> + * Copyright(c) 2015-17 Intel Corporation. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of version 2 of the GNU General Public License as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, but >> + * WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * General Public License for more details. >> + * >> + * BSD LICENSE >> + * >> + * Copyright(c) 2015-17 Intel Corporation. >> + * >> + * Redistribution and use in source and binary forms, with or without >> + * modification, are permitted provided that the following conditions >> + * are met: >> + * >> + * * Redistributions of source code must retain the above copyright >> + * notice, this list of conditions and the following disclaimer. >> + * * Redistributions in binary form must reproduce the above copyright >> + * notice, this list of conditions and the following disclaimer in >> + * the documentation and/or other materials provided with the >> + * distribution. >> + * * Neither the name of Intel Corporation nor the names of its >> + * contributors may be used to endorse or promote products derived >> + * from this software without specific prior written permission. >> + * >> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS >> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT >> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR >> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT >> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, >> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT >> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, >> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY >> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT >> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE >> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > Are you _sure_ that code that interacts with the driver core can have a > dual-license here? Have you explained to lawyers what you are doing > here (wrapping gpl-only symbols with non-gpl-only exports)? > > And why dual license something that will only ever work on Linux? > > And finally, put a real SPDX header up there so that people don't have > to parse that horrid amount of text to try to determine exactly what > that license is. Vinod: It is hard to parse for people ... but it is quite hard for tools to catch this too. This license notice is so peculiar and special that I had to make a special detection rule just for it [1] in my tool :| Please have mercy: could you not pick something simpler? [1] https://github.com/nexB/scancode-toolkit/blob/develop/src/licensedcode/data/rules/gpl-2.0_or_bsd-new33.RULE
On Fri, Oct 20, 2017 at 05:50:57PM +0200, Takashi Iwai wrote: > On Fri, 20 Oct 2017 17:46:49 +0200, > Vinod Koul wrote: > > > > > > --- 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. > > > > > > > > sorry not quite clear, do you mind elaborating which ones to align? > > > > > > kernel_ulong_t may be aligned to 4 or 8 bytes, depending on > > > architecture, so there can be a hole between class_id and driver_data. > > > It's not an ABI, so we don't have to care too much, but it's still > > > something exposed, hence better to be conscious about alignment. > > > > ah :) is that why hda is unsigned long :) Btw doesnt that cause compat > > issues, should we not do something like u64 here? > > Oh, don't take the HD-audio case as a good reference, it's a bad guy > ;) In the case of hda, the definition isn't really exposed. Not really it is for ext-hda codecs > The alignment doesn't matter whether it's unsigned long or > kernel_ulong_t. It's a generic issue when you define some struct and > expose it. In a safer side, you can put the enough pad bytes so that > the long field is aligned in 8 bytes. Or use packed struct. Or you > can just ignore and let it be so, but aware of the possible holes in > your code. that makes sense, I can add some reserved fields for padding here to fix and retain the kernel_ulong_t then
On Fri, Oct 20, 2017 at 06:03:24PM +0200, Philippe Ombredanne wrote: > On Fri, Oct 20, 2017 at 12:45 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > >> 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 > >> @@ -0,0 +1,229 @@ > >> +/* > >> + * This file is provided under a dual BSD/GPLv2 license. When using or > >> + * redistributing this file, you may do so under either license. > >> + * > >> + * GPL LICENSE SUMMARY > >> + * > >> + * Copyright(c) 2015-17 Intel Corporation. > >> + * > >> + * This program is free software; you can redistribute it and/or modify > >> + * it under the terms of version 2 of the GNU General Public License as > >> + * published by the Free Software Foundation. > >> + * > >> + * This program is distributed in the hope that it will be useful, but > >> + * WITHOUT ANY WARRANTY; without even the implied warranty of > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > >> + * General Public License for more details. > >> + * > >> + * BSD LICENSE > >> + * > >> + * Copyright(c) 2015-17 Intel Corporation. > >> + * > >> + * Redistribution and use in source and binary forms, with or without > >> + * modification, are permitted provided that the following conditions > >> + * are met: > >> + * > >> + * * Redistributions of source code must retain the above copyright > >> + * notice, this list of conditions and the following disclaimer. > >> + * * Redistributions in binary form must reproduce the above copyright > >> + * notice, this list of conditions and the following disclaimer in > >> + * the documentation and/or other materials provided with the > >> + * distribution. > >> + * * Neither the name of Intel Corporation nor the names of its > >> + * contributors may be used to endorse or promote products derived > >> + * from this software without specific prior written permission. > >> + * > >> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > >> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > >> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > >> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > >> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > >> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > >> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > >> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > >> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > >> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > >> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > > > Are you _sure_ that code that interacts with the driver core can have a > > dual-license here? Have you explained to lawyers what you are doing > > here (wrapping gpl-only symbols with non-gpl-only exports)? > > > > And why dual license something that will only ever work on Linux? > > > > And finally, put a real SPDX header up there so that people don't have > > to parse that horrid amount of text to try to determine exactly what > > that license is. > > Vinod: > > It is hard to parse for people ... but it is quite hard for tools to catch > this too. This license notice is so peculiar and special that I had to > make a special detection rule just for it [1] in my tool :| > Please have mercy: could you not pick something simpler? Sorry for the trouble it caused. The code is Dual license as indicated by first two lines. I didn't invent the text here, legal folks did and used the standard template available in my company I quick grep on Dual license users looks like we already have this in kernel code. See drivers/ntb/hw/intel/ntb_hw_intel.c > [1] https://github.com/nexB/scancode-toolkit/blob/develop/src/licensedcode/data/rules/gpl-2.0_or_bsd-new33.RULE
On Fri, Oct 20, 2017 at 09:31:34PM +0530, Vinod Koul wrote: > On Fri, Oct 20, 2017 at 12:45:28PM +0200, Greg Kroah-Hartman wrote: > > On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > > > +/* > > > + * This file is provided under a dual BSD/GPLv2 license. When using or > > > + * redistributing this file, you may do so under either license. > > > + * > > > + * GPL LICENSE SUMMARY > > > + * > > > + * Copyright(c) 2015-17 Intel Corporation. > > > + * > > > + * This program is free software; you can redistribute it and/or modify > > > + * it under the terms of version 2 of the GNU General Public License as > > > + * published by the Free Software Foundation. > > > + * > > > + * This program is distributed in the hope that it will be useful, but > > > + * WITHOUT ANY WARRANTY; without even the implied warranty of > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > > + * General Public License for more details. > > > + * > > > + * BSD LICENSE > > > + * > > > + * Copyright(c) 2015-17 Intel Corporation. > > > + * > > > + * Redistribution and use in source and binary forms, with or without > > > + * modification, are permitted provided that the following conditions > > > + * are met: > > > + * > > > + * * Redistributions of source code must retain the above copyright > > > + * notice, this list of conditions and the following disclaimer. > > > + * * Redistributions in binary form must reproduce the above copyright > > > + * notice, this list of conditions and the following disclaimer in > > > + * the documentation and/or other materials provided with the > > > + * distribution. > > > + * * Neither the name of Intel Corporation nor the names of its > > > + * contributors may be used to endorse or promote products derived > > > + * from this software without specific prior written permission. > > > + * > > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > > > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > > > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > > > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > > > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > > > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > > > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > > > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > > > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > > > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > > > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > > > Are you _sure_ that code that interacts with the driver core can have a > > dual-license here? Have you explained to lawyers what you are doing > > here (wrapping gpl-only symbols with non-gpl-only exports)? > > Sorry, the intention is not to wrap gpl symbols for non-gpl-only exports. > > > And why dual license something that will only ever work on Linux? > > We have non Linux users (mostly RTOS folks) which we would like to support > with as much as common code. Note, you need to be VERY CAREFUL about doing this. You need to have all sorts of infrastructure set up and in place and paperwork up the wazoo in order to make it work properly. In the end, I can almost guarantee it will not be worth the extra hassle and effort you are trying to do here. Seriously, go talk to your managers and corporate lawyer about this, you are in for a world of hurt if you want to do this in a way that actually works (i.e. doesn't just degrade to GPLv2 only instantly.) I recommend not doing this unless you have money to burn. If you do, then great! If not, it is much easier just to have two separate code repos. > > And finally, put a real SPDX header up there so that people don't have > > to parse that horrid amount of text to try to determine exactly what > > that license is. > > Sorry for confusion, For the record we are trying to do Dual GPL v2/ BSD 3 > clause here. Can you give me example of SPDX use. I will be gald to use that I could give you an example, but you need to get the real marking from your company as I am not the one to pick it for you :) > > > +struct bus_type sdw_bus_type = { > > > + .name = "soundwire", > > > + .match = sdw_bus_match, > > > + .uevent = sdw_uevent, > > > +}; > > > +EXPORT_SYMBOL(sdw_bus_type); > > > > EXPORT_SYMBOL_GPL()? > > This can be EXPORT_SYMBOL_GPL as non Linux users wont have this. But then > would it be to okay to have a module with some symbols _GPL and some non > _GPL (the SoundWire protocol ones would need to be non GPL) Again, don't even try to do that, it's not going to work. The only team I know that ever has done this successfully is the core ACPI code. Go talk to them about the work involved in doing this properly to see if you are willing to do that. good luck! greg k-h
On Fri, Oct 20, 2017 at 09:50:42PM +0530, Vinod Koul wrote: > On Fri, Oct 20, 2017 at 06:03:24PM +0200, Philippe Ombredanne wrote: > > On Fri, Oct 20, 2017 at 12:45 PM, Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > > >> 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 > > >> @@ -0,0 +1,229 @@ > > >> +/* > > >> + * This file is provided under a dual BSD/GPLv2 license. When using or > > >> + * redistributing this file, you may do so under either license. > > >> + * > > >> + * GPL LICENSE SUMMARY > > >> + * > > >> + * Copyright(c) 2015-17 Intel Corporation. > > >> + * > > >> + * This program is free software; you can redistribute it and/or modify > > >> + * it under the terms of version 2 of the GNU General Public License as > > >> + * published by the Free Software Foundation. > > >> + * > > >> + * This program is distributed in the hope that it will be useful, but > > >> + * WITHOUT ANY WARRANTY; without even the implied warranty of > > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > >> + * General Public License for more details. > > >> + * > > >> + * BSD LICENSE > > >> + * > > >> + * Copyright(c) 2015-17 Intel Corporation. > > >> + * > > >> + * Redistribution and use in source and binary forms, with or without > > >> + * modification, are permitted provided that the following conditions > > >> + * are met: > > >> + * > > >> + * * Redistributions of source code must retain the above copyright > > >> + * notice, this list of conditions and the following disclaimer. > > >> + * * Redistributions in binary form must reproduce the above copyright > > >> + * notice, this list of conditions and the following disclaimer in > > >> + * the documentation and/or other materials provided with the > > >> + * distribution. > > >> + * * Neither the name of Intel Corporation nor the names of its > > >> + * contributors may be used to endorse or promote products derived > > >> + * from this software without specific prior written permission. > > >> + * > > >> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > > >> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > > >> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > > >> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > > >> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > > >> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > > >> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > > >> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > > >> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > > >> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > > >> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > > > > > Are you _sure_ that code that interacts with the driver core can have a > > > dual-license here? Have you explained to lawyers what you are doing > > > here (wrapping gpl-only symbols with non-gpl-only exports)? > > > > > > And why dual license something that will only ever work on Linux? > > > > > > And finally, put a real SPDX header up there so that people don't have > > > to parse that horrid amount of text to try to determine exactly what > > > that license is. > > > > Vinod: > > > > It is hard to parse for people ... but it is quite hard for tools to catch > > this too. This license notice is so peculiar and special that I had to > > make a special detection rule just for it [1] in my tool :| > > Please have mercy: could you not pick something simpler? > > Sorry for the trouble it caused. The code is Dual license as indicated by > first two lines. I didn't invent the text here, legal folks did and used > the standard template available in my company > > I quick grep on Dual license users looks like we already have this in kernel > code. See drivers/ntb/hw/intel/ntb_hw_intel.c Just because someone did something wrong in the past, doesn't mean they should keep doing more wrong things in the future :)
On Fri, Oct 20, 2017 at 06:21:23PM +0200, Greg Kroah-Hartman wrote: > On Fri, Oct 20, 2017 at 09:31:34PM +0530, Vinod Koul wrote: > > On Fri, Oct 20, 2017 at 12:45:28PM +0200, Greg Kroah-Hartman wrote: > > > On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > > > > +/* > > > > + * This file is provided under a dual BSD/GPLv2 license. When using or > > > > + * redistributing this file, you may do so under either license. > > > > + * > > > > + * GPL LICENSE SUMMARY > > > > + * > > > > + * Copyright(c) 2015-17 Intel Corporation. > > > > + * > > > > + * This program is free software; you can redistribute it and/or modify > > > > + * it under the terms of version 2 of the GNU General Public License as > > > > + * published by the Free Software Foundation. > > > > + * > > > > + * This program is distributed in the hope that it will be useful, but > > > > + * WITHOUT ANY WARRANTY; without even the implied warranty of > > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > > > + * General Public License for more details. > > > > + * > > > > + * BSD LICENSE > > > > + * > > > > + * Copyright(c) 2015-17 Intel Corporation. > > > > + * > > > > + * Redistribution and use in source and binary forms, with or without > > > > + * modification, are permitted provided that the following conditions > > > > + * are met: > > > > + * > > > > + * * Redistributions of source code must retain the above copyright > > > > + * notice, this list of conditions and the following disclaimer. > > > > + * * Redistributions in binary form must reproduce the above copyright > > > > + * notice, this list of conditions and the following disclaimer in > > > > + * the documentation and/or other materials provided with the > > > > + * distribution. > > > > + * * Neither the name of Intel Corporation nor the names of its > > > > + * contributors may be used to endorse or promote products derived > > > > + * from this software without specific prior written permission. > > > > + * > > > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > > > > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > > > > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > > > > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > > > > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > > > > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > > > > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > > > > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > > > > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > > > > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > > > > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > > > > > Are you _sure_ that code that interacts with the driver core can have a > > > dual-license here? Have you explained to lawyers what you are doing > > > here (wrapping gpl-only symbols with non-gpl-only exports)? > > > > Sorry, the intention is not to wrap gpl symbols for non-gpl-only exports. > > > > > And why dual license something that will only ever work on Linux? > > > > We have non Linux users (mostly RTOS folks) which we would like to support > > with as much as common code. > > Note, you need to be VERY CAREFUL about doing this. You need to have > all sorts of infrastructure set up and in place and paperwork up the > wazoo in order to make it work properly. > > In the end, I can almost guarantee it will not be worth the extra hassle > and effort you are trying to do here. > > Seriously, go talk to your managers and corporate lawyer about this, you > are in for a world of hurt if you want to do this in a way that actually > works (i.e. doesn't just degrade to GPLv2 only instantly.) > > I recommend not doing this unless you have money to burn. If you do, > then great! If not, it is much easier just to have two separate code > repos. > > > > And finally, put a real SPDX header up there so that people don't have > > > to parse that horrid amount of text to try to determine exactly what > > > that license is. > > > > Sorry for confusion, For the record we are trying to do Dual GPL v2/ BSD 3 > > clause here. Can you give me example of SPDX use. I will be gald to use that > > I could give you an example, but you need to get the real marking from > your company as I am not the one to pick it for you :) > > > > > +struct bus_type sdw_bus_type = { > > > > + .name = "soundwire", > > > > + .match = sdw_bus_match, > > > > + .uevent = sdw_uevent, > > > > +}; > > > > +EXPORT_SYMBOL(sdw_bus_type); > > > > > > EXPORT_SYMBOL_GPL()? > > > > This can be EXPORT_SYMBOL_GPL as non Linux users wont have this. But then > > would it be to okay to have a module with some symbols _GPL and some non > > _GPL (the SoundWire protocol ones would need to be non GPL) > > Again, don't even try to do that, it's not going to work. > > The only team I know that ever has done this successfully is the core > ACPI code. Go talk to them about the work involved in doing this > properly to see if you are willing to do that. Thanks for the advice Greg, really appreciate it! I will work with the folks and try to come up with a better proposal
On Fri, Oct 20, 2017 at 06:27:23PM +0200, Greg Kroah-Hartman wrote: > On Fri, Oct 20, 2017 at 09:50:42PM +0530, Vinod Koul wrote: > > On Fri, Oct 20, 2017 at 06:03:24PM +0200, Philippe Ombredanne wrote: > > > On Fri, Oct 20, 2017 at 12:45 PM, Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > > > >> 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 > > > >> @@ -0,0 +1,229 @@ > > > >> +/* > > > >> + * This file is provided under a dual BSD/GPLv2 license. When using or > > > >> + * redistributing this file, you may do so under either license. > > > >> + * > > > >> + * GPL LICENSE SUMMARY > > > >> + * > > > >> + * Copyright(c) 2015-17 Intel Corporation. > > > >> + * > > > >> + * This program is free software; you can redistribute it and/or modify > > > >> + * it under the terms of version 2 of the GNU General Public License as > > > >> + * published by the Free Software Foundation. > > > >> + * > > > >> + * This program is distributed in the hope that it will be useful, but > > > >> + * WITHOUT ANY WARRANTY; without even the implied warranty of > > > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > > >> + * General Public License for more details. > > > >> + * > > > >> + * BSD LICENSE > > > >> + * > > > >> + * Copyright(c) 2015-17 Intel Corporation. > > > >> + * > > > >> + * Redistribution and use in source and binary forms, with or without > > > >> + * modification, are permitted provided that the following conditions > > > >> + * are met: > > > >> + * > > > >> + * * Redistributions of source code must retain the above copyright > > > >> + * notice, this list of conditions and the following disclaimer. > > > >> + * * Redistributions in binary form must reproduce the above copyright > > > >> + * notice, this list of conditions and the following disclaimer in > > > >> + * the documentation and/or other materials provided with the > > > >> + * distribution. > > > >> + * * Neither the name of Intel Corporation nor the names of its > > > >> + * contributors may be used to endorse or promote products derived > > > >> + * from this software without specific prior written permission. > > > >> + * > > > >> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > > > >> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > > > >> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > > > >> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > > > >> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > > > >> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > > > >> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > > > >> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > > > >> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > > > >> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > > > >> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > > > > > > > Are you _sure_ that code that interacts with the driver core can have a > > > > dual-license here? Have you explained to lawyers what you are doing > > > > here (wrapping gpl-only symbols with non-gpl-only exports)? > > > > > > > > And why dual license something that will only ever work on Linux? > > > > > > > > And finally, put a real SPDX header up there so that people don't have > > > > to parse that horrid amount of text to try to determine exactly what > > > > that license is. > > > > > > Vinod: > > > > > > It is hard to parse for people ... but it is quite hard for tools to catch > > > this too. This license notice is so peculiar and special that I had to > > > make a special detection rule just for it [1] in my tool :| > > > Please have mercy: could you not pick something simpler? > > > > Sorry for the trouble it caused. The code is Dual license as indicated by > > first two lines. I didn't invent the text here, legal folks did and used > > the standard template available in my company > > > > I quick grep on Dual license users looks like we already have this in kernel > > code. See drivers/ntb/hw/intel/ntb_hw_intel.c > > Just because someone did something wrong in the past, doesn't mean they > should keep doing more wrong things in the future :) Sure, I agree w/ you that. But this is still a standard header to use for me. I will check with folks to see if we can use as you suggested.
On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > + ret = dev_pm_domain_attach(dev, false); > + if (ret) { > + 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; > + } We don't detach the power domain if the probe fails.
On Sat, Oct 21, 2017 at 10:03:48AM +0100, Mark Brown wrote: > On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > > > + ret = dev_pm_domain_attach(dev, false); > > + if (ret) { > > + 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; > > + } > > We don't detach the power domain if the probe fails. we should, thanks for spotting
> > > And why dual license something that will only ever work on Linux? > > > > We have non Linux users (mostly RTOS folks) which we would like to > > support > > with as much as common code. > > Note, you need to be VERY CAREFUL about doing this.  You need to have > all sorts of infrastructure set up and in place and paperwork up the > wazoo in order to make it work properly. Intel knows that. We have processes in place. We've been doing it for years (ACPI, Graphics, ..) > In the end, I can almost guarantee it will not be worth the extra > hassle and effort you are trying to do here. We don't think that is the case, and I'd imagine the NetBSD, FreeBSD, OpenBSD, Dragonfly and other projects probably also prefer our approach. Seriously, go talk to your managers and corporate lawyer about this, you are in for a world of hurt if you want to do this in a way that actually works (i.e. doesn't just degrade to GPLv2 only instantly.) It's far from the only project we do this with for at least the core OS independent parts of the driver. Alan
> > I quick grep on Dual license users looks like we already have this > > in kernel > > code. See drivers/ntb/hw/intel/ntb_hw_intel.c > > Just because someone did something wrong in the past, doesn't mean > they > should keep doing more wrong things in the future :) What makes you think it's wrong ? It's the same approach the kernel has been using since back to Linux 1.2 and maybe earlier. I know it's not your intention but you are also going to look bad to the BSD and other communities if you try and break dual licensing as a standard Linux thing for some code. It goes back decades (Ted's random driver for example). Alan
On Fri, Oct 20, 2017 at 09:31:34PM +0530, Vinod Koul wrote: > On Fri, Oct 20, 2017 at 12:45:28PM +0200, Greg Kroah-Hartman wrote: > > On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > > > +struct bus_type sdw_bus_type = { > > > + .name = "soundwire", > > > + .match = sdw_bus_match, > > > + .uevent = sdw_uevent, > > > +}; > > > +EXPORT_SYMBOL(sdw_bus_type); > > > > No release callback? Who frees the device? Hmmm, bus_type doesn't seem to have a release callback. The sdw_device has release and that is added in the next patch. So I am not changing anything here. Let me know If I missed anything... Thanks
On Thu, Oct 26, 2017 at 02:03:42PM +0530, Vinod Koul wrote: > On Fri, Oct 20, 2017 at 09:31:34PM +0530, Vinod Koul wrote: > > On Fri, Oct 20, 2017 at 12:45:28PM +0200, Greg Kroah-Hartman wrote: > > > On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > > > > > +struct bus_type sdw_bus_type = { > > > > + .name = "soundwire", > > > > + .match = sdw_bus_match, > > > > + .uevent = sdw_uevent, > > > > +}; > > > > +EXPORT_SYMBOL(sdw_bus_type); > > > > > > No release callback? Who frees the device? > > Hmmm, bus_type doesn't seem to have a release callback. The sdw_device has > release and that is added in the next patch. So I am not changing anything > here. Let me know If I missed anything... Ok, as long as you really have a release callback for when the device goes away, and you have tested device removal...
On Fri, Oct 27, 2017 at 10:57:39AM +0200, Greg Kroah-Hartman wrote: > On Thu, Oct 26, 2017 at 02:03:42PM +0530, Vinod Koul wrote: > > On Fri, Oct 20, 2017 at 09:31:34PM +0530, Vinod Koul wrote: > > > On Fri, Oct 20, 2017 at 12:45:28PM +0200, Greg Kroah-Hartman wrote: > > > > On Thu, Oct 19, 2017 at 08:33:18AM +0530, Vinod Koul wrote: > > > > > > > +struct bus_type sdw_bus_type = { > > > > > + .name = "soundwire", > > > > > + .match = sdw_bus_match, > > > > > + .uevent = sdw_uevent, > > > > > +}; > > > > > +EXPORT_SYMBOL(sdw_bus_type); > > > > > > > > No release callback? Who frees the device? > > > > Hmmm, bus_type doesn't seem to have a release callback. The sdw_device has > > release and that is added in the next patch. So I am not changing anything > > here. Let me know If I missed anything... > > Ok, as long as you really have a release callback for when the device > goes away, and you have tested device removal... Yup both checks done, thanks for confirming...
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? > + ---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; > +} > + >
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. > > >+ ---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? Yes :) I will double check though > > >+#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. 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 :) > > >+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? I don't think so we do not have slave->id, we pass the id in probe as an argument > >+ 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? > > > >+ 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.
On Fri, Nov 10, 2017 at 10:29:51AM +0530, 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. > > > > > >+ ---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? > > Yes :) I will double check though > > > > > > >+#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. > > ah thanks for pointing will add I ran the kernel-doc script, seems to not complain about this. Looking at it closely scripts/kernel-doc parenthesis is not expected
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. >
On 10/11/17 08:55, Vinod Koul wrote: >>>> + >>>> +/** >>>> + * 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 > I ran the kernel-doc script, seems to not complain about this. > > Looking at it closely scripts/kernel-doc parenthesis is not expected https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/doc-guide/kernel-doc.rst?h=v4.14-rc8#n204 Examples in the doc does suggest that we need (). Am not 100% sure if : is also a valid with functions.
On Fri, Nov 10, 2017 at 10:42:06AM +0000, Srinivas Kandagatla wrote: > > > 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 removing this makes it build as module, sorry I assumed you have already seen Takashi's comment. I have fixed it in v2. Posting anytime now :) > > > >> > >>>+ * @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? we dont store in id so we have to lookup again. I see the point in doing so, let me check that > >>>+ 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... Ah thanks for pointing, let me check that > >>>+ 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. yup!
diff --git a/drivers/Kconfig b/drivers/Kconfig index 505c676fa9c7..61b6e245c052 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -152,6 +152,8 @@ source "drivers/remoteproc/Kconfig" source "drivers/rpmsg/Kconfig" +source "drivers/soundwire/Kconfig" + source "drivers/soc/Kconfig" source "drivers/devfreq/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index d90fdc413648..39c032f8abf3 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -154,6 +154,7 @@ obj-$(CONFIG_MAILBOX) += mailbox/ obj-$(CONFIG_HWSPINLOCK) += hwspinlock/ obj-$(CONFIG_REMOTEPROC) += remoteproc/ obj-$(CONFIG_RPMSG) += rpmsg/ +obj-$(CONFIG_SOUNDWIRE) += soundwire/ # Virtualization drivers obj-$(CONFIG_VIRT_DRIVERS) += virt/ diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig new file mode 100644 index 000000000000..35792728c0aa --- /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 + +endif diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile new file mode 100644 index 000000000000..d1281def7662 --- /dev/null +++ b/drivers/soundwire/Makefile @@ -0,0 +1,7 @@ +# +# Makefile for soundwire core +# + +#Bus Objs +soundwire-bus-objs := bus_type.o +obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h new file mode 100644 index 000000000000..2683c6798b95 --- /dev/null +++ b/drivers/soundwire/bus.h @@ -0,0 +1,62 @@ +/* + * This file is provided under a dual BSD/GPLv2 license. When using or + * redistributing this file, you may do so under either license. + * + * GPL LICENSE SUMMARY + * + * Copyright(c) 2015-17 Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * BSD LICENSE + * + * Copyright(c) 2015-17 Intel Corporation. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Intel Corporation nor the names of its + * contributors may be used to endorse or promote products derived + * from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#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> +#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 @@ -0,0 +1,229 @@ +/* + * This file is provided under a dual BSD/GPLv2 license. When using or + * redistributing this file, you may do so under either license. + * + * GPL LICENSE SUMMARY + * + * Copyright(c) 2015-17 Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * BSD LICENSE + * + * Copyright(c) 2015-17 Intel Corporation. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Intel Corporation nor the names of its + * contributors may be used to endorse or promote products derived + * from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#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 + * + * @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. + */ +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); + if (!id) + return -ENODEV; + + /* + * attach to power domain but don't turn on (last arg) + */ + ret = dev_pm_domain_attach(dev, false); + if (ret) { + 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; + } + + return 0; +} + +static int sdw_drv_remove(struct device *dev) +{ + struct sdw_slave *slave = dev_to_sdw_dev(dev); + struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); + + if (drv->remove) + drv->remove(slave); + + dev_pm_domain_detach(dev, false); + + return 0; +} + +static void sdw_drv_shutdown(struct device *dev) +{ + struct sdw_slave *slave = dev_to_sdw_dev(dev); + struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); + + if (drv->shutdown) + drv->shutdown(slave); +} + +/** + * __sdw_register_driver - register a SoundWire Slave driver + * + * @drv: driver to register + * @owner: owning module/driver + * + * Return: zero on success, else a negative error code. + */ +int __sdw_register_driver(struct sdw_driver *drv, struct module *owner) +{ + drv->driver.bus = &sdw_bus_type; + + if (!drv->probe) { + pr_err("driver %s didn't provide SDW probe routine\n", + drv->name); + return -EINVAL; + } + + drv->driver.owner = owner; + drv->driver.probe = sdw_drv_probe; + + if (drv->remove) + drv->driver.remove = sdw_drv_remove; + + if (drv->shutdown) + drv->driver.shutdown = sdw_drv_shutdown; + + return driver_register(&drv->driver); +} +EXPORT_SYMBOL(__sdw_register_driver); + +/* + * sdw_unregister_driver: unregisters the SoundWire Slave driver + * + * @drv: driver to unregister + */ +void sdw_unregister_driver(struct sdw_driver *drv) +{ + driver_unregister(&drv->driver); +} +EXPORT_SYMBOL(sdw_unregister_driver); + +static int __init sdw_bus_init(void) +{ + return bus_register(&sdw_bus_type); +} + +static void __exit sdw_bus_exit(void) +{ + bus_unregister(&sdw_bus_type); +} + +postcore_initcall(sdw_bus_init); +module_exit(sdw_bus_exit); + +MODULE_DESCRIPTION("SoundWire bus"); +MODULE_LICENSE("Dual BSD/GPL"); diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index 694cebb50f72..e2a9dcd52afc 100644 --- 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; +}; + /* * Struct used for matching a device */ diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h new file mode 100644 index 000000000000..2b089463104d --- /dev/null +++ b/include/linux/soundwire/sdw.h @@ -0,0 +1,170 @@ +/* + * This file is provided under a dual BSD/GPLv2 license. When using or + * redistributing this file, you may do so under either license. + * + * GPL LICENSE SUMMARY + * + * Copyright(c) 2015-17 Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * BSD LICENSE + * + * Copyright(c) 2015-17 Intel Corporation. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Intel Corporation nor the names of its + * contributors may be used to endorse or promote products derived + * from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#ifndef __SOUNDWIRE_H +#define __SOUNDWIRE_H + +#include <linux/device.h> +#include <linux/mod_devicetable.h> + +struct sdw_bus; +struct sdw_slave; + +#define SDW_MAX_DEVICES 11 + +/** + * enum sdw_slave_status: Slave status + * + * @SDW_SLAVE_UNATTACHED: Slave is not attached with the bus. + * @SDW_SLAVE_ATTACHED: Slave is attached with bus. + * @SDW_SLAVE_ALERT: Some alert condition on the Slave + * @SDW_SLAVE_RESERVED: Reserved for future use + */ +enum sdw_slave_status { + SDW_SLAVE_UNATTACHED = 0, + SDW_SLAVE_ATTACHED = 1, + SDW_SLAVE_ALERT = 2, + SDW_SLAVE_RESERVED = 3, +}; + +/* + * SDW Slave Structures and APIs + */ + +/** + * struct sdw_slave_id: Slave ID + * + * @mfg_id: MIPI Manufacturer ID + * @part_id: Device Part ID + * @class_id: MIPI Class ID ,unused now. + * Currently a placeholder in MIPI SoundWire Spec + * @unique_id: Device unique ID + * @sdw_version: SDW version implemented + * + * The order of the IDs here does not follow the DisCo spec definitions + */ +struct sdw_slave_id { + __u16 mfg_id; + __u16 part_id; + __u8 class_id; + __u8 unique_id:4; + __u8 sdw_version:4; +}; + +/** + * struct sdw_slave: SoundWire Slave + * + * @id: MIPI device ID + * @dev: Linux device + * @status: Status reported by the Slave + * @bus: Bus handle + * @node: node for bus list + * @dev_num: Device Number assigned by Bus + */ +struct sdw_slave { + struct sdw_slave_id id; + struct device dev; + enum sdw_slave_status status; + struct sdw_bus *bus; + struct list_head node; + u16 dev_num; +}; + +#define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev) + +extern struct bus_type sdw_bus_type; + +struct sdw_driver { + const char *name; + + int (*probe)(struct sdw_slave *sdw, + const struct sdw_device_id *id); + int (*remove)(struct sdw_slave *sdw); + void (*shutdown)(struct sdw_slave *sdw); + + const struct sdw_device_id *id_table; + const struct sdw_slave_ops *ops; + + struct device_driver driver; +}; + +#define SDW_SLAVE_ENTRY(_mfg_id, _part_id, _drv_data) \ + { .mfg_id = (_mfg_id), .part_id = (_part_id), \ + .driver_data = (unsigned long)(_drv_data) } + +#define drv_to_sdw_driver(_drv) container_of(_drv, struct sdw_driver, driver) + +#define sdw_register_driver(drv) \ + __sdw_register_driver(drv, THIS_MODULE) + +int __sdw_register_driver(struct sdw_driver *drv, struct module *); +void sdw_unregister_driver(struct sdw_driver *drv); + +/* + * SDW master structures and APIs + */ + +/** + * 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]; + struct mutex bus_lock; +}; + +#endif /* __SOUNDWIRE_H */ diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c index e4d90e50f6fe..5c79292d81cf 100644 --- a/scripts/mod/devicetable-offsets.c +++ b/scripts/mod/devicetable-offsets.c @@ -202,6 +202,11 @@ int main(void) DEVID_FIELD(hda_device_id, rev_id); DEVID_FIELD(hda_device_id, api_version); + DEVID(sdw_device_id); + DEVID_FIELD(sdw_device_id, mfg_id); + DEVID_FIELD(sdw_device_id, part_id); + DEVID_FIELD(sdw_device_id, class_id); + DEVID(fsl_mc_device_id); DEVID_FIELD(fsl_mc_device_id, vendor); DEVID_FIELD(fsl_mc_device_id, obj_type); diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c index 29d6699d5a06..3f1706d59fb9 100644 --- a/scripts/mod/file2alias.c +++ b/scripts/mod/file2alias.c @@ -1289,6 +1289,22 @@ static int do_hda_entry(const char *filename, void *symval, char *alias) } ADD_TO_DEVTABLE("hdaudio", hda_device_id, do_hda_entry); +/* Looks like: sdw:mNpN */ +static int do_sdw_entry(const char *filename, void *symval, char *alias) +{ + DEF_FIELD(symval, sdw_device_id, mfg_id); + DEF_FIELD(symval, sdw_device_id, part_id); + DEF_FIELD(symval, sdw_device_id, class_id); + + strcpy(alias, "sdw:"); + ADD(alias, "m", mfg_id != 0, mfg_id); + ADD(alias, "p", part_id != 0, part_id); + + add_wildcard(alias); + return 1; +} +ADD_TO_DEVTABLE("sdw", sdw_device_id, do_sdw_entry); + /* Looks like: fsl-mc:vNdN */ static int do_fsl_mc_entry(const char *filename, void *symval, char *alias)