[PATCH v2] Input: ads7846: Convert to hwmon_device_register_with_groups

[PATCH v2] Input: ads7846: Convert to hwmon_device_register_with_groups

[Guenter Roeck]

Simplify the code and create mandatory 'name' attribute by using
new hwmon API.

Also use is_visible to determine visible attributes instead of creating
several different attribute groups.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Use hwmon_device_register_with_groups instead of
    devm_hwmon_device_register_with_groups to keep
    device removal aligned with resource removal.

 drivers/input/touchscreen/ads7846.c |   81 +++++++++++------------------------
 1 file changed, 25 insertions(+), 56 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index ea19536..932d4cf 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -102,7 +102,6 @@ struct ads7846 {
 	struct regulator	*reg;
 
 #if defined(CONFIG_HWMON) || defined(CONFIG_HWMON_MODULE)
-	struct attribute_group	*attr_group;
 	struct device		*hwmon;
 #endif
 
@@ -479,42 +478,36 @@ static inline unsigned vbatt_adjust(struct ads7846 *ts, ssize_t v)
 SHOW(in0_input, vaux, vaux_adjust)
 SHOW(in1_input, vbatt, vbatt_adjust)
 
-static struct attribute *ads7846_attributes[] = {
-	&dev_attr_temp0.attr,
-	&dev_attr_temp1.attr,
-	&dev_attr_in0_input.attr,
-	&dev_attr_in1_input.attr,
-	NULL,
-};
-
-static struct attribute_group ads7846_attr_group = {
-	.attrs = ads7846_attributes,
-};
+static umode_t ads7846_is_visible(struct kobject *kobj, struct attribute *attr,
+				  int index)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct ads7846 *ts = dev_get_drvdata(dev);
 
-static struct attribute *ads7843_attributes[] = {
-	&dev_attr_in0_input.attr,
-	&dev_attr_in1_input.attr,
-	NULL,
-};
+	if (ts->model == 7843 && index < 2)	/* in0, in1 */
+		return 0;
+	if (ts->model == 7845 && index != 2)	/* in0 */
+		return 0;
 
-static struct attribute_group ads7843_attr_group = {
-	.attrs = ads7843_attributes,
-};
+	return attr->mode;
+}
 
