* [PATCH v4 1/2] power: supply: add sbs-charger driver
From: Nicolas Saenz Julienne @ 2016-12-20 15:31 UTC (permalink / raw)
To: sre-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8
Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
nicolas.saenz-gbiq2sxWoaasTnJN9+BGXg
In-Reply-To: <1482247874-28713-1-git-send-email-nicolas.saenz-gbiq2sxWoaasTnJN9+BGXg@public.gmane.org>
This adds support for sbs-charger compilant chips as defined here:
http://sbs-forum.org/specs/sbc110.pdf
This was tested on a arm board connected to an LTC4100 battery charger
chip.
Signed-off-by: Nicolas Saenz Julienne <nicolas.saenz-gbiq2sxWoaasTnJN9+BGXg@public.gmane.org>
---
v3 -> v4
- drop "lltc,ltc4100" compatible string for now
v2 -> v3:
- add readable_reg() function to regmap config
- update compatible strings with part number
v1 -> v2:
- add spec link in header
- use proper gpio/interrupt interface
- update regmap configuration (max register & endianness)
- dropped oldschool .supplied_to assignments
- use devm_* APIs
drivers/power/supply/Kconfig | 6 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/sbs-charger.c | 274 +++++++++++++++++++++++++++++++++++++
3 files changed, 281 insertions(+)
create mode 100644 drivers/power/supply/sbs-charger.c
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 76806a0..42877ff 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -164,6 +164,12 @@ config BATTERY_SBS
Say Y to include support for SBS battery driver for SBS-compliant
gas gauges.
+config CHARGER_SBS
+ tristate "SBS Compliant charger"
+ depends on I2C
+ help
+ Say Y to include support for SBS compilant battery chargers.
+
config BATTERY_BQ27XXX
tristate "BQ27xxx battery driver"
help
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 36c599d..06d9ef5 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_BATTERY_COLLIE) += collie_battery.o
obj-$(CONFIG_BATTERY_IPAQ_MICRO) += ipaq_micro_battery.o
obj-$(CONFIG_BATTERY_WM97XX) += wm97xx_battery.o
obj-$(CONFIG_BATTERY_SBS) += sbs-battery.o
+obj-$(CONFIG_CHARGER_SBS) += sbs-charger.o
obj-$(CONFIG_BATTERY_BQ27XXX) += bq27xxx_battery.o
obj-$(CONFIG_BATTERY_BQ27XXX_I2C) += bq27xxx_battery_i2c.o
obj-$(CONFIG_BATTERY_DA9030) += da9030_battery.o
diff --git a/drivers/power/supply/sbs-charger.c b/drivers/power/supply/sbs-charger.c
new file mode 100644
index 0000000..353765a
--- /dev/null
+++ b/drivers/power/supply/sbs-charger.c
@@ -0,0 +1,274 @@
+/*
+ * Copyright (c) 2016, Prodys S.L.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This adds support for sbs-charger compilant chips as defined here:
+ * http://sbs-forum.org/specs/sbc110.pdf
+ *
+ * Implemetation based on sbs-battery.c
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/power_supply.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/gpio.h>
+#include <linux/regmap.h>
+#include <linux/of_gpio.h>
+#include <linux/bitops.h>
+
+#define SBS_CHARGER_REG_SPEC_INFO 0x11
+#define SBS_CHARGER_REG_STATUS 0x13
+#define SBS_CHARGER_REG_ALARM_WARNING 0x16
+
+#define SBS_CHARGER_STATUS_CHARGE_INHIBITED BIT(1)
+#define SBS_CHARGER_STATUS_RES_COLD BIT(9)
+#define SBS_CHARGER_STATUS_RES_HOT BIT(10)
+#define SBS_CHARGER_STATUS_BATTERY_PRESENT BIT(14)
+#define SBS_CHARGER_STATUS_AC_PRESENT BIT(15)
+
+#define SBS_CHARGER_POLL_TIME 500
+
+struct sbs_info {
+ struct i2c_client *client;
+ struct power_supply *power_supply;
+ struct regmap *regmap;
+ struct delayed_work work;
+ unsigned int last_state;
+};
+
+static int sbs_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct sbs_info *chip = power_supply_get_drvdata(psy);
+ unsigned int reg;
+
+ reg = chip->last_state;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_PRESENT:
+ val->intval = !!(reg & SBS_CHARGER_STATUS_BATTERY_PRESENT);
+ break;
+
+ case POWER_SUPPLY_PROP_ONLINE:
+ val->intval = !!(reg & SBS_CHARGER_STATUS_AC_PRESENT);
+ break;
+
+ case POWER_SUPPLY_PROP_STATUS:
+ val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
+
+ if (!(reg & SBS_CHARGER_STATUS_BATTERY_PRESENT))
+ val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+ else if (reg & SBS_CHARGER_STATUS_AC_PRESENT &&
+ !(reg & SBS_CHARGER_STATUS_CHARGE_INHIBITED))
+ val->intval = POWER_SUPPLY_STATUS_CHARGING;
+ else
+ val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+
+ break;
+
+ case POWER_SUPPLY_PROP_HEALTH:
+ if (reg & SBS_CHARGER_STATUS_RES_COLD)
+ val->intval = POWER_SUPPLY_HEALTH_COLD;
+ if (reg & SBS_CHARGER_STATUS_RES_HOT)
+ val->intval = POWER_SUPPLY_HEALTH_OVERHEAT;
+ else
+ val->intval = POWER_SUPPLY_HEALTH_GOOD;
+
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int sbs_check_state(struct sbs_info *chip)
+{
+ unsigned int reg;
+ int ret;
+
+ ret = regmap_read(chip->regmap, SBS_CHARGER_REG_STATUS, ®);
+ if (!ret && reg != chip->last_state) {
+ chip->last_state = reg;
+ power_supply_changed(chip->power_supply);
+ return 1;
+ }
+
+ return 0;
+}
+
+static void sbs_delayed_work(struct work_struct *work)
+{
+ struct sbs_info *chip = container_of(work, struct sbs_info, work.work);
+
+ sbs_check_state(chip);
+
+ schedule_delayed_work(&chip->work,
+ msecs_to_jiffies(SBS_CHARGER_POLL_TIME));
+}
+
+static irqreturn_t sbs_irq_thread(int irq, void *data)
+{
+ struct sbs_info *chip = data;
+ int ret;
+
+ ret = sbs_check_state(chip);
+
+ return ret ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static enum power_supply_property sbs_properties[] = {
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_PRESENT,
+ POWER_SUPPLY_PROP_ONLINE,
+ POWER_SUPPLY_PROP_HEALTH,
+};
+
+static bool sbs_readable_reg(struct device *dev, unsigned int reg)
+{
+ if (reg < SBS_CHARGER_REG_SPEC_INFO)
+ return false;
+ else
+ return true;
+}
+
+static bool sbs_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case SBS_CHARGER_REG_STATUS:
+ return true;
+ }
+
+ return false;
+}
+
+static const struct regmap_config sbs_regmap = {
+ .reg_bits = 8,
+ .val_bits = 16,
+ .max_register = SBS_CHARGER_REG_ALARM_WARNING,
+ .readable_reg = sbs_readable_reg,
+ .volatile_reg = sbs_volatile_reg,
+ .val_format_endian = REGMAP_ENDIAN_LITTLE, /* since based on SMBus */
+};
+
+static const struct power_supply_desc sbs_desc = {
+ .name = "sbs-charger",
+ .type = POWER_SUPPLY_TYPE_MAINS,
+ .properties = sbs_properties,
+ .num_properties = ARRAY_SIZE(sbs_properties),
+ .get_property = sbs_get_property,
+};
+
+static int sbs_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct power_supply_config psy_cfg = {};
+ struct sbs_info *chip;
+ int ret, val;
+
+ chip = devm_kzalloc(&client->dev, sizeof(struct sbs_info), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->client = client;
+ psy_cfg.of_node = client->dev.of_node;
+ psy_cfg.drv_data = chip;
+
+ i2c_set_clientdata(client, chip);
+
+ chip->regmap = devm_regmap_init_i2c(client, &sbs_regmap);
+ if (IS_ERR(chip->regmap))
+ return PTR_ERR(chip->regmap);
+
+ /*
+ * Before we register, we need to make sure we can actually talk
+ * to the battery.
+ */
+ ret = regmap_read(chip->regmap, SBS_CHARGER_REG_STATUS, &val);
+ if (ret) {
+ dev_err(&client->dev, "Failed to get device status\n");
+ return ret;
+ }
+ chip->last_state = val;
+
+ chip->power_supply = devm_power_supply_register(&client->dev, &sbs_desc,
+ &psy_cfg);
+ if (IS_ERR(chip->power_supply)) {
+ dev_err(&client->dev, "Failed to register power supply\n");
+ return PTR_ERR(chip->power_supply);
+ }
+
+ /*
+ * The sbs-charger spec doesn't impose the use of an interrupt. So in
+ * the case it wasn't provided we use polling in order get the charger's
+ * status.
+ */
+ if (client->irq) {
+ ret = devm_request_threaded_irq(&client->dev, client->irq,
+ NULL, sbs_irq_thread,
+ IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+ dev_name(&client->dev), chip);
+ if (ret) {
+ dev_err(&client->dev, "Failed to request irq, %d\n", ret);
+ return ret;
+ }
+ } else {
+ INIT_DELAYED_WORK(&chip->work, sbs_delayed_work);
+ schedule_delayed_work(&chip->work,
+ msecs_to_jiffies(SBS_CHARGER_POLL_TIME));
+ }
+
+ dev_info(&client->dev,
+ "%s: smart charger device registered\n", client->name);
+
+ return 0;
+}
+
+static int sbs_remove(struct i2c_client *client)
+{
+ struct sbs_info *chip = i2c_get_clientdata(client);
+
+ cancel_delayed_work_sync(&chip->work);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id sbs_dt_ids[] = {
+ { .compatible = "sbs,sbs-charger" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, sbs_dt_ids);
+#endif
+
+static const struct i2c_device_id sbs_id[] = {
+ { "sbs-charger", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, sbs_id);
+
+static struct i2c_driver sbs_driver = {
+ .probe = sbs_probe,
+ .remove = sbs_remove,
+ .id_table = sbs_id,
+ .driver = {
+ .name = "sbs-charger",
+ .of_match_table = of_match_ptr(sbs_dt_ids),
+ },
+};
+module_i2c_driver(sbs_driver);
+
+MODULE_AUTHOR("Nicolas Saenz Julienne <nicolassaenzj-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("SBS smart charger driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
--
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
* [PATCH v4 0/2] power: supply: add sbs-charger driver
From: Nicolas Saenz Julienne @ 2016-12-20 15:31 UTC (permalink / raw)
To: sre, robh+dt, mark.rutland
Cc: linux-pm, devicetree, linux-kernel, nicolas.saenz
Hi,
This series adds support for all SBS compatible battery chargers, as defined
here: http://sbs-forum.org/specs/sbc110.pdf.
Regards,
Nicolas
changes since v3:
- update dt-binding to use part number and fallback
changes since v2:
- updated driver and dt-binding with Sebatian's comments
changes since v1:
- added dt bindings
- updated driver with Sebastian's comments
- s/Nicola/Nicolas/ in commits
Nicolas Saenz Julienne (2):
power: supply: add sbs-charger driver
dt-bindings: power: add bindings for sbs-charger
.../bindings/power/supply/sbs_sbs-charger.txt | 23 ++
drivers/power/supply/Kconfig | 6 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/sbs-charger.c | 274 +++++++++++++++++++++
4 files changed, 304 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/sbs_sbs-charger.txt
create mode 100644 drivers/power/supply/sbs-charger.c
--
2.7.4
^ permalink raw reply
* Re: [PATCH 3/7] iio: adc: sun4i-gpadc-iio: add support for A33 thermal sensor
From: Quentin Schulz @ 2016-12-20 15:20 UTC (permalink / raw)
To: Maxime Ripard
Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
wens-jdAy2FN1RRM, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
linux-I+IVW8TIWO2tmTQ+vhA3Yw,
stefan.mavrodiev-Re5JQEeQqe8AvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
In-Reply-To: <20161220144434.2u6ivige6kdto6an@lukather>
[-- Attachment #1.1: Type: text/plain, Size: 17529 bytes --]
Hi,
On 20/12/2016 15:44, Maxime Ripard wrote:
> On Tue, Dec 20, 2016 at 11:27:05AM +0100, Quentin Schulz wrote:
>> This adds support for the Allwinner A33 thermal sensor.
>>
>> Unlike the A10, A13 and A31, the Allwinner A33 only has one channel
>> which is dedicated to the thermal sensor. Moreover, its thermal sensor
>> does not generate interruptions, thus we only need to directly read the
>> register storing the temperature value.
>>
>> The MFD used by the A10, A13 and A31, was created to avoid breaking the
>> DT binding, but since the nodes for the ADC weren't there for the A33,
>> it is not needed.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> ---
>> drivers/iio/adc/Kconfig | 21 ++--
>> drivers/iio/adc/sun4i-gpadc-iio.c | 204 ++++++++++++++++++++++++++++----------
>> include/linux/mfd/sun4i-gpadc.h | 4 +
>> 3 files changed, 172 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 6a6d369..06041ff 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -437,17 +437,24 @@ config STX104
>> config SUN4I_GPADC
>> tristate "Support for the Allwinner SoCs GPADC"
>> depends on IIO
>> - depends on MFD_SUN4I_GPADC
>> - help
>> - Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
>> - GPADC. This ADC provides 4 channels which can be used as an ADC or as
>> - a touchscreen input and one channel for thermal sensor.
>> -
>> - The thermal sensor slows down ADC readings and can be disabled by
>> +# MFD_SUN4I_GPADC is needed for sun4i, sun5i and sun6i but not for sun8i
>
> The indentation is wrong here, and I wouldn't list all the families
> there, this is just going to be always amended, for no particular
> reason.
>
ACK.
>> + select MFD_SUN4I_GPADC if MACH_SUN4I || MACH_SUN5I || MACH_SUN6I
>
> Why did you change the depends on to a select?
>
The "depends on" does not have an if condition but "select" has. See:
https://www.kernel.org/doc/Documentation/kbuild/kconfig-language.txt
> And isn't that redundant with the comment just above?
>
>> +# THERMAL_OF can be disabled on sun4i, sun5i and sun6i to quicken ADC readings
>
> I'm not sure this is worth adding either. Any option can be added with
> any number of side effects, I'm not sure we want to list all of them.
>
ACK.
>> + depends on THERMAL_OF || MACH_SUN4I || MACH_SUN5I || MACH_SUN6I
>
> So you can't disable THERMAL_OF on MACH_SUN8I?
>
Not in the current state. devm_thermal_zone_of_sensor_register from
sun4i_gpadc_probe_dt returns an error if THERMAL_OF is disabled and
thus, the probe fails. Maybe it should rather fail silently and let the
user choose whether (s)he wants the thermal framework to be able to read
data from this driver?
>> + depends on !TOUCHSCREEN_SUN4I
>
> This should be a different patch.
>
>> + help
>> + Say yes here to build support for Allwinner (A10, A13, A31 and A33)
>> + SoCs GPADC.
>> +
>> + The ADC on A10, A13 and A31 provides 4 channels which can be used as
>> + an ADC or as a touchscreen input and one channel for thermal sensor.
>> + Their thermal sensor slows down ADC readings and can be disabled by
>
> Again, I'm not sure putting all those details in the Kconfig help
> really helps. This is only going to grow with more and more cases, and
> this isn't something really helpful anyway.
>
Are you suggesting to remove completely the paragraph on why it is
possible to disable CONFIG_THERMAL_OF for the A10, A13 and A31 or only
to remove the mention to SoCs?
>> disabling CONFIG_THERMAL_OF. However, the thermal sensor should be
>> enabled by default since the SoC temperature is usually more critical
>> than ADC readings.
>>
>> + The ADC on A33 provides one channel for thermal sensor.
>> +
>> To compile this driver as a module, choose M here: the module will be
>> called sun4i-gpadc-iio.
>>
>> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
>> index a8e134f..8be694e 100644
>> --- a/drivers/iio/adc/sun4i-gpadc-iio.c
>> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
>> @@ -1,4 +1,4 @@
>> -/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
>> +/* ADC driver for sunxi platforms' (A10, A13, A31 and A33) GPADC
>> *
>> * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons.com>
>> *
>> @@ -85,6 +85,12 @@ static const struct gpadc_data sun6i_gpadc_data = {
>> .adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
>> };
>>
>> +static const struct gpadc_data sun8i_gpadc_data = {
>> + .temp_offset = -1662,
>> + .temp_scale = 162,
>> + .tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
>> +};
>> +
>> struct sun4i_gpadc_iio {
>> struct iio_dev *indio_dev;
>> struct completion completion;
>> @@ -96,6 +102,7 @@ struct sun4i_gpadc_iio {
>> unsigned int temp_data_irq;
>> atomic_t ignore_temp_data_irq;
>> const struct gpadc_data *data;
>> + bool use_dt;
>> /* prevents concurrent reads of temperature and ADC */
>> struct mutex mutex;
>> };
>> @@ -138,6 +145,23 @@ static const struct iio_chan_spec sun4i_gpadc_channels_no_temp[] = {
>> SUN4I_GPADC_ADC_CHANNEL(3, "adc_chan3"),
>> };
>>
>> +static const struct iio_chan_spec sun8i_gpadc_channels[] = {
>
> sun8i_a33. Other SoCs from that same family might have different
> channels.
>
ACK.
>> + {
>> + .type = IIO_TEMP,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_SCALE) |
>> + BIT(IIO_CHAN_INFO_OFFSET),
>> + .datasheet_name = "temp_adc",
>> + },
>> +};
>> +
>> +static const struct regmap_config sun4i_gpadc_regmap_config = {
>> + .reg_bits = 32,
>> + .val_bits = 32,
>> + .reg_stride = 4,
>> + .fast_io = true,
>> +};
>> +
>> static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel,
>> unsigned int irq)
>> {
>> @@ -231,7 +255,6 @@ static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int *val,
>> err:
>> pm_runtime_put_autosuspend(indio_dev->dev.parent);
>> mutex_unlock(&info->mutex);
>> -
>> return ret;
>> }
>>
>> @@ -246,6 +269,19 @@ static int sun4i_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
>> static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
>> {
>> struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>> + int ret;
>> +
>> + if (info->use_dt) {
>> + pm_runtime_get_sync(indio_dev->dev.parent);
>> +
>> + ret = regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
>> + if (!ret)
>> + pm_runtime_mark_last_busy(indio_dev->dev.parent);
>> +
>> + pm_runtime_put_autosuspend(indio_dev->dev.parent);
>> +
>> + return 0;
>> + }
>
> Why is runtime_pm linked to the DT support or not?
>
>>
>> return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
>> }
The same runtime_pm functions are called when the driver is not probed
from DT.
sun4i_gpadc_read will call sun4i_prepare_for_irq which does a
pm_runtime_get_sync and then at the end of sun4i_gpadc_read,
pm_runtime_mark_last_busy and pm_runtime_put_autosuspend are called.
I just noticed I forgot to add a comment on this one. The A33
documentation tells us there is an interrupt for the thermal sensor but
after struggling with it, it is just false. I validated my guess with
Allwinner Linux kernel which does not wait an interrupt to read the data
register of the thermal sensor.
sun4i_gpadc_read always wait for an interrupt before reading data regs,
so I'm just not calling it and doing the "logic" (reading the data reg
and interfacing with runtime_pm) directly here in sun4i_gpadc_temp.
I'll add a comment on this one. Maybe I should create a function just
for the "logic" of the A33 thermal sensor, so it is less weird than
currently.
>> @@ -410,7 +446,7 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
>> unsigned int *irq, atomic_t *atomic)
>> {
>> int ret;
>> - struct sun4i_gpadc_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent);
>> + struct sun4i_gpadc_dev *mfd_dev;
>> struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(&pdev->dev));
>>
>> /*
>> @@ -427,6 +463,8 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
>> */
>> atomic_set(atomic, 1);
>>
>> + mfd_dev = dev_get_drvdata(pdev->dev.parent);
>> +
>
> Why is that change necessary?
>
>> ret = platform_get_irq_byname(pdev, name);
>> if (ret < 0) {
>> dev_err(&pdev->dev, "no %s interrupt registered\n", name);
>> @@ -454,31 +492,68 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
>> return 0;
>> }
>>
>> -static int sun4i_gpadc_probe(struct platform_device *pdev)
>> +static const struct of_device_id sun4i_gpadc_of_id[] = {
>> + {
>> + .compatible = "allwinner,sun8i-a33-gpadc-iio",
>> + .data = &sun8i_gpadc_data,
>> + },
>> + { /* sentinel */ }
>> +};
>> +
>> +static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
>> + struct iio_dev *indio_dev)
>> {
>> - struct sun4i_gpadc_iio *info;
>> - struct iio_dev *indio_dev;
>> + struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>> + const struct of_device_id *of_dev;
>> + struct thermal_zone_device *tzd;
>> + struct resource *mem;
>> + void __iomem *base;
>> int ret;
>> - struct sun4i_gpadc_dev *sun4i_gpadc_dev;
>>
>> - sun4i_gpadc_dev = dev_get_drvdata(pdev->dev.parent);
>> + of_dev = of_match_device(sun4i_gpadc_of_id, &pdev->dev);
>> + if (!of_dev)
>> + return -ENODEV;
>> +
>> + info->use_dt = true;
>> + info->data = (struct gpadc_data *)of_dev->data;
>> + indio_dev->num_channels = ARRAY_SIZE(sun8i_gpadc_channels);
>> + indio_dev->channels = sun8i_gpadc_channels;
>> +
>> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + base = devm_ioremap_resource(&pdev->dev, mem);
>> + if (IS_ERR(base))
>> + return PTR_ERR(base);
>> +
>> + info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
>> + &sun4i_gpadc_regmap_config);
>> + if (IS_ERR(info->regmap)) {
>> + ret = PTR_ERR(info->regmap);
>> + dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
>> + return ret;
>> + }
>>
>> - indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
>> - if (!indio_dev)
>> - return -ENOMEM;
>> + tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
>> + &sun4i_ts_tz_ops);
>> + if (IS_ERR(tzd)) {
>> + dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
>> + PTR_ERR(tzd));
>> + return PTR_ERR(tzd);
>> + }
>>
>> - info = iio_priv(indio_dev);
>> - platform_set_drvdata(pdev, indio_dev);
>> + return 0;
>> +}
>>
>> - mutex_init(&info->mutex);
>> +static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
>> + struct iio_dev *indio_dev)
>> +{
>> + struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>> + struct sun4i_gpadc_dev *sun4i_gpadc_dev;
>> + int ret;
>> +
>> + info->use_dt = false;
>> + sun4i_gpadc_dev = dev_get_drvdata(pdev->dev.parent);
>> info->regmap = sun4i_gpadc_dev->regmap;
>> - info->indio_dev = indio_dev;
>> - init_completion(&info->completion);
>> - indio_dev->name = dev_name(&pdev->dev);
>> - indio_dev->dev.parent = &pdev->dev;
>> - indio_dev->dev.of_node = pdev->dev.of_node;
>> - indio_dev->info = &sun4i_gpadc_iio_info;
>> - indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>
> You should have two patches. One to split the MFD part from the probe,
> and a second one to add the DT probing.
>
ACK.
>> indio_dev->num_channels = ARRAY_SIZE(sun4i_gpadc_channels);
>> indio_dev->channels = sun4i_gpadc_channels;
>>
>> @@ -494,7 +569,6 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>> * register the sensor if that option is enabled, eventually leaving
>> * that choice to the user.
>> */
>> -
>
> Spurious change?
>
Yes, lots of. Sorry, I will be more careful on that.
>> if (IS_ENABLED(CONFIG_THERMAL_OF)) {
>> /*
>> * This driver is a child of an MFD which has a node in the DT
>> @@ -519,8 +593,7 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>> dev_err(&pdev->dev,
>> "could not register thermal sensor: %ld\n",
>> PTR_ERR(tzd));
>> - ret = PTR_ERR(tzd);
>> - goto err;
>> + return PTR_ERR(tzd);
>> }
>> } else {
>> indio_dev->num_channels =
>> @@ -528,49 +601,78 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>> indio_dev->channels = sun4i_gpadc_channels_no_temp;
>> }
>>
>> - pm_runtime_set_autosuspend_delay(&pdev->dev,
>> - SUN4I_GPADC_AUTOSUSPEND_DELAY);
>> - pm_runtime_use_autosuspend(&pdev->dev);
>> - pm_runtime_set_suspended(&pdev->dev);
>> - pm_runtime_enable(&pdev->dev);
>> -
>> if (IS_ENABLED(CONFIG_THERMAL_OF)) {
>> ret = sun4i_irq_init(pdev, "TEMP_DATA_PENDING",
>> sun4i_gpadc_temp_data_irq_handler,
>> "temp_data", &info->temp_data_irq,
>> &info->ignore_temp_data_irq);
>> if (ret < 0)
>> - goto err;
>> - }
>> + return ret;
>>
>> - ret = sun4i_irq_init(pdev, "FIFO_DATA_PENDING",
>> - sun4i_gpadc_fifo_data_irq_handler, "fifo_data",
>> - &info->fifo_data_irq, &info->ignore_fifo_data_irq);
>> - if (ret < 0)
>> - goto err;
>> + ret = sun4i_irq_init(pdev, "FIFO_DATA_PENDING",
>> + sun4i_gpadc_fifo_data_irq_handler,
>> + "fifo_data", &info->fifo_data_irq,
>> + &info->ignore_fifo_data_irq);
>> + if (ret < 0)
>> + return ret;
>> + }
>>
>> - if (IS_ENABLED(CONFIG_THERMAL_OF)) {
>> - ret = iio_map_array_register(indio_dev, sun4i_gpadc_hwmon_maps);
>> - if (ret < 0) {
>> - dev_err(&pdev->dev,
>> - "failed to register iio map array\n");
>> - goto err;
>> - }
>> + ret = iio_map_array_register(indio_dev, sun4i_gpadc_hwmon_maps);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "failed to register iio map array\n");
>> + return ret;
>> }
>>
>> + return 0;
>> +}
>> +
>> +static int sun4i_gpadc_probe(struct platform_device *pdev)
>> +{
>> + struct sun4i_gpadc_iio *info;
>> + struct iio_dev *indio_dev;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + info = iio_priv(indio_dev);
>> + platform_set_drvdata(pdev, indio_dev);
>> +
>> + mutex_init(&info->mutex);
>> + info->indio_dev = indio_dev;
>> + init_completion(&info->completion);
>> + indio_dev->name = dev_name(&pdev->dev);
>> + indio_dev->dev.parent = &pdev->dev;
>> + indio_dev->dev.of_node = pdev->dev.of_node;
>> + indio_dev->info = &sun4i_gpadc_iio_info;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + if (pdev->dev.of_node)
>> + ret = sun4i_gpadc_probe_dt(pdev, indio_dev);
>> + else
>> + ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + pm_runtime_set_autosuspend_delay(&pdev->dev,
>> + SUN4I_GPADC_AUTOSUSPEND_DELAY);
>> + pm_runtime_use_autosuspend(&pdev->dev);
>> + pm_runtime_set_suspended(&pdev->dev);
>> + pm_runtime_enable(&pdev->dev);
>> +
>> ret = devm_iio_device_register(&pdev->dev, indio_dev);
>> if (ret < 0) {
>> dev_err(&pdev->dev, "could not register the device\n");
>> - goto err_map;
>> + goto err;
>> }
>>
>> return 0;
>>
>> -err_map:
>> - if (IS_ENABLED(CONFIG_THERMAL_OF))
>> - iio_map_array_unregister(indio_dev);
>> -
>> err:
>> + if (!info->use_dt && IS_ENABLED(CONFIG_THERMAL_OF))
>> + iio_map_array_unregister(indio_dev);
>> pm_runtime_put(&pdev->dev);
>> pm_runtime_disable(&pdev->dev);
>>
>> @@ -580,10 +682,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
>> static int sun4i_gpadc_remove(struct platform_device *pdev)
>> {
>> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> + struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>>
>> pm_runtime_put(&pdev->dev);
>> pm_runtime_disable(&pdev->dev);
>> - if (IS_ENABLED(CONFIG_THERMAL_OF))
>> + if (!info->use_dt && IS_ENABLED(CONFIG_THERMAL_OF))
>> iio_map_array_unregister(indio_dev);
>>
>> return 0;
>> @@ -599,6 +702,7 @@ static const struct platform_device_id sun4i_gpadc_id[] = {
>> static struct platform_driver sun4i_gpadc_driver = {
>> .driver = {
>> .name = "sun4i-gpadc-iio",
>> + .of_match_table = sun4i_gpadc_of_id,
>> .pm = &sun4i_gpadc_pm_ops,
>> },
>> .id_table = sun4i_gpadc_id,
>> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
>> index 509e736..139872c 100644
>> --- a/include/linux/mfd/sun4i-gpadc.h
>> +++ b/include/linux/mfd/sun4i-gpadc.h
>> @@ -38,6 +38,10 @@
>> #define SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(x) (GENMASK(3, 0) & BIT(x))
>> #define SUN6I_GPADC_CTRL1_ADC_CHAN_MASK GENMASK(3, 0)
>>
>> +/* TP_CTRL1 bits for sun8i SoCs */
>> +#define SUN8I_GPADC_CTRL1_CHOP_TEMP_EN BIT(8)
>> +#define SUN8I_GPADC_CTRL1_GPADC_CALI_EN BIT(7)
>> +
>> #define SUN4I_GPADC_CTRL2 0x08
>>
>> #define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x) ((GENMASK(3, 0) & (x)) << 28)
>> --
>> 2.9.3
>>
>
> Thanks,
> Maxime
>
Thanks,
Quentin
--
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/4] mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC
From: Cyrille Pitchen @ 2016-12-20 15:17 UTC (permalink / raw)
To: Cédric Le Goater, Cyrille Pitchen,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Mark Rutland, Boris Brezillon, devicetree-u79uwXL29TY76Z2rM5mHXA,
Richard Weinberger, Marek Vasut, Rob Herring, Joel Stanley,
Brian Norris, David Woodhouse
In-Reply-To: <a7cb781c-a4a8-f0f7-e7db-5cd9f19da0aa-Bxea+6Xhats@public.gmane.org>
Le 16/12/2016 à 15:56, Cédric Le Goater a écrit :
> Hello Cyrille,
>
> On 12/16/2016 12:15 AM, Cyrille Pitchen wrote:
>> Hi Cedric,
>>
>> Le 12/12/2016 à 16:40, Cédric Le Goater a écrit :
>>> This driver adds mtd support for the Aspeed AST2500 SoC static memory
>>> controllers :
>>>
>>> * Firmware SPI Memory Controller (FMC)
>>> . BMC firmware
>>> . 3 chip select pins (CE0 ~ CE2)
>>> . supports SPI type flash memory (CE0-CE1)
>>> . CE2 can be of NOR type flash but this is not supported by the
>>> driver
>>>
>>> * SPI Flash Controller (SPI1 and SPI2)
>>> . host firmware
>>> . 2 chip select pins (CE0 ~ CE1)
>>> . supports SPI type flash memory
>>>
>>> Each controller has a memory range on which it maps its flash module
>>> slaves. Each slave is assigned a memory window for its mapping that
>>> can be changed at bootime with the Segment Address Register.
>>>
>>> Each SPI flash slave can then be accessed in two modes: Command and
>>> User. When in User mode, accesses to the memory segment of the slaves
>>> are translated in SPI transfers. When in Command mode, the HW
>>> generates the SPI commands automatically and the memory segment is
>>> accessed as if doing a MMIO.
>>>
>>> Currently, only the User mode is supported. Command mode needs a
>>> little more work to check that the memory window on the AHB bus fits
>>> the module size.
>>>
>>> Based on previous work from Milton D. Miller II <miltonm-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>>>
>>> Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>
>>> Reviewed-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
>>> ---
>>>
>>> Changes since v3:
>>> - reworked IO routines to use io{read,write}32_rep
>>> - changed config option to SPI_ASPEED_SMC
>>> - fixed aspeed_smc_chip_setup_init() returned value
>>> - merged the use of the "label" property"
>>>
>>> drivers/mtd/spi-nor/Kconfig | 10 +
>>> drivers/mtd/spi-nor/Makefile | 1 +
>>> drivers/mtd/spi-nor/aspeed-smc.c | 719 +++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 730 insertions(+)
>>> create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c
>>>
>>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>>> index 4a682ee0f632..42168e9d6097 100644
>>> --- a/drivers/mtd/spi-nor/Kconfig
>>> +++ b/drivers/mtd/spi-nor/Kconfig
>>> @@ -29,6 +29,16 @@ config MTD_SPI_NOR_USE_4K_SECTORS
>>> Please note that some tools/drivers/filesystems may not work with
>>> 4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum).
>>>
>>> +config SPI_ASPEED_SMC
>>> + tristate "Aspeed flash controllers in SPI mode"
>>> + depends on ARCH_ASPEED || COMPILE_TEST
>>> + depends on HAS_IOMEM && OF
>>> + help
>>> + This enables support for the Firmware Memory controller (FMC)
>>> + in the Aspeed AST2500 SoC when attached to SPI NOR chips,
>>> + and support for the SPI flash memory controller (SPI) for
>>> + the host firmware. The implementation only supports SPI NOR.
>>> +
>>> config SPI_ATMEL_QUADSPI
>>> tristate "Atmel Quad SPI Controller"
>>> depends on ARCH_AT91 || (ARM && COMPILE_TEST)
>>> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
>>> index 121695e83542..6ff64bc7fa0e 100644
>>> --- a/drivers/mtd/spi-nor/Makefile
>>> +++ b/drivers/mtd/spi-nor/Makefile
>>> @@ -1,4 +1,5 @@
>>> obj-$(CONFIG_MTD_SPI_NOR) += spi-nor.o
>>> +obj-$(CONFIG_SPI_ASPEED_SMC) += aspeed-smc.o
>>> obj-$(CONFIG_SPI_ATMEL_QUADSPI) += atmel-quadspi.o
>>> obj-$(CONFIG_SPI_CADENCE_QUADSPI) += cadence-quadspi.o
>>> obj-$(CONFIG_SPI_FSL_QUADSPI) += fsl-quadspi.o
>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>> new file mode 100644
>>> index 000000000000..2667ab7aeb9b
>>> --- /dev/null
>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>> @@ -0,0 +1,719 @@
>>> +/*
>>> + * ASPEED Static Memory Controller driver
>>> + *
>>> + * Copyright (c) 2015-2016, IBM Corporation.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * as published by the Free Software Foundation; either version
>>> + * 2 of the License, or (at your option) any later version.
>>> + */
>>> +
>>> +#include <linux/bug.h>
>>> +#include <linux/device.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/mtd/mtd.h>
>>> +#include <linux/mtd/partitions.h>
>>> +#include <linux/mtd/spi-nor.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/sysfs.h>
>>> +
>>> +#define DEVICE_NAME "aspeed-smc"
>>> +
>>> +/*
>>> + * The driver only support SPI flash
>>> + */
>>> +enum aspeed_smc_flash_type {
>>> + smc_type_nor = 0,
>>> + smc_type_nand = 1,
>>> + smc_type_spi = 2,
>>> +};
>>> +
>>> +struct aspeed_smc_chip;
>>> +
>>> +struct aspeed_smc_info {
>>> + u32 maxsize; /* maximum size of chip window */
>>> + u8 nce; /* number of chip enables */
>>> + bool hastype; /* flash type field exists in config reg */
>>> + u8 we0; /* shift for write enable bit for CE0 */
>>> + u8 ctl0; /* offset in regs of ctl for CE0 */
>>> +
>>> + void (*set_4b)(struct aspeed_smc_chip *chip);
>>> +};
>>> +
>>> +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip);
>>> +
>>> +static const struct aspeed_smc_info fmc_2500_info = {
>>> + .maxsize = 256 * 1024 * 1024,
>>> + .nce = 3,
>>> + .hastype = true,
>>> + .we0 = 16,
>>> + .ctl0 = 0x10,
>>> + .set_4b = aspeed_smc_chip_set_4b,
>>> +};
>>> +
>>> +static const struct aspeed_smc_info spi_2500_info = {
>>> + .maxsize = 128 * 1024 * 1024,
>>> + .nce = 2,
>>> + .hastype = false,
>>> + .we0 = 16,
>>> + .ctl0 = 0x10,
>>> + .set_4b = aspeed_smc_chip_set_4b,
>>> +};
>>> +
>>> +enum aspeed_smc_ctl_reg_value {
>>> + smc_base, /* base value without mode for other commands */
>>> + smc_read, /* command reg for (maybe fast) reads */
>>> + smc_write, /* command reg for writes */
>>> + smc_max,
>>> +};
>>> +
>>> +struct aspeed_smc_controller;
>>> +
>>> +struct aspeed_smc_chip {
>>> + int cs;
>>> + struct aspeed_smc_controller *controller;
>>> + void __iomem *ctl; /* control register */
>>> + void __iomem *ahb_base; /* base of chip window */
>>> + u32 ctl_val[smc_max]; /* control settings */
>>> + enum aspeed_smc_flash_type type; /* what type of flash */
>>> + struct spi_nor nor;
>>> +};
>>> +
>>> +struct aspeed_smc_controller {
>>> + struct device *dev;
>>> +
>>> + struct mutex mutex; /* controller access mutex */
>>> + const struct aspeed_smc_info *info; /* type info of controller */
>>> + void __iomem *regs; /* controller registers */
>>> + void __iomem *ahb_base; /* per-chip windows resource */
>>> +
>>> + struct aspeed_smc_chip *chips[0]; /* pointers to attached chips */
>>> +};
>>> +
>>> +/*
>>> + * SPI Flash Configuration Register (AST2500 SPI)
>>> + * or
>>> + * Type setting Register (AST2500 FMC).
>>> + * CE0 and CE1 can only be of type SPI. CE2 can be of type NOR but the
>>> + * driver does not support it.
>>> + */
>>> +#define CONFIG_REG 0x0
>>> +#define CONFIG_DISABLE_LEGACY BIT(31) /* 1 */
>>> +
>>> +#define CONFIG_CE2_WRITE BIT(18)
>>> +#define CONFIG_CE1_WRITE BIT(17)
>>> +#define CONFIG_CE0_WRITE BIT(16)
>>> +
>>> +#define CONFIG_CE2_TYPE BIT(4) /* AST2500 FMC only */
>>> +#define CONFIG_CE1_TYPE BIT(2) /* AST2500 FMC only */
>>> +#define CONFIG_CE0_TYPE BIT(0) /* AST2500 FMC only */
>>> +
>>> +/*
>>> + * CE Control Register
>>> + */
>>> +#define CE_CONTROL_REG 0x4
>>> +
>>> +/*
>>> + * CEx Control Register
>>> + */
>>> +#define CONTROL_AAF_MODE BIT(31)
>>> +#define CONTROL_IO_MODE_MASK GENMASK(30, 28)
>>> +#define CONTROL_IO_DUAL_DATA BIT(29)
>>> +#define CONTROL_IO_DUAL_ADDR_DATA (BIT(29) | BIT(28))
>>> +#define CONTROL_IO_QUAD_DATA BIT(30)
>>> +#define CONTROL_IO_QUAD_ADDR_DATA (BIT(30) | BIT(28))
>>> +#define CONTROL_CE_INACTIVE_SHIFT 24
>>> +#define CONTROL_CE_INACTIVE_MASK GENMASK(27, \
>>> + CONTROL_CE_INACTIVE_SHIFT)
>>> +/* 0 = 16T ... 15 = 1T T=HCLK */
>>> +#define CONTROL_COMMAND_SHIFT 16
>>> +#define CONTROL_DUMMY_COMMAND_OUT BIT(15)
>>> +#define CONTROL_IO_DUMMY_HI BIT(14)
>>> +#define CONTROL_IO_DUMMY_HI_SHIFT 14
>>> +#define CONTROL_CLK_DIV4 BIT(13) /* others */
>>> +#define CONTROL_RW_MERGE BIT(12)
>>> +#define CONTROL_IO_DUMMY_LO_SHIFT 6
>>> +#define CONTROL_IO_DUMMY_LO GENMASK(7, \
>>> + CONTROL_IO_DUMMY_LO_SHIFT)
>>> +#define CONTROL_IO_DUMMY_MASK (CONTROL_IO_DUMMY_HI | \
>>> + CONTROL_IO_DUMMY_LO)
>>> +#define CONTROL_IO_DUMMY_SET(dummy) \
>>> + (((((dummy) >> 2) & 0x1) << CONTROL_IO_DUMMY_HI_SHIFT) | \
>>> + (((dummy) & 0x3) << CONTROL_IO_DUMMY_LO_SHIFT))
>>> +
>>> +#define CONTROL_CLOCK_FREQ_SEL_SHIFT 8
>>> +#define CONTROL_CLOCK_FREQ_SEL_MASK GENMASK(11, \
>>> + CONTROL_CLOCK_FREQ_SEL_SHIFT)
>>> +#define CONTROL_LSB_FIRST BIT(5)
>>> +#define CONTROL_CLOCK_MODE_3 BIT(4)
>>> +#define CONTROL_IN_DUAL_DATA BIT(3)
>>> +#define CONTROL_CE_STOP_ACTIVE_CONTROL BIT(2)
>>> +#define CONTROL_COMMAND_MODE_MASK GENMASK(1, 0)
>>> +#define CONTROL_COMMAND_MODE_NORMAL 0
>>> +#define CONTROL_COMMAND_MODE_FREAD 1
>>> +#define CONTROL_COMMAND_MODE_WRITE 2
>>> +#define CONTROL_COMMAND_MODE_USER 3
>>> +
>>> +#define CONTROL_KEEP_MASK \
>>> + (CONTROL_AAF_MODE | CONTROL_CE_INACTIVE_MASK | CONTROL_CLK_DIV4 | \
>>> + CONTROL_IO_DUMMY_MASK | CONTROL_CLOCK_FREQ_SEL_MASK | \
>>> + CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
>>> +
>>> +/*
>>> + * The Segment Register uses a 8MB unit to encode the start address
>>> + * and the end address of the mapping window of a flash SPI slave :
>>> + *
>>> + * | byte 1 | byte 2 | byte 3 | byte 4 |
>>> + * +--------+--------+--------+--------+
>>> + * | end | start | 0 | 0 |
>>> + */
>>> +#define SEGMENT_ADDR_REG0 0x30
>>> +#define SEGMENT_ADDR_START(_r) ((((_r) >> 16) & 0xFF) << 23)
>>> +#define SEGMENT_ADDR_END(_r) ((((_r) >> 24) & 0xFF) << 23)
>>> +
>>> +/*
>>> + * In user mode all data bytes read or written to the chip decode address
>>> + * range are transferred to or from the SPI bus. The range is treated as a
>>> + * fifo of arbitratry 1, 2, or 4 byte width but each write has to be aligned
>>> + * to its size. The address within the multiple 8kB range is ignored when
>>> + * sending bytes to the SPI bus.
>>> + *
>>> + * On the arm architecture, as of Linux version 4.3, memcpy_fromio and
>>> + * memcpy_toio on little endian targets use the optimized memcpy routines
>>> + * that were designed for well behavied memory storage. These routines
>>> + * have a stutter if the source and destination are not both word aligned,
>>> + * once with a duplicate access to the source after aligning to the
>>> + * destination to a word boundary, and again with a duplicate access to
>>> + * the source when the final byte count is not word aligned.
>>> + *
>>> + * When writing or reading the fifo this stutter discards data or sends
>>> + * too much data to the fifo and can not be used by this driver.
>>> + *
>>> + * While the low level io string routines that implement the insl family do
>>> + * the desired accesses and memory increments, the cross architecture io
>>> + * macros make them essentially impossible to use on a memory mapped address
>>> + * instead of a a token from the call to iomap of an io port.
>>> + *
>>> + * These fifo routines use readl and friends to a constant io port and update
>>> + * the memory buffer pointer and count via explicit code. The final updates
>>> + * to len are optimistically suppressed.
>>> + */
>>> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src,
>>> + size_t len)
>>> +{
>>> + if (IS_ALIGNED((uintptr_t)src, sizeof(uintptr_t)) &&
>>> + IS_ALIGNED((uintptr_t)buf, sizeof(uintptr_t)) &&
>>> + IS_ALIGNED(len, sizeof(u32))) {
>>> + ioread32_rep(src, buf, len >> 2);
>>> + } else {
>>> + ioread8_rep(src, buf, len);
>>> + }
>>> + return 0;
>>> +}
>>> +
>>
>> Maybe It might be something like:
>>
>> size_t offset = 0;
>>
>> if (IS_ALIGNED((uintptr_t)src, sizeof(uintptr_t)) &&
>> IS_ALIGNED((uintptr_t)buf, sizeof(uintptr_t))) {
>> ioread32_rep(src, buf, len >> 2);
>> offset = len & ~0x3;
>> len -= offset;
>> }
>> ioread8_rep(src, (const u8 *)buf + offset, len);
>
> yes. We had an earlier version doing something similar, not as well
> crafted tough.
>
>> I assume the Aspeed SPI controller allows to read as much 32-bit words
>> as possible before reading the remaining bytes.
>
> yes.
>
>> This is just a suggested optimization, no need to use it if you don't
>> want to.
>
> well, it depends if there is a v4. If so, I will.
>
>> In v3, with readl()/readb(), you used to increment both src and buf in
>> your while() loop but now with ioreadX_rep() only buf is incremented: it
>> always reads from src without incrementing this latest address.
>>
>> I guess it means that the Aspeed SPI controller doesn't care of the
>> actual value of src as long as it lays inside the chip address range.
>
> yes :) in 'User' mode, the address has no meaning.
>
> The previous routine was practical to cover both mode: the currently
> supported 'User' mode in which we read/write the ops in the memory window
> of the flash, and the 'Command' mode in which we read/write directly the
> flash contents from the AHB bus. This mode requires a preliminary setup
> of the CEx Control Register, which is what the ctl_val field is for.
>
> We have postponed 'Command' mode for the moment because we have flash
> modules which exceed the maximum window size on some boards, and 'Command'
> mode does not work in that case. Covering this mode and these special
> cases needs more work. 'User' mode is simpler to start with but the code
> prepares ground for the other mode.
>
>> This is what you explain in the 1st paragraph of the comment, isn't it?
>
> yes. That might be a little outdated.
>
>>> +static int aspeed_smc_write_to_ahb(void __iomem *dst, const void *buf,
>>> + size_t len)
>>> +{
>>> + if (IS_ALIGNED((uintptr_t)dst, sizeof(uintptr_t)) &&
>>> + IS_ALIGNED((uintptr_t)buf, sizeof(uintptr_t)) &&
>>> + IS_ALIGNED(len, sizeof(u32))) {
>>> + iowrite32_rep(dst, buf, len >> 2);
>>> + } else {
>>> + iowrite8_rep(dst, buf, len);
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static inline u32 aspeed_smc_chip_write_bit(struct aspeed_smc_chip *chip)
>>> +{
>>> + return BIT(chip->controller->info->we0 + chip->cs);
>>> +}
>>> +
>>> +static void aspeed_smc_chip_check_config(struct aspeed_smc_chip *chip)
>>> +{
>>> + struct aspeed_smc_controller *controller = chip->controller;
>>> + u32 reg;
>>> +
>>> + reg = readl(controller->regs + CONFIG_REG);
>>> +
>>> + if (reg & aspeed_smc_chip_write_bit(chip))
>>> + return;
>>> +
>>> + dev_dbg(controller->dev, "config write is not set ! @%p: 0x%08x\n",
>>> + controller->regs + CONFIG_REG, reg);
>>> + reg |= aspeed_smc_chip_write_bit(chip);
>>> + writel(reg, controller->regs + CONFIG_REG);
>>> +}
>>> +
>>> +static void aspeed_smc_start_user(struct spi_nor *nor)
>>> +{
>>> + struct aspeed_smc_chip *chip = nor->priv;
>>> + u32 ctl = chip->ctl_val[smc_base];
>>> +
>>> + /*
>>> + * When the chip is controlled in user mode, we need write
>>> + * access to send the opcodes to it. So check the config.
>>> + */
>>> + aspeed_smc_chip_check_config(chip);
>>> +
>>> + ctl |= CONTROL_COMMAND_MODE_USER |
>>> + CONTROL_CE_STOP_ACTIVE_CONTROL;
>>> + writel(ctl, chip->ctl);
>>> +
>>> + ctl &= ~CONTROL_CE_STOP_ACTIVE_CONTROL;
>>> + writel(ctl, chip->ctl);
>>> +}
>>> +
>>> +static void aspeed_smc_stop_user(struct spi_nor *nor)
>>> +{
>>> + struct aspeed_smc_chip *chip = nor->priv;
>>> +
>>> + u32 ctl = chip->ctl_val[smc_read];
>>> + u32 ctl2 = ctl | CONTROL_COMMAND_MODE_USER |
>>> + CONTROL_CE_STOP_ACTIVE_CONTROL;
>>> +
>>> + writel(ctl2, chip->ctl); /* stop user CE control */
>>> + writel(ctl, chip->ctl); /* default to fread or read mode */
>>> +}
>>> +
>>
>> This driver seems to use only the "USER" mode so why do you go back the
>> the "FREAD" or "READ" modes at the very end of aspeed_smc_stop_user() as
>> the comment suggests?
>>
>> Do you plan to implement other modes later?
>
> yes. I would like to, I have some code for it already but there some
> little issues as said above.
>
>> Can't you just stay in "USER" mode?
>
> well, yes. we are also preparing ground for the Command mode and
> the DMA support.
>
>> I guess you just need the chip-select control part.
>
> yes.
>
>>> +static int aspeed_smc_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>>> +{
>>> + struct aspeed_smc_chip *chip = nor->priv;
>>> +
>>> + mutex_lock(&chip->controller->mutex);
>>> + return 0;
>>> +}
>>> +
>>> +static void aspeed_smc_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>>> +{
>>> + struct aspeed_smc_chip *chip = nor->priv;
>>> +
>>> + mutex_unlock(&chip->controller->mutex);
>>> +}
>>> +
>>> +static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>>> +{
>>> + struct aspeed_smc_chip *chip = nor->priv;
>>> +
>>> + aspeed_smc_start_user(nor);
>>> + aspeed_smc_write_to_ahb(chip->ahb_base, &opcode, 1);
>>> + aspeed_smc_read_from_ahb(buf, chip->ahb_base, len);
>>> + aspeed_smc_stop_user(nor);
>>> + return 0;
>>> +}
>>> +
>>> +static int aspeed_smc_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
>>> + int len)
>>> +{
>>> + struct aspeed_smc_chip *chip = nor->priv;
>>> +
>>> + aspeed_smc_start_user(nor);
>>> + aspeed_smc_write_to_ahb(chip->ahb_base, &opcode, 1);
>>> + aspeed_smc_write_to_ahb(chip->ahb_base, buf, len);
>>> + aspeed_smc_stop_user(nor);
>>> + return 0;
>>> +}
>>> +
>>> +static void aspeed_smc_send_cmd_addr(struct spi_nor *nor, u8 cmd, u32 addr)
>>> +{
>>> + struct aspeed_smc_chip *chip = nor->priv;
>>> + __be32 temp;
>>> + u32 cmdaddr;
>>> +
>>> + switch (nor->addr_width) {
>>> + default:
>>> + WARN_ONCE(1, "Unexpected address width %u, defaulting to 3\n",
>>> + nor->addr_width);
>>> + /* FALLTHROUGH */
>>> + case 3:
>>> + cmdaddr = addr & 0xFFFFFF;
>>> + cmdaddr |= cmd << 24;
>>> +
>>> + temp = cpu_to_be32(cmdaddr);
>>> + aspeed_smc_write_to_ahb(chip->ahb_base, &temp, 4);
>>> + break;
>>> + case 4:
>>> + temp = cpu_to_be32(addr);
>>> + aspeed_smc_write_to_ahb(chip->ahb_base, &cmd, 1);
>>> + aspeed_smc_write_to_ahb(chip->ahb_base, &temp, 4);
>>> + break;
>>> + }
>>> +}
>>> +
>>> +static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>> + size_t len, u_char *read_buf)
>>> +{
>>> + struct aspeed_smc_chip *chip = nor->priv;
>>> +
>>> + aspeed_smc_start_user(nor);
>>> + aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>
>> Here, please check nor->read_dummy to write the relevant number dummy
>> bytes between the address and data cycles.
>>
>> It should not need too much work to add support to the dummy clock
>> cycles and it's more reliable/safe.
>>
>> Indeed, even if you call the current spi_nor_scan() function with the
>> enum read_mode SPI_NOR_NORMAL value, this function just doesn't care and
>> selects the Fast Read (0Bh) command instead of the Read (03h) command
>> for nor->read_opcode if the "m25p,fast-read" DT property is set.
>>
>> So if any end user sets this property in a custom DT,
>> aspeed_smc_read_user() would just fail.
>>
>> Hence I think it's worth dealing with dummy cycles now rather than later.
>>
>> Actually all (Fast) Read commands but the legacy Read (03h) command need
>> dummy cycles. So the Read SFDP (5Ah) command does.
>>
>> For all the (Q)SPI memories I've seen till now, the default factory
>> settings for the number of dummy cycles are chosen so it always
>> corresponds to entire bytes, whatever the SPI protocol is (SPI 1-1-2,
>> 1-2-2, 1-1-4, 1-4-4, ...).
>>
>> Besides, I recommend you use the 0xFF value for dummy cycles: this value
>> prevents the memory from entering its continuous mode by mistake.
>> The 0xFF value works for all manufacturers. The SFDP specification seems
>> to confirm that.
>
> OK. I will take a closer look at that.
>
>>
>>> + aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>>> + aspeed_smc_stop_user(nor);
>>> + return len;
>>> +}
>>> +
>>> +static ssize_t aspeed_smc_write_user(struct spi_nor *nor, loff_t to,
>>> + size_t len, const u_char *write_buf)
>>> +{
>>> + struct aspeed_smc_chip *chip = nor->priv;
>>> +
>>> + aspeed_smc_start_user(nor);
>>> + aspeed_smc_send_cmd_addr(nor, nor->program_opcode, to);
>>> + aspeed_smc_write_to_ahb(chip->ahb_base, write_buf, len);
>>> + aspeed_smc_stop_user(nor);
>>> + return len;
>>> +}
>>> +
>>> +static int aspeed_smc_unregister(struct aspeed_smc_controller *controller)
>>> +{
>>> + struct aspeed_smc_chip *chip;
>>> + int n;
>>> +
>>> + for (n = 0; n < controller->info->nce; n++) {
>>> + chip = controller->chips[n];
>>> + if (chip)
>>> + mtd_device_unregister(&chip->nor.mtd);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int aspeed_smc_remove(struct platform_device *dev)
>>> +{
>>> + return aspeed_smc_unregister(platform_get_drvdata(dev));
>>> +}
>>> +
>>> +static const struct of_device_id aspeed_smc_matches[] = {
>>> + { .compatible = "aspeed,ast2500-fmc", .data = &fmc_2500_info },
>>> + { .compatible = "aspeed,ast2500-spi", .data = &spi_2500_info },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, aspeed_smc_matches);
>>> +
>>> +/*
>>> + * Each chip has a mapping window defined by a segment address
>>> + * register defining a start and an end address on the AHB bus. These
>>> + * addresses can be configured to fit the chip size and offer a
>>> + * contiguous memory region across chips. For the moment, we only
>>> + * check that each chip segment is valid.
>>> + */
>>> +static void __iomem *aspeed_smc_chip_base(struct aspeed_smc_chip *chip,
>>> + struct resource *res)
>>> +{
>>> + struct aspeed_smc_controller *controller = chip->controller;
>>> + u32 offset = 0;
>>> + u32 reg;
>>> +
>>> + if (controller->info->nce > 1) {
>>> + reg = readl(controller->regs + SEGMENT_ADDR_REG0 +
>>> + chip->cs * 4);
>>> +
>>> + if (SEGMENT_ADDR_START(reg) >= SEGMENT_ADDR_END(reg))
>>> + return NULL;
>>> +
>>> + offset = SEGMENT_ADDR_START(reg) - res->start;
>>> + }
>>> +
>>> + return controller->ahb_base + offset;
>>> +}
>>> +
>>> +static void aspeed_smc_chip_enable_write(struct aspeed_smc_chip *chip)
>>> +{
>>> + struct aspeed_smc_controller *controller = chip->controller;
>>> + u32 reg;
>>> +
>>> + reg = readl(controller->regs + CONFIG_REG);
>>> +
>>> + reg |= aspeed_smc_chip_write_bit(chip);
>>> + writel(reg, controller->regs + CONFIG_REG);
>>> +}
>>> +
>>> +static void aspeed_smc_chip_set_type(struct aspeed_smc_chip *chip, int type)
>>> +{
>>> + struct aspeed_smc_controller *controller = chip->controller;
>>> + u32 reg;
>>> +
>>> + chip->type = type;
>>> +
>>> + reg = readl(controller->regs + CONFIG_REG);
>>> + reg &= ~(3 << (chip->cs * 2));
>>> + reg |= chip->type << (chip->cs * 2);
>>> + writel(reg, controller->regs + CONFIG_REG);
>>> +}
>>> +
>>> +/*
>>> + * The AST2500 FMC flash controller should be strapped by hardware, or
>>> + * autodetected, but the AST2500 SPI flash needs to be set.
>>> + */
>>> +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip)
>>> +{
>>> + struct aspeed_smc_controller *controller = chip->controller;
>>> + u32 reg;
>>> +
>>> + if (chip->controller->info == &spi_2500_info) {
>>> + reg = readl(controller->regs + CE_CONTROL_REG);
>>> + reg |= 1 << chip->cs;
>>> + writel(reg, controller->regs + CE_CONTROL_REG);
>>> + }
>>> +}
>>> +
>>> +static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip,
>>> + struct resource *res)
>>> +{
>>> + struct aspeed_smc_controller *controller = chip->controller;
>>> + const struct aspeed_smc_info *info = controller->info;
>>> + u32 reg, base_reg;
>>> +
>>> + /*
>>> + * Always turn on the write enable bit to allow opcodes to be
>>> + * sent in user mode.
>>> + */
>>> + aspeed_smc_chip_enable_write(chip);
>>> +
>>> + /* The driver only supports SPI type flash */
>>> + if (info->hastype)
>>> + aspeed_smc_chip_set_type(chip, smc_type_spi);
>>> +
>>> + /*
>>> + * Configure chip base address in memory
>>> + */
>>> + chip->ahb_base = aspeed_smc_chip_base(chip, res);
>>> + if (!chip->ahb_base) {
>>> + dev_warn(chip->nor.dev, "CE segment window closed.\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /*
>>> + * Get value of the inherited control register. U-Boot usually
>>> + * does some timing calibration on the FMC chip, so it's good
>>> + * to keep them. In the future, we should handle calibration
>>> + * from Linux.
>>> + */
>>> + reg = readl(chip->ctl);
>>> + dev_dbg(controller->dev, "control register: %08x\n", reg);
>>> +
>>> + base_reg = reg & CONTROL_KEEP_MASK;
>>> + if (base_reg != reg) {
>>> + dev_info(controller->dev,
>>> + "control register changed to: %08x\n",
>>> + base_reg);
>>
>> dev_dbg() should be enough: end users don't know what to do with the new
>> control register value, do they?
>>
>> This is just a suggestion, you can keep dev_info() if you want, I don't
>> mind :)
>
> No, you are right. This is debug stuff. We had a bunch of user space tools
> poking in the SMC controller MMIO region in early days and it was nice to
> know what was the initial setup.
>
>>> + }
>>> + chip->ctl_val[smc_base] = base_reg;
>>> +
>>> + /*
>>> + * Retain the prior value of the control register as the
>>> + * default if it was normal access mode. Otherwise start with
>>> + * the sanitized base value set to read mode.
>>> + */
>>> + if ((reg & CONTROL_COMMAND_MODE_MASK) ==
>>> + CONTROL_COMMAND_MODE_NORMAL)
>>> + chip->ctl_val[smc_read] = reg;
>>> + else
>>> + chip->ctl_val[smc_read] = chip->ctl_val[smc_base] |
>>> + CONTROL_COMMAND_MODE_NORMAL;
>>> +
>>> + dev_dbg(controller->dev, "default control register: %08x\n",
>>> + chip->ctl_val[smc_read]);
>>> + return 0;
>>> +}
>>> +
>>> +static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>> +{
>>> + struct aspeed_smc_controller *controller = chip->controller;
>>> + const struct aspeed_smc_info *info = controller->info;
>>> + u32 cmd;
>>> +
>>> + if (chip->nor.addr_width == 4 && info->set_4b)
>>> + info->set_4b(chip);
>>> +
>>> + /*
>>> + * base mode has not been optimized yet. use it for writes.
>>> + */
>>> + chip->ctl_val[smc_write] = chip->ctl_val[smc_base] |
>>> + chip->nor.program_opcode << CONTROL_COMMAND_SHIFT |
>>> + CONTROL_COMMAND_MODE_WRITE;
>>> +
>>> + dev_dbg(controller->dev, "write control register: %08x\n",
>>> + chip->ctl_val[smc_write]);
>>> +
>>> + /*
>>> + * TODO: Adjust clocks if fast read is supported and interpret
>>> + * SPI-NOR flags to adjust controller settings.
>>> + */
>>> + switch (chip->nor.flash_read) {
>>> + case SPI_NOR_NORMAL:
>>> + cmd = CONTROL_COMMAND_MODE_NORMAL;
>>> + break;
>>> + case SPI_NOR_FAST:
>>> + cmd = CONTROL_COMMAND_MODE_FREAD;
>>> + break;
>>> + default:
>>> + dev_err(chip->nor.dev, "unsupported SPI read mode\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + chip->ctl_val[smc_read] |= cmd |
>>> + CONTROL_IO_DUMMY_SET(chip->nor.read_dummy / 8);
>>> +
>>> + dev_dbg(controller->dev, "base control register: %08x\n",
>>> + chip->ctl_val[smc_read]);
>>> + return 0;
>>> +}
>>> +
>>
>> Why do you configure both chip->ctrl_val[smc_write] and
>> chip->ctrl_val[smc_read] if the driver actually only uses
>> chip->ctrl_val[smc_base] ?
>
> we expect Command mode support and DMAs will use it.
>
>> all aspeed_smc_[read|write]_[reg|user]() functions call
>> aspeed_smc_[start|stop]_user(), so this driver always selects the "USER"
>> mode and configures the control register based on chip->ctrl_val[smc_base].
>
> yes.
>
> Maybe you think it is too early to prepare ground for the other
> mode ? Is that really confusing in the code ? Do you think I should
> remove it for the initial support in the driver and stick to 'User'
> mode.
>
I think it is not a big deal, at least technically. This is more a
psychological aspect of the review: the bigger patches, the more scarier.
I mean it requires more time and courage to dig into the source code hence
to understand what the driver actually does.
Sometime, it's better to split a big patch into many incremental and
smaller patches so it's more easy for reviewers to understand each part as
an independent feature. It also reveals the driver evolution during the
development process hence it helps to understand where it goes and what are
the next improvements to come.
Anyway, since the review is done now, on my side I won't ask you to remove
or split the support of the 'Command' mode in a separated patch.
I let you do as you want, if it help you to introduce some part of the
support of this 'Command' mode now even if not completed yet, no problem on
my side :)
I was just giving you some pieces of advice for the next time if you want
to speed up the review of another patch introducing new features.
However, I will just ask you one more version handling the dummy cycles
properly as it would help us for the global maintenance of the spi-nor
subsystem. This is the only mandatory modification I ask you, after that I
think it would be ok for me and since Marek has already reviewed your
driver, it would be ready to be merged into the spi-nor tree.
Anyway, thanks for taking time to explain us how your driver work :)
Best regards,
Cyrille
>>> +static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>>> + struct device_node *np, struct resource *r)
>>> +{
>>> + const struct aspeed_smc_info *info = controller->info;
>>> + struct device *dev = controller->dev;
>>> + struct device_node *child;
>>> + unsigned int cs;
>>> + int ret = -ENODEV;
>>> +
>>> + for_each_available_child_of_node(np, child) {
>>> + struct aspeed_smc_chip *chip;
>>> + struct spi_nor *nor;
>>> + struct mtd_info *mtd;
>>> +
>>> + /* This driver does not support NAND or NOR flash devices. */
>>> + if (!of_device_is_compatible(child, "jedec,spi-nor"))
>>> + continue;
>>> +
>>> + ret = of_property_read_u32(child, "reg", &cs);
>>> + if (ret) {
>>> + dev_err(dev, "Couldn't not read chip select.\n");
>>> + break;
>>> + }
>>> +
>>> + if (cs >= info->nce) {
>>> + dev_err(dev, "Chip select %d out of range.\n",
>>> + cs);
>>> + ret = -ERANGE;
>>> + break;
>>> + }
>>> +
>>> + if (controller->chips[cs]) {
>>> + dev_err(dev, "Chip select %d already in use by %s\n",
>>> + cs, dev_name(controller->chips[cs]->nor.dev));
>>> + ret = -EBUSY;
>>> + break;
>>> + }
>>> +
>>> + chip = devm_kzalloc(controller->dev, sizeof(*chip), GFP_KERNEL);
>>> + if (!chip) {
>>> + ret = -ENOMEM;
>>> + break;
>>> + }
>>> +
>>> + chip->controller = controller;
>>> + chip->ctl = controller->regs + info->ctl0 + cs * 4;
>>> + chip->cs = cs;
>>> +
>>> + nor = &chip->nor;
>>> + mtd = &nor->mtd;
>>> +
>>> + nor->dev = dev;
>>> + nor->priv = chip;
>>> + spi_nor_set_flash_node(nor, child);
>>> + nor->read = aspeed_smc_read_user;
>>> + nor->write = aspeed_smc_write_user;
>>> + nor->read_reg = aspeed_smc_read_reg;
>>> + nor->write_reg = aspeed_smc_write_reg;
>>> + nor->prepare = aspeed_smc_prep;
>>> + nor->unprepare = aspeed_smc_unprep;
>>> +
>>> + mtd->name = of_get_property(child, "label", NULL);
>>
>> This new "label" DT property should be removed from this patch and send
>> in a dedicated patch to be discussed separately. However I agree with
>> Marek: it looks generic so maybe it could be managed directly from
>> mtd_device_register() since setting such as name could also be done when
>> using a NAND flash. Anyway, I don't see the link between this name and
>> spi-nor. Hence I don't think the DT property should be documented in
>> jedec,spi-nor.txt.
>
> OK. I will remove it in the next version.
>
>> Be patient because I expect such a topic to be discussed quite a long
>> time before we all agree, even if this is "just" a new DT property ;)
>
> yeah. I expected that also :) But it is quite pratical to give user
> space a hint on the flash nature. Systems can have up to 4 different
> ones. So there is need for it IMO.
>
> How should I proceed then ? Shall I start a discussion with a single
> patch changing mtd_device_register() ? but I need to know which binding
> would be the more consensual for such a prop.
>
> Thanks,
>
> C.
>
>>
>> Best regards,
>>
>> Cyrille
>>
>>
>>> +
>>> + ret = aspeed_smc_chip_setup_init(chip, r);
>>> + if (ret)
>>> + break;
>>> +
>>> + /*
>>> + * TODO: Add support for SPI_NOR_QUAD and SPI_NOR_DUAL
>>> + * attach when board support is present as determined
>>> + * by of property.
>>> + */
>>> + ret = spi_nor_scan(nor, NULL, SPI_NOR_NORMAL);
>>> + if (ret)
>>> + break;
>>> +
>>> + ret = aspeed_smc_chip_setup_finish(chip);
>>> + if (ret)
>>> + break;
>>> +
>>> + ret = mtd_device_register(mtd, NULL, 0);
>>> + if (ret)
>>> + break;
>>> +
>>> + controller->chips[cs] = chip;
>>> + }
>>> +
>>> + if (ret)
>>> + aspeed_smc_unregister(controller);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int aspeed_smc_probe(struct platform_device *pdev)
>>> +{
>>> + struct device_node *np = pdev->dev.of_node;
>>> + struct device *dev = &pdev->dev;
>>> + struct aspeed_smc_controller *controller;
>>> + const struct of_device_id *match;
>>> + const struct aspeed_smc_info *info;
>>> + struct resource *res;
>>> + int ret;
>>> +
>>> + match = of_match_device(aspeed_smc_matches, &pdev->dev);
>>> + if (!match || !match->data)
>>> + return -ENODEV;
>>> + info = match->data;
>>> +
>>> + controller = devm_kzalloc(&pdev->dev, sizeof(*controller) +
>>> + info->nce * sizeof(controller->chips[0]), GFP_KERNEL);
>>> + if (!controller)
>>> + return -ENOMEM;
>>> + controller->info = info;
>>> + controller->dev = dev;
>>> +
>>> + mutex_init(&controller->mutex);
>>> + platform_set_drvdata(pdev, controller);
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + controller->regs = devm_ioremap_resource(dev, res);
>>> + if (IS_ERR(controller->regs)) {
>>> + dev_err(dev, "Cannot remap controller address.\n");
>>> + return PTR_ERR(controller->regs);
>>> + }
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>> + controller->ahb_base = devm_ioremap_resource(dev, res);
>>> + if (IS_ERR(controller->ahb_base)) {
>>> + dev_err(dev, "Cannot remap controller address.\n");
>>> + return PTR_ERR(controller->ahb_base);
>>> + }
>>> +
>>> + ret = aspeed_smc_setup_flash(controller, np, res);
>>> + if (ret)
>>> + dev_err(dev, "Aspeed SMC probe failed %d\n", ret);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static struct platform_driver aspeed_smc_driver = {
>>> + .probe = aspeed_smc_probe,
>>> + .remove = aspeed_smc_remove,
>>> + .driver = {
>>> + .name = DEVICE_NAME,
>>> + .of_match_table = aspeed_smc_matches,
>>> + }
>>> +};
>>> +
>>> +module_platform_driver(aspeed_smc_driver);
>>> +
>>> +MODULE_DESCRIPTION("ASPEED Static Memory Controller Driver");
>>> +MODULE_AUTHOR("Cedric Le Goater <clg-Bxea+6Xhats@public.gmane.org>");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>
>
--
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: [PATCH] misc: eeprom: implement compatible DT probing
From: Linus Walleij @ 2016-12-20 15:00 UTC (permalink / raw)
To: Peter Rosin
Cc: Wolfram Sang, linux-i2c@vger.kernel.org,
devicetree@vger.kernel.org
In-Reply-To: <806f55f3-3fed-572d-4859-7c7dc76c5e08@axentia.se>
On Fri, Dec 9, 2016 at 6:48 AM, Peter Rosin <peda@axentia.se> wrote:
> On 2016-12-09 00:32, Linus Walleij wrote:
>> Bah I probably just screwed up syntactically and now worked around
>> a non-existing problem. I will try as you suggest, just "vendor,type"
>> and see if it works. It probably does.
(It does)
> But it is a bit strange. Grepping for compatible.*24c finds quite
> a few instances of "bad" compatible strings.
>
> Many on the patterns at,24c256 and at24,24c256 (should be probably
> be atmel,24c256) but also a few atmel,at24c16 and atmel,at24c128b.
> I don't understand how those last ones ever worked, if it is not
> working for you? Especially those with the trailing "b". WTF?
Those are the erroneous device trees that I copied as inspiration
instead of looking directly at the bindings.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 3/7] iio: adc: sun4i-gpadc-iio: add support for A33 thermal sensor
From: Maxime Ripard @ 2016-12-20 14:44 UTC (permalink / raw)
To: Quentin Schulz
Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
wens-jdAy2FN1RRM, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
linux-I+IVW8TIWO2tmTQ+vhA3Yw,
stefan.mavrodiev-Re5JQEeQqe8AvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
In-Reply-To: <20161220102709.9504-4-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 15292 bytes --]
On Tue, Dec 20, 2016 at 11:27:05AM +0100, Quentin Schulz wrote:
> This adds support for the Allwinner A33 thermal sensor.
>
> Unlike the A10, A13 and A31, the Allwinner A33 only has one channel
> which is dedicated to the thermal sensor. Moreover, its thermal sensor
> does not generate interruptions, thus we only need to directly read the
> register storing the temperature value.
>
> The MFD used by the A10, A13 and A31, was created to avoid breaking the
> DT binding, but since the nodes for the ADC weren't there for the A33,
> it is not needed.
>
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> drivers/iio/adc/Kconfig | 21 ++--
> drivers/iio/adc/sun4i-gpadc-iio.c | 204 ++++++++++++++++++++++++++++----------
> include/linux/mfd/sun4i-gpadc.h | 4 +
> 3 files changed, 172 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 6a6d369..06041ff 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -437,17 +437,24 @@ config STX104
> config SUN4I_GPADC
> tristate "Support for the Allwinner SoCs GPADC"
> depends on IIO
> - depends on MFD_SUN4I_GPADC
> - help
> - Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
> - GPADC. This ADC provides 4 channels which can be used as an ADC or as
> - a touchscreen input and one channel for thermal sensor.
> -
> - The thermal sensor slows down ADC readings and can be disabled by
> +# MFD_SUN4I_GPADC is needed for sun4i, sun5i and sun6i but not for sun8i
The indentation is wrong here, and I wouldn't list all the families
there, this is just going to be always amended, for no particular
reason.
> + select MFD_SUN4I_GPADC if MACH_SUN4I || MACH_SUN5I || MACH_SUN6I
Why did you change the depends on to a select?
And isn't that redundant with the comment just above?
> +# THERMAL_OF can be disabled on sun4i, sun5i and sun6i to quicken ADC readings
I'm not sure this is worth adding either. Any option can be added with
any number of side effects, I'm not sure we want to list all of them.
> + depends on THERMAL_OF || MACH_SUN4I || MACH_SUN5I || MACH_SUN6I
So you can't disable THERMAL_OF on MACH_SUN8I?
> + depends on !TOUCHSCREEN_SUN4I
This should be a different patch.
> + help
> + Say yes here to build support for Allwinner (A10, A13, A31 and A33)
> + SoCs GPADC.
> +
> + The ADC on A10, A13 and A31 provides 4 channels which can be used as
> + an ADC or as a touchscreen input and one channel for thermal sensor.
> + Their thermal sensor slows down ADC readings and can be disabled by
Again, I'm not sure putting all those details in the Kconfig help
really helps. This is only going to grow with more and more cases, and
this isn't something really helpful anyway.
> disabling CONFIG_THERMAL_OF. However, the thermal sensor should be
> enabled by default since the SoC temperature is usually more critical
> than ADC readings.
>
> + The ADC on A33 provides one channel for thermal sensor.
> +
> To compile this driver as a module, choose M here: the module will be
> called sun4i-gpadc-iio.
>
> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
> index a8e134f..8be694e 100644
> --- a/drivers/iio/adc/sun4i-gpadc-iio.c
> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
> @@ -1,4 +1,4 @@
> -/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
> +/* ADC driver for sunxi platforms' (A10, A13, A31 and A33) GPADC
> *
> * Copyright (c) 2016 Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> *
> @@ -85,6 +85,12 @@ static const struct gpadc_data sun6i_gpadc_data = {
> .adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
> };
>
> +static const struct gpadc_data sun8i_gpadc_data = {
> + .temp_offset = -1662,
> + .temp_scale = 162,
> + .tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
> +};
> +
> struct sun4i_gpadc_iio {
> struct iio_dev *indio_dev;
> struct completion completion;
> @@ -96,6 +102,7 @@ struct sun4i_gpadc_iio {
> unsigned int temp_data_irq;
> atomic_t ignore_temp_data_irq;
> const struct gpadc_data *data;
> + bool use_dt;
> /* prevents concurrent reads of temperature and ADC */
> struct mutex mutex;
> };
> @@ -138,6 +145,23 @@ static const struct iio_chan_spec sun4i_gpadc_channels_no_temp[] = {
> SUN4I_GPADC_ADC_CHANNEL(3, "adc_chan3"),
> };
>
> +static const struct iio_chan_spec sun8i_gpadc_channels[] = {
sun8i_a33. Other SoCs from that same family might have different
channels.
> + {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_OFFSET),
> + .datasheet_name = "temp_adc",
> + },
> +};
> +
> +static const struct regmap_config sun4i_gpadc_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .fast_io = true,
> +};
> +
> static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel,
> unsigned int irq)
> {
> @@ -231,7 +255,6 @@ static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int *val,
> err:
> pm_runtime_put_autosuspend(indio_dev->dev.parent);
> mutex_unlock(&info->mutex);
> -
> return ret;
> }
>
> @@ -246,6 +269,19 @@ static int sun4i_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
> static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
> {
> struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
> + int ret;
> +
> + if (info->use_dt) {
> + pm_runtime_get_sync(indio_dev->dev.parent);
> +
> + ret = regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
> + if (!ret)
> + pm_runtime_mark_last_busy(indio_dev->dev.parent);
> +
> + pm_runtime_put_autosuspend(indio_dev->dev.parent);
> +
> + return 0;
> + }
Why is runtime_pm linked to the DT support or not?
>
> return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
> }
> @@ -410,7 +446,7 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
> unsigned int *irq, atomic_t *atomic)
> {
> int ret;
> - struct sun4i_gpadc_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent);
> + struct sun4i_gpadc_dev *mfd_dev;
> struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(&pdev->dev));
>
> /*
> @@ -427,6 +463,8 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
> */
> atomic_set(atomic, 1);
>
> + mfd_dev = dev_get_drvdata(pdev->dev.parent);
> +
Why is that change necessary?
> ret = platform_get_irq_byname(pdev, name);
> if (ret < 0) {
> dev_err(&pdev->dev, "no %s interrupt registered\n", name);
> @@ -454,31 +492,68 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
> return 0;
> }
>
> -static int sun4i_gpadc_probe(struct platform_device *pdev)
> +static const struct of_device_id sun4i_gpadc_of_id[] = {
> + {
> + .compatible = "allwinner,sun8i-a33-gpadc-iio",
> + .data = &sun8i_gpadc_data,
> + },
> + { /* sentinel */ }
> +};
> +
> +static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
> + struct iio_dev *indio_dev)
> {
> - struct sun4i_gpadc_iio *info;
> - struct iio_dev *indio_dev;
> + struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
> + const struct of_device_id *of_dev;
> + struct thermal_zone_device *tzd;
> + struct resource *mem;
> + void __iomem *base;
> int ret;
> - struct sun4i_gpadc_dev *sun4i_gpadc_dev;
>
> - sun4i_gpadc_dev = dev_get_drvdata(pdev->dev.parent);
> + of_dev = of_match_device(sun4i_gpadc_of_id, &pdev->dev);
> + if (!of_dev)
> + return -ENODEV;
> +
> + info->use_dt = true;
> + info->data = (struct gpadc_data *)of_dev->data;
> + indio_dev->num_channels = ARRAY_SIZE(sun8i_gpadc_channels);
> + indio_dev->channels = sun8i_gpadc_channels;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + base = devm_ioremap_resource(&pdev->dev, mem);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> + &sun4i_gpadc_regmap_config);
> + if (IS_ERR(info->regmap)) {
> + ret = PTR_ERR(info->regmap);
> + dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
> + return ret;
> + }
>
> - indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> - if (!indio_dev)
> - return -ENOMEM;
> + tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
> + &sun4i_ts_tz_ops);
> + if (IS_ERR(tzd)) {
> + dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
> + PTR_ERR(tzd));
> + return PTR_ERR(tzd);
> + }
>
> - info = iio_priv(indio_dev);
> - platform_set_drvdata(pdev, indio_dev);
> + return 0;
> +}
>
> - mutex_init(&info->mutex);
> +static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
> + struct iio_dev *indio_dev)
> +{
> + struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
> + struct sun4i_gpadc_dev *sun4i_gpadc_dev;
> + int ret;
> +
> + info->use_dt = false;
> + sun4i_gpadc_dev = dev_get_drvdata(pdev->dev.parent);
> info->regmap = sun4i_gpadc_dev->regmap;
> - info->indio_dev = indio_dev;
> - init_completion(&info->completion);
> - indio_dev->name = dev_name(&pdev->dev);
> - indio_dev->dev.parent = &pdev->dev;
> - indio_dev->dev.of_node = pdev->dev.of_node;
> - indio_dev->info = &sun4i_gpadc_iio_info;
> - indio_dev->modes = INDIO_DIRECT_MODE;
> +
You should have two patches. One to split the MFD part from the probe,
and a second one to add the DT probing.
> indio_dev->num_channels = ARRAY_SIZE(sun4i_gpadc_channels);
> indio_dev->channels = sun4i_gpadc_channels;
>
> @@ -494,7 +569,6 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
> * register the sensor if that option is enabled, eventually leaving
> * that choice to the user.
> */
> -
Spurious change?
> if (IS_ENABLED(CONFIG_THERMAL_OF)) {
> /*
> * This driver is a child of an MFD which has a node in the DT
> @@ -519,8 +593,7 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
> dev_err(&pdev->dev,
> "could not register thermal sensor: %ld\n",
> PTR_ERR(tzd));
> - ret = PTR_ERR(tzd);
> - goto err;
> + return PTR_ERR(tzd);
> }
> } else {
> indio_dev->num_channels =
> @@ -528,49 +601,78 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
> indio_dev->channels = sun4i_gpadc_channels_no_temp;
> }
>
> - pm_runtime_set_autosuspend_delay(&pdev->dev,
> - SUN4I_GPADC_AUTOSUSPEND_DELAY);
> - pm_runtime_use_autosuspend(&pdev->dev);
> - pm_runtime_set_suspended(&pdev->dev);
> - pm_runtime_enable(&pdev->dev);
> -
> if (IS_ENABLED(CONFIG_THERMAL_OF)) {
> ret = sun4i_irq_init(pdev, "TEMP_DATA_PENDING",
> sun4i_gpadc_temp_data_irq_handler,
> "temp_data", &info->temp_data_irq,
> &info->ignore_temp_data_irq);
> if (ret < 0)
> - goto err;
> - }
> + return ret;
>
> - ret = sun4i_irq_init(pdev, "FIFO_DATA_PENDING",
> - sun4i_gpadc_fifo_data_irq_handler, "fifo_data",
> - &info->fifo_data_irq, &info->ignore_fifo_data_irq);
> - if (ret < 0)
> - goto err;
> + ret = sun4i_irq_init(pdev, "FIFO_DATA_PENDING",
> + sun4i_gpadc_fifo_data_irq_handler,
> + "fifo_data", &info->fifo_data_irq,
> + &info->ignore_fifo_data_irq);
> + if (ret < 0)
> + return ret;
> + }
>
> - if (IS_ENABLED(CONFIG_THERMAL_OF)) {
> - ret = iio_map_array_register(indio_dev, sun4i_gpadc_hwmon_maps);
> - if (ret < 0) {
> - dev_err(&pdev->dev,
> - "failed to register iio map array\n");
> - goto err;
> - }
> + ret = iio_map_array_register(indio_dev, sun4i_gpadc_hwmon_maps);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to register iio map array\n");
> + return ret;
> }
>
> + return 0;
> +}
> +
> +static int sun4i_gpadc_probe(struct platform_device *pdev)
> +{
> + struct sun4i_gpadc_iio *info;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + info = iio_priv(indio_dev);
> + platform_set_drvdata(pdev, indio_dev);
> +
> + mutex_init(&info->mutex);
> + info->indio_dev = indio_dev;
> + init_completion(&info->completion);
> + indio_dev->name = dev_name(&pdev->dev);
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->dev.of_node = pdev->dev.of_node;
> + indio_dev->info = &sun4i_gpadc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + if (pdev->dev.of_node)
> + ret = sun4i_gpadc_probe_dt(pdev, indio_dev);
> + else
> + ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
> +
> + if (ret)
> + return ret;
> +
> + pm_runtime_set_autosuspend_delay(&pdev->dev,
> + SUN4I_GPADC_AUTOSUSPEND_DELAY);
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_set_suspended(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> +
> ret = devm_iio_device_register(&pdev->dev, indio_dev);
> if (ret < 0) {
> dev_err(&pdev->dev, "could not register the device\n");
> - goto err_map;
> + goto err;
> }
>
> return 0;
>
> -err_map:
> - if (IS_ENABLED(CONFIG_THERMAL_OF))
> - iio_map_array_unregister(indio_dev);
> -
> err:
> + if (!info->use_dt && IS_ENABLED(CONFIG_THERMAL_OF))
> + iio_map_array_unregister(indio_dev);
> pm_runtime_put(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
>
> @@ -580,10 +682,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
> static int sun4i_gpadc_remove(struct platform_device *pdev)
> {
> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
>
> pm_runtime_put(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> - if (IS_ENABLED(CONFIG_THERMAL_OF))
> + if (!info->use_dt && IS_ENABLED(CONFIG_THERMAL_OF))
> iio_map_array_unregister(indio_dev);
>
> return 0;
> @@ -599,6 +702,7 @@ static const struct platform_device_id sun4i_gpadc_id[] = {
> static struct platform_driver sun4i_gpadc_driver = {
> .driver = {
> .name = "sun4i-gpadc-iio",
> + .of_match_table = sun4i_gpadc_of_id,
> .pm = &sun4i_gpadc_pm_ops,
> },
> .id_table = sun4i_gpadc_id,
> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
> index 509e736..139872c 100644
> --- a/include/linux/mfd/sun4i-gpadc.h
> +++ b/include/linux/mfd/sun4i-gpadc.h
> @@ -38,6 +38,10 @@
> #define SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(x) (GENMASK(3, 0) & BIT(x))
> #define SUN6I_GPADC_CTRL1_ADC_CHAN_MASK GENMASK(3, 0)
>
> +/* TP_CTRL1 bits for sun8i SoCs */
> +#define SUN8I_GPADC_CTRL1_CHOP_TEMP_EN BIT(8)
> +#define SUN8I_GPADC_CTRL1_GPADC_CALI_EN BIT(7)
> +
> #define SUN4I_GPADC_CTRL2 0x08
>
> #define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x) ((GENMASK(3, 0) & (x)) << 28)
> --
> 2.9.3
>
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] ARM: dts: sun6i: Add the SPDIF block to the A31
From: Code Kipper @ 2016-12-20 14:34 UTC (permalink / raw)
To: Maxime Ripard; +Cc: linux-arm-kernel, devicetree, linux-sunxi
In-Reply-To: <20161220140709.pxvzcm3osmbromfh@lukather>
On 20 December 2016 at 15:07, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
>
> On Tue, Dec 20, 2016 at 11:40:37AM +0100, codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> From: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> Add the SPDIF transceiver controller block to the A31 dtsi.
>>
>> Signed-off-by: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> arch/arm/boot/dts/sun6i-a31.dtsi | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
>> index 7370ba6c9993..559c53efa7e6 100644
>> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
>> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
>> @@ -613,6 +613,20 @@
>> reg = <0x01c20ca0 0x20>;
>> };
>>
>> + spdif: spdif@01c21000 {
>> + #sound-dai-cells = <0>;
>> + compatible = "allwinner,sun6i-a31-spdif";
>> + reg = <0x01c21000 0x400>;
>> + interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_APB1_SPDIF>, <&ccu CLK_SPDIF>;
>> + resets = <&ccu RST_APB1_SPDIF>;
>> + clock-names = "apb", "spdif";
>> + dmas = <&dma 2>, <&dma 2>;
>> + dma-names = "rx", "tx";
>> + spdif-out = "disabled";
>
> That property isn't documented anywhere, and doesn't seem to be used
> in your driver either.
Ooops....do you want me to respin a new patch or will you do your
magic with 'dd'? It fell through the cracks as it was cherry picked
from my dev branch where I was at one time playing with spdif-in. This
has pretty much been relegated to the bottom of my todo/finish list.
>
> On a separate topic, is the channel inversion bug also found on the
> A31?
I have seen this and I'm sure that was also on my A31 hardware but
I've just fired her up and the speaker test worked as expected. I also
repeated the test on my A10 device and didn't hear the issue.
CK
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
^ permalink raw reply
* Re: [PATCH 2/7] Documentation: DT: bindings: mfd: add documentation for Allwinner SoCs' GPADC MFD driver
From: Maxime Ripard @ 2016-12-20 14:26 UTC (permalink / raw)
To: Quentin Schulz
Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
wens-jdAy2FN1RRM, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
linux-I+IVW8TIWO2tmTQ+vhA3Yw,
stefan.mavrodiev-Re5JQEeQqe8AvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
In-Reply-To: <20161220102709.9504-3-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3105 bytes --]
On Tue, Dec 20, 2016 at 11:27:04AM +0100, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a thermal sensor
> and sometimes as a touchscreen controller. If there is a touchscreen
> controller, the first four channels can be used either for the ADC or
> the touchscreen and the fifth channel is used for the thermal sensor.
> If there is not a touchscreen controller, the one and only channel is
> used for the thermal sensor.
>
> The Allwinner SoCs already have an existing DT binding for the
> touchscreen controller and thermal sensor for the sun4i-ts input driver
> which does let the user use the ADC. To keep backward compatibility,
> this MFD driver re-uses the same bindings as the sun4i-ts input driver
> and will probe the required drivers to make the ADC and thermal sensor
> work.
>
> This patch adds the binding documentation for the MFD driver of the
> Allwinner SoCs' GPADC.
>
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> .../devicetree/bindings/mfd/sun4i-gpadc.txt | 47 ++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> new file mode 100644
> index 0000000..bc4b4f6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> @@ -0,0 +1,47 @@
> +Allwinner SoCs' GPADC Device Tree bindings
> +------------------------------------------
> +
> +The Allwinner SoCs all have an ADC that can also act as a thermal sensor and
> +sometimes as a touchscreen controller. If there is a touchscreen controller, the
> +first four channels can be used either for the ADC or the touchscreen and the
> +fifth channel is used for the thermal sensor.
> +If there is not a touchscreen controller, the one and only channel is used for
> +the thermal sensor.
> +
> +Currently, the touchscreen controller does not have a driver using this ADC
> +driver. The touchscreen controller is currently driven only by
> +input/touchscreen/sun4i-ts.c which is absolutely incompatible with this driver.
> +
> +The Allwinner A10, A13 and A31 SoCs already have a DT binding for the
> +aforementioned input driver, thus this MFD driver matches the existing DT
> +binding (mfd/sun4i-gpadc.c).
> +To keep DT binding compatibility, the MFD replaces the sun4i-ts input driver and
> +probes required drivers (IIO GPADC driver (iio/adc/sun4i-gpadc-iio.c),
> +iio-hwmon and soon the touchscreen driver) without the need for a DT binding for
> +each driver.
> +
> +Required properties:
> + - compatible: one of:
> + - "allwinner,sun4i-a10-ts",
> + - "allwinner,sun5i-a13-ts",
> + - "allwinner,sun6i-a31-ts"
> + - #thermal-sensor-cells = <0>;
Same thing here, we already have such a document, please amend it if
needed.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 1/7] Documentation: DT: bindings: iio: adc: add documentation for Allwinner SoCs' GPADC driver
From: Maxime Ripard @ 2016-12-20 14:25 UTC (permalink / raw)
To: Quentin Schulz
Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
wens-jdAy2FN1RRM, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
linux-I+IVW8TIWO2tmTQ+vhA3Yw,
stefan.mavrodiev-Re5JQEeQqe8AvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
In-Reply-To: <20161220102709.9504-2-quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3408 bytes --]
Hi,
On Tue, Dec 20, 2016 at 11:27:03AM +0100, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a thermal sensor
> and sometimes as a touchscreen controller. If there is a touchscreen
> controller, the first four channels can be used either for the ADC or
> the touchscreen and the fifth channel is used for the thermal sensor.
> If there is not a touchscreen controller, the one and only channel is
> used for the thermal sensor.
>
> This patch adds the documentation for the driver of the Allwinner SoCs'
> GPADC.
>
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> .../bindings/iio/adc/sun4i-gpadc-iio.txt | 57 ++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/sun4i-gpadc-iio.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/sun4i-gpadc-iio.txt b/Documentation/devicetree/bindings/iio/adc/sun4i-gpadc-iio.txt
> new file mode 100644
> index 0000000..aab768d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/sun4i-gpadc-iio.txt
> @@ -0,0 +1,57 @@
> +Allwinner SoCs' GPADC Device Tree bindings
> +------------------------------------------
> +
> +The Allwinner SoCs all have an ADC that can also act as a thermal sensor and
> +sometimes as a touchscreen controller. If there is a touchscreen controller, the
> +first four channels can be used either for the ADC or the touchscreen and the
> +fifth channel is used for the thermal sensor.
> +If there is not a touchscreen controller, the one and only channel is used for
> +the thermal sensor.
> +
> +Currently, the touchscreen controller does not have a driver using this ADC
> +driver. The touchscreen controller is currently driven only by
> +input/touchscreen/sun4i-ts.c which is absolutely incompatible with this driver.
> +
> +The Allwinner A10, A13 and A31 SoCs already have a DT binding for the
> +aforementioned input driver, thus an MFD driver matches the existing DT binding
> +(mfd/sun4i-gpadc.c) and replaces the input driver. No DT binding is required for
> +these SoCs' ADC, everything is handled by the MFD which is matching the existing
> +DT binding for input/touchscreen/sun4i-ts.c.
> +
> +The Allwinner A33 GPADC only have a thermal sensor and have a proper DT binding
> +for this driver unlike the previously mentioned SoCs.
The DT bindings should be agnostic from the OS. You can remove all
mention of the implementations details in Linux.
(and you should wrap at 72 characters).
But we already have a binding document for that controller, so you
shouldn't create a new one, reuse the old one that is already there.
> +Required properties:
> + - compatible: "allwinner,sun8i-a33-gpadc-iio"
IIO is an implementation detail. The IP is called GPADC.
You're also missing reg.
> +
> +Optional properties:
> +(for use with thermal framework for CPU thermal throttling for example, and/or
> + IIO consumers)
> + - #thermal-sensor-cells = <0>; (see
> +Documentation/devicetree/bindings/thermal/thermal.txt)
> + - #io-channel-cells = <0>; (see
> +Documentation/devicetree/bindings/iio/iio-bindings.txt)
I wouldn't list that as optional.
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [linux-sunxi][PATCH] ARM: dts: sun4i: A1000: add axp209 regulator nodes
From: Maxime Ripard @ 2016-12-20 14:16 UTC (permalink / raw)
To: codekipper-Re5JQEeQqe8AvxtiuMwx3w
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20161220102242.2423-1-codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]
On Tue, Dec 20, 2016 at 11:22:42AM +0100, codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> This patch adds the regulator nodes for the axp209 by including
> the axp209 dtsi.
>
> Signed-off-by: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> arch/arm/boot/dts/sun4i-a10-a1000.dts | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts
> index 68c6bdb2cf7c..e7394d701856 100644
> --- a/arch/arm/boot/dts/sun4i-a10-a1000.dts
> +++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts
> @@ -196,6 +196,40 @@
> };
> };
>
> +#include "axp209.dtsi"
> +
> +®_dcdc2 {
> + regulator-always-on;
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1400000>;
> + regulator-name = "vdd-cpu";
> +};
> +
> +®_dcdc3 {
> + regulator-always-on;
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1250000>;
> + regulator-name = "vdd-int-dll";
> +};
> +
> +®_ldo1 {
> + regulator-name = "vdd-rtc";
> +};
> +
> +®_ldo2 {
> + regulator-always-on;
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3000000>;
> + regulator-name = "avcc";
> +};
> +
> +®_ldo3 {
> + regulator-always-on;
> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
> + regulator-name = "vcc-wifi";
If this is used only for the wifi, there's no point in keeping it
enabled.
Also, taking the chance to enable cpufreq by setting the cpu-suplly
property would be a good idea.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] ARM: dts: sun6i: Add the SPDIF block to the A31
From: Maxime Ripard @ 2016-12-20 14:07 UTC (permalink / raw)
To: codekipper-Re5JQEeQqe8AvxtiuMwx3w
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20161220104038.22532-3-codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]
Hi,
On Tue, Dec 20, 2016 at 11:40:37AM +0100, codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Add the SPDIF transceiver controller block to the A31 dtsi.
>
> Signed-off-by: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> arch/arm/boot/dts/sun6i-a31.dtsi | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> index 7370ba6c9993..559c53efa7e6 100644
> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> @@ -613,6 +613,20 @@
> reg = <0x01c20ca0 0x20>;
> };
>
> + spdif: spdif@01c21000 {
> + #sound-dai-cells = <0>;
> + compatible = "allwinner,sun6i-a31-spdif";
> + reg = <0x01c21000 0x400>;
> + interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&ccu CLK_APB1_SPDIF>, <&ccu CLK_SPDIF>;
> + resets = <&ccu RST_APB1_SPDIF>;
> + clock-names = "apb", "spdif";
> + dmas = <&dma 2>, <&dma 2>;
> + dma-names = "rx", "tx";
> + spdif-out = "disabled";
That property isn't documented anywhere, and doesn't seem to be used
in your driver either.
On a separate topic, is the channel inversion bug also found on the
A31?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH] ARM: dts: sun8i-q8-common: enable bluetooth on SDIO Wi-Fi
From: Maxime Ripard @ 2016-12-20 13:50 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Hans de Goede, devicetree@vger.kernel.org, Icenowy Zheng,
linux-arm-kernel@lists.infradead.org, linux-kernel
In-Reply-To: <CAGb2v67wU+Hw-6oa9FxzB33sZY8Up-59HZ_mneFBdcRhXbU1pA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3618 bytes --]
On Mon, Dec 19, 2016 at 10:24:44PM +0800, Chen-Yu Tsai wrote:
> On Mon, Dec 19, 2016 at 10:08 PM, Icenowy Zheng <icenowy@aosc.xyz> wrote:
> >
> >
> > 19.12.2016, 18:09, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> >> On Fri, Dec 16, 2016 at 10:40:00PM +0800, Icenowy Zheng wrote:
> >>> >> > > &r_pio {
> >>> >> > > wifi_pwrseq_pin_q8: wifi_pwrseq_pin@0 {
> >>> >> > > - pins = "PL6", "PL7", "PL11";
> >>> >> > > + pins = "PL6", "PL7", "PL8", "PL11";
> >>> >> > > function = "gpio_in";
> >>> >> > > bias-pull-up;
> >>> >> > > };
> >>> >> >
> >>> >> > There's several things wrong here. The first one is that you rely
> >>> >> > solely on the pinctrl state to maintain a reset line. This is very
> >>> >> > fragile (especially since the GPIO pinctrl state are likely to go away
> >>> >> > at some point), but it also means that if your driver wants to recover
> >>> >> > from that situation at some point, it won't work.
> >>> >> >
> >>> >> > The other one is that the bluetooth and wifi chips are two devices in
> >>> >> > linux, and you assign that pin to the wrong device (wifi).
> >>> >> >
> >>> >> > rfkill-gpio is made just for that, so please use it.
> >>> >>
> >>> >> The GPIO is not for the radio, but for the full Bluetooth part.
> >>> >
> >>> > I know.
> >>> >
> >>> >> If it's set to 0, then the bluetooth part will reset, and the
> >>> >> hciattach will fail.
> >>> >
> >>> > Both rfkill-gpio and rfkill-regulator will shutdown when called
> >>> > (either by poking the reset pin or shutting down the regulator), so
> >>> > that definitely seems like an expected behavior to put the device in
> >>> > reset.
> >>> >
> >>> >> The BSP uses this as a rfkill, and the result is that the bluetooth
> >>> >> on/off switch do not work properly.
> >>> >
> >>> > Then rfkill needs fixing, but working around it by hoping that the
> >>> > core will probe an entirely different device, and enforcing a default
> >>> > that the rest of the kernel might or might not change is both fragile
> >>> > and wrong.
> >>>
> >>> I think a rfkill-gpio here works just like the BSP rfkill...
> >>>
> >>> The real problem is that the Realtek UART bluetooth driver is a userspace
> >>> program (a modified hciattach), which is not capable of the GPIO reset...
> >>
> >> Can't you run rfkill before attaching? What is the problem exactly?
> >> It's not in reset for long enough?
> >>
> >> This seems more and more like an issue in the BT stack you're
> >> using. We might consider workarounds in the kernel, but they have to
> >> be correct.
> >
> > One more rfkill interface will be generated for hci0 after hciattach, which can
> > be safely toggled block and unblock.
> >
> > However, if the GPIO is toggled down, the hciattach program will die.
> >
> > The bluetooth stack I used is fd.o's BlueZ.
>
> I think the bigger issue is that the tty/serial subsystem does not have
> power sequencing support. Here we're trying to use rfkill to do that,
> but that doesn't seem to be what it was intended for. It might work with
> standalone USB bluetooth controllers that don't need any special setup,
> since the device will just appear and get registered. But it might not
> work so well with UART based adapters that need userspace fiddling with
> firmware and hciattach.
Then you can also have a look at the generic power sequence patches
that are floating around.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6] media: et8ek8: add device tree binding documentation
From: Sakari Ailus @ 2016-12-20 13:05 UTC (permalink / raw)
To: Pavel Machek
Cc: Rob Herring, ivo.g.dimitrov.75, sre, pali.rohar, linux-media,
pawel.moll, mark.rutland, ijc+devicetree, galak, mchehab,
devicetree, linux-kernel
In-Reply-To: <20161114183040.GB28778@amd>
Hi Pavel,
On Mon, Nov 14, 2016 at 07:30:40PM +0100, Pavel Machek wrote:
> Add device tree binding documentation for toshiba et8ek8 sensor.
>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
> ---
>
> v6: added missing article, fixed signal polarity.
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt b/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
> new file mode 100644
> index 0000000..b03b21d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
> @@ -0,0 +1,53 @@
> +Toshiba et8ek8 5MP sensor
> +
> +Toshiba et8ek8 5MP sensor is an image sensor found in Nokia N900 device
> +
> +More detailed documentation can be found in
> +Documentation/devicetree/bindings/media/video-interfaces.txt .
> +
> +
> +Mandatory properties
> +--------------------
> +
> +- compatible: "toshiba,et8ek8"
> +- reg: I2C address (0x3e, or an alternative address)
> +- vana-supply: Analogue voltage supply (VANA), 2.8 volts
> +- clocks: External clock to the sensor
> +- clock-frequency: Frequency of the external clock to the sensor. Camera
> + driver will set this frequency on the external clock. The clock frequency is
> + a pre-determined frequency known to be suitable to the board.
> +- reset-gpios: XSHUTDOWN GPIO. The XSHUTDOWN signal is active low. The sensor
> + is in hardware standby mode when the signal is in the low state.
> +
> +
> +Endpoint node mandatory properties
> +----------------------------------
> +
> +- remote-endpoint: A phandle to the bus receiver's endpoint node.
> +
> +Endpoint node optional properties
> +----------------------------------
> +
> +- clock-lanes: <0>
> +- data-lanes: <1..n>
The driver makes no use of them and CCP2 only supports a single lane. I'll
just remove these and apply it to my tree. Let's continue discussing the
driver patch in the other thread.
> +
> +Example
> +-------
> +
> +&i2c3 {
> + clock-frequency = <400000>;
> +
> + cam1: camera@3e {
> + compatible = "toshiba,et8ek8";
> + reg = <0x3e>;
> + vana-supply = <&vaux4>;
> + clocks = <&isp 0>;
> + clock-frequency = <9600000>;
> + reset-gpio = <&gpio4 6 GPIO_ACTIVE_HIGH>; /* 102 */
> + port {
> + csi_cam1: endpoint {
> + remote-endpoint = <&csi_out1>;
> + };
> + };
> + };
> +};
>
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply
* Re: [PATCH v2] mmc: sdhci-cadence: add Socionext UniPhier specific compatible string
From: Ulf Hansson @ 2016-12-20 10:50 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-mmc@vger.kernel.org, Rob Herring, Adrian Hunter,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Rob Herring, Mark Rutland
In-Reply-To: <1481681446-29832-1-git-send-email-yamada.masahiro@socionext.com>
On 14 December 2016 at 03:10, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Add a Socionext SoC specific compatible (suggested by Rob Herring).
>
> No SoC specific data are associated with the compatible strings for
> now, but other SoC vendors may use this IP and want to differentiate
> IP variants in the future.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Thanks, applied for fixes!
Kind regards
Uffe
> ---
>
> Changes in v2:
> - Add "uniphier" to the compatible to make it more SoC-specific
>
> Documentation/devicetree/bindings/mmc/sdhci-cadence.txt | 6 ++++--
> drivers/mmc/host/sdhci-cadence.c | 1 +
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> index 750374f..c0f37cb 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> @@ -1,7 +1,9 @@
> * Cadence SD/SDIO/eMMC Host Controller
>
> Required properties:
> -- compatible: should be "cdns,sd4hc".
> +- compatible: should be one of the following:
> + "cdns,sd4hc" - default of the IP
> + "socionext,uniphier-sd4hc" - for Socionext UniPhier SoCs
> - reg: offset and length of the register set for the device.
> - interrupts: a single interrupt specifier.
> - clocks: phandle to the input clock.
> @@ -19,7 +21,7 @@ if supported. See mmc.txt for details.
>
> Example:
> emmc: sdhci@5a000000 {
> - compatible = "cdns,sd4hc";
> + compatible = "socionext,uniphier-sd4hc", "cdns,sd4hc";
> reg = <0x5a000000 0x400>;
> interrupts = <0 78 4>;
> clocks = <&clk 4>;
> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
> index 1501cfd..4b0ecb9 100644
> --- a/drivers/mmc/host/sdhci-cadence.c
> +++ b/drivers/mmc/host/sdhci-cadence.c
> @@ -262,6 +262,7 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
> }
>
> static const struct of_device_id sdhci_cdns_match[] = {
> + { .compatible = "socionext,uniphier-sd4hc" },
> { .compatible = "cdns,sd4hc" },
> { /* sentinel */ }
> };
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH v2 1/5] clk: samsung: exynos5433: Set NoC (Network On Chip) clocks as critical
From: Sylwester Nawrocki @ 2016-12-20 10:47 UTC (permalink / raw)
To: Chanwoo Choi
Cc: krzk-DgEjT+Ai2ygdnm+yROfE0A, javier-JPH+aEBZ4P+UEJcrhfAQsw,
kgene-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w,
myungjoo.ham-Sze3O3UU22JBDgjK7y7TUQ,
kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Michael Turquette,
Stephen Boyd
In-Reply-To: <58521D0F.5040704-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
On 12/15/2016 05:33 AM, Chanwoo Choi wrote:
> Could you please review this patch?
Chanwoo, the patch looks good to me, I'm going to queue it after
v4.10-rc1 is released.
--
Thanks,
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
* [PATCH 3/3] ARM: dts: sun6i: Add SPDIF to the Mele I7
From: codekipper-Re5JQEeQqe8AvxtiuMwx3w @ 2016-12-20 10:40 UTC (permalink / raw)
To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Marcus Cooper
In-Reply-To: <20161220104038.22532-1-codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Enable the S/PDIF transmitter that is present on the Mele I7.
Signed-off-by: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
arch/arm/boot/dts/sun6i-a31-i7.dts | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/arch/arm/boot/dts/sun6i-a31-i7.dts b/arch/arm/boot/dts/sun6i-a31-i7.dts
index a2193309a199..2bc57d2dcd80 100644
--- a/arch/arm/boot/dts/sun6i-a31-i7.dts
+++ b/arch/arm/boot/dts/sun6i-a31-i7.dts
@@ -69,6 +69,23 @@
gpios = <&pio 7 13 GPIO_ACTIVE_HIGH>;
};
};
+
+ sound {
+ compatible = "simple-audio-card";
+ simple-audio-card,name = "On-board SPDIF";
+ simple-audio-card,cpu {
+ sound-dai = <&spdif>;
+ };
+
+ simple-audio-card,codec {
+ sound-dai = <&spdif_out>;
+ };
+ };
+
+ spdif_out: spdif-out {
+ #sound-dai-cells = <0>;
+ compatible = "linux,spdif-dit";
+ };
};
&codec {
@@ -138,6 +155,13 @@
status = "okay";
};
+&spdif {
+ pinctrl-names = "default";
+ pinctrl-0 = <&spdif_pins_a>;
+ spdif-out = "okay";
+ status = "okay";
+};
+
&uart0 {
pinctrl-names = "default";
pinctrl-0 = <&uart0_pins_a>;
--
2.11.0
^ permalink raw reply related
* [PATCH 2/3] ARM: dts: sun6i: Add the SPDIF block to the A31
From: codekipper-Re5JQEeQqe8AvxtiuMwx3w @ 2016-12-20 10:40 UTC (permalink / raw)
To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Marcus Cooper
In-Reply-To: <20161220104038.22532-1-codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Add the SPDIF transceiver controller block to the A31 dtsi.
Signed-off-by: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
arch/arm/boot/dts/sun6i-a31.dtsi | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 7370ba6c9993..559c53efa7e6 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -613,6 +613,20 @@
reg = <0x01c20ca0 0x20>;
};
+ spdif: spdif@01c21000 {
+ #sound-dai-cells = <0>;
+ compatible = "allwinner,sun6i-a31-spdif";
+ reg = <0x01c21000 0x400>;
+ interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&ccu CLK_APB1_SPDIF>, <&ccu CLK_SPDIF>;
+ resets = <&ccu RST_APB1_SPDIF>;
+ clock-names = "apb", "spdif";
+ dmas = <&dma 2>, <&dma 2>;
+ dma-names = "rx", "tx";
+ spdif-out = "disabled";
+ status = "disabled";
+ };
+
lradc: lradc@01c22800 {
compatible = "allwinner,sun4i-a10-lradc-keys";
reg = <0x01c22800 0x100>;
--
2.11.0
^ permalink raw reply related
* [PATCH 1/3] ARM: dts: sun6i: Add SPDIF TX pin to the A31
From: codekipper-Re5JQEeQqe8AvxtiuMwx3w @ 2016-12-20 10:40 UTC (permalink / raw)
To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Marcus Cooper
In-Reply-To: <20161220104038.22532-1-codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Add the SPDIF TX pin to the A31 dtsi.
Signed-off-by: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 20a0331ddfb5..7370ba6c9993 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -586,6 +586,11 @@
bias-pull-up;
};
+ spdif_pins_a: spdif@0 {
+ pins = "PH28";
+ function = "spdif";
+ };
+
uart0_pins_a: uart0@0 {
pins = "PH20", "PH21";
function = "uart0";
--
2.11.0
^ permalink raw reply related
* [PATCH 0/3] Enable SPDIF on the Mele I7
From: codekipper-Re5JQEeQqe8AvxtiuMwx3w @ 2016-12-20 10:40 UTC (permalink / raw)
To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Marcus Cooper
From: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi All,
here is the patch set required to enable SPDIF on the Mele I7 which is
a A31 based TV-box. To get this working a fix has to be applied to the
clock driver and this will be pushed seperately.
For now the dtsi changes can be applied and when the clk change is
merged then we can finialise this push by applying the I7 patch.
BR,
CK
Marcus Cooper (3):
ARM: dts: sun6i: Add SPDIF TX pin to the A31
ARM: dts: sun6i: Add the SPDIF block to the A31
ARM: dts: sun6i: Add SPDIF to the Mele I7
arch/arm/boot/dts/sun6i-a31-i7.dts | 24 ++++++++++++++++++++++++
arch/arm/boot/dts/sun6i-a31.dtsi | 19 +++++++++++++++++++
2 files changed, 43 insertions(+)
--
2.11.0
^ permalink raw reply
* Re: [PATCH v3 2/2] iio: adc: hx711: Add IIO driver for AVIA HX711
From: Andreas Klinger @ 2016-12-20 10:33 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, pawel.moll-5wv7dgnIgG8,
mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, jic23-DgEjT+Ai2ygdnm+yROfE0A,
knaack.h-Mmb7MZpHnFY, pmeerw-jW+XmwGofnusTnJN9+BGXg
In-Reply-To: <78dfc4c0-f792-12b4-ca07-0242e95f7ee5-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
Hello Lars,
thank you for the thorough review.
I have some questions. See below.
Thanks,
Andreas
Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> schrieb am Mon, 19. Dec 17:28:
> On 12/14/2016 05:17 PM, Andreas Klinger wrote:
> [...]
> > +#include <linux/err.h>
> > +#include <linux/gpio.h>
>
> Since you used the consumer API linux/gpio.h is not needed.
>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/sched.h>
> > +#include <linux/delay.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#define HX711_GAIN_32 2 /* gain = 32 for channel B */
> > +#define HX711_GAIN_64 3 /* gain = 64 for channel A */
> > +#define HX711_GAIN_128 1 /* gain = 128 for channel A */
> > +
> > +struct hx711_data {
> > + struct device *dev;
> > + struct gpio_desc *gpiod_sck;
> > + struct gpio_desc *gpiod_dout;
> > + int gain_pulse;
> > + struct mutex lock;
> > +};
> > +
> > +static int hx711_read(struct hx711_data *hx711_data)
> > +{
> > + int i, ret;
> > + int value = 0;
> > +
> > + mutex_lock(&hx711_data->lock);
> > +
> > + if (hx711_reset(hx711_data)) {
>
> If you reset the device before each conversion wont this clear the channel
> and gain selection? Wouldn't the driver always read from channel A at 128
> gain regardless of what has been selected?
>
This is a bug, i need to fix. Thank you.
> > + dev_err(hx711_data->dev, "reset failed!");
> > + mutex_unlock(&hx711_data->lock);
> > + return -1;
>
> If there is an error it should be propagated to the higher layers. At the
> moment you only return a bogus conversion value.
>
> > + }
> > +
> > + for (i = 0; i < 24; i++) {
> > + value <<= 1;
> > + ret = hx711_cycle(hx711_data);
> > + if (ret)
> > + value++;
> > + }
> > +
> > + value ^= 0x800000;
> > +
> > + for (i = 0; i < hx711_data->gain_pulse; i++)
> > + ret = hx711_cycle(hx711_data);
> > +
> > + mutex_unlock(&hx711_data->lock);
> > +
> > + return value;
> > +}
> > +
> > +static int hx711_read_raw(struct iio_dev *iio_dev,
> > + const struct iio_chan_spec *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct hx711_data *hx711_data = iio_priv(iio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + switch (chan->type) {
> > + case IIO_VOLTAGE:
> > + *val = hx711_read(hx711_data);
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> [...]
> > +static struct attribute *hx711_attributes[] = {
> > + &iio_dev_attr_gain.dev_attr.attr,
>
> For IIO devices the gain is typically expressed through the scale attribute.
> Which is kind of the inverse of gain. It would be good if this driver
> follows this standard notation. The scale is the value of 1LSB in mV. So
> this includes the resolution of the ADC, the reference voltage and any gain
> that is applied to the input signal.
>
> The possible values can be listed in the scale_available attribute.
>
The reference voltage is in the hardware.
Should i use a DT entry for the reference voltage?
Or is it better to use a buildin scale and make it changeable?
> > + NULL,
> > +};
> > +
> > +static struct attribute_group hx711_attribute_group = {
> > + .attrs = hx711_attributes,
> > +};
> > +
> > +static const struct iio_info hx711_iio_info = {
> > + .driver_module = THIS_MODULE,
> > + .read_raw = hx711_read_raw,
> > + .attrs = &hx711_attribute_group,
> > +};
> > +
> > +static const struct iio_chan_spec hx711_chan_spec[] = {
> > + { .type = IIO_VOLTAGE,
> > + .info_mask_separate =
> > + BIT(IIO_CHAN_INFO_RAW),
>
> Given that there are two separate physical input channels this should be
> expressed here and there should be two IIO channels for the device.
>
One who is toggling between channel A and B will cause a dummy read
additional to every normal read.
Should i offer a "toggling mode" which means that after reading
channel A the channel B is selected for the next read and after
reading channel B channel A is selected? Simply expecting the channel
is toggled on every read. If it's not toggled there need to be a dummy
read, of course. This should be an attribute, right?
> > + },
> > +};
> > +
> > +static int hx711_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct hx711_data *hx711_data = NULL;
>
> The = NULL is not needed as it is overwritten a few lines below.
>
> > + struct iio_dev *iio;
> > + int ret = 0;
>
> Again = 0 no needed.
>
> > +
> > + iio = devm_iio_device_alloc(dev, sizeof(struct hx711_data));
> > + if (!iio) {
> > + dev_err(dev, "failed to allocate IIO device\n");
> > + return -ENOMEM;
> > + }
> > +
> > + hx711_data = iio_priv(iio);
> > + hx711_data->dev = dev;
> > +
> > + mutex_init(&hx711_data->lock);
> > +
> > + hx711_data->gpiod_sck = devm_gpiod_get(dev, "sck", GPIOD_OUT_HIGH);
> > + if (IS_ERR(hx711_data->gpiod_sck)) {
> > + dev_err(dev, "failed to get sck-gpiod: err=%ld\n",
> > + PTR_ERR(hx711_data->gpiod_sck));
> > + return PTR_ERR(hx711_data->gpiod_sck);
> > + }
> > +
> > + hx711_data->gpiod_dout = devm_gpiod_get(dev, "dout", GPIOD_OUT_HIGH);
> > + if (IS_ERR(hx711_data->gpiod_dout)) {
> > + dev_err(dev, "failed to get dout-gpiod: err=%ld\n",
> > + PTR_ERR(hx711_data->gpiod_dout));
> > + return PTR_ERR(hx711_data->gpiod_dout);
> > + }
> > +
> > + ret = gpiod_direction_input(hx711_data->gpiod_dout);
>
> If dout is used as a input GPIO you should request it with GPIOD_IN. In that
> case you can remove the gpiod_direction_input() call.
>
> > + if (ret < 0) {
> > + dev_err(hx711_data->dev, "gpiod_direction_input: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = gpiod_direction_output(hx711_data->gpiod_sck, 0);
>
> Similar to above. If you want this to be a output GPIO with the default
> value of 0 request it with GPIOD_OUT_LOW.
>
> > + if (ret < 0) {
> > + dev_err(hx711_data->dev, "gpiod_direction_output: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + platform_set_drvdata(pdev, iio);
>
> There is no matching platform_get_drvdata() so this can probably be removed.
>
> > +
> > + iio->name = pdev->name;
>
> This should be the part name. E.g. "hx711" in this case.
>
> > + iio->dev.parent = &pdev->dev;
> > + iio->info = &hx711_iio_info;
> > + iio->modes = INDIO_DIRECT_MODE;
> > + iio->channels = hx711_chan_spec;
> > + iio->num_channels = ARRAY_SIZE(hx711_chan_spec);
> > +
> > + return devm_iio_device_register(dev, iio);
> > +}
>
--
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 7/7] ARM: dtsi: sun8i-a33: add CPU thermal throttling
From: Quentin Schulz @ 2016-12-20 10:27 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland,
maxime.ripard, wens, lee.jones, linux, stefan.mavrodiev
Cc: Quentin Schulz, linux-iio, devicetree, linux-arm-kernel,
linux-kernel, thomas.petazzoni
In-Reply-To: <20161220102709.9504-1-quentin.schulz@free-electrons.com>
This adds CPU thermal throttling for the Allwinner A33. It uses the
thermal sensor present in the SoC's GPADC.
Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
arch/arm/boot/dts/sun8i-a33.dtsi | 47 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-a33.dtsi b/arch/arm/boot/dts/sun8i-a33.dtsi
index 1fcae81..735ebea 100644
--- a/arch/arm/boot/dts/sun8i-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a33.dtsi
@@ -43,6 +43,7 @@
*/
#include "sun8i-a23-a33.dtsi"
+#include <dt-bindings/thermal/thermal.h>
/ {
cpu0_opp_table: opp_table0 {
@@ -79,6 +80,9 @@
clocks = <&ccu CLK_CPUX>;
clock-names = "cpu";
operating-points-v2 = <&cpu0_opp_table>;
+ cooling-min-level = <0>;
+ cooling-max-level = <3>;
+ #cooling-cells = <2>;
};
cpu@2 {
@@ -100,6 +104,49 @@
status = "disabled";
};
+ thermal-zones {
+ cpu_thermal {
+ /* milliseconds */
+ polling-delay-passive = <250>;
+ polling-delay = <1000>;
+ thermal-sensors = <&rtp>;
+
+ cooling-maps {
+ map0 {
+ trip = <&cpu_alert0>;
+ cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+ };
+ map1 {
+ trip = <&cpu_alert1>;
+ cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+ };
+ };
+
+ trips {
+ cpu_alert0: cpu_alert0 {
+ /* milliCelsius */
+ temperature = <75000>;
+ hysteresis = <2000>;
+ type = "passive";
+ };
+
+ cpu_alert1: cpu_alert1 {
+ /* milliCelsius */
+ temperature = <90000>;
+ hysteresis = <2000>;
+ type = "hot";
+ };
+
+ cpu_crit: cpu_crit {
+ /* milliCelsius */
+ temperature = <110000>;
+ hysteresis = <2000>;
+ type = "critical";
+ };
+ };
+ };
+ };
+
memory {
reg = <0x40000000 0x80000000>;
};
--
2.9.3
^ permalink raw reply related
* [PATCH 6/7] ARM: dtsi: sun8i-a33: add A33 thermal sensor
From: Quentin Schulz @ 2016-12-20 10:27 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland,
maxime.ripard, wens, lee.jones, linux, stefan.mavrodiev
Cc: Quentin Schulz, linux-iio, devicetree, linux-arm-kernel,
linux-kernel, thomas.petazzoni
In-Reply-To: <20161220102709.9504-1-quentin.schulz@free-electrons.com>
This adds the DT node for the thermal sensor present in the Allwinner
A33 GPADC.
Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
arch/arm/boot/dts/sun8i-a33.dtsi | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-a33.dtsi b/arch/arm/boot/dts/sun8i-a33.dtsi
index 2878a77..1fcae81 100644
--- a/arch/arm/boot/dts/sun8i-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a33.dtsi
@@ -151,6 +151,13 @@
reset-names = "ahb";
};
+ rtp: rtp@01c25000 {
+ compatible = "allwinner,sun8i-a33-gpadc-iio";
+ reg = <0x01c25000 0x100>;
+ #thermal-sensor-cells = <0>;
+ #io-channel-cells = <0>;
+ };
+
fe0: display-frontend@01e00000 {
compatible = "allwinner,sun8i-a33-display-frontend";
reg = <0x01e00000 0x20000>;
@@ -261,6 +268,11 @@
};
};
};
+
+ iio-hwmon {
+ compatible = "iio-hwmon";
+ io-channels = <&rtp>;
+ };
};
&ccu {
--
2.9.3
^ permalink raw reply related
* [PATCH 5/7] ARM: dts: sun8i-a33-olinuxino: add cpu-supply
From: Quentin Schulz @ 2016-12-20 10:27 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland,
maxime.ripard, wens, lee.jones, linux, stefan.mavrodiev
Cc: Quentin Schulz, linux-iio, devicetree, linux-arm-kernel,
linux-kernel, thomas.petazzoni
In-Reply-To: <20161220102709.9504-1-quentin.schulz@free-electrons.com>
This adds the cpu-supply DT property to the cpu0 DT node needed by
the board to adapt the regulator voltage depending on the currently use
OPP.
Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
This hasn't been tested on the board but it is what I understand from the
schematics[1] of the board.
Stefan (or anyone owning this board), could you test this series of patch on the
Olinuxino A33, test CPUfreq on it and tell us if it works? Thanks!
[1] https://github.com/OLIMEX/OLINUXINO/raw/master/HARDWARE/A33/A33-OLinuXino_Rev_B1.pdf
arch/arm/boot/dts/sun8i-a33-olinuxino.dts | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-a33-olinuxino.dts b/arch/arm/boot/dts/sun8i-a33-olinuxino.dts
index 9ea637e..df55f54 100644
--- a/arch/arm/boot/dts/sun8i-a33-olinuxino.dts
+++ b/arch/arm/boot/dts/sun8i-a33-olinuxino.dts
@@ -72,6 +72,10 @@
};
};
+&cpu0 {
+ cpu-supply = <®_dcdc3>;
+};
+
&ehci0 {
status = "okay";
};
--
2.9.3
^ permalink raw reply related
* [PATCH 4/7] ARM: dts: sun8i-a33-sinlinx-sina33: add cpu-supply
From: Quentin Schulz @ 2016-12-20 10:27 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland,
maxime.ripard, wens, lee.jones, linux, stefan.mavrodiev
Cc: thomas.petazzoni, devicetree, linux-iio, linux-kernel,
Quentin Schulz, linux-arm-kernel
In-Reply-To: <20161220102709.9504-1-quentin.schulz@free-electrons.com>
This adds the cpu-supply DT property to the cpu0 DT node needed by
the board to adapt the regulator voltage depending on the currently used
OPP.
Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts b/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
index fef6abc..0901c57 100644
--- a/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
+++ b/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
@@ -63,6 +63,10 @@
};
};
+&cpu0 {
+ cpu-supply = <®_dcdc3>;
+};
+
&ehci0 {
status = "okay";
};
--
2.9.3
^ permalink raw reply related
* [PATCH 3/7] iio: adc: sun4i-gpadc-iio: add support for A33 thermal sensor
From: Quentin Schulz @ 2016-12-20 10:27 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland,
maxime.ripard, wens, lee.jones, linux, stefan.mavrodiev
Cc: thomas.petazzoni, devicetree, linux-iio, linux-kernel,
Quentin Schulz, linux-arm-kernel
In-Reply-To: <20161220102709.9504-1-quentin.schulz@free-electrons.com>
This adds support for the Allwinner A33 thermal sensor.
Unlike the A10, A13 and A31, the Allwinner A33 only has one channel
which is dedicated to the thermal sensor. Moreover, its thermal sensor
does not generate interruptions, thus we only need to directly read the
register storing the temperature value.
The MFD used by the A10, A13 and A31, was created to avoid breaking the
DT binding, but since the nodes for the ADC weren't there for the A33,
it is not needed.
Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
drivers/iio/adc/Kconfig | 21 ++--
drivers/iio/adc/sun4i-gpadc-iio.c | 204 ++++++++++++++++++++++++++++----------
include/linux/mfd/sun4i-gpadc.h | 4 +
3 files changed, 172 insertions(+), 57 deletions(-)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 6a6d369..06041ff 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -437,17 +437,24 @@ config STX104
config SUN4I_GPADC
tristate "Support for the Allwinner SoCs GPADC"
depends on IIO
- depends on MFD_SUN4I_GPADC
- help
- Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
- GPADC. This ADC provides 4 channels which can be used as an ADC or as
- a touchscreen input and one channel for thermal sensor.
-
- The thermal sensor slows down ADC readings and can be disabled by
+# MFD_SUN4I_GPADC is needed for sun4i, sun5i and sun6i but not for sun8i
+ select MFD_SUN4I_GPADC if MACH_SUN4I || MACH_SUN5I || MACH_SUN6I
+# THERMAL_OF can be disabled on sun4i, sun5i and sun6i to quicken ADC readings
+ depends on THERMAL_OF || MACH_SUN4I || MACH_SUN5I || MACH_SUN6I
+ depends on !TOUCHSCREEN_SUN4I
+ help
+ Say yes here to build support for Allwinner (A10, A13, A31 and A33)
+ SoCs GPADC.
+
+ The ADC on A10, A13 and A31 provides 4 channels which can be used as
+ an ADC or as a touchscreen input and one channel for thermal sensor.
+ Their thermal sensor slows down ADC readings and can be disabled by
disabling CONFIG_THERMAL_OF. However, the thermal sensor should be
enabled by default since the SoC temperature is usually more critical
than ADC readings.
+ The ADC on A33 provides one channel for thermal sensor.
+
To compile this driver as a module, choose M here: the module will be
called sun4i-gpadc-iio.
diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index a8e134f..8be694e 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -1,4 +1,4 @@
-/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
+/* ADC driver for sunxi platforms' (A10, A13, A31 and A33) GPADC
*
* Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons.com>
*
@@ -85,6 +85,12 @@ static const struct gpadc_data sun6i_gpadc_data = {
.adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
};
+static const struct gpadc_data sun8i_gpadc_data = {
+ .temp_offset = -1662,
+ .temp_scale = 162,
+ .tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
+};
+
struct sun4i_gpadc_iio {
struct iio_dev *indio_dev;
struct completion completion;
@@ -96,6 +102,7 @@ struct sun4i_gpadc_iio {
unsigned int temp_data_irq;
atomic_t ignore_temp_data_irq;
const struct gpadc_data *data;
+ bool use_dt;
/* prevents concurrent reads of temperature and ADC */
struct mutex mutex;
};
@@ -138,6 +145,23 @@ static const struct iio_chan_spec sun4i_gpadc_channels_no_temp[] = {
SUN4I_GPADC_ADC_CHANNEL(3, "adc_chan3"),
};
+static const struct iio_chan_spec sun8i_gpadc_channels[] = {
+ {
+ .type = IIO_TEMP,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_OFFSET),
+ .datasheet_name = "temp_adc",
+ },
+};
+
+static const struct regmap_config sun4i_gpadc_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .fast_io = true,
+};
+
static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel,
unsigned int irq)
{
@@ -231,7 +255,6 @@ static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int *val,
err:
pm_runtime_put_autosuspend(indio_dev->dev.parent);
mutex_unlock(&info->mutex);
-
return ret;
}
@@ -246,6 +269,19 @@ static int sun4i_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
{
struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
+ int ret;
+
+ if (info->use_dt) {
+ pm_runtime_get_sync(indio_dev->dev.parent);
+
+ ret = regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
+ if (!ret)
+ pm_runtime_mark_last_busy(indio_dev->dev.parent);
+
+ pm_runtime_put_autosuspend(indio_dev->dev.parent);
+
+ return 0;
+ }
return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
}
@@ -410,7 +446,7 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
unsigned int *irq, atomic_t *atomic)
{
int ret;
- struct sun4i_gpadc_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent);
+ struct sun4i_gpadc_dev *mfd_dev;
struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(&pdev->dev));
/*
@@ -427,6 +463,8 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
*/
atomic_set(atomic, 1);
+ mfd_dev = dev_get_drvdata(pdev->dev.parent);
+
ret = platform_get_irq_byname(pdev, name);
if (ret < 0) {
dev_err(&pdev->dev, "no %s interrupt registered\n", name);
@@ -454,31 +492,68 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
return 0;
}
-static int sun4i_gpadc_probe(struct platform_device *pdev)
+static const struct of_device_id sun4i_gpadc_of_id[] = {
+ {
+ .compatible = "allwinner,sun8i-a33-gpadc-iio",
+ .data = &sun8i_gpadc_data,
+ },
+ { /* sentinel */ }
+};
+
+static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
+ struct iio_dev *indio_dev)
{
- struct sun4i_gpadc_iio *info;
- struct iio_dev *indio_dev;
+ struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
+ const struct of_device_id *of_dev;
+ struct thermal_zone_device *tzd;
+ struct resource *mem;
+ void __iomem *base;
int ret;
- struct sun4i_gpadc_dev *sun4i_gpadc_dev;
- sun4i_gpadc_dev = dev_get_drvdata(pdev->dev.parent);
+ of_dev = of_match_device(sun4i_gpadc_of_id, &pdev->dev);
+ if (!of_dev)
+ return -ENODEV;
+
+ info->use_dt = true;
+ info->data = (struct gpadc_data *)of_dev->data;
+ indio_dev->num_channels = ARRAY_SIZE(sun8i_gpadc_channels);
+ indio_dev->channels = sun8i_gpadc_channels;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ base = devm_ioremap_resource(&pdev->dev, mem);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
+ &sun4i_gpadc_regmap_config);
+ if (IS_ERR(info->regmap)) {
+ ret = PTR_ERR(info->regmap);
+ dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
+ return ret;
+ }
- indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
- if (!indio_dev)
- return -ENOMEM;
+ tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
+ &sun4i_ts_tz_ops);
+ if (IS_ERR(tzd)) {
+ dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
+ PTR_ERR(tzd));
+ return PTR_ERR(tzd);
+ }
- info = iio_priv(indio_dev);
- platform_set_drvdata(pdev, indio_dev);
+ return 0;
+}
- mutex_init(&info->mutex);
+static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
+ struct iio_dev *indio_dev)
+{
+ struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
+ struct sun4i_gpadc_dev *sun4i_gpadc_dev;
+ int ret;
+
+ info->use_dt = false;
+ sun4i_gpadc_dev = dev_get_drvdata(pdev->dev.parent);
info->regmap = sun4i_gpadc_dev->regmap;
- info->indio_dev = indio_dev;
- init_completion(&info->completion);
- indio_dev->name = dev_name(&pdev->dev);
- indio_dev->dev.parent = &pdev->dev;
- indio_dev->dev.of_node = pdev->dev.of_node;
- indio_dev->info = &sun4i_gpadc_iio_info;
- indio_dev->modes = INDIO_DIRECT_MODE;
+
indio_dev->num_channels = ARRAY_SIZE(sun4i_gpadc_channels);
indio_dev->channels = sun4i_gpadc_channels;
@@ -494,7 +569,6 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
* register the sensor if that option is enabled, eventually leaving
* that choice to the user.
*/
-
if (IS_ENABLED(CONFIG_THERMAL_OF)) {
/*
* This driver is a child of an MFD which has a node in the DT
@@ -519,8 +593,7 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
dev_err(&pdev->dev,
"could not register thermal sensor: %ld\n",
PTR_ERR(tzd));
- ret = PTR_ERR(tzd);
- goto err;
+ return PTR_ERR(tzd);
}
} else {
indio_dev->num_channels =
@@ -528,49 +601,78 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
indio_dev->channels = sun4i_gpadc_channels_no_temp;
}
- pm_runtime_set_autosuspend_delay(&pdev->dev,
- SUN4I_GPADC_AUTOSUSPEND_DELAY);
- pm_runtime_use_autosuspend(&pdev->dev);
- pm_runtime_set_suspended(&pdev->dev);
- pm_runtime_enable(&pdev->dev);
-
if (IS_ENABLED(CONFIG_THERMAL_OF)) {
ret = sun4i_irq_init(pdev, "TEMP_DATA_PENDING",
sun4i_gpadc_temp_data_irq_handler,
"temp_data", &info->temp_data_irq,
&info->ignore_temp_data_irq);
if (ret < 0)
- goto err;
- }
+ return ret;
- ret = sun4i_irq_init(pdev, "FIFO_DATA_PENDING",
- sun4i_gpadc_fifo_data_irq_handler, "fifo_data",
- &info->fifo_data_irq, &info->ignore_fifo_data_irq);
- if (ret < 0)
- goto err;
+ ret = sun4i_irq_init(pdev, "FIFO_DATA_PENDING",
+ sun4i_gpadc_fifo_data_irq_handler,
+ "fifo_data", &info->fifo_data_irq,
+ &info->ignore_fifo_data_irq);
+ if (ret < 0)
+ return ret;
+ }
- if (IS_ENABLED(CONFIG_THERMAL_OF)) {
- ret = iio_map_array_register(indio_dev, sun4i_gpadc_hwmon_maps);
- if (ret < 0) {
- dev_err(&pdev->dev,
- "failed to register iio map array\n");
- goto err;
- }
+ ret = iio_map_array_register(indio_dev, sun4i_gpadc_hwmon_maps);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to register iio map array\n");
+ return ret;
}
+ return 0;
+}
+
+static int sun4i_gpadc_probe(struct platform_device *pdev)
+{
+ struct sun4i_gpadc_iio *info;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ info = iio_priv(indio_dev);
+ platform_set_drvdata(pdev, indio_dev);
+
+ mutex_init(&info->mutex);
+ info->indio_dev = indio_dev;
+ init_completion(&info->completion);
+ indio_dev->name = dev_name(&pdev->dev);
+ indio_dev->dev.parent = &pdev->dev;
+ indio_dev->dev.of_node = pdev->dev.of_node;
+ indio_dev->info = &sun4i_gpadc_iio_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ if (pdev->dev.of_node)
+ ret = sun4i_gpadc_probe_dt(pdev, indio_dev);
+ else
+ ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
+
+ if (ret)
+ return ret;
+
+ pm_runtime_set_autosuspend_delay(&pdev->dev,
+ SUN4I_GPADC_AUTOSUSPEND_DELAY);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_set_suspended(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+
ret = devm_iio_device_register(&pdev->dev, indio_dev);
if (ret < 0) {
dev_err(&pdev->dev, "could not register the device\n");
- goto err_map;
+ goto err;
}
return 0;
-err_map:
- if (IS_ENABLED(CONFIG_THERMAL_OF))
- iio_map_array_unregister(indio_dev);
-
err:
+ if (!info->use_dt && IS_ENABLED(CONFIG_THERMAL_OF))
+ iio_map_array_unregister(indio_dev);
pm_runtime_put(&pdev->dev);
pm_runtime_disable(&pdev->dev);
@@ -580,10 +682,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
static int sun4i_gpadc_remove(struct platform_device *pdev)
{
struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+ struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
pm_runtime_put(&pdev->dev);
pm_runtime_disable(&pdev->dev);
- if (IS_ENABLED(CONFIG_THERMAL_OF))
+ if (!info->use_dt && IS_ENABLED(CONFIG_THERMAL_OF))
iio_map_array_unregister(indio_dev);
return 0;
@@ -599,6 +702,7 @@ static const struct platform_device_id sun4i_gpadc_id[] = {
static struct platform_driver sun4i_gpadc_driver = {
.driver = {
.name = "sun4i-gpadc-iio",
+ .of_match_table = sun4i_gpadc_of_id,
.pm = &sun4i_gpadc_pm_ops,
},
.id_table = sun4i_gpadc_id,
diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
index 509e736..139872c 100644
--- a/include/linux/mfd/sun4i-gpadc.h
+++ b/include/linux/mfd/sun4i-gpadc.h
@@ -38,6 +38,10 @@
#define SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(x) (GENMASK(3, 0) & BIT(x))
#define SUN6I_GPADC_CTRL1_ADC_CHAN_MASK GENMASK(3, 0)
+/* TP_CTRL1 bits for sun8i SoCs */
+#define SUN8I_GPADC_CTRL1_CHOP_TEMP_EN BIT(8)
+#define SUN8I_GPADC_CTRL1_GPADC_CALI_EN BIT(7)
+
#define SUN4I_GPADC_CTRL2 0x08
#define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x) ((GENMASK(3, 0) & (x)) << 28)
--
2.9.3
^ 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