This was discussed on linux kernel in June and then went silent for
some reason, or I'm missing a search term.
> Basically, this driver will read x, y, and z coordinate registers
> from the device and report the values to users through sysfs
> interface.
The normal behaviour would be just to report them via the input layer
not also have the sysfs node.
> hg_int: generate an interrupt when the high-g threshold criteria are
> met 0: disable (default)
Do these need to be configurable this way - I don't see from the code
what is done with them and there isn't any way to make use of the
interrupt from user space I can see ?
> power_mode: indicate the current power mode
> 0: low power mode (default)
> 1: normal power mode
Ideally this should be done automatically via the power management
support.
> temperature: print the value of temperature register
This one I suspect belongs as a separate hwmon device - ie you'd
register an input device and an hwmon device for this actual driver ?
That is a detail to look at last I think.
> +static int bma023_write_reg(struct i2c_client *client, u8 reg, u8
> val) +{
> + int ret;
> + /*
> + * According to the datasheet, the interrupt should be
> deactivated
> + * on the host side when write sequences operate
> + */
> + disable_irq_nosync(client->irq);
> + ret = i2c_smbus_write_byte_data(client, reg, val);
> + enable_irq(client->irq);
> + if (ret < 0)
> + dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err
> %d\n",
> + __func__, reg, val, ret);
> + return ret;
> +}
I am curious why it says this. If it is trying to guide driver writing
then you probably just want to use suitable locking in an OS like
Linux. I couldn't find the data sheet to check at a quick glance.
> +static int bma023_set_reg_bits(struct i2c_client *client,
> + int val, int shift, u8 mask,
> u8 reg) +{
> + u8 data = bma023_read_reg(client, reg);
> +
> + data = (data & ~mask) | ((val << shift) & mask);
> + return bma023_write_reg(client, reg, data);
> +}
What happens if two of these get done at once off different sysfs nodes
- I see no locking ?
> + struct bma023_sensor *sensor = dev_get_drvdata(dev);
> + if(bma023_get_power_mode(sensor->client) == 0){
> + bma023_set_power_mode(sensor->client, 1);
> + msleep(4); /* wait for accel chip resume */
> + }
What guarantees I don't then turn it off again during this gap ?
> +#define BMA023_SYSFS(name) \
> +static ssize_t bma023_show_##name(struct device *dev, \
> + struct device_attribute *att, char *buf) \
> +{ \
> + struct bma023_sensor *sensor = dev_get_drvdata(dev); \
> + return sprintf(buf, "%d\n", sensor->name); \
> +} \
We strongly discourage this sort of preprocessor magic - it makes code
harder to understand and often much bigger.
> +static ssize_t bma023_store_##name(struct device *dev, \
> + struct device_attribute *attr, const char *buf,
> size_t count) \ +{ \
> + struct bma023_sensor *sensor = dev_get_drvdata(dev); \
> + unsigned long val; \
> + int ret; \
> + u8 result; \
> + if(!count) \
> + return count;\
> + ret = strict_strtoul(buf, 10, &val); \
> + if (!ret) { \
> + bma023_set_##name(sensor->client, val); \
> + result = bma023_get_##name(sensor->client); \
Again what if two are running at once - we set, set, get, get .. oops
> + mutex_lock(&sensor->lock); \
> + sensor->name = result; \
> + mutex_unlock(&sensor->lock); \
> + return count; \
> + } \
> + else \
> + return ret; \
Probably more acceptable if this was a function that took a table entry
or similar, and a table for the rest ?
> +static irqreturn_t bma023_irq(int irq, void *dev_id)
> +{
> + struct bma023_sensor *sensor = dev_id;
> + if (!work_pending(&sensor->work)) {
> + disable_irq_nosync(irq);
> + schedule_work(&sensor->work);
> + }
> + return IRQ_HANDLED;
This should nowdays be a threaded IRQ I think - which will also clean
up a bit
> +#ifdef CONFIG_PM_RUNTIME
> +static int bma023_runtime_suspend(struct device *dev)
> +{
> + struct bma023_sensor *sensor = dev_get_drvdata(dev);
> + bma023_set_power_mode(sensor->client, 1);
> + return 0;
> +}
> +
> +static int bma023_runtime_resume(struct device *dev)
> +{
> + struct bma023_sensor *sensor = dev_get_drvdata(dev);
> + bma023_set_power_mode(sensor->client, 0);
> + return 0;
> +}
> +
> +static const struct dev_pm_ops bma023_pm = {
> + .runtime_suspend = bma023_runtime_suspend,
> + .runtime_resume = bma023_runtime_resume,
> +};
> +#endif
This probably the best way to do the power handling - it refcounts and
with the right get/put calls you can then avoid problems with user
configured on/off races.
Bit of cleaning up and it looks like it can be made suitable for
submission to the Linus kernel tree.
Alan
_______________________________________________
MeeGo-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