-static struct attribute *ads7845_attributes[] = {
-	&dev_attr_in0_input.attr,
+static struct attribute *ads7846_attributes[] = {
+	&dev_attr_temp0.attr,		/* 0 */
+	&dev_attr_temp1.attr,		/* 1 */
+	&dev_attr_in0_input.attr,	/* 2 */
+	&dev_attr_in1_input.attr,	/* 3 */
 	NULL,
 };
 
-static struct attribute_group ads7845_attr_group = {
-	.attrs = ads7845_attributes,
+static struct attribute_group ads7846_attr_group = {
+	.attrs = ads7846_attributes,
+	.is_visible = ads7846_is_visible,
 };
+__ATTRIBUTE_GROUPS(ads7846_attr);
 
 static int ads784x_hwmon_register(struct spi_device *spi, struct ads7846 *ts)
 {
-	struct device *hwmon;
-	int err;
-
 	/* hwmon sensors need a reference voltage */
 	switch (ts->model) {
 	case 7846:
@@ -535,43 +528,19 @@ static int ads784x_hwmon_register(struct spi_device *spi, struct ads7846 *ts)
 		break;
 	}
 
-	/* different chips have different sensor groups */
-	switch (ts->model) {
-	case 7846:
-		ts->attr_group = &ads7846_attr_group;
-		break;
-	case 7845:
-		ts->attr_group = &ads7845_attr_group;
-		break;
-	case 7843:
-		ts->attr_group = &ads7843_attr_group;
-		break;
-	default:
-		dev_dbg(&spi->dev, "ADS%d not recognized\n", ts->model);
-		return 0;
-	}
-
-	err = sysfs_create_group(&spi->dev.kobj, ts->attr_group);
-	if (err)
-		return err;
-
-	hwmon = hwmon_device_register(&spi->dev);
-	if (IS_ERR(hwmon)) {
-		sysfs_remove_group(&spi->dev.kobj, ts->attr_group);
-		return PTR_ERR(hwmon);
-	}
+	ts->hwmon = hwmon_device_register_with_groups(&spi->dev, spi->modalias,
+						      ts, ads7846_attr_groups);
+	if (IS_ERR(ts->hwmon))
+		return PTR_ERR(ts->hwmon);
 
-	ts->hwmon = hwmon;
 	return 0;
 }
 
 static void ads784x_hwmon_unregister(struct spi_device *spi,
 				     struct ads7846 *ts)
 {
-	if (ts->hwmon) {
-		sysfs_remove_group(&spi->dev.kobj, ts->attr_group);
+	if (ts->hwmon)
 		hwmon_device_unregister(ts->hwmon);
-	}
 }
 
 #else
-- 
1.7.9.7

Re: [PATCH v2] Input: ads7846: Convert to hwmon_device_register_with_groups

[Dmitry Torokhov]

Hi Guenter,

On Mon, Nov 25, 2013 at 08:39:04PM -0800, Guenter Roeck wrote:
> Simplify the code and create mandatory 'name' attribute by using
> new hwmon API.

So this moves hwmon attributes from the parent i2c device to the hwmon
device, right? Would not that break userspace which expects to find the
attributes where they were?

Thanks.

-- 
Dmitry

Re: [lm-sensors] [PATCH v2] Input: ads7846: Convert to hwmon_device_register_with_groups

[Jean Delvare]

Hi Dmitry,

On Tue, 26 Nov 2013 10:58:46 -0800, Dmitry Torokhov wrote:
> On Mon, Nov 25, 2013 at 08:39:04PM -0800, Guenter Roeck wrote:
> > Simplify the code and create mandatory 'name' attribute by using
> > new hwmon API.
> 
> So this moves hwmon attributes from the parent i2c device to the hwmon
> device, right? Would not that break userspace which expects to find the
> attributes where they were?

libsensors supports that just fine since version 3.0.3 which was
released in September 2008, over 5 years ago.

Applications bypassing libsensors are on their own, and always have
been. But several other drivers have had their attributes in the hwmon
device forever so even these applications are likely to be happy with
other drivers moving in that direction.

-- 
Jean Delvare

Re: [PATCH v2] Input: ads7846: Convert to hwmon_device_register_with_groups

[Guenter Roeck]

On 11/26/2013 10:58 AM, Dmitry Torokhov wrote:
> Hi Guenter,
>
> On Mon, Nov 25, 2013 at 08:39:04PM -0800, Guenter Roeck wrote:
>> Simplify the code and create mandatory 'name' attribute by using
>> new hwmon API.
>
> So this moves hwmon attributes from the parent i2c device to the hwmon
> device, right? Would not that break userspace which expects to find the
> attributes where they were?
>

In addition to Jean's earlier comments ... s/i2c/spi/, I assume. spi devices
don't create the mandatory name attribute automatically, which means
that the created hwmon device was not recognized by standard user space
applications (eg the sensors command or anything else using libsensors)
in the first place. Which in turn means that only applications which don't
support the standard hwmon ABI - if there are any - would be affected.
What we are more concerned about is to make sure that applications
which _do_ follow the hwmon ABI are working.

Thanks,
Guenter

Re: [PATCH v2] Input: ads7846: Convert to hwmon_device_register_with_groups

[Dmitry Torokhov]

On Tue, Nov 26, 2013 at 05:20:13PM -0800, Guenter Roeck wrote:
> On 11/26/2013 10:58 AM, Dmitry Torokhov wrote:
> >Hi Guenter,
> >
> >On Mon, Nov 25, 2013 at 08:39:04PM -0800, Guenter Roeck wrote:
> >>Simplify the code and create mandatory 'name' attribute by using
> >>new hwmon API.
> >
> >So this moves hwmon attributes from the parent i2c device to the hwmon
> >device, right? Would not that break userspace which expects to find the
> >attributes where they were?
> >
> 
> In addition to Jean's earlier comments ... s/i2c/spi/, I assume. spi devices
> don't create the mandatory name attribute automatically, which means
> that the created hwmon device was not recognized by standard user space
> applications (eg the sensors command or anything else using libsensors)
> in the first place. Which in turn means that only applications which don't
> support the standard hwmon ABI - if there are any - would be affected.
> What we are more concerned about is to make sure that applications
> which _do_ follow the hwmon ABI are working.

OK, fair enough, I'll apply this then.

-- 
Dmitry