Re: [RFC 3/8] mfd: altr_a10sr: Add Altera Arria10 DevKit System Resource Chip

[Lee Jones <lee.jones@linaro.org>]

Mr Brown,

On Wed, 30 Mar 2016, Thor Thayer wrote:
> On 03/30/2016 06:51 AM, Lee Jones wrote:
> >On Tue, 29 Mar 2016, tthayer@opensource.altera.com wrote:
> >
> >>From: Thor Thayer <tthayer@opensource.altera.com>
> >>
> >>Add support for the Altera Arria10 Development Kit System Resource
> >>chip which is implemented using a MAX5 as a external gpio extender,
> >>and hardware monitor with the regmap framework over a SPI bus.
> >>
> >>Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> >>---
> 
> [...]
> 
> >>index 0000000..13665d4
> >>--- /dev/null
> >>+++ b/drivers/mfd/altera-a10sr.c
> >>@@ -0,0 +1,177 @@
> >>+/*
> >>+ * Copyright Altera Corporation (C) 2014-2016. All Rights Reserved
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify it
> >>+ * under the terms and conditions of the GNU General Public License,
> >>+ * version 2, as published by the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope it will be useful, but WITHOUT
> >>+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >>+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> >>+ * more details.
> >>+ *
> >>+ * You should have received a copy of the GNU General Public License along with
> >>+ * this program.  If not, see <http://www.gnu.org/licenses/>.
> >
> >Any chance you can use the shorter copyright header?
> >
> 
> This is the header that Altera has specified and that we're
> operating under. We haven't received guidance on Intel's header yet
> but it may change as a result of our acquisition by Intel.

Fair enough.

> >>+
> >>+	/*
> >>+	 * We can't use the standard regmap_update_bits function because
> >>+	 * the read register has a different address than the write register.
> >>+	 * Therefore, just do a read, modify, write operation here.
> >>+	 */
> >>+	ret = regmap_read(a10sr->regmap, (reg | READ_REG_MASK), &rval);
> >>+	if (ret < 0)
> >>+		return ret;
> >>+
> >>+	rval = ((rval & ~bit_mask) | (reg_val & bit_mask));
> >>+
> >>+	ret = regmap_write(a10sr->regmap, (reg & WRITE_REG_MASK), rval);
> >>+
> >>+	return ret;
> >>+}
> >
> >Why can't you use the Regmap update function(s)?
> 
> The read register has a different address than the write register
> which is handled in this function with the masks (read address is
> odd, write address is even).

Mark, do we have an API which handled such a configuration?

> Thank you for the review of my patch set. I will implement the
> changes that you pointed out. Thank you for reviewing!

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog