[PATCH 0/3] iio: light: tcs3472: devm conversion, wait time, locking cleanup

Aldo Conte posted 3 patches 1 month, 1 week ago
drivers/iio/light/tcs3472.c | 287 ++++++++++++++++++++++++++----------
1 file changed, 206 insertions(+), 81 deletions(-)
[PATCH 0/3] iio: light: tcs3472: devm conversion, wait time, locking cleanup
Posted by Aldo Conte 1 month, 1 week ago
This series of changes modernizes the tcs3472 driver by completing the
devm conversion and updating the locking style to use an automatic
guard (mutex). Furthermore, it implement control of the WAIT state,
resolving a TODO.

Patch1: Converts the driver to use device-managed resource allocation.
This removes the need for an explicit remove() callback, since cleanup
is now handled automatically. It also adds a new function called
tcs3472_powerdown_action() that powers down the chip when the driver is 
unloaded, removed or when probe fails after the chip has been enabled.

Patch2: adds support for the wait time resolving the old TODO comment.
The user can control the WTIME indirectly by writing to the
sampling_frequency attribute. Changing the sampling
frequency attribute results in a change to the wtime while preserving
the current integration time.
Similarly, if the user has previously set a sampling frequency and
then changes the integration time, the driver re-computes WTIME so the 
previous frequency is preserved. The patch also handles the deactivation
of wtime when the requested frequency is very high, as well as the
extension of wtime to its maximum values (by enabling the use of WLONG)
when the requested frequency is very low.

Patch3: converts the remaining mutex_lock()/mutex_unlock() pairs to
guard(mutex).

All patches have been tested on a Raspberry Pi 3B with a TCS3472
connected to I2C-1 at address 0x29. Sampling frequency and integration
time changes have been exercised checking the value of WTIME and
consequently WEN and WLONG. Raw RGBC reads, calibscale, integration_time
and threshold events continue to work as before.

Aldo Conte (3):
  iio: light: tcs3472: use devm for resource management
  iio: light: tcs3472: implement wait time and sampling frequency
  iio: light: tcs3472: Use guard(mutex)() family over manual locking

 drivers/iio/light/tcs3472.c | 287 ++++++++++++++++++++++++++----------
 1 file changed, 206 insertions(+), 81 deletions(-)

-- 
2.54.0
Re: [PATCH 0/3] iio: light: tcs3472: devm conversion, wait time, locking cleanup
Posted by Andy Shevchenko 1 month, 1 week ago
On Wed, May 06, 2026 at 11:43:08AM +0200, Aldo Conte wrote:
> This series of changes modernizes the tcs3472 driver by completing the
> devm conversion and updating the locking style to use an automatic
> guard (mutex). Furthermore, it implement control of the WAIT state,
> resolving a TODO.
> 
> Patch1: Converts the driver to use device-managed resource allocation.
> This removes the need for an explicit remove() callback, since cleanup
> is now handled automatically. It also adds a new function called
> tcs3472_powerdown_action() that powers down the chip when the driver is 
> unloaded, removed or when probe fails after the chip has been enabled.
> 
> Patch2: adds support for the wait time resolving the old TODO comment.
> The user can control the WTIME indirectly by writing to the
> sampling_frequency attribute. Changing the sampling
> frequency attribute results in a change to the wtime while preserving
> the current integration time.
> Similarly, if the user has previously set a sampling frequency and
> then changes the integration time, the driver re-computes WTIME so the 
> previous frequency is preserved. The patch also handles the deactivation
> of wtime when the requested frequency is very high, as well as the
> extension of wtime to its maximum values (by enabling the use of WLONG)
> when the requested frequency is very low.
> 
> Patch3: converts the remaining mutex_lock()/mutex_unlock() pairs to
> guard(mutex).
> 
> All patches have been tested on a Raspberry Pi 3B with a TCS3472
> connected to I2C-1 at address 0x29. Sampling frequency and integration
> time changes have been exercised checking the value of WTIME and
> consequently WEN and WLONG. Raw RGBC reads, calibscale, integration_time
> and threshold events continue to work as before.

