| 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
| ||
|
Date: Fri, 09 Sep 2011 14:06:37 +0900
From: Donggeun Kim <dg77.kim@...sung.com>
To: Jonathan Cameron <jic23@....ac.uk>
Cc: linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
akpm@...ux-foundation.org, gregkh@...e.de,
kyungmin.park@...sung.com
Subject: Re: [PATCH] misc: Add driver for GP2AP002 proximity/ambient light
sensor
On 2011년 09월 08일 23:25, Jonathan Cameron wrote:
>> +This chip only exports current as the result of ambient light sensor.
>> +To get illuminance, CPU measures the current exported
>> +from the sensor through ADC.
>> +The relationship between current and illuminance is as follows:
>> + illuminance = 10^(current/10) - (1)
>> +This driver only exposes the measured current.
>> +Illuminance should be calculated at the user space by (1) formula.
>> +
>> +Sysfs Interface
>> +---------------
>> +prox0_input proximity sensor result
>> + 0: object is not detected
>> + 1: object is detected
>> + RO
>> +
>> +adc0_input ADC result for ambient light sensor
>> + current [unit: uA]
>> + RO
> My issue here is generality. To use this value userspace needs to
> have the conversion function. That means a large library of
> conversion functions for every sensor out there. Does such a thing
> exist? For sensors like this we have typically done the maths
> in kernel simply to avoid the issue (be it somewhat hideous!)
>
> Also it's really not a general adc so that name is missleading.
> At very least call it curr0_input to match (more or less hwmon).
>
I don't have good idea to handle this.
A current-illuminance mapping table can be used.
But I guess the illuminance may be the approximate value when handling
in kernel by using current-illuminance mapping table.
>> +static void gp2ap002_get_proximity(struct gp2ap002_chip *chip)
>> +{
>> + struct gp2ap002_platform_data *pdata = chip->pdata;
>> + int prox_mode;
>> +
>> + prox_mode = pdata->prox_mode << OPMOD_VCON_SHIFT;
>> + /* interrupt output mode */
>> + if ((prox_mode & OPMOD_VCON_MASK) == OPMOD_VCON_IRQ) {
>> + /* Determine whether the object is detected
>> + by reading proximity output register */
>> + chip->proximity = i2c_smbus_read_byte_data(chip->client,
>> + GP2AP002_PROX) & PROX_VO_DETECT;
>> + } else { /* normal output mode */
>> + /* vo_gpio is changed from high to low
>> + when the object is detected */
>> + chip->proximity = !gpio_get_value(pdata->vout_gpio);
>> + }
> Silly question but is the value really not available in that register
> if we aren't in interrupt mode? Seems 'odd', but then it's hardware so
> what the heck. If it is I'd just always read that register so as to
> have cleaner code (at the cost of a small bus transaction).
I overlooked that gpio value and register value can be read for
proximity sensing results. So, this should be changed to access the
register.
>> +}
>> +
>> +static int gp2ap002_chip_enable(struct gp2ap002_chip *chip, bool enable)
>> +{
>> + struct gp2ap002_platform_data *pdata = chip->pdata;
>> + int ret;
>> + u8 value;
>> +
>> + if (enable == chip->enabled)
>> + return 0;
>> +
>> + value = (pdata->analog_sleep << OPMOD_ASD_SHIFT) |
>> + (pdata->prox_mode << OPMOD_VCON_SHIFT);
>> + /* software shutdown mode */
>> + if (enable)
>> + value |= OPMOD_SSD_OPERATING;
>> + else /* operating mode */
>> + value |= OPMOD_SSD_SHUTDOWN;
>> +
>> + ret = i2c_smbus_write_byte_data(chip->client, GP2AP002_OPMOD, value);
>> + if (ret < 0)
>> + goto out;
>> +
>> + chip->enabled = enable;
>> +out:
>> + return ret;
>> +}
>> +
>> +static void gp2ap002_work(struct work_struct *work)
>> +{
>> + struct gp2ap002_chip *chip = container_of(work,
>> + struct gp2ap002_chip, work);
>> +
>> + mutex_lock(&chip->lock);
>> + gp2ap002_get_proximity(chip);
>> +
>> + kobject_uevent(&chip->client->dev.kobj, KOBJ_CHANGE);
> as you can poll on sysfs files, would it not be cleaner to use
> that interface to tell userspace there are new values?
I will add sysfs_notify function for sysfs files.
>> + if (client->irq > 0) {
>> + unsigned long irq_flag = IRQF_DISABLED;
>> +
> Could do with some explanation. I have no idea what VCON is!
I don't exactly know what VCON stands for. I just followed the name in
datasheet.
This chip can be set to operate interrupt mode or normal mode.
The macros having OPMOD_VCON_ prefix is related to interrupt or normal mode.
But the following code should be changed to only set flags as
IRQF_TRIGGER_FALLING.
>> + if (chip->mode == OPMOD_VCON_IRQ)
>> + irq_flag |= IRQF_TRIGGER_FALLING;
>> + else
>> + irq_flag |= IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING;
>> +
> WHy not threaded?
I should have used threaded function. I will change it.
> None of these needs to be int as far as I can see so
> you might as well make them u8s/u16s or bitfields of
> the right size.
>> + int led_mode;
>> + int hysd;
>> + int hysc;
>> + int hysf;
>> + int cycle;
>> + int oscillator;
>> + int analog_sleep;
>> + int prox_mode;
>> + int vout_control;
>> +
>> + bool chip_enable;
>> +};
>> +
>> +#endif
>
It would be better to change the data type.
Thank you for your review.
--
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/
Powered by blists - more mailing lists