linux-kernel - Re: [PATCH v2 1/1] input: add driver for Bosch Sensortec's BMA150 accelerometer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

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

Message-ID: <20110623163631.2c43671e@lxorguk.ukuu.org.uk>
Date:	Thu, 23 Jun 2011 16:36:31 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Eric Andersson <eric.andersson@...xphere.com>
Cc:	dmitry.torokhov@...il.com, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org, zhengguang.guo@...ch-sensortec.com,
	stefan.nilsson@...xphere.com,
	Albert Zhang <xu.zhang@...ch-sensortec.com>
Subject: Re: [PATCH v2 1/1] input: add driver for Bosch Sensortec's BMA150
 accelerometer

> +Description:	This is used to select the full scale acceleration range. The
> +		values represent the range given as +/- G.
> +		Possible values are: 2, 4, 8.
> +
> +		Reading: returns the current acceleration range.
> +
> +		Writing: sets a new acceleration range.

As there is no way to know the valid values I suspect it ought to pick
nearest inclusive for others <=8 ?

Similarly for the others

> +			normal - Sets the sensor in full running mode.
> +			sleep  - Sets the sensor in deep sleep.
> +			wakeup - Sets the sensor to low-power mode using
> +				 sequential sleep period.
> +
> +		Reading: returns the current operational mode.
> +
> +		Writing: sets a new operational mode.

We have runtime_pm for this (and in fact to switch from the bma023 driver
I posted to yours we'd need runtime pm)

> +
> +
> +What:		/sys/bus/i2c/devices/<busnum>-<devaddr>/value
> +Date:		May 2011
> +Contact:	Eric Andersson <eric.andersson@...xphere.com>
> +Description:	This is used to get the current acceleration values for each
> +		axis. The values are represented as (x,y,z), where each axis can
> +		hold a value between -512 and 511.
> +
> +		Reading: returns the current acceleration values.

This was nacked in the bma023 driver by the IIO folks - and Dmitry
pointed out you can do this without a sysfs hack. The trick is to do an
initial poll in input_open at which point the ioctl query for the
position will have data that is current and open/ioctl/close works and
providing we go nail that into other drivers that need that kind of use
the API is generic and input event based.

> +
> +
> +What:		/sys/bus/i2c/devices/<busnum>-<devaddr>/delay
> +Date:		May 2011
> +Contact:	Eric Andersson <eric.andersson@...xphere.com>
> +Description:	This is used to select the polling rate of the driver. The
> +		value is represented in ms and can be between 0 and 200. Any

0-200 whats ?


> +What:		/sys/bus/i2c/devices/<busnum>-<devaddr>/enable
> +Date:		May 2011
> +Contact:	Eric Andersson <eric.andersson@...xphere.com>
> +Description:	This is used to enable and disable the chip. The chip will only
> +		be disabled if there are no input device users.

How does this differ from the power one and why is it needed, why doesn't
it disable when not in use ? How does this interact with runtime pm


>
> +static int bma150_set_bandwidth(struct i2c_client *client, unsigned char bw)
> +{
> +	s32 ret;
> +	unsigned char data;
> +	struct bma150_data *bma150 = i2c_get_clientdata(client);
> +
> +	mutex_lock(&bma150->mutex);
> +	ret = i2c_smbus_read_byte_data(client, BMA150_BANDWIDTH_REG);
> +	if (ret < 0)
> +		goto error;
> +
> +	data = (ret & ~BMA150_BANDWIDTH_MSK) |
> +		((bw << BMA150_BANDWIDTH_POS) & BMA150_BANDWIDTH_MSK);
> +
> +	ret = i2c_smbus_write_byte_data(client, BMA150_BANDWIDTH_REG, data);
> +error:
> +	mutex_unlock(&bma150->mutex);
> +	return ret;
> +}

A lot of remarkably similar functions


> +	mutex_lock(&bma150->mutex);
> +	bma150->x = x;
> +	bma150->y = y;
> +	bma150->z = z;
> +	mutex_unlock(&bma150->mutex);

This locking would go away if you dropped the un-needed sysfs node

>
> +static int bma150_open(struct input_dev *inputdev)
> +{
> +	struct bma150_data *dev = input_get_drvdata(inputdev);
> +
> +	if (!dev->sysfs_enable)
> +		return bma150_start_polling(inputdev);
> +
> +	return 0;
> +}

Most bma023 users will be IRQ based, so this driver would need chunks
extracting from the other bma023 driver submission for that to be handled.

> +
> +static void bma150_close(struct input_dev *inputdev)
> +{
> +	struct bma150_data *dev = input_get_drvdata(inputdev);
> +
> +	if (!dev->sysfs_enable)
> +		bma150_stop_polling(inputdev);

What locks sysfs_enable ?


>From the Intel side to use your driver instead of bma023 we'd need IRQ
support and runtime pm but otherwise it looks basically ok. I really
don't see how to reconcile proper power management with all your sysfs
enables and power bits though ?



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/