Hi Donggeun,
...
>> If they were auxiliary adcs (measuring some pin input) they would be
>> in0_raw and in1_raw. Here I believe they are providing raw access to
>> the readings on two different light sensors? Can we have names that
>> give us some clue of the range that each of these sensors cover?
> Because overall structure is similar to tsl2563,
> the channel 0 is resposive to both visible and infrared light and channel 1 is responsive to infraread light.
> I will change adc0 and adc1 into intensity_both_raw and intensity_ir_raw likewise in tsl2563.c file, respectively
>>> + &dev_attr_adc0.attr,
>>> + &dev_attr_adc1.attr,
>>> + &dev_attr_illuminance0_input.attr,
>>> + &dev_attr_power_on.attr,
>>
>> Please provide details on each of the next three attributes. They are
>> non standard and so need to be documented.
> The device have bitfields for each proximity and light sensor.
> The proximity_en and light_en attributes are related to the corresponding bit fields in the register.
> And the device is in the sleep mode, after a power-on-reset.
> As soon as the power_on attribute is enabled, the device will move to the start state.
> It will then continue through proximity sensing state, wait state, light sensing state, and start state.
> This procedure will be continuing if the device does not enter into sleep mode.
> When wait_en attribute is disabled, the device jumps the wait state.
That makes sense. Please add a comment to the code somewhere to explain this.
>>> + &dev_attr_wait_en.attr,
>>> + &dev_attr_proximity_en.attr,
>>> + &dev_attr_light_en.attr,
>>> + &iio_const_attr_name.dev_attr.attr,
>>> + NULL
>>> +};
...
>>
>> As this isn't done yet, can I confirm what exactly we have here?
>> I would propose updating to the new abi when it is confirmed
>> as a separate patch (so as not to delay your driver).
>>> +static struct attribute *tmd2771x_event_attributes[] = {
>>> + &iio_event_attr_proximity_mag_either_rising_en.dev_attr.attr,
>>> + &dev_attr_proximity_mag_pos_rising_value.attr,
>>> + &dev_attr_proximity_mag_neg_rising_value.attr,
>> So we have a single enable for both directions. As this isn't
>> signed, I guess we can put a threshold on the value falling and
>> one on it rising? So if our current is say 10, we can get an
>> event if it falls below 8 or rises above 20?
>> I think under the previous scheme this should have been
>> mag_pos_rising and mag_pos_falling (neg implies a theshold on the
>> actual negative value). Is there any way of enabling only
>> one of these at a time? Under the new scheme these will
>> be (if it doesn't change) proxmity_thresh_en,
>> proximity_thresh_rising_value and proximity_thresh_falling_value.
>>
> Actually, every raw data is positive number. I made some mistakes
> when naming the event attributes.
> The threshold values allows the user to define thresholds above and below a desired
> channel0 of light sensor and proximity level.
> An interrupt can be generated when the raw data exceeds the upper threshold value or
> falls below the lower threshold value.
> I'm not the proper attribute name, but I want to change it as belows:
> proximity_mag_either_rising_en -> proximity_thresh_en
> proximity_mag_pos_rising_value -> proximity_thresh_high_value
> proximity_mag_neg_rising_value -> proximity_thresh_low_value
Rising and falling rather than low and high as we care about the
direction of change causing the interrupt. Again, don't worry too
much about these, now I know what they are I can sort them out along
with all other drivers when we have agreed what the final interface will be.
> proximity_mag_either_rising_period -> proximity_thresh_period
> light_mag_either_rising_en -> intensity_both_thresh_en
> light_mag_pos_rising_value -> intensity_both_thresh_high_value
> light_mag_neg_rising_value -> intensity_both_thresh_low_value
> light_mag_either_rising_period -> intensity_both_thresh_period
>>> + &dev_attr_proximity_mag_either_rising_period.attr,
>>> + &iio_event_attr_light_mag_either_rising_en.dev_attr.attr,
>>> + &dev_attr_light_mag_pos_rising_value.attr,
>>> + &dev_attr_light_mag_neg_rising_value.attr,
>>> + &dev_attr_light_mag_either_rising_period.attr,
>>> +#define TMD2771X_POWERUP_WAIT_TIME 12
>>> +
>>
>> Please document this structure. Kernel doc format ideally.
> Sorry. I'm not familiar with kernel doc format.
> Where sholud I locate it after making a document file?
Stick (with all elements documented...) the following in here.
/**
* struct tmd2771x_platform_data - device instance specific data
* @control_power_source: function that controls power to the device
* @power_on: cache of poweron elements of TMD2771X_DEFAULT_COMMAND register
* ...
* @glass_attenuation: implementatation specific attenuation factor
**/
>>> +struct tmd2771x_platform_data {
>>> + void (*control_power_source)(int);
>>> +
>>> + u8 power_on; /* TMD2771X_PON */
>>> + u8 wait_enable; /* TMD2771X_WEN */
>>> + u8 wait_long; /* TMD2771X_WLONG */
>>> + u8 wait_time; /* 0x00 ~ 0xff */
>>> +
>>> + /* Proximity */
>>> + u8 ps_enable; /* TMD2771X_PEN */
>>> + u16 ps_interrupt_h_thres; /* 0x0000 ~ 0xffff */
>>> + u16 ps_interrupt_l_thres; /* 0x0000 ~ 0xffff */
>>> + u8 ps_interrupt_enable; /* TMD2771X_PIEN */
>>> + u8 ps_time; /* 0x00 ~ 0xff */
>>> + u8 ps_interrupt_persistence; /* 0x00 ~ 0xf0 (top four bits) */
>>> + u8 ps_pulse_count; /* 0x00 ~ 0xff */
>>> + u8 ps_drive_strength; /* TMD2771X_PDRIVE_12MA ~
>>> + TMD2771X_PDRIVE_100MA */
>>> + u8 ps_diode; /* TMD2771X_PDIODE_CH0_DIODE ~
>>> + TMD2771X_PDIODE_BOTH_DIODE */
>>> +
>>> + /* Ambient Light */
>>> + u8 als_enable; /* TMD2771X_AEN */
>>> + u16 als_interrupt_h_thres; /* 0x0000 ~ 0xffff */
>>> + u16 als_interrupt_l_thres; /* 0x0000 ~ 0xffff */
>>> + u8 als_interrupt_enable; /* TMD2771X_AIEN */
>>> + u8 als_time; /* 0x00 ~ 0xff */
>>> + u8 als_interrupt_persistence; /* 0x00 ~ 0x0f (bottom four bits) */
>>> + u8 als_gain; /* TMD2771X_AGAIN_1X ~
>>> + TMD2771X_AGAIN_120X */
>>> + u8 glass_attenuation;
>>> +};
>>> +
>>> +#endif
>>> diff --git a/drivers/staging/iio/sysfs.h b/drivers/staging/iio/sysfs.h
>>> index 6083416..c74eb83 100644
>>> --- a/drivers/staging/iio/sysfs.h
>>> +++ b/drivers/staging/iio/sysfs.h
>>> @@ -330,6 +330,7 @@ struct iio_const_attr {
>>> #define IIO_EVENT_CODE_ADC_BASE 500
>>> #define IIO_EVENT_CODE_MISC_BASE 600
>>> #define IIO_EVENT_CODE_LIGHT_BASE 700
>>> +#define IIO_EVENT_CODE_PROXIMITY_BASE 800
>>>
>>> #define IIO_EVENT_CODE_DEVICE_SPECIFIC 1000
>>>
>>> @@ -367,4 +368,6 @@ struct iio_const_attr {
>>> #define IIO_EVENT_CODE_RING_75_FULL (IIO_EVENT_CODE_RING_BASE + 1)
>>> #define IIO_EVENT_CODE_RING_100_FULL (IIO_EVENT_CODE_RING_BASE + 2)
>>>
>>> +#define IIO_EVENT_CODE_PROXIMITY_THRESH IIO_EVENT_CODE_PROXIMITY_BASE
>>> +
>>> #endif /* _INDUSTRIAL_IO_SYSFS_H_ */
>>
>> Thanks again.
>>
>> Jonathan
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
> Thanks for reviewing.
You are welcome,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
