From: Vinod Koul <vkoul@kernel.org>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: alsa-devel@alsa-project.org, tiwai@suse.de,
gregkh@linuxfoundation.org, broonie@kernel.org,
srinivas.kandagatla@linaro.org,
Bard liao <yung-chuan.liao@linux.intel.com>,
Rander Wang <rander.wang@linux.intel.com>
Subject: Re: [PATCH v2 2/2] soundwire: sysfs: add slave status and device number before probe
Date: Fri, 18 Sep 2020 17:46:14 +0530 [thread overview]
Message-ID: <20200918121614.GS2968@vkoul-mobl> (raw)
In-Reply-To: <20200917160007.153106-3-pierre-louis.bossart@linux.intel.com>
On 17-09-20, 11:00, Pierre-Louis Bossart wrote:
> The MIPI DisCo device properties that are read by the driver from
> platform firmware, or hard-coded in the driver, should only be
> provided as sysfs entries when a driver probes successfully.
>
> However the device status and device number is updated even when there
> is no driver present, and hence can be updated when a Slave device is
> detected on the bus without being described in platform firmware and
> without any driver registered/probed.
>
> As suggested by GregKH, the attribute group for Slave status and
> device number is is added by default upon device registration.
>
> Credits to Vinod Koul for the status_show() function, shared in a
> separate patch and used as is here. The status table was modified to
> remove an unnecessary enum and status_show() is handled in a different
> group attribute than what was suggested by Vinod.
>
> Tested-by: Srinivas Kandgatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
> .../ABI/testing/sysfs-bus-soundwire-slave | 18 ++++++
> drivers/soundwire/slave.c | 2 +
> drivers/soundwire/sysfs_local.h | 4 ++
> drivers/soundwire/sysfs_slave.c | 59 ++++++++++++++++++-
> 4 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-soundwire-slave b/Documentation/ABI/testing/sysfs-bus-soundwire-slave
> index db4c9511d1aa..425adf7b8aec 100644
> --- a/Documentation/ABI/testing/sysfs-bus-soundwire-slave
> +++ b/Documentation/ABI/testing/sysfs-bus-soundwire-slave
> @@ -1,3 +1,21 @@
> +What: /sys/bus/soundwire/devices/sdw:.../dev-status/status
> + /sys/bus/soundwire/devices/sdw:.../dev-status/device_number
> +
> +Date: September 2020
> +
> +Contact: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> + Bard Liao <yung-chuan.liao@linux.intel.com>
> + Vinod Koul <vkoul@kernel.org>
> +
> +Description: SoundWire Slave status
> +
> + These properties report the Slave status, e.g. if it
> + is UNATTACHED or not, and in the latter case show the
> + device_number. This status information is useful to
> + detect devices exposed by platform firmware but not
> + physically present on the bus, and conversely devices
> + not exposed in platform firmware but enumerated.
> +
> What: /sys/bus/soundwire/devices/sdw:.../dev-properties/mipi_revision
> /sys/bus/soundwire/devices/sdw:.../dev-properties/wake_capable
> /sys/bus/soundwire/devices/sdw:.../dev-properties/test_mode_capable
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index 19b012310c29..a08f4081c1c4 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -6,6 +6,7 @@
> #include <linux/soundwire/sdw.h>
> #include <linux/soundwire/sdw_type.h>
> #include "bus.h"
> +#include "sysfs_local.h"
>
> static void sdw_slave_release(struct device *dev)
> {
> @@ -51,6 +52,7 @@ int sdw_slave_add(struct sdw_bus *bus,
> slave->dev.bus = &sdw_bus_type;
> slave->dev.of_node = of_node_get(to_of_node(fwnode));
> slave->dev.type = &sdw_slave_type;
> + slave->dev.groups = sdw_slave_status_attr_groups;
> slave->bus = bus;
> slave->status = SDW_SLAVE_UNATTACHED;
> init_completion(&slave->enumeration_complete);
> diff --git a/drivers/soundwire/sysfs_local.h b/drivers/soundwire/sysfs_local.h
> index ff60adee3c41..7268bc24c538 100644
> --- a/drivers/soundwire/sysfs_local.h
> +++ b/drivers/soundwire/sysfs_local.h
> @@ -8,6 +8,10 @@
> * SDW sysfs APIs -
> */
>
> +/* basic attributes to report status of Slave (attachment, dev_num) */
> +extern const struct attribute_group *sdw_slave_status_attr_groups[];
> +
> +/* additional device-managed properties reported after driver probe */
> int sdw_slave_sysfs_init(struct sdw_slave *slave);
> int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave);
>
> diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c
> index f510071b0add..99c0bdd4a726 100644
> --- a/drivers/soundwire/sysfs_slave.c
> +++ b/drivers/soundwire/sysfs_slave.c
> @@ -16,10 +16,14 @@
>
> /*
> * The sysfs for Slave reflects the MIPI description as given
> - * in the MIPI DisCo spec
> + * in the MIPI DisCo spec. dev-status properties come directly
> + * from the MIPI SoundWire specification.
> *
> * Base file is device
> * |---- modalias
> + * |---- dev-status
> + * |---- status
> + * |---- device_number
Any reason why we want this under dev-status.
Both the status and device_number belong to the device, so we can
put them under device and use device properties
> * |---- dev-properties
> * |---- mipi_revision
> * |---- wake_capable
> @@ -212,3 +216,56 @@ int sdw_slave_sysfs_init(struct sdw_slave *slave)
>
> return 0;
> }
> +
> +/*
> + * the status is shown in capital letters for UNATTACHED and RESERVED
> + * on purpose, to highligh users to the fact that these status values
> + * are not expected.
Thanks for highlighting this, indeed this was intentional
> + */
> +static const char *const slave_status[] = {
> + [SDW_SLAVE_UNATTACHED] = "UNATTACHED",
> + [SDW_SLAVE_ATTACHED] = "Attached",
> + [SDW_SLAVE_ALERT] = "Alert",
> + [SDW_SLAVE_RESERVED] = "RESERVED",
> +};
> +
> +static ssize_t status_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +
> + return sprintf(buf, "%s\n", slave_status[slave->status]);
> +}
> +static DEVICE_ATTR_RO(status);
> +
> +static ssize_t device_number_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +
> + if (slave->status == SDW_SLAVE_UNATTACHED)
> + return sprintf(buf, "%s", "N/A");
Do we really want N/A here, 0 should imply UNATTACHED and then the
status_show would tell UNATTACHED.
> + else
> + return sprintf(buf, "%d", slave->dev_num);
> +}
> +static DEVICE_ATTR_RO(device_number);
> +
> +static struct attribute *slave_status_attrs[] = {
> + &dev_attr_status.attr,
> + &dev_attr_device_number.attr,
> + NULL,
> +};
> +
> +/*
> + * we don't use ATTRIBUTES_GROUP here since we want to add a subdirectory
> + * for device-status
> + */
> +static const struct attribute_group sdw_slave_status_attr_group = {
> + .attrs = slave_status_attrs,
> + .name = "dev-status",
> +};
> +
> +const struct attribute_group *sdw_slave_status_attr_groups[] = {
> + &sdw_slave_status_attr_group,
> + NULL
> +};
> --
> 2.25.1
--
~Vinod
next prev parent reply other threads:[~2020-09-18 12:17 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-17 16:00 [PATCH v2 0/2] soundwire: sysfs: expose device number and status Pierre-Louis Bossart
2020-09-17 16:00 ` [PATCH v2 1/2] soundwire: bus: add enumerated Slave device to device list Pierre-Louis Bossart
2020-09-18 12:05 ` Vinod Koul
2020-09-18 13:54 ` Pierre-Louis Bossart
2020-09-19 11:13 ` Vinod Koul
2020-09-17 16:00 ` [PATCH v2 2/2] soundwire: sysfs: add slave status and device number before probe Pierre-Louis Bossart
2020-09-17 16:36 ` Greg KH
2020-09-18 12:16 ` Vinod Koul [this message]
2020-09-18 14:21 ` Pierre-Louis Bossart
2020-09-19 11:19 ` Vinod Koul
2020-09-21 14:34 ` Pierre-Louis Bossart
2020-09-23 10:03 ` Vinod Koul
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200918121614.GS2968@vkoul-mobl \
--to=vkoul@kernel.org \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=rander.wang@linux.intel.com \
--cc=srinivas.kandagatla@linaro.org \
--cc=tiwai@suse.de \
--cc=yung-chuan.liao@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).