* Re: [RFCv1 4/4] mfd: twl4030-madc: Move driver to drivers/iio/adc
From: Sebastian Reichel @ 2014-02-23 0:35 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Marek Belisko, Lee Jones, Samuel Ortiz, Lars-Peter Clausen,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Grant Likely, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <53089C47.5050404-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 43202 bytes --]
On Sat, Feb 22, 2014 at 12:47:03PM +0000, Jonathan Cameron wrote:
> On 14/02/14 18:46, Sebastian Reichel wrote:
> >This is a driver for an A/D converter, which belongs into
> >drivers/iio/adc.
> >
> >Signed-off-by: Sebastian Reichel <sre-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
>
> Being inherently lazy I'm going to review this patch as it gives the complete
> driver rather than taking on the conversion patch directly!
OK.
> It's a long shot, but are there any docs freely available for this part?
There is documentation on Texas Instrument's website for TPS65950,
which is supposed to be very similar to the TWL4030 and contains a
section about the MADC module:
http://www.ti.com/lit/gpn/tps65950
> Obviously that may well mean that some of my comments apply to the driver
> in general rather than the changes you've made. Please feel free to
> pick and choose!
Having skipped over your comments I guess 70% are about the driver
in general, but I can add another patch to the patchset, which fixes
the driver style.
> I'd like ideally to see a little more generic tidying up in this driver.
> As you'll notice inline I get irritated at having lots and lots of error
> messages. Still that's personal preference but I felt a lot more justified
> in moaning after finding one that was incorrect ;)
>
> There is also a bit of functionality in here that I'm not sure is ever
> used (the request callback functions). It complicates the code so if no
> using it I'd be tempted to drop it.
I think the optimal workflow is:
1. convert madc driver to IIO
2. convert twl4030-madc-battery driver to IIO API
3. convert rx51-battery to IIO API
4. convert twl4030-madc-hwmon to IIO API / deprecate it
5. remove old in-kernel ABI from madc
6. cleanup/simplify madc
I guess its much simpler to do the driver cleanup/simplification
once we can get rid of the old API.
> It's nice in a way that the driver before your conversion provided only
> a very limited and in kernel interface given we don't have to hang on to
> any old ABI elements.
>
> >---
> > drivers/iio/adc/Kconfig | 10 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/twl4030-madc.c | 922 +++++++++++++++++++++++++++++++++++++++++
> > drivers/mfd/Kconfig | 10 -
> > drivers/mfd/Makefile | 1 -
> > drivers/mfd/twl4030-madc.c | 922 -----------------------------------------
> > 6 files changed, 933 insertions(+), 933 deletions(-)
> > create mode 100644 drivers/iio/adc/twl4030-madc.c
> > delete mode 100644 drivers/mfd/twl4030-madc.c
> >
> >diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> >index 2209f28..427f75c 100644
> >--- a/drivers/iio/adc/Kconfig
> >+++ b/drivers/iio/adc/Kconfig
> >@@ -183,6 +183,16 @@ config TI_AM335X_ADC
> > Say yes here to build support for Texas Instruments ADC
> > driver which is also a MFD client.
> >
> >+config TWL4030_MADC
> >+ tristate "TWL4030 MADC (Monitoring A/D Converter)"
> >+ depends on TWL4030_CORE
> >+ help
> >+ This driver provides support for Triton TWL4030-MADC. The
> >+ driver supports both RT and SW conversion methods.
> >+
> >+ This driver can also be built as a module. If so, the module will be
> >+ called twl4030-madc.
> >+
> > config TWL6030_GPADC
> > tristate "TWL6030 GPADC (General Purpose A/D Converter) Support"
> > depends on TWL4030_CORE
> >diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> >index ba9a10a..9acf2df 100644
> >--- a/drivers/iio/adc/Makefile
> >+++ b/drivers/iio/adc/Makefile
> >@@ -20,5 +20,6 @@ obj-$(CONFIG_MCP3422) += mcp3422.o
> > obj-$(CONFIG_NAU7802) += nau7802.o
> > obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> > obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> >+obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
> > obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
> > obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
> >diff --git a/drivers/iio/adc/twl4030-madc.c b/drivers/iio/adc/twl4030-madc.c
> >new file mode 100644
> >index 0000000..4da61c4
> >--- /dev/null
> >+++ b/drivers/iio/adc/twl4030-madc.c
> >@@ -0,0 +1,922 @@
> >+/*
> >+ *
> >+ * TWL4030 MADC module driver-This driver monitors the real time
> >+ * conversion of analog signals like battery temperature,
> >+ * battery type, battery level etc.
> >+ *
> >+ * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
> >+ * J Keerthy <j-keerthy-l0cyMroinI0@public.gmane.org>
> >+ *
> >+ * Based on twl4030-madc.c
> >+ * Copyright (C) 2008 Nokia Corporation
> >+ * Mikko Ylinen <mikko.k.ylinen-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> >+ *
> >+ * Amit Kucheria <amit.kucheria-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> >+ *
> >+ * This program is free software; you can redistribute it and/or
> >+ * modify it under the terms of the GNU General Public License
> >+ * version 2 as published by the Free Software Foundation.
> >+ *
> >+ * This program is distributed in the hope that 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, write to the Free Software
> >+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> >+ * 02110-1301 USA
> >+ *
> >+ */
> >+
> >+#include <linux/init.h>
> >+#include <linux/device.h>
> >+#include <linux/interrupt.h>
> >+#include <linux/kernel.h>
> >+#include <linux/delay.h>
> >+#include <linux/platform_device.h>
> >+#include <linux/slab.h>
> >+#include <linux/i2c/twl.h>
> >+#include <linux/i2c/twl4030-madc.h>
> >+#include <linux/module.h>
> >+#include <linux/stddef.h>
> >+#include <linux/mutex.h>
> >+#include <linux/bitops.h>
> >+#include <linux/jiffies.h>
> >+#include <linux/types.h>
> >+#include <linux/gfp.h>
> >+#include <linux/err.h>
> >+
> >+#include <linux/iio/iio.h>
> Why include machine.h or driver.h. You aren't currently using the
> mapping coding anyway that these provide.
My bad. I copied that over from another driver without checking if
it is actually needed.
> >+#include <linux/iio/machine.h>
> >+#include <linux/iio/driver.h>
> >+
> >+/*
> >+ * struct twl4030_madc_data - a container for madc info
> >+ * @dev - pointer to device structure for madc
> >+ * @lock - mutex protecting this data structure
> >+ * @requests - Array of request struct corresponding to SW1, SW2 and RT
> >+ * @imr - Interrupt mask register of MADC
> >+ * @isr - Interrupt status register of MADC
> >+ */
> >+struct twl4030_madc_data {
> >+ struct device *dev;
> >+ struct mutex lock; /* mutex protecting this data structure */
> >+ struct twl4030_madc_request requests[TWL4030_MADC_NUM_METHODS];
> >+ bool use_second_irq;
>
> These are 32 bit signed values?
These were already defined as int in the original driver. I will
change these to u8 in an additional patch.
> >+ int imr;
> >+ int isr;
> >+};
> >+
> >+static int twl4030_madc_read(struct iio_dev *iio_dev,
> >+ const struct iio_chan_spec *chan,
> >+ int *val, int *val2, long mask)
> >+{
> >+ struct twl4030_madc_data *madc = iio_priv(iio_dev);
> >+ struct twl4030_madc_request req;
> >+ int channel = chan->channel;
> >+ int ret;
> >+
> >+ req.method = madc->use_second_irq ? TWL4030_MADC_SW2 : TWL4030_MADC_SW1;
> >+
> >+ req.channels = BIT(channel);
> >+ req.active = 0;
> >+ req.func_cb = NULL;
> req.raw = !(mask & IIO_CHAN_INFO_PROCESSED);
> req.do_avg = !!(mask & IIO_CHAN_INFO_AVERAGE_RAW);
Right.
> >+ req.raw = (mask & IIO_CHAN_INFO_PROCESSED) ? false : true;
> >+ req.do_avg = (mask & IIO_CHAN_INFO_AVERAGE_RAW) ? true : false;
> >+
> >+ ret = twl4030_madc_conversion(&req);
> >+ if (ret < 0)
> >+ return ret;
> >+
> >+ *val = req.rbuf[channel];
> >+
> >+ return IIO_VAL_INT;
> >+}
> >+
> >+static const struct iio_info twl4030_madc_iio_info = {
> >+ .read_raw = &twl4030_madc_read,
> >+ .driver_module = THIS_MODULE,
> >+};
> >+
> Please give this a prefixed name. Chances of ADC_CHANNEL
> turning up in an IIO header is a little too high to do this
> without! e.g. #define TWL4030_ADC_CHANNEL
Probably drivers/iio/adc/exynos_adc.c should also be fixed.
> >+#define ADC_CHANNEL(_channel, _type, _name, _mask) { \
> >+ .type = _type, \
> I don't think you use scan_type anywhere so don't bother specifying it.
> here.
ok.
> >+ .scan_type = IIO_ST('u', 10, 16, 0), \
> >+ .channel = _channel, \
> >+ .info_mask_separate = _mask, \
> >+ .datasheet_name = _name, \
> >+ .indexed = 1, \
> >+}
> >+
>
> Is the average have an adjustable number of samples?
The TWL supports reading a value directly (1 sample) or reading
the average of 4 samples.
> If not I'd be tempted to use the more common option of IIO_CHAN_INFO_RAW
> rather than the average version (tends only to be used in fairly obscure
> cases where both a raw access and an averaged one are available).
currently some drivers use the averaged read and some use the direct
read.
> >+static const struct iio_chan_spec twl4030_madc_iio_channels[] = {
> >+ ADC_CHANNEL(0, IIO_VOLTAGE, "ADCIN0", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(1, IIO_TEMP, "ADCIN1", BIT(IIO_CHAN_INFO_PROCESSED) |
> >+ BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(2, IIO_VOLTAGE, "ADCIN2", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(3, IIO_VOLTAGE, "ADCIN3", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(4, IIO_VOLTAGE, "ADCIN4", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(5, IIO_VOLTAGE, "ADCIN5", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(6, IIO_VOLTAGE, "ADCIN6", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(7, IIO_VOLTAGE, "ADCIN7", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(8, IIO_VOLTAGE, "ADCIN8", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(9, IIO_VOLTAGE, "ADCIN9", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(10, IIO_CURRENT, "ADCIN10", BIT(IIO_CHAN_INFO_PROCESSED) |
> >+ BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(11, IIO_VOLTAGE, "ADCIN11", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(12, IIO_VOLTAGE, "ADCIN12", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(13, IIO_VOLTAGE, "ADCIN13", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(14, IIO_VOLTAGE, "ADCIN14", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+ ADC_CHANNEL(15, IIO_VOLTAGE, "ADCIN15", BIT(IIO_CHAN_INFO_AVERAGE_RAW)),
> >+};
> >+
> Why the artificial limitation of one of these devices? I guess this is to
> allow the exported function to work without needing to be associated with
> any particular device... Hmm.
Artificial limitation? The madc is a ADC, which has some special
functions defined for some pins in the datasheet:
ADC0 = Battery Voltage
ADC1 = Battery Temperature
ADC10 = Battery Current
Nokia misused some of those pins on the Nokia N900, though. For
example they have their own conversion tables for the temperature,
which is not compatible with the generic one for the madc module.
I planned to convert the rx51-battery driver to simply read the raw
values. All other users can use the processed information.
> >+static struct twl4030_madc_data *twl4030_madc;
> >+
> >+struct twl4030_prescale_divider_ratios {
> >+ s16 numerator;
> >+ s16 denominator;
> >+};
> >+
> >+static const struct twl4030_prescale_divider_ratios
> >+twl4030_divider_ratios[16] = {
> >+ {1, 1}, /* CHANNEL 0 No Prescaler */
> >+ {1, 1}, /* CHANNEL 1 No Prescaler */
> >+ {6, 10}, /* CHANNEL 2 */
> >+ {6, 10}, /* CHANNEL 3 */
> >+ {6, 10}, /* CHANNEL 4 */
> >+ {6, 10}, /* CHANNEL 5 */
> >+ {6, 10}, /* CHANNEL 6 */
> >+ {6, 10}, /* CHANNEL 7 */
> >+ {3, 14}, /* CHANNEL 8 */
> >+ {1, 3}, /* CHANNEL 9 */
> >+ {1, 1}, /* CHANNEL 10 No Prescaler */
> >+ {15, 100}, /* CHANNEL 11 */
> >+ {1, 4}, /* CHANNEL 12 */
> >+ {1, 1}, /* CHANNEL 13 Reserved channels */
> >+ {1, 1}, /* CHANNEL 14 Reseved channels */
> >+ {5, 11}, /* CHANNEL 15 */
> >+};
> >+
> >+
> >+/*
> >+ * Conversion table from -3 to 55 degree Celcius
> >+ */
> >+static int therm_tbl[] = {
> >+30800, 29500, 28300, 27100,
> >+26000, 24900, 23900, 22900, 22000, 21100, 20300, 19400, 18700, 17900,
> >+17200, 16500, 15900, 15300, 14700, 14100, 13600, 13100, 12600, 12100,
> >+11600, 11200, 10800, 10400, 10000, 9630, 9280, 8950, 8620, 8310,
> >+8020, 7730, 7460, 7200, 6950, 6710, 6470, 6250, 6040, 5830,
> >+5640, 5450, 5260, 5090, 4920, 4760, 4600, 4450, 4310, 4170,
> >+4040, 3910, 3790, 3670, 3550
> >+};
> >+
> >+/*
> >+ * Structure containing the registers
> >+ * of different conversion methods supported by MADC.
> >+ * Hardware or RT real time conversion request initiated by external host
> >+ * processor for RT Signal conversions.
> >+ * External host processors can also request for non RT conversions
> >+ * SW1 and SW2 software conversions also called asynchronous or GPC request.
> >+ */
> >+static
> >+const struct twl4030_madc_conversion_method twl4030_conversion_methods[] = {
> >+ [TWL4030_MADC_RT] = {
> >+ .sel = TWL4030_MADC_RTSELECT_LSB,
> >+ .avg = TWL4030_MADC_RTAVERAGE_LSB,
> >+ .rbase = TWL4030_MADC_RTCH0_LSB,
> >+ },
> >+ [TWL4030_MADC_SW1] = {
> >+ .sel = TWL4030_MADC_SW1SELECT_LSB,
> >+ .avg = TWL4030_MADC_SW1AVERAGE_LSB,
> >+ .rbase = TWL4030_MADC_GPCH0_LSB,
> >+ .ctrl = TWL4030_MADC_CTRL_SW1,
> >+ },
> >+ [TWL4030_MADC_SW2] = {
> >+ .sel = TWL4030_MADC_SW2SELECT_LSB,
> >+ .avg = TWL4030_MADC_SW2AVERAGE_LSB,
> >+ .rbase = TWL4030_MADC_GPCH0_LSB,
> >+ .ctrl = TWL4030_MADC_CTRL_SW2,
> >+ },
> >+};
> >+
> >+/*
> >+ * Function to read a particular channel value.
> >+ * @madc - pointer to struct twl4030_madc_data
> >+ * @reg - lsb of ADC Channel
> >+ * If the i2c read fails it returns an error else returns 0.
> >+ */
> >+static int twl4030_madc_channel_raw_read(struct twl4030_madc_data *madc, u8 reg)
> >+{
> >+ u8 msb, lsb;
> >+ int ret;
> >+ /*
> >+ * For each ADC channel, we have MSB and LSB register pair. MSB address
> >+ * is always LSB address+1. reg parameter is the address of LSB register
> >+ */
> >+ ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, &msb, reg + 1);
> >+ if (ret) {
> >+ dev_err(madc->dev, "unable to read MSB register 0x%X\n",
> >+ reg + 1);
> >+ return ret;
> >+ }
> >+ ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, &lsb, reg);
> >+ if (ret) {
> >+ dev_err(madc->dev, "unable to read LSB register 0x%X\n", reg);
> >+ return ret;
> >+ }
> >+
> >+ return (int)(((msb << 8) | lsb) >> 6);
> >+}
> >+
> >+/*
> >+ * Return battery temperature
> >+ * Or < 0 on failure.
> >+ */
> >+static int twl4030battery_temperature(int raw_volt)
> >+{
> >+ u8 val;
> >+ int temp, curr, volt, res, ret;
> >+
> >+ volt = (raw_volt * TEMP_STEP_SIZE) / TEMP_PSR_R;
> >+ /* Getting and calculating the supply current in micro ampers */
> >+ ret = twl_i2c_read_u8(TWL_MODULE_MAIN_CHARGE, &val,
> >+ REG_BCICTL2);
> >+ if (ret < 0)
> >+ return ret;
> blank line here and after similar error cases makes it a little easier to
> read.
>
> >+ curr = ((val & TWL4030_BCI_ITHEN) + 1) * 10;
> >+ /* Getting and calculating the thermistor resistance in ohms */
> >+ res = volt * 1000 / curr;
> >+ /* calculating temperature */
> >+ for (temp = 58; temp >= 0; temp--) {
> >+ int actual = therm_tbl[temp];
> >+
> No blank line here.
I added the newline to the style fix patch, since these lines were
not touched by me before.
> >+ if ((actual - res) >= 0)
> >+ break;
> >+ }
> >+
> >+ return temp + 1;
> >+}
> >+
> >+static int twl4030battery_current(int raw_volt)
> >+{
> >+ int ret;
> >+ u8 val;
> >+
> >+ ret = twl_i2c_read_u8(TWL_MODULE_MAIN_CHARGE, &val,
> >+ TWL4030_BCI_BCICTL1);
> >+ if (ret)
> >+ return ret;
> >+ if (val & TWL4030_BCI_CGAIN) /* slope of 0.44 mV/mA */
> >+ return (raw_volt * CURR_STEP_SIZE) / CURR_PSR_R1;
> >+ else /* slope of 0.88 mV/mA */
> >+ return (raw_volt * CURR_STEP_SIZE) / CURR_PSR_R2;
> >+}
> blank line here please.
Also added to the style fix patch.
> Various comments on this in the next function:
> This would be much simpler if any error immediately resulted in an
> exit with error code. If it's gone wrong enough to issue a dev_err
> then don't try to muddle through. Given there is nothing to clear
> up in here, just error out directly on the first problem.
I guess this should be fixed once the old API is removed.
> >+/*
> >+ * Function to read channel values
> >+ * @madc - pointer to twl4030_madc_data struct
> >+ * @reg_base - Base address of the first channel
> >+ * @Channels - 16 bit bitmap. If the bit is set, channel value is read
> >+ * @buf - The channel values are stored here. if read fails error
> >+ * @raw - Return raw values without conversion
> >+ * value is stored
> >+ * Returns the number of successfully read channels.
> >+ */
> >+static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
> >+ u8 reg_base, unsigned
> >+ long channels, int *buf,
> >+ bool raw)
> >+{
> >+ int count = 0, count_req = 0, i;
> >+ u8 reg;
> >+
> >+ for_each_set_bit(i, &channels, TWL4030_MADC_MAX_CHANNELS) {
> Skip the temporary for reg. Yes, it gets used twice, but easier to read
> inline.
Also fixed in the style patch.
> >+ reg = reg_base + 2 * i;
> >+ buf[i] = twl4030_madc_channel_raw_read(madc, reg);
> >+ if (buf[i] < 0) {
> >+ dev_err(madc->dev,
> >+ "Unable to read register 0x%X\n", reg);
> >+ count_req++;
> >+ continue;
> >+ }
> I'd prefer this as
> if (raw) {
> count ++;
> } else {
> switch (i) {
> ..
> }
That increases the code indention by another level. I don't think
that's a good idea.
> Also near as I can tell count gets incremented unless there is an error
> anyway. So just increment it outside and allow this function to return the
> error code.
Returning error codes would change the existing API. I would prefer
to do this change after the old API is removed.
> >+ if (raw) {
> >+ count++;
> >+ continue;
> >+ }
> >+ switch (i) {
> These cases could do with a suitable #define to give them a name.
TWL4030_MADC_ADCIN10 is already in use for (1 << 10). I guess this
can be simplified more easily once the old API is removed.
> >+ case 10:
> >+ buf[i] = twl4030battery_current(buf[i]);
> >+ if (buf[i] < 0) {
> >+ dev_err(madc->dev, "err reading current\n");
> >+ count_req++;
> >+ } else {
> >+ count++;
> >+ buf[i] = buf[i] - 750;
> >+ }
> >+ break;
> >+ case 1:
> >+ buf[i] = twl4030battery_temperature(buf[i]);
> >+ if (buf[i] < 0) {
> >+ dev_err(madc->dev, "err reading temperature\n");
> >+ count_req++;
> >+ } else {
> >+ buf[i] -= 3;
> >+ count++;
> >+ }
> >+ break;
> >+ default:
> >+ count++;
> >+ /* Analog Input (V) = conv_result * step_size / R
> >+ * conv_result = decimal value of 10-bit conversion
> >+ * result
> >+ * step size = 1.5 / (2 ^ 10 -1)
> >+ * R = Prescaler ratio for input channels.
> >+ * Result given in mV hence multiplied by 1000.
> >+ */
> >+ buf[i] = (buf[i] * 3 * 1000 *
> >+ twl4030_divider_ratios[i].denominator)
> >+ / (2 * 1023 *
> >+ twl4030_divider_ratios[i].numerator);
> >+ }
> >+ }
> This is already apparant from the errors emmited earlier. I'd drop this
> and the count_req counter in general.
> Personally I'd error out in those cases anyway rather than carrying on
> with the rest of the channels. If it's worth of a dev_err it's worthy
> of getting out as fast as possible rather than trying to muddle on.
I also think this should be postponed until the old API is removed.
> >+ if (count_req)
> >+ dev_err(madc->dev, "%d channel conversion failed\n", count_req);
> >+
> >+ return count;
> >+}
> >+
> >+/*
> >+ * Enables irq.
> >+ * @madc - pointer to twl4030_madc_data struct
> >+ * @id - irq number to be enabled
> >+ * can take one of TWL4030_MADC_RT, TWL4030_MADC_SW1, TWL4030_MADC_SW2
> >+ * corresponding to RT, SW1, SW2 conversion requests.
> >+ * If the i2c read fails it returns an error else returns 0.
> >+ */
> >+static int twl4030_madc_enable_irq(struct twl4030_madc_data *madc, u8 id)
> >+{
> >+ u8 val;
> >+ int ret;
> >+
> >+ ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, &val, madc->imr);
> >+ if (ret) {
> >+ dev_err(madc->dev, "unable to read imr register 0x%X\n",
> >+ madc->imr);
> >+ return ret;
> >+ }
> A blank line here and in similar cases would slight aid readability.
> >+ val &= ~(1 << id);
> >+ ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, val, madc->imr);
> >+ if (ret) {
> >+ dev_err(madc->dev,
> >+ "unable to write imr register 0x%X\n", madc->imr);
> >+ return ret;
> >+
> Loose blank line here.
both fixed in the style patch.
> >+ }
> >+
> >+ return 0;
> >+}
> >+
> >+/*
> >+ * Disables irq.
> >+ * @madc - pointer to twl4030_madc_data struct
> >+ * @id - irq number to be disabled
> >+ * can take one of TWL4030_MADC_RT, TWL4030_MADC_SW1, TWL4030_MADC_SW2
> >+ * corresponding to RT, SW1, SW2 conversion requests.
> >+ * Returns error if i2c read/write fails.
> >+ */
> >+static int twl4030_madc_disable_irq(struct twl4030_madc_data *madc, u8 id)
> >+{
> >+ u8 val;
> >+ int ret;
> >+
> >+ ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, &val, madc->imr);
> >+ if (ret) {
> >+ dev_err(madc->dev, "unable to read imr register 0x%X\n",
> >+ madc->imr);
> >+ return ret;
> >+ }
> >+ val |= (1 << id);
> >+ ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, val, madc->imr);
> >+ if (ret) {
> >+ dev_err(madc->dev,
> >+ "unable to write imr register 0x%X\n", madc->imr);
> >+ return ret;
> >+ }
> >+
> >+ return 0;
> >+}
> >+
> >+static irqreturn_t twl4030_madc_threaded_irq_handler(int irq, void *_madc)
> >+{
> >+ struct twl4030_madc_data *madc = _madc;
> >+ const struct twl4030_madc_conversion_method *method;
> >+ u8 isr_val, imr_val;
> >+ int i, len, ret;
> >+ struct twl4030_madc_request *r;
> >+
> >+ mutex_lock(&madc->lock);
> >+ ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, &isr_val, madc->isr);
> >+ if (ret) {
> >+ dev_err(madc->dev, "unable to read isr register 0x%X\n",
> >+ madc->isr);
> >+ goto err_i2c;
> >+ }
> >+ ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, &imr_val, madc->imr);
> >+ if (ret) {
> >+ dev_err(madc->dev, "unable to read imr register 0x%X\n",
> >+ madc->imr);
> >+ goto err_i2c;
> >+ }
> >+ isr_val &= ~imr_val;
> >+ for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> >+ if (!(isr_val & (1 << i)))
> >+ continue;
> >+ ret = twl4030_madc_disable_irq(madc, i);
> >+ if (ret < 0)
> >+ dev_dbg(madc->dev, "Disable interrupt failed%d\n", i);
> >+ madc->requests[i].result_pending = 1;
> >+ }
> Can this and the previous loop not be combined thus simplifying some tests?
It probably can be, but I think this can wait until the old API is
removed.
> >+ for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> >+ r = &madc->requests[i];
> >+ /* No pending results for this method, move to next one */
> >+ if (!r->result_pending)
> >+ continue;
> >+ method = &twl4030_conversion_methods[r->method];
> >+ /* Read results */
> >+ len = twl4030_madc_read_channels(madc, method->rbase,
> >+ r->channels, r->rbuf, r->raw);
> >+ /* Return results to caller */
> >+ if (r->func_cb != NULL) {
> >+ r->func_cb(len, r->channels, r->rbuf);
> >+ r->func_cb = NULL;
> >+ }
> >+ /* Free request */
> >+ r->result_pending = 0;
> >+ r->active = 0;
> >+ }
> >+ mutex_unlock(&madc->lock);
> >+
> >+ return IRQ_HANDLED;
> >+
> >+err_i2c:
> >+ /*
> >+ * In case of error check whichever request is active
> >+ * and service the same.
> >+ */
> >+ for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> >+ r = &madc->requests[i];
> >+ if (r->active == 0)
> >+ continue;
> >+ method = &twl4030_conversion_methods[r->method];
> >+ /* Read results */
> >+ len = twl4030_madc_read_channels(madc, method->rbase,
> >+ r->channels, r->rbuf, r->raw);
> >+ /* Return results to caller */
> >+ if (r->func_cb != NULL) {
> >+ r->func_cb(len, r->channels, r->rbuf);
> >+ r->func_cb = NULL;
> >+ }
> >+ /* Free request */
> >+ r->result_pending = 0;
> >+ r->active = 0;
> >+ }
> >+ mutex_unlock(&madc->lock);
> >+
> >+ return IRQ_HANDLED;
> >+}
> >+
> This structure does seem a little complex. I'd have been more tempted
> by using a completion and having the calling function wait on it. How
> often does an actual callback make sense?
This is part of the old API, so let's convert all consumers to the
IIO API and deprecate the callback stuff.
> >+static int twl4030_madc_set_irq(struct twl4030_madc_data *madc,
> >+ struct twl4030_madc_request *req)
> >+{
> >+ struct twl4030_madc_request *p;
> >+ int ret;
> >+
> >+ p = &madc->requests[req->method];
> >+ memcpy(p, req, sizeof(*req));
> >+ ret = twl4030_madc_enable_irq(madc, req->method);
> >+ if (ret < 0) {
> >+ dev_err(madc->dev, "enable irq failed!!\n");
> >+ return ret;
> >+ }
> >+
> >+ return 0;
> >+}
> >+
> >+/*
> >+ * Function which enables the madc conversion
> >+ * by writing to the control register.
> >+ * @madc - pointer to twl4030_madc_data struct
> >+ * @conv_method - can be TWL4030_MADC_RT, TWL4030_MADC_SW2, TWL4030_MADC_SW1
> >+ * corresponding to RT SW1 or SW2 conversion methods.
> >+ * Returns 0 if succeeds else a negative error value
> >+ */
> >+static int twl4030_madc_start_conversion(struct twl4030_madc_data *madc,
> >+ int conv_method)
> >+{
> >+ const struct twl4030_madc_conversion_method *method;
> >+ int ret = 0;
> >+ method = &twl4030_conversion_methods[conv_method];
> >+ switch (conv_method) {
> Can we get here via any paths where these aren't the methods set?
conv_method is set from outside of the driver, so it's probably
safer to check. This can be simplified once the old API is removed.
> >+ case TWL4030_MADC_SW1:
> >+ case TWL4030_MADC_SW2:
> >+ ret = twl_i2c_write_u8(TWL4030_MODULE_MADC,
> >+ TWL4030_MADC_SW_START, method->ctrl);
> >+ if (ret) {
> >+ dev_err(madc->dev,
> >+ "unable to write ctrl register 0x%X\n",
> >+ method->ctrl);
> >+ return ret;
> >+ }
> >+ break;
> >+ default:
> >+ break;
> >+ }
> >+
> >+ return 0;
> >+}
> >+
> Could we fix up the kernel doc in this file in general. It's so nearly
> there but not quite!
I assume, that the driver will get a huge cleanup once all consumers
of the old API are converted. Maybe a deprecation flag should be
added together with this patchset?
> >+/*
> >+ * Function that waits for conversion to be ready
> >+ * @madc - pointer to twl4030_madc_data struct
> >+ * @timeout_ms - timeout value in milliseconds
> >+ * @status_reg - ctrl register
> >+ * returns 0 if succeeds else a negative error value
> >+ */
> >+static int twl4030_madc_wait_conversion_ready(struct twl4030_madc_data *madc,
> >+ unsigned int timeout_ms,
> >+ u8 status_reg)
> >+{
> >+ unsigned long timeout;
> >+ int ret;
> >+
> >+ timeout = jiffies + msecs_to_jiffies(timeout_ms);
> >+ do {
> >+ u8 reg;
> >+
> >+ ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, ®, status_reg);
> >+ if (ret) {
> >+ dev_err(madc->dev,
> >+ "unable to read status register 0x%X\n",
> >+ status_reg);
> >+ return ret;
> >+ }
> >+ if (!(reg & TWL4030_MADC_BUSY) && (reg & TWL4030_MADC_EOC_SW))
> >+ return 0;
> >+ usleep_range(500, 2000);
> >+ } while (!time_after(jiffies, timeout));
> >+ dev_err(madc->dev, "conversion timeout!\n");
> >+
> >+ return -EAGAIN;
> >+}
> >+
>
> >+/*
> >+ * An exported function which can be called from other kernel drivers.
> What uses this currently? Ideally we'd do this through the standard IIO paths
> for in kernel users. Right now the device tree mappings for that are less
> than ideal (ask Mark Rutland if you want some choice comments ;) Perhaps
> we take the view this can be cleaned up later. There are some elements in here
> we haven't yet looked at supporting for client drivers. Will have to think
> about that.
>
> It would be particularly nice if possible to use a generic battery driver
> for the two cases that seem to be using this functionality at the moment.
Currently there are 3 consumer drivers using this:
* twl4030-madc-battery
* rx51-battery
* twl4030-madc-hwmon
I guess the first two can be updated to use the IIO API and the last
one can be replaced by the IIO hwmon driver. Since it's an
in-kernel-api there should be no problems to simply remove the API
afterwards.
> >+ * @req twl4030_madc_request structure
> >+ * req->rbuf will be filled with read values of channels based on the
> >+ * channel index. If a particular channel reading fails there will
> >+ * be a negative error value in the corresponding array element.
> >+ * returns 0 if succeeds else error value
> >+ */
> >+int twl4030_madc_conversion(struct twl4030_madc_request *req)
> >+{
> >+ const struct twl4030_madc_conversion_method *method;
> >+ u8 ch_msb, ch_lsb;
> >+ int ret;
> >+
> >+ if (!req || !twl4030_madc)
> >+ return -EINVAL;
> >+
> >+ mutex_lock(&twl4030_madc->lock);
> >+ if (req->method < TWL4030_MADC_RT || req->method > TWL4030_MADC_SW2) {
> >+ ret = -EINVAL;
> >+ goto out;
> >+ }
> >+ /* Do we have a conversion request ongoing */
> >+ if (twl4030_madc->requests[req->method].active) {
> >+ ret = -EBUSY;
> >+ goto out;
> >+ }
> >+ ch_msb = (req->channels >> 8) & 0xff;
> >+ ch_lsb = req->channels & 0xff;
> >+ method = &twl4030_conversion_methods[req->method];
> >+ /* Select channels to be converted */
> >+ ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, ch_msb, method->sel + 1);
> >+ if (ret) {
> >+ dev_err(twl4030_madc->dev,
> >+ "unable to write sel register 0x%X\n", method->sel + 1);
> >+ goto out;
> >+ }
> >+ ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, ch_lsb, method->sel);
> >+ if (ret) {
> >+ dev_err(twl4030_madc->dev,
> >+ "unable to write sel register 0x%X\n", method->sel + 1);
> >+ goto out;
> >+ }
> >+ /* Select averaging for all channels if do_avg is set */
> >+ if (req->do_avg) {
> >+ ret = twl_i2c_write_u8(TWL4030_MODULE_MADC,
> >+ ch_msb, method->avg + 1);
> >+ if (ret) {
> >+ dev_err(twl4030_madc->dev,
> >+ "unable to write avg register 0x%X\n",
> >+ method->avg + 1);
> >+ goto out;
> >+ }
> >+ ret = twl_i2c_write_u8(TWL4030_MODULE_MADC,
> >+ ch_lsb, method->avg);
> >+ if (ret) {
> >+ dev_err(twl4030_madc->dev,
> Excesive error messages are sometimes irritating as they break up code
> flow. They are really irritating when incorrect ;)
Added to the fixes patch.
> >+ "unable to write sel reg 0x%X\n",
> >+ method->sel + 1);
> >+ goto out;
> >+ }
> >+ }
> >+ if (req->type == TWL4030_MADC_IRQ_ONESHOT && req->func_cb != NULL) {
> >+ ret = twl4030_madc_set_irq(twl4030_madc, req);
> >+ if (ret < 0)
> >+ goto out;
> >+ ret = twl4030_madc_start_conversion(twl4030_madc, req->method);
> >+ if (ret < 0)
> >+ goto out;
> >+ twl4030_madc->requests[req->method].active = 1;
> >+ ret = 0;
> >+ goto out;
> >+ }
> Is it possible to get here? This comment suggests not. If so, why have
> the test?
I guess it's an additional safety check. This function can be
simplified a lot once the old API is removed, so I guess this
can also be postponed.
> >+ /* With RT method we should not be here anymore */
> >+ if (req->method == TWL4030_MADC_RT) {
> >+ ret = -EINVAL;
> >+ goto out;
> >+ }
> >+ ret = twl4030_madc_start_conversion(twl4030_madc, req->method);
> >+ if (ret < 0)
> >+ goto out;
> >+ twl4030_madc->requests[req->method].active = 1;
> >+ /* Wait until conversion is ready (ctrl register returns EOC) */
> So no interrupts in this case?
> >+ ret = twl4030_madc_wait_conversion_ready(twl4030_madc, 5, method->ctrl);
> >+ if (ret) {
> >+ twl4030_madc->requests[req->method].active = 0;
> >+ goto out;
> >+ }
> >+ ret = twl4030_madc_read_channels(twl4030_madc, method->rbase,
> >+ req->channels, req->rbuf, req->raw);
> >+ twl4030_madc->requests[req->method].active = 0;
> >+
> >+out:
> >+ mutex_unlock(&twl4030_madc->lock);
> >+
> >+ return ret;
> >+}
> >+EXPORT_SYMBOL_GPL(twl4030_madc_conversion);
> >+
> >+/*
> Proper kerneldoc would be nice here. Otherwise the comment doesn't
> really add anything so I'd be tempted to drop it ;)
removed in fixes patch.
> >+ * Return channel value
> >+ * Or < 0 on failure.
> >+ */
> >+int twl4030_get_madc_conversion(int channel_no)
> >+{
> >+ struct twl4030_madc_request req;
> >+ int temp = 0;
> >+ int ret;
> >+
> >+ req.channels = (1 << channel_no);
> >+ req.method = TWL4030_MADC_SW2;
> >+ req.active = 0;
> >+ req.func_cb = NULL;
> >+ ret = twl4030_madc_conversion(&req);
> >+ if (ret < 0)
> >+ return ret;
> >+ if (req.rbuf[channel_no] > 0)
> >+ temp = req.rbuf[channel_no];
> >+
> >+ return temp;
> >+}
> >+EXPORT_SYMBOL_GPL(twl4030_get_madc_conversion);
> >+
> >+/*
> >+ * Function to enable or disable bias current for
> >+ * main battery type reading or temperature sensing
> >+ * @madc - pointer to twl4030_madc_data struct
> >+ * @chan - can be one of the two values
> What is a battery type reading? Voltage? Current? Charge?
It's used for battery type identification.
> >+ * TWL4030_BCI_ITHEN - Enables bias current for main battery type reading
> >+ * TWL4030_BCI_TYPEN - Enables bias current for main battery temperature
> These constants have rather incomprehensible names. Whilst I gues they
> are straight off the data sheet, seems to me that we could make them a little
> longer and easier to follow! The comment here helps, but sensible names
> would be better!
The names come from the datasheet. I think we should fix this after
the old API is removed, since the constants are exposed for the API.
> >+ * sensing
> >+ * @on - enable or disable chan.
> >+ */
> >+static int twl4030_madc_set_current_generator(struct twl4030_madc_data *madc,
> >+ int chan, int on)
> >+{
> >+ int ret;
> >+ u8 regval;
> >+
> >+ ret = twl_i2c_read_u8(TWL_MODULE_MAIN_CHARGE,
> >+ ®val, TWL4030_BCI_BCICTL1);
> >+ if (ret) {
> >+ dev_err(madc->dev, "unable to read BCICTL1 reg 0x%X",
> >+ TWL4030_BCI_BCICTL1);
> >+ return ret;
> >+ }
> Introduce an intermediate variable and this could be nice and easy to read
> Say
>
> int regmask;
> if (chan == 0)
> regmask = TWL4030_BCI_TYPEN;
> else
> regmask = TWL4030_BFI_ITHEN;
>
> if (on)
> regval |= regmask;
> else
> regval &= ~regmask;
>
> It's longer but doesn't give me a headache.
I added this to the fixes patch:
int regmask;
regmask = (chan == 0) ? TWL4030_BCI_TYPEN : TWL4030_BFI_ITHEN;
if (on)
regval |= regmask;
else
regval &= ~regmask;
> >+ if (on)
> >+ regval |= chan ? TWL4030_BCI_ITHEN : TWL4030_BCI_TYPEN;
> >+ else
> >+ regval &= chan ? ~TWL4030_BCI_ITHEN : ~TWL4030_BCI_TYPEN;
> >+ ret = twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE,
> >+ regval, TWL4030_BCI_BCICTL1);
> >+ if (ret) {
> >+ dev_err(madc->dev, "unable to write BCICTL1 reg 0x%X\n",
> >+ TWL4030_BCI_BCICTL1);
> >+ return ret;
> >+ }
> >+
> >+ return 0;
> >+}
> >+
> >+/*
> >+ * Function that sets MADC software power on bit to enable MADC
> >+ * @madc - pointer to twl4030_madc_data struct
> >+ * @on - Enable or disable MADC software powen on bit.
> >+ * returns error if i2c read/write fails else 0
> >+ */
> >+static int twl4030_madc_set_power(struct twl4030_madc_data *madc, int on)
> >+{
> >+ u8 regval;
> >+ int ret;
> >+
> >+ ret = twl_i2c_read_u8(TWL_MODULE_MAIN_CHARGE,
> >+ ®val, TWL4030_MADC_CTRL1);
> >+ if (ret) {
> >+ dev_err(madc->dev, "unable to read madc ctrl1 reg 0x%X\n",
> >+ TWL4030_MADC_CTRL1);
> >+ return ret;
> >+ }
> >+ if (on)
> >+ regval |= TWL4030_MADC_MADCON;
> >+ else
> >+ regval &= ~TWL4030_MADC_MADCON;
> >+ ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, regval, TWL4030_MADC_CTRL1);
> >+ if (ret) {
> >+ dev_err(madc->dev, "unable to write madc ctrl1 reg 0x%X\n",
> >+ TWL4030_MADC_CTRL1);
> >+ return ret;
> >+ }
> >+
> >+ return 0;
> >+}
> >+
> >+/*
> >+ * Initialize MADC and request for threaded irq
> >+ */
> >+static int twl4030_madc_probe(struct platform_device *pdev)
> >+{
> >+ struct twl4030_madc_data *madc;
> >+ struct twl4030_madc_platform_data *pdata = dev_get_platdata(&pdev->dev);
> >+ struct device_node *np = pdev->dev.of_node;
> >+ int irq, ret;
> >+ u8 regval;
> >+ struct iio_dev *iio_dev = NULL;
> >+
> >+ if (!pdata && !np) {
> >+ dev_err(&pdev->dev, "platform_data not available\n");
> >+ return -EINVAL;
> >+ }
> >+
> >+ iio_dev = devm_iio_device_alloc(&pdev->dev,
> >+ sizeof(struct twl4030_madc_data));
> >+ if (!iio_dev) {
> >+ dev_err(&pdev->dev, "failed allocating iio device\n");
> >+ return -ENOMEM;
> >+ }
> >+
> >+ madc = iio_priv(iio_dev);
> >+ madc->dev = &pdev->dev;
> >+
> >+ iio_dev->name = dev_name(&pdev->dev);
> >+ iio_dev->dev.parent = &pdev->dev;
> >+ iio_dev->dev.of_node = pdev->dev.of_node;
> >+ iio_dev->info = &twl4030_madc_iio_info;
> >+ iio_dev->modes = INDIO_DIRECT_MODE;
> >+ iio_dev->channels = twl4030_madc_iio_channels;
> >+ iio_dev->num_channels = 16;
> >+
> >+ /*
> >+ * Phoenix provides 2 interrupt lines. The first one is connected to
> >+ * the OMAP. The other one can be connected to the other processor such
> >+ * as modem. Hence two separate ISR and IMR registers.
> >+ */
> >+ if (pdata)
> >+ madc->use_second_irq = pdata->irq_line != 1;
> >+ else
> >+ madc->use_second_irq = false;
> >+
> >+ madc->imr = (madc->use_second_irq == 1) ?
> >+ TWL4030_MADC_IMR1 : TWL4030_MADC_IMR2;
> >+ madc->isr = (madc->use_second_irq == 1) ?
> >+ TWL4030_MADC_ISR1 : TWL4030_MADC_ISR2;
> >+
> >+ ret = twl4030_madc_set_power(madc, 1);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = twl4030_madc_set_current_generator(madc, 0, 1);
> >+ if (ret < 0)
> >+ goto err_current_generator;
> >+
> >+ ret = twl_i2c_read_u8(TWL_MODULE_MAIN_CHARGE,
> >+ ®val, TWL4030_BCI_BCICTL1);
> >+ if (ret) {
> >+ dev_err(&pdev->dev, "unable to read reg BCI CTL1 0x%X\n",
> >+ TWL4030_BCI_BCICTL1);
> >+ goto err_i2c;
> >+ }
> >+ regval |= TWL4030_BCI_MESBAT;
> >+ ret = twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE,
> >+ regval, TWL4030_BCI_BCICTL1);
> >+ if (ret) {
> >+ dev_err(&pdev->dev, "unable to write reg BCI Ctl1 0x%X\n",
> >+ TWL4030_BCI_BCICTL1);
> >+ goto err_i2c;
> >+ }
> >+
> >+ /* Check that MADC clock is on */
> >+ ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, ®val, TWL4030_REG_GPBR1);
> >+ if (ret) {
> Personally I'd rank the driver as rather to vebose on read error messages
> given they are pretty unusual. Ah well I guess each to their own.
All of this is not written by me.
> >+ dev_err(&pdev->dev, "unable to read reg GPBR1 0x%X\n",
> >+ TWL4030_REG_GPBR1);
> >+ goto err_i2c;
> >+ }
> >+
> >+ /* If MADC clk is not on, turn it on */
> >+ if (!(regval & TWL4030_GPBR1_MADC_HFCLK_EN)) {
> >+ dev_info(&pdev->dev, "clk disabled, enabling\n");
> >+ regval |= TWL4030_GPBR1_MADC_HFCLK_EN;
> >+ ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, regval,
> >+ TWL4030_REG_GPBR1);
> >+ if (ret) {
> >+ dev_err(&pdev->dev, "unable to write reg GPBR1 0x%X\n",
> >+ TWL4030_REG_GPBR1);
> >+ goto err_i2c;
> >+ }
> >+ }
> >+
> >+ platform_set_drvdata(pdev, iio_dev);
> >+ mutex_init(&madc->lock);
> >+
> >+ irq = platform_get_irq(pdev, 0);
>
> As ever using devm for a irq request makes for a bit of a review
> nightmare as we have to be very careful that nothing has been removed
> that this might use before the devm cleanup occurs (at end of remove).
> In this case the fact that the current generator is off and the adc unit
> is disabled sounds to me like it might cause interesting results in the
> interrupt handler?
>
> It's one of those things that will probably never happen but I find it
> hard to convince myself 'can't happen'
>
> >+ ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> >+ twl4030_madc_threaded_irq_handler,
> >+ IRQF_TRIGGER_RISING, "twl4030_madc", madc);
> >+ if (ret) {
> >+ dev_dbg(&pdev->dev, "could not request irq\n");
> >+ goto err_i2c;
> >+ }
>
> Err, where is this defined? Ah, found it above. I'm not keen on
> preventing multiple instances of a device like this. It is so easy
> to not do this I'd prefer to see the driver fixed to remove this.
This concept has also not introduced by me of course. I think we
should fix this after removal of the old API, since the old API does
not have an device parameter.
> >+ twl4030_madc = madc;
> >+
> >+ ret = iio_device_register(iio_dev);
> >+ if (ret) {
> >+ dev_dbg(&pdev->dev, "could not register iio device\n");
> >+ goto err_i2c;
> >+ }
> >+
> >+ return 0;
> >+
> >+err_i2c:
> >+ twl4030_madc_set_current_generator(madc, 0, 0);
> >+err_current_generator:
> >+ twl4030_madc_set_power(madc, 0);
> >+ return ret;
> >+}
> >+
> >+static int twl4030_madc_remove(struct platform_device *pdev)
> >+{
> >+ struct iio_dev *iio_dev = platform_get_drvdata(pdev);
> >+ struct twl4030_madc_data *madc = iio_priv(iio_dev);
> >+
> >+ twl4030_madc_set_current_generator(madc, 0, 0);
> >+ twl4030_madc_set_power(madc, 0);
> >+
> >+ iio_device_unregister(iio_dev);
> >+
> >+ return 0;
> >+}
> >+
> >+#ifdef CONFIG_OF
> >+static const struct of_device_id twl_madc_of_match[] = {
> >+ {.compatible = "ti,twl4030-madc", },
> >+ { },
> >+};
> >+MODULE_DEVICE_TABLE(of, twl_madc_of_match);
> >+#endif
> >+
> >+static struct platform_driver twl4030_madc_driver = {
> >+ .probe = twl4030_madc_probe,
> >+ .remove = twl4030_madc_remove,
> >+ .driver = {
> >+ .name = "twl4030_madc",
> >+ .owner = THIS_MODULE,
> >+ .of_match_table = of_match_ptr(twl_madc_of_match),
> >+ },
> >+};
> >+
> >+module_platform_driver(twl4030_madc_driver);
> >+
> >+MODULE_DESCRIPTION("TWL4030 ADC driver");
> >+MODULE_LICENSE("GPL");
> >+MODULE_AUTHOR("J Keerthy");
> >+MODULE_ALIAS("platform:twl4030_madc");
>
> Rest of patch was removal of code so no comments!
FYI: For me it was a bit odd to see inline comments about code lines
coming after the inline comment. Normally I receive patch reviews
with comments after the referenced code, which makes more sense imho.
-- Sebastian
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v4 03/10] Documentation: devicetree: Update Samsung FIMC DT binding
From: Sylwester Nawrocki @ 2014-02-22 22:52 UTC (permalink / raw)
To: Tomasz Figa
Cc: Mark Rutland, Sylwester Nawrocki, linux-media@vger.kernel.org,
devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, robh+dt@kernel.org,
galak@codeaurora.org, kyungmin.park@samsung.com,
kgene.kim@samsung.com, a.hajda@samsung.com, Mike Turquette,
Tomasz Figa
In-Reply-To: <5309241B.5000702@gmail.com>
On 02/22/2014 11:26 PM, Tomasz Figa wrote:
> [Ccing Mike]
>
> On 22.02.2014 23:02, Sylwester Nawrocki wrote:
>> On 02/21/2014 04:50 PM, Mark Rutland wrote:
>>> On Thu, Feb 20, 2014 at 07:40:30PM +0000, Sylwester Nawrocki wrote:
>>>> +- #clock-cells: from the common clock bindings
>>>> (../clock/clock-bindings.txt),
>>>> + must be 1. A clock provider is associated with the 'camera' node
>>>> and it should
>>>> + be referenced by external sensors that use clocks provided by the
>>>> SoC on
>>>> + CAM_*_CLKOUT pins. The clock specifier cell stores an index of a
>>>> clock.
>>>> + The indices are 0, 1 for CAM_A_CLKOUT, CAM_B_CLKOUT clocks
>>>> respectively.
>>>> +
>>>> +- clock-output-names: from the common clock bindings, should contain
>>>> names of
>>>> + clocks registered by the camera subsystem corresponding to
>>>> CAM_A_CLKOUT,
>>>> + CAM_B_CLKOUT output clocks, in this order. Parent clock of these
>>>> clocks are
>>>> + specified be first two entries of the clock-names property.
>>>
>>> Do you need this?
>>
>> All right, that might have been a bad idea, it mixes names of clocks
>> registered
>> by the main clock controller with names of clock input lines at the
>> device.
>> It's a mistake I have been usually sensitive to and now made it
>> myself. :/
>>
>> My intention was to maintain the clock tree, since the camera block
>> doesn't
>> generate the clock itself, it merely passes through the clocks from the
>> SoC main
>> clock controller (CMU). So clk parents need to be properly set and since
>> there is no clock-output-names property at the CMU DT node,
>> of_clk_get_parent_name() cannot be used.
>>
>> So presumably the DT binding would be only specifying that the sclk_cam0,
>> sclk_cam1 clock input entries are associated with output clocks named as
>> in clock-output-names property.
>>
>> And the driver could either:
>> 1) hard code those (well defined) CMU clock (clk parent) names,
>
> I don't think this would be a good idea, as those CMU clock names may
> vary between SoCs.
For the record, I'm not in favour of this approach, even though these clock
names happen to be same for all relevant SoCs.
>> 2) clk_get() its input clock, retrieve name with __clk_get_name() and
>> pass
>> it as parent name to clk_register() - it sounds a bit hacky though.
>
> This looks fine, at least until proper interface is added to CCF. Exynos
> audio subsystem clock driver does exactly the same.
Hmm, I looked at this driver and somehow missed that.
> However, the right thing would be to make it possible to use pointers to
> struct clk instead of strings to list parent(s). This could be done by
> adding .parents field (struct clk **) to clk_init_data struct and make
> clk_register() use it if available.
We would just need to ensure the locking is done properly.
--
Regards,
Sylwester
^ permalink raw reply
* [PATCH v2 3/5] dt-bindings: add documentation for s3c2412 clock controller
From: Heiko Stübner @ 2014-02-22 22:48 UTC (permalink / raw)
To: Kukjin Kim, devicetree, Pawel Moll, Stephen Warren, Ian Campbell
Cc: t.figa, mturquette, linux-arm-kernel, linux-samsung-soc, robh+dt,
mark.rutland
In-Reply-To: <1835515.oOiOlPWUhB@phil>
Describe the clock controller of the s3c2412.
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
.../bindings/clock/samsung,s3c2412-clock.txt | 50 ++++++++++++++++++++++
1 file changed, 50 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/samsung,s3c2412-clock.txt
diff --git a/Documentation/devicetree/bindings/clock/samsung,s3c2412-clock.txt b/Documentation/devicetree/bindings/clock/samsung,s3c2412-clock.txt
new file mode 100644
index 0000000..2b43096
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/samsung,s3c2412-clock.txt
@@ -0,0 +1,50 @@
+* Samsung S3C2412 Clock Controller
+
+The S3C2412 clock controller generates and supplies clock to various controllers
+within the SoC. The clock binding described here is applicable to the s3c2412
+and s3c2413 SoCs in the s3c24x family.
+
+Required Properties:
+
+- compatible: should be "samsung,s3c2412-clock"
+- reg: physical base address of the controller and length of memory mapped
+ region.
+- #clock-cells: should be 1.
+
+Each clock is assigned an identifier and client nodes can use this identifier
+to specify the clock which they consume. Some of the clocks are available only
+on a particular SoC.
+
+All available clocks are defined as preprocessor macros in
+dt-bindings/clock/s3c2412.h header and can be used in device
+tree sources.
+
+External clocks:
+
+There are several clocks that are generated outside the SoC. It is expected
+that they are defined using standard clock bindings with following
+clock-output-names:
+ - "xti" - crystal input - required,
+ - "ext" - external clock source - optional,
+
+Example: Clock controller node:
+
+ clocks: clock-controller@4c000000 {
+ compatible = "samsung,s3c2412-clock";
+ reg = <0x4c000000 0x20>;
+ #clock-cells = <1>;
+ };
+
+Example: UART controller node that consumes the clock generated by the clock
+ controller (refer to the standard clock bindings for information about
+ "clocks" and "clock-names" properties):
+
+ serial@50004000 {
+ compatible = "samsung,s3c2412-uart";
+ reg = <0x50004000 0x4000>;
+ interrupts = <1 23 3 4>, <1 23 4 4>;
+ clock-names = "uart", "clk_uart_baud2", "clk_uart_baud3";
+ clocks = <&clocks PCLK_UART0>, <&clocks PCLK_UART0>,
+ <&clocks SCLK_UART>;
+ status = "disabled";
+ };
--
1.8.5.3
^ permalink raw reply related
* Re: [PATCH v4 03/10] Documentation: devicetree: Update Samsung FIMC DT binding
From: Tomasz Figa @ 2014-02-22 22:26 UTC (permalink / raw)
To: Sylwester Nawrocki, Mark Rutland
Cc: devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
Mike Turquette, Tomasz Figa, a.hajda@samsung.com,
kyungmin.park@samsung.com, robh+dt@kernel.org, Sylwester Nawrocki,
galak@codeaurora.org, kgene.kim@samsung.com,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <53091E72.3090107@gmail.com>
[Ccing Mike]
On 22.02.2014 23:02, Sylwester Nawrocki wrote:
> On 02/21/2014 04:50 PM, Mark Rutland wrote:
>> On Thu, Feb 20, 2014 at 07:40:30PM +0000, Sylwester Nawrocki wrote:
>>> +- #clock-cells: from the common clock bindings
>>> (../clock/clock-bindings.txt),
>>> + must be 1. A clock provider is associated with the 'camera' node
>>> and it should
>>> + be referenced by external sensors that use clocks provided by the
>>> SoC on
>>> + CAM_*_CLKOUT pins. The clock specifier cell stores an index of a
>>> clock.
>>> + The indices are 0, 1 for CAM_A_CLKOUT, CAM_B_CLKOUT clocks
>>> respectively.
>>> +
>>> +- clock-output-names: from the common clock bindings, should contain
>>> names of
>>> + clocks registered by the camera subsystem corresponding to
>>> CAM_A_CLKOUT,
>>> + CAM_B_CLKOUT output clocks, in this order. Parent clock of these
>>> clocks are
>>> + specified be first two entries of the clock-names property.
>>
>> Do you need this?
>
> All right, that might have been a bad idea, it mixes names of clocks
> registered
> by the main clock controller with names of clock input lines at the device.
> It's a mistake I have been usually sensitive to and now made it myself. :/
>
> My intention was to maintain the clock tree, since the camera block doesn't
> generate the clock itself, it merely passes through the clocks from the
> SoC main
> clock controller (CMU). So clk parents need to be properly set and since
> there
> is no clock-output-names property at the CMU DT node,
> of_clk_get_parent_name()
> cannot be used.
>
> So presumably the DT binding would be only specifying that the sclk_cam0,
> sclk_cam1 clock input entries are associated with output clocks named as
> in clock-output-names property.
>
> And the driver could either:
> 1) hard code those (well defined) CMU clock (clk parent) names,
I don't think this would be a good idea, as those CMU clock names may
vary between SoCs.
> 2) clk_get() its input clock, retrieve name with __clk_get_name() and
> pass
> it as parent name to clk_register() - it sounds a bit hacky though.
This looks fine, at least until proper interface is added to CCF. Exynos
audio subsystem clock driver does exactly the same.
However, the right thing would be to make it possible to use pointers to
struct clk instead of strings to list parent(s). This could be done by
adding .parents field (struct clk **) to clk_init_data struct and make
clk_register() use it if available.
>
> The output clock names could be also well defined by the binding per the
> IP's
> compatible. Nevertheless using clock-output-names seems cleaner to me than
> defining character strings in the driver.
>
> What do you think ?
<RANT>
Personally, I don't like clock-output-names at all. The idea of having a
global namespace for all clock providers looks flawed to me. This
property only tries to work around the not-quite-right design by giving
device tree the right to force CCF to use particular internal clock
identifiers and avoid collisions if two providers happen to have the
same internal clock names.
I believe there should be completely no need for clock-output-names,
(other than a textual label for debugging purposes, analogically to
regulator-name). If one clock provider needs a clock from another, then
you should list it using clock bindings in node of the former, not rely
on some static name assignments.
Then, after eliminating the need to use static names anymore, the
namespace could be split into per-provider namespaces and we would have
no collision issue anymore.
Of course it's probably to late already for such changes, as there are
already systems relying on clock-output-names (i.e. without inter-IP
clock dependencies listed using clock bindings) and we would have to
keep backwards compatibility anyway...
</RANT>
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v4 07/10] exynos4-is: Add clock provider for the SCLK_CAM clock outputs
From: Sylwester Nawrocki @ 2014-02-22 22:18 UTC (permalink / raw)
To: Mark Rutland
Cc: Sylwester Nawrocki,
linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org,
kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org,
kgene.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org,
a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org
In-Reply-To: <20140221160504.GG20449-NuALmloUBlrZROr8t4l/smS4ubULX0JqMm0uRHvK7Nw@public.gmane.org>
On 02/21/2014 05:05 PM, Mark Rutland wrote:
>> ---
>> drivers/media/platform/exynos4-is/media-dev.c | 110 +++++++++++++++++++++++++
>> drivers/media/platform/exynos4-is/media-dev.h | 19 ++++-
>> 2 files changed, 128 insertions(+), 1 deletion(-)
...
>
>> +static int fimc_md_register_clk_provider(struct fimc_md *fmd)
>> +{
>> + struct cam_clk_provider *cp =&fmd->clk_provider;
>> + struct device *dev =&fmd->pdev->dev;
>> + int i, ret;
>> +
>> + for (i = 0; i< ARRAY_SIZE(cp->clks); i++) {
>> + struct cam_clk *camclk =&cp->camclk[i];
>> + struct clk_init_data init;
>> +
>> + ret = of_property_read_string_index(dev->of_node,
>> + "clock-output-names", i,&init.name);
>
> Are there not well-defined names for the clock outputs of the block?
There are, but this driver handles multiple SoCs so I thought it's better
to define these names in devicetree. They are supposed to be unique. Isn't
that what the clock-output-names property is for ?
>> + if (ret< 0)
>> + break;
>> +
>> + ret = of_property_read_string_index(dev->of_node,
>> + "clock-names", i, init.parent_names);
>
> This shouldn't be a parent name. It should be the input line name.
>
> I don't think this makes sense.
>
> Why do you need the name of the parent clock?
As explained in previous e-mail, it is needed to maintain the clock tree,
the parent clock names must be set properly. As the above is not correct
I could hard code these names or retrieve with __clk_get_name() from proper
input clocks. of_clk_get_parent_name() cannot be used.
>> + if (ret< 0)
>> + break;
>> +
>> + init.num_parents = 1;
>> + init.ops =&cam_clk_ops;
>> + init.flags = CLK_SET_RATE_PARENT;
>> + camclk->hw.init =&init;
>> + camclk->fmd = fmd;
>> +
>> + cp->clks[i] = clk_register(NULL,&camclk->hw);
>> + if (IS_ERR(cp->clks[i])) {
>> + dev_err(dev, "failed to register clock: %s (%ld)\n",
>> + init.name, PTR_ERR(cp->clks[i]));
>> + ret = PTR_ERR(cp->clks[i]);
>> + goto err;
>> + }
>> + cp->num_clocks++;
>> + }
>> +
>> + if (cp->num_clocks == 0) {
>> + dev_warn(dev, "clk provider not registered\n");
>> + return 0;
>> + }
>> +
>> + cp->clk_data.clks = cp->clks;
>> + cp->clk_data.clk_num = cp->num_clocks;
>> + cp->of_node = dev->of_node;
>> + ret = of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
>> + &cp->clk_data);
>
> Are _all_ of the input clock lines available to children in hardware?
This code is only for two clocks, ARRAY_SIZE(cp->clks) == 2 and
cp->num_clocks can't be greater than 2. Nevertheless, for some SoC
variants this driver handles (e.g. S5PV210) there can be only up to
two clocks listed in the camera node.
The camera interface IP block doesn't generate the clock itself, it just
passes it through when is active, i.e. its other clocks are enabled and
the power domain activated.
> The binding and commit message(s) implied only two clocks were, so
> what's the point in exporting clocks which aren't available?
I'm not sure what makes you think there is more than two ?
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC 0/3] ahci_platform: drop support for a bunch of obsolete platform device types
From: Tejun Heo @ 2014-02-22 22:13 UTC (permalink / raw)
To: Hans de Goede
Cc: Marek Vasut, Maxime Ripard, Oliver Schinagl, Richard Zhu,
Roger Quadros, Lee Jones, linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <1393086175-6383-1-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Sat, Feb 22, 2014 at 05:22:52PM +0100, Hans de Goede wrote:
> Hi Tejun, Marek,
>
> This series, which is to be applied on top of my ahci_platform restructering
> series, mostly speaks for itself.
>
> I've some doubts about the 2nd patch, which removes the imx53-ahci platform
> device support from ahci_platform.c. I'm pretty sure this is obsolete and
> replaced by Marek's ahci_imx.c work, but not 100%, which is why this series
> is marked as RFC. Marek can you confirm that the code the 2nd patch removes
> is inded dead code?
>
> Tejun, if Marek confirms this is indeed dead code feel free to apply this
> despite the RFC marking.
Applied 1-3 to libata/for-3.15.
Thanks.
--
tejun
^ permalink raw reply
* Re: [RFC 0/3] ahci_platform: drop support for a bunch of obsolete platform device types
From: Marek Vasut @ 2014-02-22 22:04 UTC (permalink / raw)
To: Hans de Goede
Cc: Tejun Heo, Maxime Ripard, Oliver Schinagl, Richard Zhu,
Roger Quadros, Lee Jones, linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <1393086175-6383-1-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Saturday, February 22, 2014 at 05:22:52 PM, Hans de Goede wrote:
> Hi Tejun, Marek,
>
> This series, which is to be applied on top of my ahci_platform
> restructering series, mostly speaks for itself.
>
> I've some doubts about the 2nd patch, which removes the imx53-ahci platform
> device support from ahci_platform.c. I'm pretty sure this is obsolete and
> replaced by Marek's ahci_imx.c work, but not 100%, which is why this series
> is marked as RFC. Marek can you confirm that the code the 2nd patch removes
> is inded dead code?
I can confirm that with your sunxi 15-patch patchset + these three patches , the
SATA port still works on i.MX53 DENX M53EVK board. The behavior from the user's
point of view is unchanged.
> Tejun, if Marek confirms this is indeed dead code feel free to apply this
> despite the RFC marking.
>
> Thanks & Regards,
>
> Hans
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH v4 03/10] Documentation: devicetree: Update Samsung FIMC DT binding
From: Sylwester Nawrocki @ 2014-02-22 22:02 UTC (permalink / raw)
To: Mark Rutland
Cc: Sylwester Nawrocki, linux-media@vger.kernel.org,
devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, robh+dt@kernel.org,
galak@codeaurora.org, kyungmin.park@samsung.com,
kgene.kim@samsung.com, a.hajda@samsung.com
In-Reply-To: <20140221155023.GF20449@e106331-lin.cambridge.arm.com>
On 02/21/2014 04:50 PM, Mark Rutland wrote:
> On Thu, Feb 20, 2014 at 07:40:30PM +0000, Sylwester Nawrocki wrote:
>> This patch documents following updates of the Exynos4 SoC camera subsystem
>> devicetree binding:
>> - addition of #clock-cells property to 'camera' node - the #clock-cells
>> property is needed when the sensor sub-devices use clock provided by
>> the camera host interface;
>> - addition of an optional clock-output-names property;
>> - change of the clock-frequency at image sensor node from mandatory to
>> an optional property - there should be no need to require this property
>> by the camera host device binding, a default frequency value can ofen
>> be used;
>> - addition of a requirement of specific order of values in clocks/
>> clock-names properties, so the first two entry in the clock-names
>> property can be used as parent clock names for the camera master
>> clock provider. It happens all in-kernel dts files list the clock
>> in such order, thus there should be no regression as far as in-kernel
>> dts files are concerned.
>
> I'm not sure I follow the reasoning here. Why does this matter? Why can
> child nodes not get these by name if they have to?
These input clocks won't be used directly by child nodes. More details
below...
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Acked-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>> .../devicetree/bindings/media/samsung-fimc.txt | 36 +++++++++++++++-----
>> 1 file changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> index 96312f6..1a5820d 100644
>> --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> @@ -20,6 +20,7 @@ Required properties:
>> the clock-names property;
>> - clock-names : must contain "sclk_cam0", "sclk_cam1", "pxl_async0",
>> "pxl_async1" entries, matching entries in the clocks property.
>> + First two entries must be "sclk_cam0", "sclk_cam1".
>
> I don't think this is a good idea.
>
>>
>> The pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt must be used
>> to define a required pinctrl state named "default" and optional pinctrl states:
>> @@ -32,6 +33,22 @@ way around.
>>
>> The 'camera' node must include at least one 'fimc' child node.
>>
>> +Optional properties (*:
>
> Is that a smiley face?
Sorry, it was supposed to be a reference mark, will remove this.
>> +- #clock-cells: from the common clock bindings (../clock/clock-bindings.txt),
>> + must be 1. A clock provider is associated with the 'camera' node and it should
>> + be referenced by external sensors that use clocks provided by the SoC on
>> + CAM_*_CLKOUT pins. The clock specifier cell stores an index of a clock.
>> + The indices are 0, 1 for CAM_A_CLKOUT, CAM_B_CLKOUT clocks respectively.
>> +
>> +- clock-output-names: from the common clock bindings, should contain names of
>> + clocks registered by the camera subsystem corresponding to CAM_A_CLKOUT,
>> + CAM_B_CLKOUT output clocks, in this order. Parent clock of these clocks are
>> + specified be first two entries of the clock-names property.
>
> Do you need this?
All right, that might have been a bad idea, it mixes names of clocks
registered
by the main clock controller with names of clock input lines at the device.
It's a mistake I have been usually sensitive to and now made it myself. :/
My intention was to maintain the clock tree, since the camera block doesn't
generate the clock itself, it merely passes through the clocks from the
SoC main
clock controller (CMU). So clk parents need to be properly set and since
there
is no clock-output-names property at the CMU DT node,
of_clk_get_parent_name()
cannot be used.
So presumably the DT binding would be only specifying that the sclk_cam0,
sclk_cam1 clock input entries are associated with output clocks named as
in clock-output-names property.
And the driver could either:
1) hard code those (well defined) CMU clock (clk parent) names,
2) clk_get() its input clock, retrieve name with __clk_get_name() and
pass
it as parent name to clk_register() - it sounds a bit hacky though.
The output clock names could be also well defined by the binding per the
IP's
compatible. Nevertheless using clock-output-names seems cleaner to me than
defining character strings in the driver.
What do you think ?
> That's not how clock-names is supposed to work. The clock-names property
> is for the names of the _input_ clock lines on the device, not the
> output names on whichever parent clock they came from.
Yes, I agree, that was a pretty bad abuse of the bindings.
> Any clock-names property description should define absolutely the set of
> names. As this does not, NAK.
>
>> +
>> +(* #clock-cells and clock-output-names are mandatory properties if external
>> +image sensor devices reference 'camera' device node as a clock provider.
>
> s/(*/Note:/
Thanks, will fix that.
>> 'fimc' device nodes
>> -------------------
>>
>> @@ -97,8 +114,8 @@ Image sensor nodes
>> The sensor device nodes should be added to their control bus controller (e.g.
>> I2C0) nodes and linked to a port node in the csis or the parallel-ports node,
>> using the common video interfaces bindings, defined in video-interfaces.txt.
>> -The implementation of this bindings requires clock-frequency property to be
>> -present in the sensor device nodes.
>> +An optional clock-frequency property needs to be present in the sensor device
>> +nodes. Default value when this property is not present is 24 MHz.
>
> s/needs to/should/ ?
Sounds better, I'll change it.
> What is this the frequency of?
It's frequency of an external sensor's master clock, which is provided
by the SoC.
Since the sensors have now possibility to control the clock themselves
the host
interface (binding) doesn't have to care about that frequency. This is
supposed
to be an assigned frequency, it has been discussed already on devicetree
mailing
list a few months ago.
--
Regards,
Sylwester
^ permalink raw reply
* Re: [PATCH v7 13/15] ARM: sun4i: dt: Remove grouping + simple-bus compatible for regulators
From: Maxime Ripard @ 2014-02-22 21:44 UTC (permalink / raw)
To: Hans de Goede
Cc: Tejun Heo, Oliver Schinagl, Richard Zhu, Roger Quadros, Lee Jones,
linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <1393084424-31099-14-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 571 bytes --]
Hi,
On Sat, Feb 22, 2014 at 04:53:42PM +0100, Hans de Goede wrote:
> According to Documentation/devicetree/bindings/regulator/regulator.txt
> regulator nodes should not be placed under 'simple-bus'.
>
> Mark Rutland also explains about it at:
> http://www.spinics.net/lists/linux-usb/msg101497.html
>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Applied in my sunxi/dt-for-3.15 branch.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v7 07/15] ARM: sunxi: Add support for Allwinner SUNXi SoCs sata to ahci_platform
From: Tejun Heo @ 2014-02-22 20:40 UTC (permalink / raw)
To: Hans de Goede
Cc: Maxime Ripard, Oliver Schinagl, Richard Zhu, Roger Quadros,
Lee Jones, linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <1393084424-31099-8-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Sat, Feb 22, 2014 at 04:53:36PM +0100, Hans de Goede wrote:
...
> +static int ahci_sunxi_phy_init(struct device *dev, void __iomem *reg_base)
> +{
> + u32 reg_val;
> + int timeout;
> +
> + /* This magic is from the original code */
> + writel(0, reg_base + AHCI_RWCR);
> + mdelay(5);
> +
> + sunxi_setbits(reg_base + AHCI_PHYCS1R, BIT(19));
> + sunxi_clrsetbits(reg_base + AHCI_PHYCS0R,
> + (0x7 << 24),
> + (0x5 << 24) | BIT(23) | BIT(18));
> + sunxi_clrsetbits(reg_base + AHCI_PHYCS1R,
> + (0x3 << 16) | (0x1f << 8) | (0x3 << 6),
> + (0x2 << 16) | (0x6 << 8) | (0x2 << 6));
> + sunxi_setbits(reg_base + AHCI_PHYCS1R, BIT(28) | BIT(15));
> + sunxi_clrbits(reg_base + AHCI_PHYCS1R, BIT(19));
> + sunxi_clrsetbits(reg_base + AHCI_PHYCS0R,
> + (0x7 << 20), (0x3 << 20));
> + sunxi_clrsetbits(reg_base + AHCI_PHYCS2R,
> + (0x1f << 5), (0x19 << 5));
> + mdelay(5);
Please use msleep() instead. This is called with full process
context. mdelay() is almost always wrong. Even if the hardware is
broken enough to require millisec level breather, the better thing to
do would be using threaded handler and using msleep(), not mdelay().
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v7 00/15] ahci: library-ise ahci_platform, add sunxi driver and cleanup imx driver
From: Tejun Heo @ 2014-02-22 20:37 UTC (permalink / raw)
To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
Cc: Maxime Ripard, Oliver Schinagl, Richard Zhu, Roger Quadros,
Lee Jones, linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree
In-Reply-To: <5308CFC8.4020400-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Sat, Feb 22, 2014 at 05:26:48PM +0100, Hans de Goede wrote:
> Tejun, can you please add patches 1-12 to your ata tree for 3.15 ?
Applied 1-12 to libata/for-3.15 with comment format slightly updated.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v4 10/22] ARM: MM: Add DT binding for Feroceon L2 cache
From: Jason Cooper @ 2014-02-22 20:11 UTC (permalink / raw)
To: Andrew Lunn, Russell King - ARM Linux
Cc: Sebastian Hesselbarth, Gregory Clement, linux ARM,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1393096504-11844-11-git-send-email-andrew-g2DYL2Zd6BY@public.gmane.org>
Russell,
On Sat, Feb 22, 2014 at 08:14:52PM +0100, Andrew Lunn wrote:
> Instantiate the L2 cache from DT. Indicate in DT where the cache
> control register is so that it is possible to enable/disable write
> through on the CPU.
>
> Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
> Tested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
> v2:
> Change compatible strings to follow l2x0 convention
> Only expect register for kirkwood-cache.
> Default to write through if no DT node.
> Rename writethrough to wt-override to follow l2cc binding.
> Split kirkwood.dtsi change into a patch of its own.
> v3
> Remove wt-override from the binding
> Respect CONFIG_CACHE_FEROCEON_L2_WRITETHROUGH
> ---
> .../devicetree/bindings/arm/mrvl/feroceon.txt | 16 ++++++++
> arch/arm/include/asm/hardware/cache-feroceon-l2.h | 2 +
> arch/arm/mach-kirkwood/board-dt.c | 18 ++-------
> arch/arm/mm/cache-feroceon-l2.c | 43 ++++++++++++++++++++++
> 4 files changed, 64 insertions(+), 15 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/arm/mrvl/feroceon.txt
I've asked Andrew to place the other two patches into your patch
tracker. The rest of the series depends on this one, though. Could you
please Ack it so that we can keep it together through mvebu/arm-soc?
thx,
Jason.
> diff --git a/Documentation/devicetree/bindings/arm/mrvl/feroceon.txt b/Documentation/devicetree/bindings/arm/mrvl/feroceon.txt
> new file mode 100644
> index 000000000000..0d244b999d10
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mrvl/feroceon.txt
> @@ -0,0 +1,16 @@
> +* Marvell Feroceon Cache
> +
> +Required properties:
> +- compatible : Should be either "marvell,feroceon-cache" or
> + "marvell,kirkwood-cache".
> +
> +Optional properties:
> +- reg : Address of the L2 cache control register. Mandatory for
> + "marvell,kirkwood-cache", not used by "marvell,feroceon-cache"
> +
> +
> +Example:
> + l2: l2-cache@20128 {
> + compatible = "marvell,kirkwood-cache";
> + reg = <0x20128 0x4>;
> + };
> diff --git a/arch/arm/include/asm/hardware/cache-feroceon-l2.h b/arch/arm/include/asm/hardware/cache-feroceon-l2.h
> index 8edd330aabf6..12e1588dc4f1 100644
> --- a/arch/arm/include/asm/hardware/cache-feroceon-l2.h
> +++ b/arch/arm/include/asm/hardware/cache-feroceon-l2.h
> @@ -9,3 +9,5 @@
> */
>
> extern void __init feroceon_l2_init(int l2_wt_override);
> +extern int __init feroceon_of_init(void);
> +
> diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
> index 29c246858d5a..ec0702c02d6c 100644
> --- a/arch/arm/mach-kirkwood/board-dt.c
> +++ b/arch/arm/mach-kirkwood/board-dt.c
> @@ -42,19 +42,6 @@ static void __init kirkwood_map_io(void)
> iotable_init(kirkwood_io_desc, ARRAY_SIZE(kirkwood_io_desc));
> }
>
> -static void __init kirkwood_l2_init(void)
> -{
> -#ifdef CONFIG_CACHE_FEROCEON_L2
> -#ifdef CONFIG_CACHE_FEROCEON_L2_WRITETHROUGH
> - writel(readl(L2_CONFIG_REG) | L2_WRITETHROUGH, L2_CONFIG_REG);
> - feroceon_l2_init(1);
> -#else
> - writel(readl(L2_CONFIG_REG) & ~L2_WRITETHROUGH, L2_CONFIG_REG);
> - feroceon_l2_init(0);
> -#endif
> -#endif
> -}
> -
> static struct resource kirkwood_cpufreq_resources[] = {
> [0] = {
> .start = CPU_CONTROL_PHYS,
> @@ -211,8 +198,9 @@ static void __init kirkwood_dt_init(void)
>
> BUG_ON(mvebu_mbus_dt_init());
>
> - kirkwood_l2_init();
> -
> +#ifdef CONFIG_CACHE_FEROCEON_L2
> + feroceon_of_init();
> +#endif
> kirkwood_cpufreq_init();
> kirkwood_cpuidle_init();
>
> diff --git a/arch/arm/mm/cache-feroceon-l2.c b/arch/arm/mm/cache-feroceon-l2.c
> index 898362e7972b..8dc1a2b5a8ed 100644
> --- a/arch/arm/mm/cache-feroceon-l2.c
> +++ b/arch/arm/mm/cache-feroceon-l2.c
> @@ -13,11 +13,16 @@
> */
>
> #include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> #include <linux/highmem.h>
> +#include <linux/io.h>
> #include <asm/cacheflush.h>
> #include <asm/cp15.h>
> #include <asm/hardware/cache-feroceon-l2.h>
>
> +#define L2_WRITETHROUGH_KIRKWOOD BIT(4)
> +
> /*
> * Low-level cache maintenance operations.
> *
> @@ -350,3 +355,41 @@ void __init feroceon_l2_init(int __l2_wt_override)
> printk(KERN_INFO "Feroceon L2: Cache support initialised%s.\n",
> l2_wt_override ? ", in WT override mode" : "");
> }
> +#ifdef CONFIG_OF
> +static const struct of_device_id feroceon_ids[] __initconst = {
> + { .compatible = "marvell,kirkwood-cache"},
> + { .compatible = "marvell,feroceon-cache"},
> + {}
> +};
> +
> +int __init feroceon_of_init(void)
> +{
> + struct device_node *node;
> + void __iomem *base;
> + bool l2_wt_override = false;
> + struct resource res;
> +
> +#if defined(CONFIG_CACHE_FEROCEON_L2_WRITETHROUGH)
> + l2_wt_override = true;
> +#endif
> +
> + node = of_find_matching_node(NULL, feroceon_ids);
> + if (node && of_device_is_compatible(node, "marvell,kirkwood-cache")) {
> + if (of_address_to_resource(node, 0, &res))
> + return -ENODEV;
> +
> + base = ioremap(res.start, resource_size(&res));
> + if (!base)
> + return -ENOMEM;
> +
> + if (l2_wt_override)
> + writel(readl(base) | L2_WRITETHROUGH_KIRKWOOD, base);
> + else
> + writel(readl(base) & ~L2_WRITETHROUGH_KIRKWOOD, base);
> + }
> +
> + feroceon_l2_init(l2_wt_override);
> +
> + return 0;
> +}
> +#endif
> --
> 1.8.5.3
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v4 10/22] ARM: MM: Add DT binding for Feroceon L2 cache
From: Andrew Lunn @ 2014-02-22 19:14 UTC (permalink / raw)
To: Jason Cooper, Sebastian Hesselbarth, Gregory Clement
Cc: linux ARM, Andrew Lunn, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1393096504-11844-1-git-send-email-andrew-g2DYL2Zd6BY@public.gmane.org>
Instantiate the L2 cache from DT. Indicate in DT where the cache
control register is so that it is possible to enable/disable write
through on the CPU.
Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Tested-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
v2:
Change compatible strings to follow l2x0 convention
Only expect register for kirkwood-cache.
Default to write through if no DT node.
Rename writethrough to wt-override to follow l2cc binding.
Split kirkwood.dtsi change into a patch of its own.
v3
Remove wt-override from the binding
Respect CONFIG_CACHE_FEROCEON_L2_WRITETHROUGH
---
.../devicetree/bindings/arm/mrvl/feroceon.txt | 16 ++++++++
arch/arm/include/asm/hardware/cache-feroceon-l2.h | 2 +
arch/arm/mach-kirkwood/board-dt.c | 18 ++-------
arch/arm/mm/cache-feroceon-l2.c | 43 ++++++++++++++++++++++
4 files changed, 64 insertions(+), 15 deletions(-)
create mode 100644 Documentation/devicetree/bindings/arm/mrvl/feroceon.txt
diff --git a/Documentation/devicetree/bindings/arm/mrvl/feroceon.txt b/Documentation/devicetree/bindings/arm/mrvl/feroceon.txt
new file mode 100644
index 000000000000..0d244b999d10
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mrvl/feroceon.txt
@@ -0,0 +1,16 @@
+* Marvell Feroceon Cache
+
+Required properties:
+- compatible : Should be either "marvell,feroceon-cache" or
+ "marvell,kirkwood-cache".
+
+Optional properties:
+- reg : Address of the L2 cache control register. Mandatory for
+ "marvell,kirkwood-cache", not used by "marvell,feroceon-cache"
+
+
+Example:
+ l2: l2-cache@20128 {
+ compatible = "marvell,kirkwood-cache";
+ reg = <0x20128 0x4>;
+ };
diff --git a/arch/arm/include/asm/hardware/cache-feroceon-l2.h b/arch/arm/include/asm/hardware/cache-feroceon-l2.h
index 8edd330aabf6..12e1588dc4f1 100644
--- a/arch/arm/include/asm/hardware/cache-feroceon-l2.h
+++ b/arch/arm/include/asm/hardware/cache-feroceon-l2.h
@@ -9,3 +9,5 @@
*/
extern void __init feroceon_l2_init(int l2_wt_override);
+extern int __init feroceon_of_init(void);
+
diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
index 29c246858d5a..ec0702c02d6c 100644
--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c
@@ -42,19 +42,6 @@ static void __init kirkwood_map_io(void)
iotable_init(kirkwood_io_desc, ARRAY_SIZE(kirkwood_io_desc));
}
-static void __init kirkwood_l2_init(void)
-{
-#ifdef CONFIG_CACHE_FEROCEON_L2
-#ifdef CONFIG_CACHE_FEROCEON_L2_WRITETHROUGH
- writel(readl(L2_CONFIG_REG) | L2_WRITETHROUGH, L2_CONFIG_REG);
- feroceon_l2_init(1);
-#else
- writel(readl(L2_CONFIG_REG) & ~L2_WRITETHROUGH, L2_CONFIG_REG);
- feroceon_l2_init(0);
-#endif
-#endif
-}
-
static struct resource kirkwood_cpufreq_resources[] = {
[0] = {
.start = CPU_CONTROL_PHYS,
@@ -211,8 +198,9 @@ static void __init kirkwood_dt_init(void)
BUG_ON(mvebu_mbus_dt_init());
- kirkwood_l2_init();
-
+#ifdef CONFIG_CACHE_FEROCEON_L2
+ feroceon_of_init();
+#endif
kirkwood_cpufreq_init();
kirkwood_cpuidle_init();
diff --git a/arch/arm/mm/cache-feroceon-l2.c b/arch/arm/mm/cache-feroceon-l2.c
index 898362e7972b..8dc1a2b5a8ed 100644
--- a/arch/arm/mm/cache-feroceon-l2.c
+++ b/arch/arm/mm/cache-feroceon-l2.c
@@ -13,11 +13,16 @@
*/
#include <linux/init.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
#include <linux/highmem.h>
+#include <linux/io.h>
#include <asm/cacheflush.h>
#include <asm/cp15.h>
#include <asm/hardware/cache-feroceon-l2.h>
+#define L2_WRITETHROUGH_KIRKWOOD BIT(4)
+
/*
* Low-level cache maintenance operations.
*
@@ -350,3 +355,41 @@ void __init feroceon_l2_init(int __l2_wt_override)
printk(KERN_INFO "Feroceon L2: Cache support initialised%s.\n",
l2_wt_override ? ", in WT override mode" : "");
}
+#ifdef CONFIG_OF
+static const struct of_device_id feroceon_ids[] __initconst = {
+ { .compatible = "marvell,kirkwood-cache"},
+ { .compatible = "marvell,feroceon-cache"},
+ {}
+};
+
+int __init feroceon_of_init(void)
+{
+ struct device_node *node;
+ void __iomem *base;
+ bool l2_wt_override = false;
+ struct resource res;
+
+#if defined(CONFIG_CACHE_FEROCEON_L2_WRITETHROUGH)
+ l2_wt_override = true;
+#endif
+
+ node = of_find_matching_node(NULL, feroceon_ids);
+ if (node && of_device_is_compatible(node, "marvell,kirkwood-cache")) {
+ if (of_address_to_resource(node, 0, &res))
+ return -ENODEV;
+
+ base = ioremap(res.start, resource_size(&res));
+ if (!base)
+ return -ENOMEM;
+
+ if (l2_wt_override)
+ writel(readl(base) | L2_WRITETHROUGH_KIRKWOOD, base);
+ else
+ writel(readl(base) & ~L2_WRITETHROUGH_KIRKWOOD, base);
+ }
+
+ feroceon_l2_init(l2_wt_override);
+
+ return 0;
+}
+#endif
--
1.8.5.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v6 17/18] ARM: sun4i: dt: Add ahci / sata support
From: Hans de Goede @ 2014-02-22 19:10 UTC (permalink / raw)
To: Maxime Ripard
Cc: Tejun Heo, Oliver Schinagl, Richard Zhu, Roger Quadros, Lee Jones,
linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20140222171516.GA3610@lukather>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi,
On 02/22/2014 06:15 PM, Maxime Ripard wrote:
> On Sat, Feb 22, 2014 at 11:09:25AM +0100, Hans de Goede wrote:
>> Hi Maxime,
>>
>> On 02/21/2014 07:15 PM, Maxime Ripard wrote:
>>> Hi Hans,
>>>
>>> On Wed, Feb 19, 2014 at 01:01:59PM +0100, Hans de Goede wrote:
>>>> From: Oliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
>>>>
>>>> This patch adds sunxi sata support to A10 boards that have such a connector. Some boards also feature a regulator via a GPIO and support for this is also added.
>>>>
>>>> Signed-off-by: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> --- arch/arm/boot/dts/sun4i-a10-a1000.dts | 4 ++++ arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 6 +++++ arch/arm/boot/dts/sun4i-a10.dtsi | 8 +++++++ arch/arm/boot/dts/sunxi-ahci-reg.dtsi | 36 ++++++++++++++++++++++++++++++ 4 files changed, 54 insertions(+) create mode 100644 arch/arm/boot/dts/sunxi-ahci-reg.dtsi
>>>>
>>>> diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts index cbd2e13..d6ec839 100644 --- a/arch/arm/boot/dts/sun4i-a10-a1000.dts +++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts @@ -35,6 +35,10 @@ }; };
>>>>
>>>> + ahci: sata@01c18000 { + status = "okay"; + }; + pinctrl@01c20800 { emac_power_pin_a1000: emac_power_pin@0 { allwinner,pins = "PH15"; diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts index b139ee6..6df237d8 100644 --- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts +++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts @@ -12,6 +12,7 @@
>>>>
>>>> /dts-v1/; /include/ "sun4i-a10.dtsi" +/include/ "sunxi-ahci-reg.dtsi"
>>>>
>>>> / { model = "Cubietech Cubieboard"; @@ -33,6 +34,11 @@ }; };
>>>>
>>>> + ahci: sata@01c18000 { + target-supply = <®_ahci_5v>; + status = "okay"; + }; + pinctrl@01c20800 { led_pins_cubieboard: led_pins@0 { allwinner,pins = "PH20", "PH21"; diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi index 336dbec..454077a 100644 --- a/arch/arm/boot/dts/sun4i-a10.dtsi +++ b/arch/arm/boot/dts/sun4i-a10.dtsi @@ -338,6 +338,14 @@ #size-cells = <0>; };
>>>>
>>>> + ahci: sata@01c18000 { + compatible = "allwinner,sun4i-a10-ahci"; + reg = <0x01c18000 0x1000>; + interrupts = <56>; + clocks = <&pll6 0>, <&ahb_gates 25>; + status = "disabled"; + }; + intc: interrupt-controller@01c20400 { compatible = "allwinner,sun4i-ic"; reg = <0x01c20400 0x400>; diff --git a/arch/arm/boot/dts/sunxi-ahci-reg.dtsi b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi new file mode 100644 index 0000000..7072af1 --- /dev/null +++ b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi @@ -0,0 +1,36 @@ +/* + * sunxi boards sata target power supply common code
>>>
>>>
>>> Since IIRC we have pretty much the same needs for the USB, can't we just drop the SATA specific mention and use it as the common DTSI for the usual regulators?
>>
>> On most boards with sata, there will also be 1 or 2 usb regulators, so we need differently named regulator nodes for all 3 of ahci, usb1 and usb2 vbus. On some boards how ever we may only need the usb regulators.
>
> Yes, obviously...
>
>> So if you look in my current personal sunxi-devel tree you will see separate dtsi files for both ahci and usb regulators,
>
> And this is precisely what I don't understand. Why do you *need* different DTSI files. If there's common regulators, that are used on most boards, fine, create a common regulators files. But why do you have to create a DTSI to define only one regulator.
>
>> another advantage of having these separate is that the gpio controlling the regulator can be pre-populated with the reference design gpio which is used in most boards, so that the ahci specific code in the dts becomes only the ahci: sata@... node.
>
> I understand very well the advantages of what having a reference regulators bring. What I don't understand is the benefits of having "topics" regulators DTSI.
Ok, so let me try to explain:
With topics regulator files, the ahci bits look something like this
for a board using the reference design gpio:
/include/ "sunxi-ahci-reg.dtsi"
...
ahci: sata@01c18000 {
target-supply = <®_ahci_5v>;
status = "okay";
};
If we put all regulators in one file, then the ahci regulator cannot
be enabled (so it will have status = "disabled) by default since most
boards don't have it, so things would change into:
/include/ "sunxi-common-regulators.dtsi"
...
ahci: sata@01c18000 {
target-supply = <®_ahci_5v>;
status = "okay";
};
...
reg_ahci_5v: ahci-5v {
status = "okay";
};
Notice the addition of the 2nd node. This is why I ended up doing
2 separate dtsi files for the ahci and for the usb regulators.
To me saying:
/include/ "sunxi-ahci-reg.dtsi"
Makes it clear to the reader that the board has a ahci target-supply
regulator, so enabling it separately seems being overly verbose.
Of course of we change it to:
/include/ "sunxi-common-regulators.dtsi"
Then the verbosity / explicit enabling of various regulators becomes a
good thing, as it is not directly clear what the include does.
But if we do this, then for many boards we end up replacing:
/include/ "sunxi-ahci-reg.dtsi"
/include/ "sun4i-a10-usb-vbus-reg.dtsi"
With:
/include/ "sunxi-common-regulators.dtsi"
reg_ahci_5v: ahci-5v {
status = "okay";
};
reg_usb1_vbus: usb1-vbus {
status = "okay";
};
reg_usb2_vbus: usb2-vbus {
status = "okay";
};
I prefer the shorter version, but I can completely understand if
you prefer the slightly more verbose version, this would also
get rid of having different usb regulator dtsi files for sun4i /
sun5i (as sun5i only has 1 usb host).
I hope this helps explain my reasoning, as said I'm fine with
either way, if you want to change over to a single file +
explicit enabling, let me know and I'll respin the ahci dts
patches. Note I'm going on vacation for a week starting Monday,
so you likely won't get a new version until next weekend.
Regards,
Hans
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlMI9jgACgkQF3VEtJrzE/tkEQCgm1V2Nga+RxsTELQUJxIl2Go3
4dkAnRK0CjK1YEapqN0anp2iltgp7smc
=fBXT
-----END PGP SIGNATURE-----
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/groups/opt_out.
^ permalink raw reply
* Re: [PATCH 1/3] mmc: core: Add DT bindings for SD card's UHS bus speed modes
From: Chris Ball @ 2014-02-22 18:52 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Jaehoon Chung, devicetree
In-Reply-To: <1392380829-8473-1-git-send-email-ulf.hansson@linaro.org>
Hi Ulf,
On Fri, Feb 14 2014, Ulf Hansson wrote:
> Provide the option to configure these speed modes per host, for those
> host driver's that can't distinguish this in runtime.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Thanks, pushed all 3/3 to mmc-next for 3.15.
- Chris.
--
Chris Ball <chris@printf.net> <http://printf.net/>
^ permalink raw reply
* Re: [PATCH] dt/bindings: update fsl-fec regarding compatible and clocks
From: Gerhard Sittig @ 2014-02-22 18:28 UTC (permalink / raw)
To: Shawn Guo
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Philippe De Muyter
In-Reply-To: <20140218144652.GG15716-rvtDTF3kK1ictlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org>
On Tue, Feb 18, 2014 at 22:46 +0800, Shawn Guo wrote:
>
> On Tue, Feb 18, 2014 at 02:44:30PM +0100, Gerhard Sittig wrote:
> > [ ... ]
>
> > > > > - reg : Address and length of the register set for the device
> > > > > - interrupts : Should contain fec interrupt
> > > > > +- clocks: phandle to the clocks feeding the FEC controller and phy. The
> > > > > + following two are required:
> > > > > + - "ipg": the peripheral access clock
> > > > > + - "ahb": the bus clock for MAC
> > > > > + The following two are optional:
> > > > > + - "ptp": the sampling clock for PTP (IEEE 1588). On SoC like i.MX6Q,
> > > > > + the clock could come from either the internal clock control module
> > > > > + or external oscillator via pad depending on board design.
> > > > > + - "enet_out": the phy reference clock provided by SoC via pad, which
> > > > > + is available on SoC like i.MX28.
> > > > > +- clock-names: Must contain the clock names described just above
> > > > > +
> > > >
> > > > Listing 'clocks' under the "required properties" all of a sudden
> > > > invalidates existing device trees, if they don't carry the
> > > > property which before the change was not required, not even
> > > > documented.
> > >
> > > Since the day we move to device tree clock lookup, the driver fec_main
> > > does not probe at all if the property is absent.
> >
> > That's an implementation detail. It's not what the spec says,
> > and neither is what the spec is to blindly follow after the / a
> > driver created the fact. Instead, a binding gets designed, and
> > the software follows.
> >
> > In reality, the doc may be behind as developers are more
> > concerned about the code. But still when you "update" the
> > binding, don't break compatibility! Even if you'd adjust all
> > drivers you can spot, it's still only Linux and not all device
> > tree users.
>
> So what's your suggestion? Add the properties as the optional?
Have there been i.MX device trees in mainline releases which lack
the clock specs? If so, making the clock specs mandatory breaks
backwards compatibility for these existing device trees as well.
I assume that listing the clocks as optional keeps the binding
most compatible. You might as well list them as recommended, if
optional is "too weak" for you.
I guess that listing the clocks as recommended, and telling which
clock names apply to which SoC variants, allows to keep using the
binding for all current implementations which were written
against this spec. This way you might focus on i.MX and say "the
following SoCs require the following clock names", and still
leave the PowerPC stuff or other FEC users for a followup patch.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: devicetree repository separation/migration
From: Sascha Hauer @ 2014-02-22 18:16 UTC (permalink / raw)
To: Warner Losh
Cc: Grant Likely, Olof Johansson, Rob Herring, Tim Bird, Jason Cooper,
Ian Campbell, Pawel Moll, Mark Rutland, Kumar Gala, Rob Landley,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-spec-u79uwXL29TY76Z2rM5mHXA,
devicetree-compiler-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <0F360BC4-77AC-44E4-85FA-A8AAA336BA4D-uzTCJ5RojNnQT0dZR+AlfA@public.gmane.org>
On Fri, Feb 21, 2014 at 11:27:29AM -0700, Warner Losh wrote:
>
> On Feb 20, 2014, at 4:38 AM, Grant Likely wrote:
>
> > On Wed, 19 Feb 2014 13:20:15 -0800, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
> >> On Wed, Feb 19, 2014 at 1:12 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>> One way to minimize the inconvenience is keep versioning and dev
> >>> cycles in sync with the kernel. We could also start doing things to
> >>> align the kernel workflow with how things will work when we do have a
> >>> separate repository.
> >>
> >> I don't think aligning development cycles is what we want most here it
> >> might be useful for us in Linux but it'll make things difficult for
> >> other projects since they're not aware of our release cycles. The
> >> device tree bindings and DT contents in that repo should be "always
> >> stable", i.e. no merge window / rc concept. As soon as something goes
> >> in it's live, and from then out only fixes to the DTS files (or
> >> appending the binding).
> >>
> >> For example, I don't want to have to track two trees to test against
> >> -- I'll want to keep one repo of the very latest DT files and always
> >> use those to boot any and all boards.
> >
> > This approach does have the subtle side effect that differs from what we
> > discussed in Edinburgh. We've talked about always being able to boot a
> > new kernel on an old devicetree, but not a new devicetree on an old
> > kernel. With a separate board database repo we are going to hit both
> > cases. At least to a limited extent we're going to need older kernels
> > booting with the latest devicetree, and we'll need some rules about how
> > that gets applied.
>
> I wasn't in Edinburgh... Was this at the binary level or at the
> source level? I'm thinking specifically about the move to cpp in the
> back of my mind...
Only the binary compatibility matters. The source can change
without breaking compatibility between kernels and devicetrees.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 17/18] ARM: sun4i: dt: Add ahci / sata support
From: Maxime Ripard @ 2014-02-22 17:15 UTC (permalink / raw)
To: Hans de Goede
Cc: Tejun Heo, Oliver Schinagl, Richard Zhu, Roger Quadros, Lee Jones,
linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <53087755.3090608-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4551 bytes --]
On Sat, Feb 22, 2014 at 11:09:25AM +0100, Hans de Goede wrote:
> Hi Maxime,
>
> On 02/21/2014 07:15 PM, Maxime Ripard wrote:
> >Hi Hans,
> >
> >On Wed, Feb 19, 2014 at 01:01:59PM +0100, Hans de Goede wrote:
> >>From: Oliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
> >>
> >>This patch adds sunxi sata support to A10 boards that have such a connector.
> >>Some boards also feature a regulator via a GPIO and support for this is also
> >>added.
> >>
> >>Signed-off-by: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
> >>Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>---
> >> arch/arm/boot/dts/sun4i-a10-a1000.dts | 4 ++++
> >> arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 6 +++++
> >> arch/arm/boot/dts/sun4i-a10.dtsi | 8 +++++++
> >> arch/arm/boot/dts/sunxi-ahci-reg.dtsi | 36 ++++++++++++++++++++++++++++++
> >> 4 files changed, 54 insertions(+)
> >> create mode 100644 arch/arm/boot/dts/sunxi-ahci-reg.dtsi
> >>
> >>diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts
> >>index cbd2e13..d6ec839 100644
> >>--- a/arch/arm/boot/dts/sun4i-a10-a1000.dts
> >>+++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts
> >>@@ -35,6 +35,10 @@
> >> };
> >> };
> >>
> >>+ ahci: sata@01c18000 {
> >>+ status = "okay";
> >>+ };
> >>+
> >> pinctrl@01c20800 {
> >> emac_power_pin_a1000: emac_power_pin@0 {
> >> allwinner,pins = "PH15";
> >>diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
> >>index b139ee6..6df237d8 100644
> >>--- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
> >>+++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
> >>@@ -12,6 +12,7 @@
> >>
> >> /dts-v1/;
> >> /include/ "sun4i-a10.dtsi"
> >>+/include/ "sunxi-ahci-reg.dtsi"
> >>
> >> / {
> >> model = "Cubietech Cubieboard";
> >>@@ -33,6 +34,11 @@
> >> };
> >> };
> >>
> >>+ ahci: sata@01c18000 {
> >>+ target-supply = <®_ahci_5v>;
> >>+ status = "okay";
> >>+ };
> >>+
> >> pinctrl@01c20800 {
> >> led_pins_cubieboard: led_pins@0 {
> >> allwinner,pins = "PH20", "PH21";
> >>diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
> >>index 336dbec..454077a 100644
> >>--- a/arch/arm/boot/dts/sun4i-a10.dtsi
> >>+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
> >>@@ -338,6 +338,14 @@
> >> #size-cells = <0>;
> >> };
> >>
> >>+ ahci: sata@01c18000 {
> >>+ compatible = "allwinner,sun4i-a10-ahci";
> >>+ reg = <0x01c18000 0x1000>;
> >>+ interrupts = <56>;
> >>+ clocks = <&pll6 0>, <&ahb_gates 25>;
> >>+ status = "disabled";
> >>+ };
> >>+
> >> intc: interrupt-controller@01c20400 {
> >> compatible = "allwinner,sun4i-ic";
> >> reg = <0x01c20400 0x400>;
> >>diff --git a/arch/arm/boot/dts/sunxi-ahci-reg.dtsi b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
> >>new file mode 100644
> >>index 0000000..7072af1
> >>--- /dev/null
> >>+++ b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
> >>@@ -0,0 +1,36 @@
> >>+/*
> >>+ * sunxi boards sata target power supply common code
> >
> >
> >Since IIRC we have pretty much the same needs for the USB, can't we
> >just drop the SATA specific mention and use it as the common DTSI for
> >the usual regulators?
>
> On most boards with sata, there will also be 1 or 2 usb regulators,
> so we need differently named regulator nodes for all 3 of ahci,
> usb1 and usb2 vbus. On some boards how ever we may only need the
> usb regulators.
Yes, obviously...
> So if you look in my current personal sunxi-devel tree you will see
> separate dtsi files for both ahci and usb regulators,
And this is precisely what I don't understand. Why do you *need*
different DTSI files. If there's common regulators, that are used on
most boards, fine, create a common regulators files. But why do you
have to create a DTSI to define only one regulator.
> another advantage of having these separate is that the gpio controlling
> the regulator can be pre-populated with the reference design gpio which
> is used in most boards, so that the ahci specific code in the dts
> becomes only the ahci: sata@... node.
I understand very well the advantages of what having a reference
regulators bring. What I don't understand is the benefits of having
"topics" regulators DTSI.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH v7 00/15] ahci: library-ise ahci_platform, add sunxi driver and cleanup imx driver
From: Hans de Goede @ 2014-02-22 16:26 UTC (permalink / raw)
To: Tejun Heo, Maxime Ripard
Cc: Oliver Schinagl, Richard Zhu, Roger Quadros, Lee Jones,
linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <1393084424-31099-1-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
<resend of cover-letter with a proper subject, sorry about that>
Hi all,
Here is v7 of my patchset for adding ahci-sunxi support. This is hopefully
the final final version of this set :)
Note I'm going on vacation for a week starting Monday, so if I'm not responding
that is why. Tejun if you feel some small cleanups are still necessary and
you don't want to wait for me to get back feel free to squash in any cleanups
you deem necessary.
This has been tested with Allwinner A10, Allwinner A20 and Freeware imx6x SoCs,
including suspend / resume. Note that the ahci_imx driver now also has imx53
sata support, it would be good if someone could test that with this series.
History:
v1, by Olliver Schinagl:
This was using the approach of having a platform device which probe method
creates a new child platform device which gets driven by ahci_platform.c,
as done by ahci_imx.c .
v2, by Hans de Goede:
Stand-alone platform driver based on Olliver's work
v3, by Hans de Goede:
patch-series, with 4 different parts
a) Make ahci_platform.c more generic, handle more then 1 clk, target pwr
regulator
b) New ahci-sunxi code only populating ahci_platform_data, passed to
ahci_platform.c to of_device_id matching.
c) Refactor ahci-imx code to work the same as the new ahci-sunxi code, this
is the reason why v3 is an RFC, I'm waiting for the wandboard I ordered to
arrive so that I can actually test this.
d) dts bindings for the sunxi ahci parts
v4, by Hans de Goede:
patch-series, with 5 different parts:
a) Make ahci_platform.c more generic, handle more then 1 clk, target pwr
regulator
b) Turn parts of ahci_platform.c into a library for use by other drivers
c) New ahci-sunxi driver using the ahci_platform.c library functionality
d) Refactor ahci-imx code to work the same as the new ahci-sunxi code
e) dts bindings for the sunxi ahci parts
v5:
v4 + the following changes:
1) fsl,imx6q driver is now tested
2) fixed suspend / resume on fsl,imx6q
3) Modifed devicetree node naming to match dt spec
4) Reworked the busy waiting code in the sunxi-phy handling as suggested by
Russell King
v6:
v5 rebased on top of 3.14-rc3 + the following changes
1) Added Roger Quadros' generic phy support series
2) Added a "ARM: sun4i: dt: Remove grouping + simple-bus for regulators" dts
patch
v7:
v6 + the following changes:
1) Addressed all Tejun's review remarks:
* Added function header comments to all exported ahci_platform functions
* Added comments in some other places
* Removed use of 2 empty lines to separate functions in some cases
* Use devres to automatically call ahci_platform_put_resources on
get_resource failure, probe failure and regular device remove
2) Dropped patches to move ahci_host_priv struct declaration to include/linux,
this was a left-over from v3 and is no longer necessary
3) Updated Roger's "ata: ahci_platform: Manage SATA PHY" patch:
* Update function header comments for the changes this makes
* Drop the Kconfig PHY requires hack, my patch for the phy-core to always be
built-in has been queued in Greg KH's tree, so this is no longer necessary.
4) Dropped Roger's "ata: ahci_platform: Add 'struct device' argument to ahci_platform_put_resources()"
patch, ahci_platform_put_resources already has a device argument as result
of it being changed into a devres release function
Tejun, can you please add patches 1-12 to your ata tree for 3.15 ?
Maxime, can you please add patch 13-15 to your dts tree for 3.15 ?
Thanks & Regards,
Hans
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/groups/opt_out.
^ permalink raw reply
* [RFC 3/3] ahci_platform: Drop unused ahci_platform_data members
From: Hans de Goede @ 2014-02-22 16:22 UTC (permalink / raw)
To: Tejun Heo, Marek Vasut
Cc: Maxime Ripard, Oliver Schinagl, Richard Zhu, Roger Quadros,
Lee Jones, linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Hans de Goede
In-Reply-To: <1393086175-6383-1-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
These members are not used anywhere, and in the future we want
ahci_platform_data to go away entirely so there is no reason to keep these
around.
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
drivers/ata/ahci_platform.c | 10 +---------
include/linux/ahci_platform.h | 3 ---
2 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index d48ca8a..c04e1a9 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -404,7 +404,6 @@ static int ahci_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct ahci_platform_data *pdata = dev_get_platdata(dev);
- const struct ata_port_info *pi_template;
struct ahci_host_priv *hpriv;
int rc;
@@ -428,14 +427,7 @@ static int ahci_probe(struct platform_device *pdev)
goto disable_resources;
}
- if (pdata && pdata->ata_port_info)
- pi_template = pdata->ata_port_info;
- else
- pi_template = &ahci_port_info;
-
- rc = ahci_platform_init_host(pdev, hpriv, pi_template,
- pdata ? pdata->force_port_map : 0,
- pdata ? pdata->mask_port_map : 0);
+ rc = ahci_platform_init_host(pdev, hpriv, &ahci_port_info, 0, 0);
if (rc)
goto pdata_exit;
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 542f268..1f16d50 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -33,9 +33,6 @@ struct ahci_platform_data {
void (*exit)(struct device *dev);
int (*suspend)(struct device *dev);
int (*resume)(struct device *dev);
- const struct ata_port_info *ata_port_info;
- unsigned int force_port_map;
- unsigned int mask_port_map;
};
int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
--
1.9.0
^ permalink raw reply related
* [RFC 2/3] ahci_platform: Drop support for imx53-ahci platform device type
From: Hans de Goede @ 2014-02-22 16:22 UTC (permalink / raw)
To: Tejun Heo, Marek Vasut
Cc: Maxime Ripard, Oliver Schinagl, Richard Zhu, Roger Quadros,
Lee Jones, linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Hans de Goede
In-Reply-To: <1393086175-6383-1-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Since the 3.13 release the ahci_imx driver has proper devicetree enabled
support for ahci on imx53 and that is used instead of the old board file
created imx53-ahci platform device.
Note this patch also complete drops the id-table, an id-table is not needed
for a single id platform driver, the name field in the driver struct suffices.
And the code already has an explicit "MODULE_ALIAS("platform:ahci");" so the
id-table is not needed for that either.
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
drivers/ata/ahci_platform.c | 46 ++++++---------------------------------------
1 file changed, 6 insertions(+), 40 deletions(-)
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 9fb99cf..d48ca8a 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -29,49 +29,17 @@
static void ahci_host_stop(struct ata_host *host);
-enum ahci_type {
- AHCI, /* standard platform ahci */
- IMX53_AHCI, /* ahci on i.mx53 */
-};
-
-static struct platform_device_id ahci_devtype[] = {
- {
- .name = "ahci",
- .driver_data = AHCI,
- }, {
- .name = "imx53-ahci",
- .driver_data = IMX53_AHCI,
- }, {
- /* sentinel */
- }
-};
-MODULE_DEVICE_TABLE(platform, ahci_devtype);
-
struct ata_port_operations ahci_platform_ops = {
.inherits = &ahci_ops,
.host_stop = ahci_host_stop,
};
EXPORT_SYMBOL_GPL(ahci_platform_ops);
-static struct ata_port_operations ahci_platform_retry_srst_ops = {
- .inherits = &ahci_pmp_retry_srst_ops,
- .host_stop = ahci_host_stop,
-};
-
-static const struct ata_port_info ahci_port_info[] = {
- /* by features */
- [AHCI] = {
- .flags = AHCI_FLAG_COMMON,
- .pio_mask = ATA_PIO4,
- .udma_mask = ATA_UDMA6,
- .port_ops = &ahci_platform_ops,
- },
- [IMX53_AHCI] = {
- .flags = AHCI_FLAG_COMMON,
- .pio_mask = ATA_PIO4,
- .udma_mask = ATA_UDMA6,
- .port_ops = &ahci_platform_retry_srst_ops,
- },
+static const struct ata_port_info ahci_port_info = {
+ .flags = AHCI_FLAG_COMMON,
+ .pio_mask = ATA_PIO4,
+ .udma_mask = ATA_UDMA6,
+ .port_ops = &ahci_platform_ops,
};
static struct scsi_host_template ahci_platform_sht = {
@@ -436,7 +404,6 @@ static int ahci_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct ahci_platform_data *pdata = dev_get_platdata(dev);
- const struct platform_device_id *id = platform_get_device_id(pdev);
const struct ata_port_info *pi_template;
struct ahci_host_priv *hpriv;
int rc;
@@ -464,7 +431,7 @@ static int ahci_probe(struct platform_device *pdev)
if (pdata && pdata->ata_port_info)
pi_template = pdata->ata_port_info;
else
- pi_template = &ahci_port_info[id ? id->driver_data : 0];
+ pi_template = &ahci_port_info;
rc = ahci_platform_init_host(pdev, hpriv, pi_template,
pdata ? pdata->force_port_map : 0,
@@ -670,7 +637,6 @@ static struct platform_driver ahci_driver = {
.of_match_table = ahci_of_match,
.pm = &ahci_pm_ops,
},
- .id_table = ahci_devtype,
};
module_platform_driver(ahci_driver);
--
1.9.0
^ permalink raw reply related
* [RFC 1/3] ahci_platform: Drop support for ahci-strict platform device type
From: Hans de Goede @ 2014-02-22 16:22 UTC (permalink / raw)
To: Tejun Heo, Marek Vasut
Cc: Maxime Ripard, Oliver Schinagl, Richard Zhu, Roger Quadros,
Lee Jones, linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Hans de Goede
In-Reply-To: <1393086175-6383-1-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
I've done a grep over the entire kernel tree and nothing is using this
(anymore?).
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
drivers/ata/ahci_platform.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 75698a4..9fb99cf 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -32,7 +32,6 @@ static void ahci_host_stop(struct ata_host *host);
enum ahci_type {
AHCI, /* standard platform ahci */
IMX53_AHCI, /* ahci on i.mx53 */
- STRICT_AHCI, /* delayed DMA engine start */
};
static struct platform_device_id ahci_devtype[] = {
@@ -43,9 +42,6 @@ static struct platform_device_id ahci_devtype[] = {
.name = "imx53-ahci",
.driver_data = IMX53_AHCI,
}, {
- .name = "strict-ahci",
- .driver_data = STRICT_AHCI,
- }, {
/* sentinel */
}
};
@@ -76,13 +72,6 @@ static const struct ata_port_info ahci_port_info[] = {
.udma_mask = ATA_UDMA6,
.port_ops = &ahci_platform_retry_srst_ops,
},
- [STRICT_AHCI] = {
- AHCI_HFLAGS (AHCI_HFLAG_DELAY_ENGINE),
- .flags = AHCI_FLAG_COMMON,
- .pio_mask = ATA_PIO4,
- .udma_mask = ATA_UDMA6,
- .port_ops = &ahci_platform_ops,
- },
};
static struct scsi_host_template ahci_platform_sht = {
--
1.9.0
^ permalink raw reply related
* [RFC 0/3] ahci_platform: drop support for a bunch of obsolete platform device types
From: Hans de Goede @ 2014-02-22 16:22 UTC (permalink / raw)
To: Tejun Heo, Marek Vasut
Cc: Maxime Ripard, Oliver Schinagl, Richard Zhu, Roger Quadros,
Lee Jones, linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
Hi Tejun, Marek,
This series, which is to be applied on top of my ahci_platform restructering
series, mostly speaks for itself.
I've some doubts about the 2nd patch, which removes the imx53-ahci platform
device support from ahci_platform.c. I'm pretty sure this is obsolete and
replaced by Marek's ahci_imx.c work, but not 100%, which is why this series
is marked as RFC. Marek can you confirm that the code the 2nd patch removes
is inded dead code?
Tejun, if Marek confirms this is indeed dead code feel free to apply this
despite the RFC marking.
Thanks & Regards,
Hans
^ permalink raw reply
* [PATCH v7 15/15] ARM: sun7i: dt: Add ahci / sata support
From: Hans de Goede @ 2014-02-22 15:53 UTC (permalink / raw)
To: Tejun Heo, Maxime Ripard
Cc: Oliver Schinagl, Richard Zhu, Roger Quadros, Lee Jones,
linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Hans de Goede
In-Reply-To: <1393084424-31099-1-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
This patch adds sunxi sata support to A20 boards that have such a connector.
Some boards also feature a regulator via a GPIO and support for this is also
added.
Signed-off-by: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 6 ++++++
arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 18 ++++++++++++++++++
arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 6 ++++++
arch/arm/boot/dts/sun7i-a20.dtsi | 8 ++++++++
4 files changed, 38 insertions(+)
diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
index 7bf4935..07823c2 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
@@ -13,12 +13,18 @@
/dts-v1/;
/include/ "sun7i-a20.dtsi"
+/include/ "sunxi-ahci-reg.dtsi"
/ {
model = "Cubietech Cubieboard2";
compatible = "cubietech,cubieboard2", "allwinner,sun7i-a20";
soc@01c00000 {
+ ahci: sata@01c18000 {
+ target-supply = <®_ahci_5v>;
+ status = "okay";
+ };
+
pinctrl@01c20800 {
led_pins_cubieboard2: led_pins@0 {
allwinner,pins = "PH20", "PH21";
diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index 025ce52..403bd2e 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -13,13 +13,26 @@
/dts-v1/;
/include/ "sun7i-a20.dtsi"
+/include/ "sunxi-ahci-reg.dtsi"
/ {
model = "Cubietech Cubietruck";
compatible = "cubietech,cubietruck", "allwinner,sun7i-a20";
soc@01c00000 {
+ ahci: sata@01c18000 {
+ target-supply = <®_ahci_5v>;
+ status = "okay";
+ };
+
pinctrl@01c20800 {
+ ahci_pwr_pin_cubietruck: ahci_pwr_pin@1 {
+ allwinner,pins = "PH12";
+ allwinner,function = "gpio_out";
+ allwinner,drive = <0>;
+ allwinner,pull = <0>;
+ };
+
led_pins_cubietruck: led_pins@0 {
allwinner,pins = "PH7", "PH11", "PH20", "PH21";
allwinner,function = "gpio_out";
@@ -90,4 +103,9 @@
gpios = <&pio 7 7 0>;
};
};
+
+ reg_ahci_5v: ahci-5v {
+ pinctrl-0 = <&ahci_pwr_pin_cubietruck>;
+ gpio = <&pio 7 12 0>;
+ };
};
diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
index b02a796..d5c6799 100644
--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
@@ -13,12 +13,18 @@
/dts-v1/;
/include/ "sun7i-a20.dtsi"
+/include/ "sunxi-ahci-reg.dtsi"
/ {
model = "Olimex A20-Olinuxino Micro";
compatible = "olimex,a20-olinuxino-micro", "allwinner,sun7i-a20";
soc@01c00000 {
+ ahci: sata@01c18000 {
+ target-supply = <®_ahci_5v>;
+ status = "okay";
+ };
+
pinctrl@01c20800 {
led_pins_olinuxino: led_pins@0 {
allwinner,pins = "PH2";
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index daaafd0..3385994 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -392,6 +392,14 @@
#size-cells = <0>;
};
+ ahci: sata@01c18000 {
+ compatible = "allwinner,sun4i-a10-ahci";
+ reg = <0x01c18000 0x1000>;
+ interrupts = <0 56 4>;
+ clocks = <&pll6 0>, <&ahb_gates 25>;
+ status = "disabled";
+ };
+
pio: pinctrl@01c20800 {
compatible = "allwinner,sun7i-a20-pinctrl";
reg = <0x01c20800 0x400>;
--
1.9.0
^ permalink raw reply related
* [PATCH v7 14/15] ARM: sun4i: dt: Add ahci / sata support
From: Hans de Goede @ 2014-02-22 15:53 UTC (permalink / raw)
To: Tejun Heo, Maxime Ripard
Cc: Oliver Schinagl, Richard Zhu, Roger Quadros, Lee Jones,
linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Hans de Goede
In-Reply-To: <1393084424-31099-1-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
From: Oliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
This patch adds sunxi sata support to A10 boards that have such a connector.
Some boards also feature a regulator via a GPIO and support for this is also
added.
Signed-off-by: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
arch/arm/boot/dts/sun4i-a10-a1000.dts | 4 ++++
arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 6 +++++
arch/arm/boot/dts/sun4i-a10.dtsi | 8 +++++++
arch/arm/boot/dts/sunxi-ahci-reg.dtsi | 36 ++++++++++++++++++++++++++++++
4 files changed, 54 insertions(+)
create mode 100644 arch/arm/boot/dts/sunxi-ahci-reg.dtsi
diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts
index cbd2e13..d6ec839 100644
--- a/arch/arm/boot/dts/sun4i-a10-a1000.dts
+++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts
@@ -35,6 +35,10 @@
};
};
+ ahci: sata@01c18000 {
+ status = "okay";
+ };
+
pinctrl@01c20800 {
emac_power_pin_a1000: emac_power_pin@0 {
allwinner,pins = "PH15";
diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
index b139ee6..6df237d8 100644
--- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
+++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
@@ -12,6 +12,7 @@
/dts-v1/;
/include/ "sun4i-a10.dtsi"
+/include/ "sunxi-ahci-reg.dtsi"
/ {
model = "Cubietech Cubieboard";
@@ -33,6 +34,11 @@
};
};
+ ahci: sata@01c18000 {
+ target-supply = <®_ahci_5v>;
+ status = "okay";
+ };
+
pinctrl@01c20800 {
led_pins_cubieboard: led_pins@0 {
allwinner,pins = "PH20", "PH21";
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index 336dbec..454077a 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -338,6 +338,14 @@
#size-cells = <0>;
};
+ ahci: sata@01c18000 {
+ compatible = "allwinner,sun4i-a10-ahci";
+ reg = <0x01c18000 0x1000>;
+ interrupts = <56>;
+ clocks = <&pll6 0>, <&ahb_gates 25>;
+ status = "disabled";
+ };
+
intc: interrupt-controller@01c20400 {
compatible = "allwinner,sun4i-ic";
reg = <0x01c20400 0x400>;
diff --git a/arch/arm/boot/dts/sunxi-ahci-reg.dtsi b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
new file mode 100644
index 0000000..7072af1
--- /dev/null
+++ b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
@@ -0,0 +1,36 @@
+/*
+ * sunxi boards sata target power supply common code
+ *
+ * Copyright 2014 - Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/ {
+ soc@01c00000 {
+ pio: pinctrl@01c20800 {
+ ahci_pwr_pin_a: ahci_pwr_pin@0 {
+ allwinner,pins = "PB8";
+ allwinner,function = "gpio_out";
+ allwinner,drive = <0>;
+ allwinner,pull = <0>;
+ };
+ };
+ };
+
+ reg_ahci_5v: ahci-5v {
+ compatible = "regulator-fixed";
+ pinctrl-names = "default";
+ pinctrl-0 = <&ahci_pwr_pin_a>;
+ regulator-name = "ahci-5v";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ enable-active-high;
+ gpio = <&pio 1 8 0>;
+ };
+};
--
1.9.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox