[PATCH v5 0/5] staging: iio: adc: ad7816: Fix channel handling and refactor

Gabriel Shahrouzi posted 5 patches 7 months, 4 weeks ago
drivers/staging/iio/adc/ad7816.c | 94 ++++++++++++++++++--------------
1 file changed, 54 insertions(+), 40 deletions(-)
[PATCH v5 0/5] staging: iio: adc: ad7816: Fix channel handling and refactor
Posted by Gabriel Shahrouzi 7 months, 4 weeks ago
The original patch combined a functional fix (allowing channel 7) with
several refactoring steps (introducing chip_info, renaming structs,
improving validation). As requested, these have now been separated.

The series proceeds as follows:
1. Fix: Allow diagnostic channel 7 for all device variants.
2. Refactor: Rename the main state structure for clarity before introducing
   the new chip_info struct.
3. Refactor: Introduce struct ad7816_chip_info to hold static per-variant
   data, update ID tables to store pointers, and switch to using
   device_get_match_data() for firmware-independent identification.
   This removes the old enum/id mechanism.
4. Refactor: Add has_busy_pin to chip_info and use this flag to
   determine BUSY pin handling, replacing pointer comparisons.
5. Refactor: Simplify channel validation logic using 
   chip_info->max_channels, removing strcmp() checks.

Regarding the 'fixes' tag: I've applied it only to the first commit
containing the core fix, primarily to make backporting easier. Is this
the standard practice, or should the tag typically be applied to
subsequent commits that build upon or are related to the fix as well?

Changes in v5:
	- Use correct patch version.
Changes in v4:
	- Include missing bracket for condtional statement.
Chainges in v3:
	- Split the patch into smaller patches. Make the fix first
	  followed by clean up.
	- Include missing channel for channel selection.
	- Address specific feedback regarding enums vs. chip_info data.
	- Use device_get_match_data() for device identification.
	- Move BUSY pin capability check into chip_info data.
	- Simplify channel validation using chip_info data.
Changes in v2:
        - Refactor by adding chip_info struct which simplifies
          conditional logic.

Gabriel Shahrouzi (5):
  staging: iio: adc: ad7816: Allow channel 7 for all devices
  staging: iio: adc: ad7816: Rename state structure
  staging: iio: adc: ad7816: Introduce chip_info and use pointer
    matching
  staging: iio: adc: ad7816: Use chip_info for device capabilities
  staging: iio: adc: ad7816: Simplify channel validation using chip_info

 drivers/staging/iio/adc/ad7816.c | 94 ++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 40 deletions(-)

-- 
2.43.0
Re: [PATCH v5 0/5] staging: iio: adc: ad7816: Fix channel handling and refactor
Posted by Jonathan Cameron 7 months, 3 weeks ago
On Sat, 19 Apr 2025 21:49:05 -0400
Gabriel Shahrouzi <gshahrouzi@gmail.com> wrote:

> The original patch combined a functional fix (allowing channel 7) with
> several refactoring steps (introducing chip_info, renaming structs,
> improving validation). As requested, these have now been separated.
> 
> The series proceeds as follows:
> 1. Fix: Allow diagnostic channel 7 for all device variants.
> 2. Refactor: Rename the main state structure for clarity before introducing
>    the new chip_info struct.
> 3. Refactor: Introduce struct ad7816_chip_info to hold static per-variant
>    data, update ID tables to store pointers, and switch to using
>    device_get_match_data() for firmware-independent identification.
>    This removes the old enum/id mechanism.
> 4. Refactor: Add has_busy_pin to chip_info and use this flag to
>    determine BUSY pin handling, replacing pointer comparisons.
> 5. Refactor: Simplify channel validation logic using 
>    chip_info->max_channels, removing strcmp() checks.
> 
> Regarding the 'fixes' tag: I've applied it only to the first commit
> containing the core fix, primarily to make backporting easier. Is this
> the standard practice, or should the tag typically be applied to
> subsequent commits that build upon or are related to the fix as well?
> 
> Changes in v5:
> 	- Use correct patch version.
Generally I wouldn't resend for this. Instead a single email in
reply to the messed up version saying it is infact v4 would have
done the job.

Alternatively a quick reply to that thread to say it was messed
up and please look for v5 would have worked to make a reader
move on directly to the newer version


Jonathan

> Changes in v4:
> 	- Include missing bracket for condtional statement.
> Chainges in v3:
> 	- Split the patch into smaller patches. Make the fix first
> 	  followed by clean up.
> 	- Include missing channel for channel selection.
> 	- Address specific feedback regarding enums vs. chip_info data.
> 	- Use device_get_match_data() for device identification.
> 	- Move BUSY pin capability check into chip_info data.
> 	- Simplify channel validation using chip_info data.
> Changes in v2:
>         - Refactor by adding chip_info struct which simplifies
>           conditional logic.
> 
> Gabriel Shahrouzi (5):
>   staging: iio: adc: ad7816: Allow channel 7 for all devices
>   staging: iio: adc: ad7816: Rename state structure
>   staging: iio: adc: ad7816: Introduce chip_info and use pointer
>     matching
>   staging: iio: adc: ad7816: Use chip_info for device capabilities
>   staging: iio: adc: ad7816: Simplify channel validation using chip_info
> 
>  drivers/staging/iio/adc/ad7816.c | 94 ++++++++++++++++++--------------
>  1 file changed, 54 insertions(+), 40 deletions(-)
>
Re: [PATCH v5 0/5] staging: iio: adc: ad7816: Fix channel handling and refactor
Posted by Gabriel Shahrouzi 7 months, 3 weeks ago
On Mon, Apr 21, 2025 at 7:31 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 19 Apr 2025 21:49:05 -0400
> Gabriel Shahrouzi <gshahrouzi@gmail.com> wrote:
>
> > The original patch combined a functional fix (allowing channel 7) with
> > several refactoring steps (introducing chip_info, renaming structs,
> > improving validation). As requested, these have now been separated.
> >
> > The series proceeds as follows:
> > 1. Fix: Allow diagnostic channel 7 for all device variants.
> > 2. Refactor: Rename the main state structure for clarity before introducing
> >    the new chip_info struct.
> > 3. Refactor: Introduce struct ad7816_chip_info to hold static per-variant
> >    data, update ID tables to store pointers, and switch to using
> >    device_get_match_data() for firmware-independent identification.
> >    This removes the old enum/id mechanism.
> > 4. Refactor: Add has_busy_pin to chip_info and use this flag to
> >    determine BUSY pin handling, replacing pointer comparisons.
> > 5. Refactor: Simplify channel validation logic using
> >    chip_info->max_channels, removing strcmp() checks.
> >
> > Regarding the 'fixes' tag: I've applied it only to the first commit
> > containing the core fix, primarily to make backporting easier. Is this
> > the standard practice, or should the tag typically be applied to
> > subsequent commits that build upon or are related to the fix as well?
> >
> > Changes in v5:
> >       - Use correct patch version.
> Generally I wouldn't resend for this. Instead a single email in
> reply to the messed up version saying it is infact v4 would have
> done the job.
Got it.
>
> Alternatively a quick reply to that thread to say it was messed
> up and please look for v5 would have worked to make a reader
> move on directly to the newer version
Got it.
>
>
> Jonathan
>
> > Changes in v4:
> >       - Include missing bracket for condtional statement.
> > Chainges in v3:
> >       - Split the patch into smaller patches. Make the fix first
> >         followed by clean up.
> >       - Include missing channel for channel selection.
> >       - Address specific feedback regarding enums vs. chip_info data.
> >       - Use device_get_match_data() for device identification.
> >       - Move BUSY pin capability check into chip_info data.
> >       - Simplify channel validation using chip_info data.
> > Changes in v2:
> >         - Refactor by adding chip_info struct which simplifies
> >           conditional logic.
> >
> > Gabriel Shahrouzi (5):
> >   staging: iio: adc: ad7816: Allow channel 7 for all devices
> >   staging: iio: adc: ad7816: Rename state structure
> >   staging: iio: adc: ad7816: Introduce chip_info and use pointer
> >     matching
> >   staging: iio: adc: ad7816: Use chip_info for device capabilities
> >   staging: iio: adc: ad7816: Simplify channel validation using chip_info
> >
> >  drivers/staging/iio/adc/ad7816.c | 94 ++++++++++++++++++--------------
> >  1 file changed, 54 insertions(+), 40 deletions(-)
> >
>