diff mbox series

[v1,11/15] iio: adc: ad7768-1: Add reg_write_masked function

Message ID 67649c43050d161621bc0494638bfa71fed82ea8.1736201898.git.Jonathan.Santos@analog.com (mailing list archive)
State Changes Requested
Headers show
Series iio: adc: ad7768-1: Add features, improvements, and fixes | expand

Commit Message

Jonathan Santos Jan. 7, 2025, 3:26 p.m. UTC 
From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>

This commit adds the ad7768_spi_reg_write_masked() which is a helper
function for writing specific bits inside a register, without interfering
with the other bit values.

Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

David Lechner Jan. 7, 2025, 11:46 p.m. UTC | #1 
On 1/7/25 9:26 AM, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> This commit adds the ad7768_spi_reg_write_masked() which is a helper
> function for writing specific bits inside a register, without interfering
> with the other bit values.
> 
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 574d735f2c3a..675af9ea856d 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -242,6 +242,21 @@ static int ad7768_spi_reg_write(struct ad7768_state *st,
>  	return spi_write(st->spi, st->data.d8, 2);
>  }
>  
> +static int ad7768_spi_reg_write_masked(struct ad7768_state *st,
> +				       unsigned int addr,
> +				       unsigned int mask,
> +				       unsigned int val)
> +{
> +	unsigned int reg_val;
> +	int ret;
> +
> +	ret = ad7768_spi_reg_read(st, addr, &reg_val, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ad7768_spi_reg_write(st, addr, (reg_val & ~mask) | val);
> +}
> +
>  static int ad7768_set_mode(struct ad7768_state *st,
>  			   enum ad7768_conv_mode mode)
>  {

Can we convert this driver to use regmap instead of reinventing it?
Marcelo Schmitt Jan. 10, 2025, 10:34 p.m. UTC | #2 
On 01/07, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> This commit adds the ad7768_spi_reg_write_masked() which is a helper
> function for writing specific bits inside a register, without interfering
> with the other bit values.
> 
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 574d735f2c3a..675af9ea856d 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -242,6 +242,21 @@ static int ad7768_spi_reg_write(struct ad7768_state *st,
>  	return spi_write(st->spi, st->data.d8, 2);
>  }
>  
> +static int ad7768_spi_reg_write_masked(struct ad7768_state *st,
> +				       unsigned int addr,
> +				       unsigned int mask,
> +				       unsigned int val)
> +{
> +	unsigned int reg_val;
> +	int ret;
> +
> +	ret = ad7768_spi_reg_read(st, addr, &reg_val, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ad7768_spi_reg_write(st, addr, (reg_val & ~mask) | val);
> +}
> +
ad7768_spi_reg_write_masked() should be added together with at least one
user of it otherwise it leads to build warnings.
Though, better if able to convert to regmap as David suggested.
regmap_set_bits() and regmap_update_bits() come for free.

>  static int ad7768_set_mode(struct ad7768_state *st,
>  			   enum ad7768_conv_mode mode)
>  {
> -- 
> 2.34.1
>
Jonathan Santos Jan. 12, 2025, 2:42 a.m. UTC | #3 
On 01/10, Marcelo Schmitt wrote:
> On 01/07, Jonathan Santos wrote:
> > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > 
> > This commit adds the ad7768_spi_reg_write_masked() which is a helper
> > function for writing specific bits inside a register, without interfering
> > with the other bit values.
> > 
> > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > ---
> >  drivers/iio/adc/ad7768-1.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > index 574d735f2c3a..675af9ea856d 100644
> > --- a/drivers/iio/adc/ad7768-1.c
> > +++ b/drivers/iio/adc/ad7768-1.c
> > @@ -242,6 +242,21 @@ static int ad7768_spi_reg_write(struct ad7768_state *st,
> >  	return spi_write(st->spi, st->data.d8, 2);
> >  }
> >  
> > +static int ad7768_spi_reg_write_masked(struct ad7768_state *st,
> > +				       unsigned int addr,
> > +				       unsigned int mask,
> > +				       unsigned int val)
> > +{
> > +	unsigned int reg_val;
> > +	int ret;
> > +
> > +	ret = ad7768_spi_reg_read(st, addr, &reg_val, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return ad7768_spi_reg_write(st, addr, (reg_val & ~mask) | val);
> > +}
> > +
> ad7768_spi_reg_write_masked() should be added together with at least one
> user of it otherwise it leads to build warnings.
> Though, better if able to convert to regmap as David suggested.
> regmap_set_bits() and regmap_update_bits() come for free.
> 

I also think David suggestion is better, i will convert to regmap. Will
make our lives easier later as well.

> >  static int ad7768_set_mode(struct ad7768_state *st,
> >  			   enum ad7768_conv_mode mode)
> >  {
> > -- 
> > 2.34.1
> >
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 574d735f2c3a..675af9ea856d 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -242,6 +242,21 @@  static int ad7768_spi_reg_write(struct ad7768_state *st,
 	return spi_write(st->spi, st->data.d8, 2);
 }
 
+static int ad7768_spi_reg_write_masked(struct ad7768_state *st,
+				       unsigned int addr,
+				       unsigned int mask,
+				       unsigned int val)
+{
+	unsigned int reg_val;
+	int ret;
+
+	ret = ad7768_spi_reg_read(st, addr, &reg_val, 1);
+	if (ret < 0)
+		return ret;
+
+	return ad7768_spi_reg_write(st, addr, (reg_val & ~mask) | val);
+}
+
 static int ad7768_set_mode(struct ad7768_state *st,
 			   enum ad7768_conv_mode mode)
 {