Thanks for contribution, unfortunately this series needs much more work
and more preparatory patches as well. I have reviewed the current series.
Please, follow and address (or comment why it can't be done as suggested).

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH 0/3] iio: light: tcs3472: devm conversion, wait time, locking cleanup
Posted by Aldo Conte 1 month, 1 week ago
On 5/6/26 12:21, Andy Shevchenko wrote:
> On Wed, May 06, 2026 at 11:43:08AM +0200, Aldo Conte wrote:
>> This series of changes modernizes the tcs3472 driver by completing the
>> devm conversion and updating the locking style to use an automatic
>> guard (mutex). Furthermore, it implement control of the WAIT state,
>> resolving a TODO.
>>
>> Patch1: Converts the driver to use device-managed resource allocation.
>> This removes the need for an explicit remove() callback, since cleanup
>> is now handled automatically. It also adds a new function called
>> tcs3472_powerdown_action() that powers down the chip when the driver is
>> unloaded, removed or when probe fails after the chip has been enabled.
>>
>> Patch2: adds support for the wait time resolving the old TODO comment.
>> The user can control the WTIME indirectly by writing to the
>> sampling_frequency attribute. Changing the sampling
>> frequency attribute results in a change to the wtime while preserving
>> the current integration time.
>> Similarly, if the user has previously set a sampling frequency and
>> then changes the integration time, the driver re-computes WTIME so the
>> previous frequency is preserved. The patch also handles the deactivation
>> of wtime when the requested frequency is very high, as well as the
>> extension of wtime to its maximum values (by enabling the use of WLONG)
>> when the requested frequency is very low.
>>
>> Patch3: converts the remaining mutex_lock()/mutex_unlock() pairs to
>> guard(mutex).
>>
>> All patches have been tested on a Raspberry Pi 3B with a TCS3472
>> connected to I2C-1 at address 0x29. Sampling frequency and integration
>> time changes have been exercised checking the value of WTIME and
>> consequently WEN and WLONG. Raw RGBC reads, calibscale, integration_time
>> and threshold events continue to work as before.
> 
> Thanks for contribution, unfortunately this series needs much more work
> and more preparatory patches as well. I have reviewed the current series.
> Please, follow and address (or comment why it can't be done as suggested).
> 
Hi Andy,

Thanks a lot for the detailed review. I'll address all the points in v2.

Regarding the series structure, I need to clarify the correct order:
1) Guard (mutex) conversion
2) Header sorting
3) Devm conversion (with the new TCS3472_Powerdown using Guard (mutex) 
directly)
4) Wait time implementation
Would this be OK?

Aldo
Re: [PATCH 0/3] iio: light: tcs3472: devm conversion, wait time, locking cleanup
Posted by Andy Shevchenko 1 month, 1 week ago
On Wed, May 06, 2026 at 05:35:42PM +0200, Aldo Conte wrote:
> On 5/6/26 12:21, Andy Shevchenko wrote:
> > On Wed, May 06, 2026 at 11:43:08AM +0200, Aldo Conte wrote:

...

> Thanks a lot for the detailed review. I'll address all the points in v2.
> 
> Regarding the series structure, I need to clarify the correct order:
> 1) Guard (mutex) conversion

Since this most likely wants a new cleanup.h to be added, the 2) Header sorting
should go before even this one.

> 2) Header sorting
> 3) Devm conversion (with the new TCS3472_Powerdown using Guard (mutex)
> directly)
> 4) Wait time implementation
> Would this be OK?

Otherwise sounds good, but I can't be 100% sure until I see a v2.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH 0/3] iio: light: tcs3472: devm conversion, wait time, locking cleanup
Posted by Aldo Conte 1 month, 1 week ago
On 5/6/26 12:21, Andy Shevchenko wrote:
> On Wed, May 06, 2026 at 11:43:08AM +0200, Aldo Conte wrote:
>> This series of changes modernizes the tcs3472 driver by completing the
>> devm conversion and updating the locking style to use an automatic
>> guard (mutex). Furthermore, it implement control of the WAIT state,
>> resolving a TODO.
>>
>> Patch1: Converts the driver to use device-managed resource allocation.
>> This removes the need for an explicit remove() callback, since cleanup
>> is now handled automatically. It also adds a new function called
>> tcs3472_powerdown_action() that powers down the chip when the driver is
>> unloaded, removed or when probe fails after the chip has been enabled.
>>
>> Patch2: adds support for the wait time resolving the old TODO comment.
>> The user can control the WTIME indirectly by writing to the
>> sampling_frequency attribute. Changing the sampling
>> frequency attribute results in a change to the wtime while preserving
>> the current integration time.
>> Similarly, if the user has previously set a sampling frequency and
>> then changes the integration time, the driver re-computes WTIME so the
>> previous frequency is preserved. The patch also handles the deactivation
>> of wtime when the requested frequency is very high, as well as the
>> extension of wtime to its maximum values (by enabling the use of WLONG)
>> when the requested frequency is very low.
>>
>> Patch3: converts the remaining mutex_lock()/mutex_unlock() pairs to
>> guard(mutex).
>>
>> All patches have been tested on a Raspberry Pi 3B with a TCS3472
>> connected to I2C-1 at address 0x29. Sampling frequency and integration
>> time changes have been exercised checking the value of WTIME and
>> consequently WEN and WLONG. Raw RGBC reads, calibscale, integration_time
>> and threshold events continue to work as before.
> 
> Thanks for contribution, unfortunately this series needs much more work
> and more preparatory patches as well. I have reviewed the current series.
> Please, follow and address (or comment why it can't be done as suggested).
> 

Thank you for your review.
These mistakes are undoubtedly due to my still limited experience. I 
will start working on them right away.

Best regards,
Aldo Conte