Hi Alex,
Thank you for the patch.
On Thu, Mar 19, 2020 at 06:41:53PM +0100, Alex Riesen wrote:
> The regmap provides a convenient utility for this.
>
> Signed-off-by: Alexander Riesen <alexander.riesen@xxxxxxxxxxx>
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 6 ++++++
> drivers/media/i2c/adv748x/adv748x.h | 15 ++++++++++++---
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 345f009de121..36164a254d7b 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -133,6 +133,12 @@ static int adv748x_write_check(struct adv748x_state *state, u8 page, u8 reg,
> return *error;
> }
>
> +int adv748x_update_bits(struct adv748x_state *state, u8 page, u8 reg, u8 mask,
> + u8 value)
> +{
> + return regmap_update_bits(state->regmap[page], reg, mask, value);
> +}
> +
> /* adv748x_write_block(): Write raw data with a maximum of I2C_SMBUS_BLOCK_MAX
> * size to one or more registers.
> *
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 09aab4138c3f..c5245464fffc 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -393,25 +393,34 @@ int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value);
> int adv748x_write_block(struct adv748x_state *state, int client_page,
> unsigned int init_reg, const void *val,
> size_t val_len);
> +int adv748x_update_bits(struct adv748x_state *state, u8 page, u8 reg,
> + u8 mask, u8 value);
>
> #define io_read(s, r) adv748x_read(s, ADV748X_PAGE_IO, r)
> #define io_write(s, r, v) adv748x_write(s, ADV748X_PAGE_IO, r, v)
> -#define io_clrset(s, r, m, v) io_write(s, r, (io_read(s, r) & ~(m)) | (v))
> +#define io_clrset(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_IO, r, m, v)
> +#define io_update(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_IO, r, m, v)
Those two are identical. Do we need both ? I would standardize on either
*_update or *_clrset for all the functions here. Apart from that,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> #define hdmi_read(s, r) adv748x_read(s, ADV748X_PAGE_HDMI, r)
> #define hdmi_read16(s, r, m) (((hdmi_read(s, r) << 8) | hdmi_read(s, (r)+1)) & (m))
> #define hdmi_write(s, r, v) adv748x_write(s, ADV748X_PAGE_HDMI, r, v)
> +#define hdmi_update(s, r, m, v) \
> + adv748x_update_bits(s, ADV748X_PAGE_HDMI, r, m, v)
> +
> +#define dpll_read(s, r) adv748x_read(s, ADV748X_PAGE_DPLL, r)
> +#define dpll_update(s, r, m, v) \
> + adv748x_update_bits(s, ADV748X_PAGE_DPLL, r, m, v)
>
> #define repeater_read(s, r) adv748x_read(s, ADV748X_PAGE_REPEATER, r)
> #define repeater_write(s, r, v) adv748x_write(s, ADV748X_PAGE_REPEATER, r, v)
>
> #define sdp_read(s, r) adv748x_read(s, ADV748X_PAGE_SDP, r)
> #define sdp_write(s, r, v) adv748x_write(s, ADV748X_PAGE_SDP, r, v)
> -#define sdp_clrset(s, r, m, v) sdp_write(s, r, (sdp_read(s, r) & ~(m)) | (v))
> +#define sdp_clrset(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_SDP, r, m, v)
>
> #define cp_read(s, r) adv748x_read(s, ADV748X_PAGE_CP, r)
> #define cp_write(s, r, v) adv748x_write(s, ADV748X_PAGE_CP, r, v)
> -#define cp_clrset(s, r, m, v) cp_write(s, r, (cp_read(s, r) & ~(m)) | (v))
> +#define cp_clrset(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_CP, r, m, v)
>
> #define tx_read(t, r) adv748x_read(t->state, t->page, r)
> #define tx_write(t, r, v) adv748x_write(t->state, t->page, r, v)
--
Regards,
Laurent Pinchart
