rtc: max77683: avoid regmap bulk write for max77620

Adding support to avoid regmap bulk write for the
devices which are not supported register bulk write.
Max77620 RTC device does not support register bulk write
so disabling regmap bulk write for max77620 rtc device
and enabling only for max77683.

Signed-off-by: Venkat Reddy Talla <vreddytalla@nvidia.com>
---
 drivers/rtc/rtc-max77686.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

Hi,

On Monday, December 12, 2016 05:14:45 PM Venkat Reddy Talla wrote:
> Adding support to avoid regmap bulk write for the
> devices which are not supported register bulk write.

What about register bulk reads done by the driver?

Do they also need a fixup?

> Max77620 RTC device does not support register bulk write
> so disabling regmap bulk write for max77620 rtc device
> and enabling only for max77683.
> 
> Signed-off-by: Venkat Reddy Talla <vreddytalla@nvidia.com>
> ---
>  drivers/rtc/rtc-max77686.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index 182fdd0..401ab25 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -84,6 +84,7 @@ struct max77686_rtc_driver_data {
>  	int			alarm_pending_status_reg;
>  	/* RTC IRQ CHIP for regmap */
>  	const struct regmap_irq_chip *rtc_irq_chip;
> +	bool avoid_rtc_bulk_write;

It should be grouped with other bool fields of this struct.

Reversing the logic would make it simpler and would make
the naming (rtc_bulk_write) consistent with other bool
fields in the struct.

>  };
>  
>  struct max77686_rtc_info {
> @@ -197,6 +198,7 @@ static const struct max77686_rtc_driver_data max77686_drv_data = {
>  	.alarm_pending_status_reg = MAX77686_REG_STATUS2,
>  	.rtc_i2c_addr = MAX77686_I2C_ADDR_RTC,
>  	.rtc_irq_chip = &max77686_rtc_irq_chip,
> +	.avoid_rtc_bulk_write = false,
>  };
>  
>  static const struct max77686_rtc_driver_data max77620_drv_data = {
> @@ -208,6 +210,7 @@ static const struct max77686_rtc_driver_data max77620_drv_data = {
>  	.alarm_pending_status_reg = MAX77686_INVALID_REG,
>  	.rtc_i2c_addr = MAX77620_I2C_ADDR_RTC,
>  	.rtc_irq_chip = &max77686_rtc_irq_chip,
> +	.avoid_rtc_bulk_write = true,
>  };
>  
>  static const unsigned int max77802_map[REG_RTC_END] = {
> @@ -259,6 +262,32 @@ static const struct max77686_rtc_driver_data max77802_drv_data = {
>  	.rtc_irq_chip = &max77802_rtc_irq_chip,
>  };
>  
> +static inline int _regmap_bulk_write(struct max77686_rtc_info *info,

rtc_regmap_bulk_write?

> +		unsigned int reg, void *val, int len)
> +{

Please keep arguments (except "info" one) in sync with regmap_bulk_write():

int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
		     size_t val_count)

> +	int ret = 0;
> +
> +	if (!info->drv_data->avoid_rtc_bulk_write) {
> +		/* RTC registers support sequential writing */
> +		ret = regmap_bulk_write(info->rtc_regmap, reg, val, len);
> +	} else {
> +		/* Power registers support register-data pair writing */

Hmn, maybe this can be handled be regmap_bulk_write() with proper
regmap setting (map->bus == NULL?), can anyone with more regmap
expertise comment on this?

> +		u8 *src = (u8 *)val;
> +		int i;
> +
> +		for (i = 0; i < len; i++) {
> +			ret = regmap_write(info->rtc_regmap, reg, *src++);
> +			if (ret < 0)
> +				break;
> +			reg++;
> +		}
> +	}
> +	if (ret < 0)
> +		dev_err(info->dev, "%s() failed, e %d\n", __func__, ret);

Not needed, upper layers already check ret < 0 cases.

> +	return ret;
> +}
> +
>  static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
>  				    struct max77686_rtc_info *info)
>  {
> @@ -383,7 +412,7 @@ static int max77686_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  
>  	mutex_lock(&info->lock);
>  
> -	ret = regmap_bulk_write(info->rtc_regmap,
> +	ret = _regmap_bulk_write(info,
>  				info->drv_data->map[REG_RTC_SEC],
>  				data, ARRAY_SIZE(data));
>  	if (ret < 0) {
> @@ -506,7 +535,7 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
>  		for (i = 0; i < ARRAY_SIZE(data); i++)
>  			data[i] &= ~ALARM_ENABLE_MASK;
>  
> -		ret = regmap_bulk_write(info->rtc_regmap, map[REG_ALARM1_SEC],
> +		ret = _regmap_bulk_write(info, map[REG_ALARM1_SEC],
>  					data, ARRAY_SIZE(data));
>  	}
>  
> @@ -558,7 +587,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
>  		if (data[RTC_DATE] & 0x1f)
>  			data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
>  
> -		ret = regmap_bulk_write(info->rtc_regmap, map[REG_ALARM1_SEC],
> +		ret = _regmap_bulk_write(info, map[REG_ALARM1_SEC],
>  					data, ARRAY_SIZE(data));
>  	}
>  
> @@ -588,7 +617,7 @@ static int max77686_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	if (ret < 0)
>  		goto out;
>  
> -	ret = regmap_bulk_write(info->rtc_regmap,
> +	ret = _regmap_bulk_write(info,
>  				info->drv_data->map[REG_ALARM1_SEC],
>  				data, ARRAY_SIZE(data));
>  
> @@ -654,7 +683,7 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
>  
>  	info->rtc_24hr_mode = 1;
>  
> -	ret = regmap_bulk_write(info->rtc_regmap,
> +	ret = _regmap_bulk_write(info,
>  				info->drv_data->map[REG_RTC_CONTROLM],
>  				data, ARRAY_SIZE(data));
>  	if (ret < 0) {

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

On Mon, Dec 12, 2016 at 02:28:11PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > @@ -259,6 +262,32 @@ static const struct max77686_rtc_driver_data max77802_drv_data = {
> >  	.rtc_irq_chip = &max77802_rtc_irq_chip,
> >  };
> >  
> > +static inline int _regmap_bulk_write(struct max77686_rtc_info *info,
> 
> rtc_regmap_bulk_write?

I think max77686_regmap_bulk_write would be nice. This might still pop
somewhere in the backtrace so the prefix is useful.

> 
> > +		unsigned int reg, void *val, int len)
> > +{
> 
> Please keep arguments (except "info" one) in sync with regmap_bulk_write():
> 
> int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
> 		     size_t val_count)
> 
> > +	int ret = 0;
> > +
> > +	if (!info->drv_data->avoid_rtc_bulk_write) {
> > +		/* RTC registers support sequential writing */
> > +		ret = regmap_bulk_write(info->rtc_regmap, reg, val, len);
> > +	} else {
> > +		/* Power registers support register-data pair writing */
> 
> Hmn, maybe this can be handled be regmap_bulk_write() with proper
> regmap setting (map->bus == NULL?), can anyone with more regmap
> expertise comment on this?

Good catch. This does not look like a property of this device driver but
of the bus it is attached to.

Best regards,
Krzysztof