A few fixes to the documentation and some cleanups against extcon core
module.
Andy Shevchenko (14):
extcon: Fix kernel doc of property fields to avoid warnings
extcon: Fix kernel doc of property capability fields to avoid warnings
extcon: Use DECLARE_BITMAP() to declare bit arrays
extcon: use sysfs_emit() to instead of sprintf()
extcon: Amend kernel documentation of struct extcon_dev
extcon: Allow name to be assigned outside of the framework
extcon: Use unique number for the extcon device ID
extcon: Switch to use kasprintf_strarray()
extcon: Use device_match_of_node() helper
extcon: use dev_of_node(dev) instead of dev->of_node
extcon: Remove dup device name in the message and unneeded error check
extcon: Use sizeof(*pointer) instead of sizeof(type)
extcon: Drop unneeded assignments
extcon: Use positive conditional in ternary operator
drivers/extcon/extcon.c | 126 +++++++++++++++++++++-------------------
drivers/extcon/extcon.h | 9 ++-
2 files changed, 71 insertions(+), 64 deletions(-)
--
2.40.0.1.gaa8946217a0b
Kernel documentation has to be synchronized with a code, otherwise
the validator is not happy:
Function parameter or member 'usb_propval' not described in 'extcon_cable'
Function parameter or member 'chg_propval' not described in 'extcon_cable'
Function parameter or member 'jack_propval' not described in 'extcon_cable'
Function parameter or member 'disp_propval' not described in 'extcon_cable'
Describe the fields added in the past.
Fixes: 067c1652e7a7 ("extcon: Add the support for extcon property according to extcon type")
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 3997b39680b7..1bc2639704c2 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -206,6 +206,10 @@ static const struct __extcon_info {
* @attr_name: "name" sysfs entry
* @attr_state: "state" sysfs entry
* @attrs: the array pointing to attr_name and attr_state for attr_g
+ * @usb_propval: the array of USB connector properties
+ * @chg_propval: the array of charger connector properties
+ * @jack_propval: the array of jack connector properties
+ * @disp_propval: the array of display connector properties
*/
struct extcon_cable {
struct extcon_dev *edev;
--
2.40.0.1.gaa8946217a0b
Kernel documentation has to be synchronized with a code, otherwise
the validator is not happy:
Function parameter or member 'usb_bits' not described in 'extcon_cable'
Function parameter or member 'chg_bits' not described in 'extcon_cable'
Function parameter or member 'jack_bits' not described in 'extcon_cable'
Function parameter or member 'disp_bits' not described in 'extcon_cable'
Describe the fields added in the past.
Fixes: ceaa98f442cf ("extcon: Add the support for the capability of each property")
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 1bc2639704c2..79006ab5334b 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -210,6 +210,10 @@ static const struct __extcon_info {
* @chg_propval: the array of charger connector properties
* @jack_propval: the array of jack connector properties
* @disp_propval: the array of display connector properties
+ * @usb_bits: the bit array of the USB connector property capabilities
+ * @chg_bits: the bit array of the charger connector property capabilities
+ * @jack_bits: the bit array of the jack connector property capabilities
+ * @disp_bits: the bit array of the display connector property capabilities
*/
struct extcon_cable {
struct extcon_dev *edev;
--
2.40.0.1.gaa8946217a0b
First of all, the @lock description is missing. Add it.
Second, correct the terminator value for the mutual exclusive
cabling.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
index 93b5e0306966..15616446140d 100644
--- a/drivers/extcon/extcon.h
+++ b/drivers/extcon/extcon.h
@@ -13,8 +13,8 @@
* are disabled.
* @mutually_exclusive: Array of mutually exclusive set of cables that cannot
* be attached simultaneously. The array should be
- * ending with NULL or be NULL (no mutually exclusive
- * cables). For example, if it is { 0x7, 0x30, 0}, then,
+ * ending with 0 or be NULL (no mutually exclusive cables).
+ * For example, if it is {0x7, 0x30, 0}, then,
* {0, 1}, {0, 1, 2}, {0, 2}, {1, 2}, or {4, 5} cannot
* be attached simulataneously. {0x7, 0} is equivalent to
* {0x3, 0x6, 0x5, 0}. If it is {0xFFFFFFFF, 0}, there
@@ -27,7 +27,7 @@
* @nh: Notifier for the state change events from this extcon
* @entry: To support list of extcon devices so that users can
* search for extcon devices based on the extcon name.
- * @lock:
+ * @lock: Protects device state and serialises device registration
* @max_supported: Internal value to store the number of cables.
* @extcon_dev_type: Device_type struct to provide attribute_groups
* customized for each extcon device.
--
2.40.0.1.gaa8946217a0b
Instead of open coding, use device_match_of_node() helper.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index a63e7eef02fd..5cadbfc151e6 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1411,7 +1411,7 @@ struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)
mutex_lock(&extcon_dev_list_lock);
list_for_each_entry(edev, &extcon_dev_list, entry)
- if (edev->dev.parent && edev->dev.parent->of_node == node)
+ if (edev->dev.parent && device_match_of_node(edev->dev.parent, node))
goto out;
edev = ERR_PTR(-EPROBE_DEFER);
out:
--
2.40.0.1.gaa8946217a0b
The documentation states that name of the extcon can be assigned
outside of the framework, however code does something different.
Fix the code to be aligned with the documentation.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index ac84f4aafc69..14e66e21597f 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1269,10 +1269,10 @@ int extcon_dev_register(struct extcon_dev *edev)
edev->dev.class = extcon_class;
edev->dev.release = extcon_dev_release;
- edev->name = dev_name(edev->dev.parent);
- if (IS_ERR_OR_NULL(edev->name)) {
- dev_err(&edev->dev,
- "extcon device name is null\n");
+ if (!edev->name)
+ edev->name = dev_name(edev->dev.parent);
+ if (!edev->name) {
+ dev_err(&edev->dev, "extcon device name is null\n");
return -EINVAL;
}
dev_set_name(&edev->dev, "extcon%lu",
--
2.40.0.1.gaa8946217a0b
Since we have a generic helper, switch the module to use it.
No functional change intended.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.c | 25 +++++++++++--------------
drivers/extcon/extcon.h | 1 +
2 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 0d261ec6c473..a63e7eef02fd 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -23,6 +23,7 @@
#include <linux/err.h>
#include <linux/of.h>
#include <linux/slab.h>
+#include <linux/string_helpers.h>
#include <linux/sysfs.h>
#include "extcon.h"
@@ -1104,19 +1105,17 @@ static int extcon_alloc_cables(struct extcon_dev *edev)
if (!edev->cables)
return -ENOMEM;
+ edev->cable_names = kasprintf_strarray(GFP_KERNEL, "cable", edev->max_supported);
+ if (!edev->cable_names) {
+ kfree(edev->cables);
+ return -ENOMEM;
+ }
+
for (index = 0; index < edev->max_supported; index++) {
cable = &edev->cables[index];
- str = kasprintf(GFP_KERNEL, "cable.%d", index);
- if (!str) {
- for (index--; index >= 0; index--) {
- cable = &edev->cables[index];
- kfree(cable->attr_g.name);
- }
-
- kfree(edev->cables);
- return -ENOMEM;
- }
+ str = edev->cable_names[index];
+ strreplace(str, '-', '.');
cable->edev = edev;
cable->cable_index = index;
@@ -1341,8 +1340,7 @@ int extcon_dev_register(struct extcon_dev *edev)
kfree(edev->attrs_muex);
}
err_alloc_muex:
- for (index = 0; index < edev->max_supported; index++)
- kfree(edev->cables[index].attr_g.name);
+ kfree_strarray(edev->cable_names, edev->max_supported);
if (edev->max_supported)
kfree(edev->cables);
err_alloc_cables:
@@ -1387,8 +1385,7 @@ void extcon_dev_unregister(struct extcon_dev *edev)
kfree(edev->attrs_muex);
}
- for (index = 0; index < edev->max_supported; index++)
- kfree(edev->cables[index].attr_g.name);
+ kfree_strarray(edev->cable_names, edev->max_supported);
if (edev->max_supported) {
kfree(edev->extcon_dev_type.groups);
diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
index 877c0860e300..5624f65ba17a 100644
--- a/drivers/extcon/extcon.h
+++ b/drivers/extcon/extcon.h
@@ -58,6 +58,7 @@ struct extcon_dev {
/* /sys/class/extcon/.../cable.n/... */
struct device_type extcon_dev_type;
struct extcon_cable *cables;
+ char **cable_names;
/* /sys/class/extcon/.../mutually_exclusive/... */
struct attribute_group attr_g_muex;
--
2.40.0.1.gaa8946217a0b
Bit arrays has a specific type helper for the declaration.
Use it instead of homegronw equivalent.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 79006ab5334b..70e9755ba2bc 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -230,10 +230,10 @@ struct extcon_cable {
union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT];
union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT];
- unsigned long usb_bits[BITS_TO_LONGS(EXTCON_PROP_USB_CNT)];
- unsigned long chg_bits[BITS_TO_LONGS(EXTCON_PROP_CHG_CNT)];
- unsigned long jack_bits[BITS_TO_LONGS(EXTCON_PROP_JACK_CNT)];
- unsigned long disp_bits[BITS_TO_LONGS(EXTCON_PROP_DISP_CNT)];
+ DECLARE_BITMAP(usb_bits, EXTCON_PROP_USB_CNT);
+ DECLARE_BITMAP(chg_bits, EXTCON_PROP_CHG_CNT);
+ DECLARE_BITMAP(jack_bits, EXTCON_PROP_JACK_CNT);
+ DECLARE_BITMAP(disp_bits, EXTCON_PROP_DISP_CNT);
};
static struct class *extcon_class;
--
2.40.0.1.gaa8946217a0b
It is preferred to use sizeof(*pointer) instead of sizeof(type).
The type of the variable can change and one needs not change
the former (unlike the latter). No functional change intended.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 0e04ea185c26..b3f038639cd6 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1099,9 +1099,7 @@ static int extcon_alloc_cables(struct extcon_dev *edev)
if (!edev->max_supported)
return 0;
- edev->cables = kcalloc(edev->max_supported,
- sizeof(struct extcon_cable),
- GFP_KERNEL);
+ edev->cables = kcalloc(edev->max_supported, sizeof(*edev->cables), GFP_KERNEL);
if (!edev->cables)
return -ENOMEM;
@@ -1160,14 +1158,12 @@ static int extcon_alloc_muex(struct extcon_dev *edev)
for (index = 0; edev->mutually_exclusive[index]; index++)
;
- edev->attrs_muex = kcalloc(index + 1,
- sizeof(struct attribute *),
+ edev->attrs_muex = kcalloc(index + 1, sizeof(*edev->attrs_muex),
GFP_KERNEL);
if (!edev->attrs_muex)
return -ENOMEM;
- edev->d_attrs_muex = kcalloc(index,
- sizeof(struct device_attribute),
+ edev->d_attrs_muex = kcalloc(index, sizeof(*edev->d_attrs_muex),
GFP_KERNEL);
if (!edev->d_attrs_muex) {
kfree(edev->attrs_muex);
@@ -1213,8 +1209,8 @@ static int extcon_alloc_groups(struct extcon_dev *edev)
return 0;
edev->extcon_dev_type.groups = kcalloc(edev->max_supported + 2,
- sizeof(struct attribute_group *),
- GFP_KERNEL);
+ sizeof(*edev->extcon_dev_type.groups),
+ GFP_KERNEL);
if (!edev->extcon_dev_type.groups)
return -ENOMEM;
@@ -1298,8 +1294,7 @@ int extcon_dev_register(struct extcon_dev *edev)
spin_lock_init(&edev->lock);
if (edev->max_supported) {
- edev->nh = kcalloc(edev->max_supported, sizeof(*edev->nh),
- GFP_KERNEL);
+ edev->nh = kcalloc(edev->max_supported, sizeof(*edev->nh), GFP_KERNEL);
if (!edev->nh) {
ret = -ENOMEM;
goto err_alloc_nh;
--
2.40.0.1.gaa8946217a0b
The dev_of_node function should be preferred.
In the result we may drop unneeded NULL check
of the pointer to the device object.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 5cadbfc151e6..32e96cb49067 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1429,21 +1429,17 @@ struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)
*/
struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index)
{
- struct device_node *node;
+ struct device_node *node, *np = dev_of_node(dev);
struct extcon_dev *edev;
- if (!dev)
- return ERR_PTR(-EINVAL);
-
- if (!dev->of_node) {
+ if (!np) {
dev_dbg(dev, "device does not have a device node entry\n");
return ERR_PTR(-EINVAL);
}
- node = of_parse_phandle(dev->of_node, "extcon", index);
+ node = of_parse_phandle(np, "extcon", index);
if (!node) {
- dev_dbg(dev, "failed to get phandle in %pOF node\n",
- dev->of_node);
+ dev_dbg(dev, "failed to get phandle in %pOF node\n", np);
return ERR_PTR(-ENODEV);
}
--
2.40.0.1.gaa8946217a0b
In one case the assignment is duplicative, in the other,
it's better to move it into the loop — the user of it.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index b3f038639cd6..edfa0450e605 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -246,7 +246,7 @@ static DEFINE_MUTEX(extcon_dev_list_lock);
static int check_mutually_exclusive(struct extcon_dev *edev, u32 new_state)
{
- int i = 0;
+ int i;
if (!edev->mutually_exclusive)
return 0;
@@ -1244,7 +1244,7 @@ static int extcon_alloc_groups(struct extcon_dev *edev)
*/
int extcon_dev_register(struct extcon_dev *edev)
{
- int ret, index = 0;
+ int ret, index;
ret = create_extcon_class();
if (ret < 0)
@@ -1253,7 +1253,7 @@ int extcon_dev_register(struct extcon_dev *edev)
if (!edev || !edev->supported_cable)
return -EINVAL;
- for (; edev->supported_cable[index] != EXTCON_NONE; index++);
+ for (index = 0; edev->supported_cable[index] != EXTCON_NONE; index++);
edev->max_supported = index;
if (index > SUPPORTED_CABLE_MAX) {
--
2.40.0.1.gaa8946217a0b
It's easier to parse by a human being the positive conditional.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index edfa0450e605..3e8522d522b4 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1466,7 +1466,7 @@ EXPORT_SYMBOL_GPL(extcon_get_edev_by_phandle);
*/
const char *extcon_get_edev_name(struct extcon_dev *edev)
{
- return !edev ? NULL : edev->name;
+ return edev ? edev->name : NULL;
}
EXPORT_SYMBOL_GPL(extcon_get_edev_name);
--
2.40.0.1.gaa8946217a0b
The device name is already printed with dev_err(), no need to repeat.
The device pointer itself is not supposed to be an error point, drop
that check.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 32e96cb49067..0e04ea185c26 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1367,9 +1367,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
list_del(&edev->entry);
mutex_unlock(&extcon_dev_list_lock);
- if (IS_ERR_OR_NULL(get_device(&edev->dev))) {
- dev_err(&edev->dev, "Failed to unregister extcon_dev (%s)\n",
- dev_name(&edev->dev));
+ if (!get_device(&edev->dev)) {
+ dev_err(&edev->dev, "Failed to unregister extcon_dev\n");
return;
}
--
2.40.0.1.gaa8946217a0b
The use of atomic variable is still racy when we do not control which
device has been unregistered and there is a (theoretical) possibility
of the overflow that may cause a duplicate extcon device ID number
to be allocated next time a device is registered.
Replace above mentioned approach by using IDA framework.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/extcon/extcon.c | 15 ++++++++++++---
drivers/extcon/extcon.h | 2 ++
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 14e66e21597f..0d261ec6c473 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -16,6 +16,7 @@
#include <linux/module.h>
#include <linux/types.h>
+#include <linux/idr.h>
#include <linux/init.h>
#include <linux/device.h>
#include <linux/fs.h>
@@ -238,6 +239,7 @@ struct extcon_cable {
static struct class *extcon_class;
+static DEFINE_IDA(extcon_dev_ids);
static LIST_HEAD(extcon_dev_list);
static DEFINE_MUTEX(extcon_dev_list_lock);
@@ -1248,7 +1250,6 @@ static int extcon_alloc_groups(struct extcon_dev *edev)
int extcon_dev_register(struct extcon_dev *edev)
{
int ret, index = 0;
- static atomic_t edev_no = ATOMIC_INIT(-1);
ret = create_extcon_class();
if (ret < 0)
@@ -1275,8 +1276,14 @@ int extcon_dev_register(struct extcon_dev *edev)
dev_err(&edev->dev, "extcon device name is null\n");
return -EINVAL;
}
- dev_set_name(&edev->dev, "extcon%lu",
- (unsigned long)atomic_inc_return(&edev_no));
+
+ ret = ida_simple_get(&extcon_dev_ids, 0, INT_MAX, GFP_KERNEL);
+ if (ret < 0)
+ return ret;
+
+ edev->id = ret;
+
+ dev_set_name(&edev->dev, "extcon%d", edev->id);
ret = extcon_alloc_cables(edev);
if (ret < 0)
@@ -1368,6 +1375,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
return;
}
+ ida_simple_remove(&extcon_dev_ids, edev->id);
+
device_unregister(&edev->dev);
if (edev->mutually_exclusive && edev->max_supported) {
diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
index 15616446140d..877c0860e300 100644
--- a/drivers/extcon/extcon.h
+++ b/drivers/extcon/extcon.h
@@ -20,6 +20,7 @@
* {0x3, 0x6, 0x5, 0}. If it is {0xFFFFFFFF, 0}, there
* can be no simultaneous connections.
* @dev: Device of this extcon.
+ * @id: Unique device ID of this extcon.
* @state: Attach/detach state of this extcon. Do not provide at
* register-time.
* @nh_all: Notifier for the state change events for all supported
@@ -46,6 +47,7 @@ struct extcon_dev {
/* Internal data. Please do not set. */
struct device dev;
+ int id;
struct raw_notifier_head nh_all;
struct raw_notifier_head *nh;
struct list_head entry;
--
2.40.0.1.gaa8946217a0b
On Wed, Mar 22, 2023 at 04:39:51PM +0200, Andy Shevchenko wrote:
> A few fixes to the documentation and some cleanups against extcon core
> module.
Anything I should do with the series?
Any comments on it?
--
With Best Regards,
Andy Shevchenko
Hi Andy Shevchenko
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Thursday, March 30, 2023 7:12 PM
> To: Bumwoo Lee <[email protected]>; [email protected]
> Cc: MyungJoo Ham <[email protected]>; Chanwoo Choi
> <[email protected]>
> Subject: Re: [PATCH v1 00/14] extcon: Core cleanups and documentation
> fixes
>
> On Wed, Mar 22, 2023 at 04:39:51PM +0200, Andy Shevchenko wrote:
> > A few fixes to the documentation and some cleanups against extcon core
> > module.
>
> Anything I should do with the series?
> Any comments on it?
>
> --
> With Best Regards,
> Andy Shevchenko
>
Looks fine to me.
Acked-by: Bumwoo Lee <[email protected]>
MR. Chanwoo, Would you please take a look at this patch series.
On 23. 3. 22. 23:39, Andy Shevchenko wrote:
> Kernel documentation has to be synchronized with a code, otherwise
> the validator is not happy:
>
> Function parameter or member 'usb_propval' not described in 'extcon_cable'
> Function parameter or member 'chg_propval' not described in 'extcon_cable'
> Function parameter or member 'jack_propval' not described in 'extcon_cable'
> Function parameter or member 'disp_propval' not described in 'extcon_cable'
>
> Describe the fields added in the past.
>
> Fixes: 067c1652e7a7 ("extcon: Add the support for extcon property according to extcon type")
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 3997b39680b7..1bc2639704c2 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -206,6 +206,10 @@ static const struct __extcon_info {
> * @attr_name: "name" sysfs entry
> * @attr_state: "state" sysfs entry
> * @attrs: the array pointing to attr_name and attr_state for attr_g
> + * @usb_propval: the array of USB connector properties
> + * @chg_propval: the array of charger connector properties
> + * @jack_propval: the array of jack connector properties
> + * @disp_propval: the array of display connector properties
> */
> struct extcon_cable {
> struct extcon_dev *edev;
Applied it. Thanks.
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 22. 23:39, Andy Shevchenko wrote:
> Kernel documentation has to be synchronized with a code, otherwise
> the validator is not happy:
>
> Function parameter or member 'usb_bits' not described in 'extcon_cable'
> Function parameter or member 'chg_bits' not described in 'extcon_cable'
> Function parameter or member 'jack_bits' not described in 'extcon_cable'
> Function parameter or member 'disp_bits' not described in 'extcon_cable'
>
> Describe the fields added in the past.
>
> Fixes: ceaa98f442cf ("extcon: Add the support for the capability of each property")
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 1bc2639704c2..79006ab5334b 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -210,6 +210,10 @@ static const struct __extcon_info {
> * @chg_propval: the array of charger connector properties
> * @jack_propval: the array of jack connector properties
> * @disp_propval: the array of display connector properties
> + * @usb_bits: the bit array of the USB connector property capabilities
> + * @chg_bits: the bit array of the charger connector property capabilities
> + * @jack_bits: the bit array of the jack connector property capabilities
> + * @disp_bits: the bit array of the display connector property capabilities
> */
> struct extcon_cable {
> struct extcon_dev *edev;
Applied it.
--
Best Regards,
Samsung Electronics
Chanwoo Choi
Hi,
I recommend that use the "./scripts/get_maintainer.pl" script
to get the accurate maintainer list to send the patches.
On 23. 3. 22. 23:39, Andy Shevchenko wrote:
> A few fixes to the documentation and some cleanups against extcon core
> module.
>
> Andy Shevchenko (14):
> extcon: Fix kernel doc of property fields to avoid warnings
> extcon: Fix kernel doc of property capability fields to avoid warnings
> extcon: Use DECLARE_BITMAP() to declare bit arrays
> extcon: use sysfs_emit() to instead of sprintf()
> extcon: Amend kernel documentation of struct extcon_dev
> extcon: Allow name to be assigned outside of the framework
> extcon: Use unique number for the extcon device ID
> extcon: Switch to use kasprintf_strarray()
> extcon: Use device_match_of_node() helper
> extcon: use dev_of_node(dev) instead of dev->of_node
> extcon: Remove dup device name in the message and unneeded error check
> extcon: Use sizeof(*pointer) instead of sizeof(type)
> extcon: Drop unneeded assignments
> extcon: Use positive conditional in ternary operator
>
> drivers/extcon/extcon.c | 126 +++++++++++++++++++++-------------------
> drivers/extcon/extcon.h | 9 ++-
> 2 files changed, 71 insertions(+), 64 deletions(-)
>
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 22. 23:39, Andy Shevchenko wrote:
> The documentation states that name of the extcon can be assigned
> outside of the framework, however code does something different.
> Fix the code to be aligned with the documentation.
Actually, it is not possible to initialize the name outside of the framework
because commit 20f7b53dfc24 ("extcon: Move struct extcon_cable from header file to core")
moved the 'struct extcon_dev' into drivers/extcon/extcon.c in order to prevent
the direct accessing of struct extcon_dev.
Instead of this patch, need to change the description of struct extcon_dev.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index ac84f4aafc69..14e66e21597f 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -1269,10 +1269,10 @@ int extcon_dev_register(struct extcon_dev *edev)
> edev->dev.class = extcon_class;
> edev->dev.release = extcon_dev_release;
>
> - edev->name = dev_name(edev->dev.parent);
> - if (IS_ERR_OR_NULL(edev->name)) {
> - dev_err(&edev->dev,
> - "extcon device name is null\n");
> + if (!edev->name)
> + edev->name = dev_name(edev->dev.parent);
> + if (!edev->name) {
> + dev_err(&edev->dev, "extcon device name is null\n");
> return -EINVAL;
> }
> dev_set_name(&edev->dev, "extcon%lu",
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 22. 23:39, Andy Shevchenko wrote:
> Bit arrays has a specific type helper for the declaration.
> Use it instead of homegronw equivalent.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 79006ab5334b..70e9755ba2bc 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -230,10 +230,10 @@ struct extcon_cable {
> union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT];
> union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT];
>
> - unsigned long usb_bits[BITS_TO_LONGS(EXTCON_PROP_USB_CNT)];
> - unsigned long chg_bits[BITS_TO_LONGS(EXTCON_PROP_CHG_CNT)];
> - unsigned long jack_bits[BITS_TO_LONGS(EXTCON_PROP_JACK_CNT)];
> - unsigned long disp_bits[BITS_TO_LONGS(EXTCON_PROP_DISP_CNT)];
> + DECLARE_BITMAP(usb_bits, EXTCON_PROP_USB_CNT);
> + DECLARE_BITMAP(chg_bits, EXTCON_PROP_CHG_CNT);
> + DECLARE_BITMAP(jack_bits, EXTCON_PROP_JACK_CNT);
> + DECLARE_BITMAP(disp_bits, EXTCON_PROP_DISP_CNT);
> };
>
> static struct class *extcon_class;
Applied it.
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 22. 23:39, Andy Shevchenko wrote:
> First of all, the @lock description is missing. Add it.
> Second, correct the terminator value for the mutual exclusive
> cabling.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
> index 93b5e0306966..15616446140d 100644
> --- a/drivers/extcon/extcon.h
> +++ b/drivers/extcon/extcon.h
> @@ -13,8 +13,8 @@
> * are disabled.
> * @mutually_exclusive: Array of mutually exclusive set of cables that cannot
> * be attached simultaneously. The array should be
> - * ending with NULL or be NULL (no mutually exclusive
> - * cables). For example, if it is { 0x7, 0x30, 0}, then,
> + * ending with 0 or be NULL (no mutually exclusive cables).
> + * For example, if it is {0x7, 0x30, 0}, then,
> * {0, 1}, {0, 1, 2}, {0, 2}, {1, 2}, or {4, 5} cannot
> * be attached simulataneously. {0x7, 0} is equivalent to
> * {0x3, 0x6, 0x5, 0}. If it is {0xFFFFFFFF, 0}, there
> @@ -27,7 +27,7 @@
> * @nh: Notifier for the state change events from this extcon
> * @entry: To support list of extcon devices so that users can
> * search for extcon devices based on the extcon name.
> - * @lock:
> + * @lock: Protects device state and serialises device registration
> * @max_supported: Internal value to store the number of cables.
> * @extcon_dev_type: Device_type struct to provide attribute_groups
> * customized for each extcon device.
Applied it.
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 22. 23:40, Andy Shevchenko wrote:
> Instead of open coding, use device_match_of_node() helper.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index a63e7eef02fd..5cadbfc151e6 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -1411,7 +1411,7 @@ struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)
>
> mutex_lock(&extcon_dev_list_lock);
> list_for_each_entry(edev, &extcon_dev_list, entry)
> - if (edev->dev.parent && edev->dev.parent->of_node == node)
> + if (edev->dev.parent && device_match_of_node(edev->dev.parent, node))
> goto out;
> edev = ERR_PTR(-EPROBE_DEFER);
> out:
Applied it.
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 22. 23:40, Andy Shevchenko wrote:
> The dev_of_node function should be preferred.
> In the result we may drop unneeded NULL check
> of the pointer to the device object.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 5cadbfc151e6..32e96cb49067 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -1429,21 +1429,17 @@ struct extcon_dev *extcon_find_edev_by_node(struct device_node *node)
> */
> struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index)
> {
> - struct device_node *node;
> + struct device_node *node, *np = dev_of_node(dev);
> struct extcon_dev *edev;
>
> - if (!dev)
> - return ERR_PTR(-EINVAL);
> -
> - if (!dev->of_node) {
> + if (!np) {
> dev_dbg(dev, "device does not have a device node entry\n");
> return ERR_PTR(-EINVAL);
> }
>
> - node = of_parse_phandle(dev->of_node, "extcon", index);
> + node = of_parse_phandle(np, "extcon", index);
> if (!node) {
> - dev_dbg(dev, "failed to get phandle in %pOF node\n",
> - dev->of_node);
> + dev_dbg(dev, "failed to get phandle in %pOF node\n", np);
> return ERR_PTR(-ENODEV);
> }
>
Applied it with the following patch title.
Just use capital letter at the beginning char of title
- extcon: Use dev_of_node(dev) instead of dev->of_node
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 22. 23:40, Andy Shevchenko wrote:
> It is preferred to use sizeof(*pointer) instead of sizeof(type).
> The type of the variable can change and one needs not change
> the former (unlike the latter). No functional change intended.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 0e04ea185c26..b3f038639cd6 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -1099,9 +1099,7 @@ static int extcon_alloc_cables(struct extcon_dev *edev)
> if (!edev->max_supported)
> return 0;
>
> - edev->cables = kcalloc(edev->max_supported,
> - sizeof(struct extcon_cable),
> - GFP_KERNEL);
> + edev->cables = kcalloc(edev->max_supported, sizeof(*edev->cables), GFP_KERNEL);
Even if there are strictly not limitation of the number of maximum char
at the one line, But, I recommend to keep the 80 char at the one line
for the readability as following.
- edev->cables = kcalloc(edev->max_supported, sizeof(*edev->cables), GFP_KERNEL);
+ edev->cables = kcalloc(edev->max_supported,
+ sizeof(*edev->cables), GFP_KERNEL);
> if (!edev->cables)
> return -ENOMEM;
>
> @@ -1160,14 +1158,12 @@ static int extcon_alloc_muex(struct extcon_dev *edev)
> for (index = 0; edev->mutually_exclusive[index]; index++)
> ;
>
> - edev->attrs_muex = kcalloc(index + 1,
> - sizeof(struct attribute *),
> + edev->attrs_muex = kcalloc(index + 1, sizeof(*edev->attrs_muex),
> GFP_KERNEL);
> if (!edev->attrs_muex)
> return -ENOMEM;
>
> - edev->d_attrs_muex = kcalloc(index,
> - sizeof(struct device_attribute),
> + edev->d_attrs_muex = kcalloc(index, sizeof(*edev->d_attrs_muex),
> GFP_KERNEL);
> if (!edev->d_attrs_muex) {
> kfree(edev->attrs_muex);
> @@ -1213,8 +1209,8 @@ static int extcon_alloc_groups(struct extcon_dev *edev)
> return 0;
>
> edev->extcon_dev_type.groups = kcalloc(edev->max_supported + 2,
> - sizeof(struct attribute_group *),
> - GFP_KERNEL);
> + sizeof(*edev->extcon_dev_type.groups),
> + GFP_KERNEL);
ditto.
edev->extcon_dev_type.groups = kcalloc(edev->max_supported + 2,
- sizeof(*edev->extcon_dev_type.groups),
- GFP_KERNEL);
+ sizeof(*edev->extcon_dev_type.groups),
+ GFP_KERNEL);
> if (!edev->extcon_dev_type.groups)
> return -ENOMEM;
>
> @@ -1298,8 +1294,7 @@ int extcon_dev_register(struct extcon_dev *edev)
>
> spin_lock_init(&edev->lock);
> if (edev->max_supported) {
> - edev->nh = kcalloc(edev->max_supported, sizeof(*edev->nh),
> - GFP_KERNEL);
> + edev->nh = kcalloc(edev->max_supported, sizeof(*edev->nh), GFP_KERNEL);
- edev->nh = kcalloc(edev->max_supported, sizeof(*edev->nh), GFP_KERNEL);
+ edev->nh = kcalloc(edev->max_supported,
+ sizeof(*edev->nh), GFP_KERNEL);
> if (!edev->nh) {
> ret = -ENOMEM;
> goto err_alloc_nh;
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 22. 23:40, Andy Shevchenko wrote:
> It's easier to parse by a human being the positive conditional.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index edfa0450e605..3e8522d522b4 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -1466,7 +1466,7 @@ EXPORT_SYMBOL_GPL(extcon_get_edev_by_phandle);
> */
> const char *extcon_get_edev_name(struct extcon_dev *edev)
> {
> - return !edev ? NULL : edev->name;
> + return edev ? edev->name : NULL;
> }
> EXPORT_SYMBOL_GPL(extcon_get_edev_name);
>
It is not fix-up patch and I'm not sure that it is beneficial patch.
I will not apply it.
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 22. 23:39, Andy Shevchenko wrote:
> Since we have a generic helper, switch the module to use it.
> No functional change intended.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.c | 25 +++++++++++--------------
> drivers/extcon/extcon.h | 1 +
> 2 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 0d261ec6c473..a63e7eef02fd 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -23,6 +23,7 @@
> #include <linux/err.h>
> #include <linux/of.h>
> #include <linux/slab.h>
> +#include <linux/string_helpers.h>
> #include <linux/sysfs.h>
>
> #include "extcon.h"
> @@ -1104,19 +1105,17 @@ static int extcon_alloc_cables(struct extcon_dev *edev)
> if (!edev->cables)
> return -ENOMEM;
>
> + edev->cable_names = kasprintf_strarray(GFP_KERNEL, "cable", edev->max_supported);
> + if (!edev->cable_names) {
> + kfree(edev->cables);
> + return -ENOMEM;
> + }
> +
> for (index = 0; index < edev->max_supported; index++) {
> cable = &edev->cables[index];
>
> - str = kasprintf(GFP_KERNEL, "cable.%d", index);
> - if (!str) {
> - for (index--; index >= 0; index--) {
> - cable = &edev->cables[index];
> - kfree(cable->attr_g.name);
> - }
> -
> - kfree(edev->cables);
> - return -ENOMEM;
> - }
> + str = edev->cable_names[index];
> + strreplace(str, '-', '.');
>
> cable->edev = edev;
> cable->cable_index = index;
> @@ -1341,8 +1340,7 @@ int extcon_dev_register(struct extcon_dev *edev)
> kfree(edev->attrs_muex);
> }
> err_alloc_muex:
> - for (index = 0; index < edev->max_supported; index++)
> - kfree(edev->cables[index].attr_g.name);
> + kfree_strarray(edev->cable_names, edev->max_supported);
> if (edev->max_supported)
> kfree(edev->cables);
> err_alloc_cables:
> @@ -1387,8 +1385,7 @@ void extcon_dev_unregister(struct extcon_dev *edev)
> kfree(edev->attrs_muex);
> }
>
> - for (index = 0; index < edev->max_supported; index++)
> - kfree(edev->cables[index].attr_g.name);
> + kfree_strarray(edev->cable_names, edev->max_supported);
>
> if (edev->max_supported) {
> kfree(edev->extcon_dev_type.groups);
> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
> index 877c0860e300..5624f65ba17a 100644
> --- a/drivers/extcon/extcon.h
> +++ b/drivers/extcon/extcon.h
> @@ -58,6 +58,7 @@ struct extcon_dev {
> /* /sys/class/extcon/.../cable.n/... */
> struct device_type extcon_dev_type;
> struct extcon_cable *cables;
> + char **cable_names;
The extcon cable information should be included in struct extcon_cable
in order to gather information into one point like encapsulation.
I don't prefer to add 'cable_names'.
>
> /* /sys/class/extcon/.../mutually_exclusive/... */
> struct attribute_group attr_g_muex;
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 22. 23:39, Andy Shevchenko wrote:
> The use of atomic variable is still racy when we do not control which
> device has been unregistered and there is a (theoretical) possibility
> of the overflow that may cause a duplicate extcon device ID number
> to be allocated next time a device is registered.
>
> Replace above mentioned approach by using IDA framework.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.c | 15 ++++++++++++---
> drivers/extcon/extcon.h | 2 ++
> 2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 14e66e21597f..0d261ec6c473 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -16,6 +16,7 @@
>
> #include <linux/module.h>
> #include <linux/types.h>
> +#include <linux/idr.h>
> #include <linux/init.h>
> #include <linux/device.h>
> #include <linux/fs.h>
> @@ -238,6 +239,7 @@ struct extcon_cable {
>
> static struct class *extcon_class;
>
> +static DEFINE_IDA(extcon_dev_ids);
> static LIST_HEAD(extcon_dev_list);
> static DEFINE_MUTEX(extcon_dev_list_lock);
>
> @@ -1248,7 +1250,6 @@ static int extcon_alloc_groups(struct extcon_dev *edev)
> int extcon_dev_register(struct extcon_dev *edev)
> {
> int ret, index = 0;
> - static atomic_t edev_no = ATOMIC_INIT(-1);
>
> ret = create_extcon_class();
> if (ret < 0)
> @@ -1275,8 +1276,14 @@ int extcon_dev_register(struct extcon_dev *edev)
> dev_err(&edev->dev, "extcon device name is null\n");
> return -EINVAL;
> }
> - dev_set_name(&edev->dev, "extcon%lu",
> - (unsigned long)atomic_inc_return(&edev_no));
> +
> + ret = ida_simple_get(&extcon_dev_ids, 0, INT_MAX, GFP_KERNEL);
ida_simple_get and ida_simple_remove are deprecated on
commit 3264ceec8f17 ("lib/idr.c: document that ida_simple_{get,remove}()
are deprecated"). Instead of them, better to use ida_alloc and ida_free
according to the comments.
> + if (ret < 0)
> + return ret;
> +
> + edev->id = ret;
> +
> + dev_set_name(&edev->dev, "extcon%d", edev->id);
>
> ret = extcon_alloc_cables(edev);
> if (ret < 0)
> @@ -1368,6 +1375,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
> return;
> }
>
> + ida_simple_remove(&extcon_dev_ids, edev->id);
ditto.
> +
> device_unregister(&edev->dev);
>
> if (edev->mutually_exclusive && edev->max_supported) {
> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
> index 15616446140d..877c0860e300 100644
> --- a/drivers/extcon/extcon.h
> +++ b/drivers/extcon/extcon.h
> @@ -20,6 +20,7 @@
> * {0x3, 0x6, 0x5, 0}. If it is {0xFFFFFFFF, 0}, there
> * can be no simultaneous connections.
> * @dev: Device of this extcon.
> + * @id: Unique device ID of this extcon.
> * @state: Attach/detach state of this extcon. Do not provide at
> * register-time.
> * @nh_all: Notifier for the state change events for all supported
> @@ -46,6 +47,7 @@ struct extcon_dev {
>
> /* Internal data. Please do not set. */
> struct device dev;
> + int id;
> struct raw_notifier_head nh_all;
> struct raw_notifier_head *nh;
> struct list_head entry;
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 22. 23:40, Andy Shevchenko wrote:
> The device name is already printed with dev_err(), no need to repeat.
> The device pointer itself is not supposed to be an error point, drop
> that check.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 32e96cb49067..0e04ea185c26 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -1367,9 +1367,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
> list_del(&edev->entry);
> mutex_unlock(&extcon_dev_list_lock);
>
> - if (IS_ERR_OR_NULL(get_device(&edev->dev))) {
> - dev_err(&edev->dev, "Failed to unregister extcon_dev (%s)\n",
> - dev_name(&edev->dev));
> + if (!get_device(&edev->dev)) {
> + dev_err(&edev->dev, "Failed to unregister extcon_dev\n");
> return;
> }
>
Applied it. Thanks.
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 22. 23:40, Andy Shevchenko wrote:
> In one case the assignment is duplicative, in the other,
> it's better to move it into the loop — the user of it.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/extcon/extcon.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index b3f038639cd6..edfa0450e605 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -246,7 +246,7 @@ static DEFINE_MUTEX(extcon_dev_list_lock);
>
> static int check_mutually_exclusive(struct extcon_dev *edev, u32 new_state)
> {
> - int i = 0;
> + int i;
>
> if (!edev->mutually_exclusive)
> return 0;
> @@ -1244,7 +1244,7 @@ static int extcon_alloc_groups(struct extcon_dev *edev)
> */
> int extcon_dev_register(struct extcon_dev *edev)
> {
> - int ret, index = 0;
> + int ret, index;
>
> ret = create_extcon_class();
> if (ret < 0)
> @@ -1253,7 +1253,7 @@ int extcon_dev_register(struct extcon_dev *edev)
> if (!edev || !edev->supported_cable)
> return -EINVAL;
>
> - for (; edev->supported_cable[index] != EXTCON_NONE; index++);
> + for (index = 0; edev->supported_cable[index] != EXTCON_NONE; index++);
>
> edev->max_supported = index;
> if (index > SUPPORTED_CABLE_MAX) {
It has the dependency with patch7. When I try to apply it, the conflict happen.
When you are sending v2 patches, I'll apply it if there are no conflict.
[1] [PATCH v1 07/14] extcon: Use unique number for the extcon device ID
Acked-by: Chanwoo Choi <[email protected]>
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On Mon, Apr 03, 2023 at 10:58:20PM +0900, Chanwoo Choi wrote:
> Hi,
>
> I recommend that use the "./scripts/get_maintainer.pl" script
> to get the accurate maintainer list to send the patches.
That's what I'm using. What's wrong in your opinion with the Cc and/or To
lists?
> On 23. 3. 22. 23:39, Andy Shevchenko wrote:
> > A few fixes to the documentation and some cleanups against extcon core
> > module.
> >
> > Andy Shevchenko (14):
> > extcon: Fix kernel doc of property fields to avoid warnings
> > extcon: Fix kernel doc of property capability fields to avoid warnings
> > extcon: Use DECLARE_BITMAP() to declare bit arrays
> > extcon: use sysfs_emit() to instead of sprintf()
> > extcon: Amend kernel documentation of struct extcon_dev
> > extcon: Allow name to be assigned outside of the framework
> > extcon: Use unique number for the extcon device ID
> > extcon: Switch to use kasprintf_strarray()
> > extcon: Use device_match_of_node() helper
> > extcon: use dev_of_node(dev) instead of dev->of_node
> > extcon: Remove dup device name in the message and unneeded error check
> > extcon: Use sizeof(*pointer) instead of sizeof(type)
> > extcon: Drop unneeded assignments
> > extcon: Use positive conditional in ternary operator
--
With Best Regards,
Andy Shevchenko
On Mon, Apr 03, 2023 at 11:58:41PM +0900, Chanwoo Choi wrote:
> On 23. 3. 22. 23:39, Andy Shevchenko wrote:
> > Since we have a generic helper, switch the module to use it.
> > No functional change intended.
...
> > + edev->cable_names = kasprintf_strarray(GFP_KERNEL, "cable", edev->max_supported);
> > + if (!edev->cable_names) {
> > + kfree(edev->cables);
> > + return -ENOMEM;
> > + }
> > +
> > for (index = 0; index < edev->max_supported; index++) {
> > cable = &edev->cables[index];
> >
> > + str = edev->cable_names[index];
> > + strreplace(str, '-', '.');
> >
> > cable->edev = edev;
> > cable->cable_index = index;
...
> > /* /sys/class/extcon/.../cable.n/... */
> > struct device_type extcon_dev_type;
> > struct extcon_cable *cables;
> > + char **cable_names;
>
> The extcon cable information should be included in struct extcon_cable
> in order to gather information into one point like encapsulation.
>
> I don't prefer to add 'cable_names'.
I don't get this. The idea is to allocate the cable names in one call,
we have already API for that. The cable names are kept in the struct
extcon_cable as before. So, functionally it doesn't change anything,
it is a simple cleanup.
--
With Best Regards,
Andy Shevchenko
On Mon, Apr 03, 2023 at 11:38:41PM +0900, Chanwoo Choi wrote:
> On 23. 3. 22. 23:40, Andy Shevchenko wrote:
> > It's easier to parse by a human being the positive conditional.
...
> > const char *extcon_get_edev_name(struct extcon_dev *edev)
> > {
> > - return !edev ? NULL : edev->name;
> > + return edev ? edev->name : NULL;
> > }
> > EXPORT_SYMBOL_GPL(extcon_get_edev_name);
>
> It is not fix-up patch and I'm not sure that it is beneficial patch.
> I will not apply it.
It improves the cognitive perception of the code, but I'm fine with
no patch being applied.
--
With Best Regards,
Andy Shevchenko
On Mon, Apr 03, 2023 at 11:52:46PM +0900, Chanwoo Choi wrote:
> On 23. 3. 22. 23:39, Andy Shevchenko wrote:
...
> > + ret = ida_simple_get(&extcon_dev_ids, 0, INT_MAX, GFP_KERNEL);
>
>
> ida_simple_get and ida_simple_remove are deprecated on
> commit 3264ceec8f17 ("lib/idr.c: document that ida_simple_{get,remove}()
> are deprecated"). Instead of them, better to use ida_alloc and ida_free
> according to the comments.
Done for v2.
...
> > + ida_simple_remove(&extcon_dev_ids, edev->id);
>
> ditto.
Ditto.
--
With Best Regards,
Andy Shevchenko
On Wed, Apr 05, 2023 at 11:16:36AM +0300, Andy Shevchenko wrote:
> On Mon, Apr 03, 2023 at 11:58:41PM +0900, Chanwoo Choi wrote:
> > On 23. 3. 22. 23:39, Andy Shevchenko wrote:
> > > Since we have a generic helper, switch the module to use it.
> > > No functional change intended.
...
> > > + edev->cable_names = kasprintf_strarray(GFP_KERNEL, "cable", edev->max_supported);
> > > + if (!edev->cable_names) {
> > > + kfree(edev->cables);
> > > + return -ENOMEM;
> > > + }
> > > +
> > > for (index = 0; index < edev->max_supported; index++) {
> > > cable = &edev->cables[index];
> > >
>
> > > + str = edev->cable_names[index];
> > > + strreplace(str, '-', '.');
> > >
> > > cable->edev = edev;
> > > cable->cable_index = index;
>
> ...
>
> > > /* /sys/class/extcon/.../cable.n/... */
> > > struct device_type extcon_dev_type;
> > > struct extcon_cable *cables;
> > > + char **cable_names;
> >
> > The extcon cable information should be included in struct extcon_cable
> > in order to gather information into one point like encapsulation.
> >
> > I don't prefer to add 'cable_names'.
>
> I don't get this. The idea is to allocate the cable names in one call,
> we have already API for that. The cable names are kept in the struct
> extcon_cable as before. So, functionally it doesn't change anything,
> it is a simple cleanup.
Okay, I see now your point. I can redo this a bit better I think.
--
With Best Regards,
Andy Shevchenko
Hi,
On 23. 3. 31. 09:48, Bumwoo Lee wrote:
> Hi Andy Shevchenko
>> -----Original Message-----
>> From: Andy Shevchenko <[email protected]>
>> Sent: Thursday, March 30, 2023 7:12 PM
>> To: Bumwoo Lee <[email protected]>; [email protected]
>> Cc: MyungJoo Ham <[email protected]>; Chanwoo Choi
>> <[email protected]>
>> Subject: Re: [PATCH v1 00/14] extcon: Core cleanups and documentation
>> fixes
>>
>> On Wed, Mar 22, 2023 at 04:39:51PM +0200, Andy Shevchenko wrote:
>>> A few fixes to the documentation and some cleanups against extcon core
>>> module.
>>
>> Anything I should do with the series?
>> Any comments on it?
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>>
>
> Looks fine to me.
>
> Acked-by: Bumwoo Lee <[email protected]>
>
> MR. Chanwoo, Would you please take a look at this patch series.
>
Actually, Acked tag will be replied by Maintainer or the driver owner.
If you want to review the mailing list patch, I think that Reviewed-by tag is proper.
Unfortunately, I could not see the any review comment from you even if this patchset
have the some review contents. Also you didn't review the any patches of extcon before.
I'm always welcome for many reviewers in order to improve the linux kernel.
But, in this case, I'm not sure that you are reviewing this patchset.
So that I'm sorry that I cannot take your acked-tag.
--
Best Regards,
Samsung Electronics
Chanwoo Choi
Hi
> -----Original Message-----
> From: Chanwoo Choi <[email protected]>
> Sent: Thursday, April 6, 2023 8:17 AM
> To: Bumwoo Lee <[email protected]>; 'Andy Shevchenko'
> <[email protected]>; [email protected]
> Cc: 'MyungJoo Ham' <[email protected]>; 'Chanwoo Choi'
> <[email protected]>
> Subject: Re: [PATCH v1 00/14] extcon: Core cleanups and documentation
> fixes
>
> Hi,
>
> On 23. 3. 31. 09:48, Bumwoo Lee wrote:
> > Hi Andy Shevchenko
> >> -----Original Message-----
> >> From: Andy Shevchenko <[email protected]>
> >> Sent: Thursday, March 30, 2023 7:12 PM
> >> To: Bumwoo Lee <[email protected]>; [email protected]
> >> Cc: MyungJoo Ham <[email protected]>; Chanwoo Choi
> >> <[email protected]>
> >> Subject: Re: [PATCH v1 00/14] extcon: Core cleanups and documentation
> >> fixes
> >>
> >> On Wed, Mar 22, 2023 at 04:39:51PM +0200, Andy Shevchenko wrote:
> >>> A few fixes to the documentation and some cleanups against extcon
> >>> core module.
> >>
> >> Anything I should do with the series?
> >> Any comments on it?
> >>
> >> --
> >> With Best Regards,
> >> Andy Shevchenko
> >>
> >
> > Looks fine to me.
> >
> > Acked-by: Bumwoo Lee <[email protected]>
> >
> > MR. Chanwoo, Would you please take a look at this patch series.
> >
>
> Actually, Acked tag will be replied by Maintainer or the driver owner.
> If you want to review the mailing list patch, I think that Reviewed-by tag
> is proper.
>
OK.
Thank you for your detail guidance. I will follow this guidance on next time.
> Unfortunately, I could not see the any review comment from you even if
> this patchset have the some review contents. Also you didn't review the
> any patches of extcon before.
>
> I'm always welcome for many reviewers in order to improve the linux kernel.
> But, in this case, I'm not sure that you are reviewing this patchset.
>
> So that I'm sorry that I cannot take your acked-tag.
>
I agree your comment.
Thank you~
> --
> Best Regards,
> Samsung Electronics
> Chanwoo Choi
On Thu, Apr 06, 2023 at 08:17:19AM +0900, Chanwoo Choi wrote:
> On 23. 3. 31. 09:48, Bumwoo Lee wrote:
...
> > Looks fine to me.
> >
> > Acked-by: Bumwoo Lee <[email protected]>
> >
> > MR. Chanwoo, Would you please take a look at this patch series.
>
> Actually, Acked tag will be replied by Maintainer or the driver owner. If
> you want to review the mailing list patch, I think that Reviewed-by tag is
> proper.
>
> Unfortunately, I could not see the any review comment from you even if this
> patchset have the some review contents. Also you didn't review the any
> patches of extcon before.
>
> I'm always welcome for many reviewers in order to improve the linux kernel.
> But, in this case, I'm not sure that you are reviewing this patchset.
>
> So that I'm sorry that I cannot take your acked-tag.
I probably need to update this in the v3 of the rest of the patches I have sent
as v2. Btw, can you review those before, so if any comments I can address in
v3?
--
With Best Regards,
Andy Shevchenko