[linux-sunxi] Re: [PATCH 1/4] power: Add an axp20x-ac-power driver
Hi,
On Mon, Apr 4, 2016 at 10:11 PM, Michael Haas wrote:
> Hi Maxime,
>
> thanks for taking the time to review this.
>
> On 04/04/2016 11:38 PM, Maxime Ripard wrote:
>> Hi,
>>>
>>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>>> index a57d6e9..9351c0e 100644
>>> --- a/drivers/mfd/axp20x.c
>>> +++ b/drivers/mfd/axp20x.c
>>> @@ -178,6 +178,12 @@ static struct resource axp288_power_button_resources[]
>>> = {
>>> },
>>> };
>>>
>>> +static struct resource axp20x_ac_power_supply_resources[] = {
>>> +DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
>>> +DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
>>> +DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_OVER_V, "ACIN_OVER_V"),
>>> +};
>>> +
>>> static struct resource axp288_fuel_gauge_resources[] = {
>>> {
>>> .start = AXP288_IRQ_QWBTU,
>>> @@ -440,6 +446,11 @@ static struct mfd_cell axp20x_cells[] = {
>>> .of_compatible = "x-powers,axp202-usb-power-supply",
>>> .num_resources =
>>> ARRAY_SIZE(axp20x_usb_power_supply_resources),
>>> .resources = axp20x_usb_power_supply_resources,
>>> +}, {
>>> +.name = "axp20x-ac-power-supply",
>>> +.of_compatible = "x-powers,axp202-ac-power-supply",
>>> +.num_resources = ARRAY_SIZE(axp20x_ac_power_supply_resources),
>>> +.resources = axp20x_ac_power_supply_resources,
>>> },
>>> };
>>>
>>
>> These changes should be in a separate patch.
>
> Will do!
>>
>
>>> +
>>> +static irqreturn_t axp20x_irq_ac_removal(int irq, void *devid)
>>> +{
>>> +struct axp20x_ac_power *power = devid;
>>> +
>>> +dev_info(&power->supply->dev, "IRQ#%d AC disconnected\n", irq);
>>> +power_supply_changed(power->supply);
>>> +
>>> +return IRQ_HANDLED;
>>> +}
>>
>> Logging in the interrupt handler is usually a bad idea, for several
>> reasons:
>>- If you have a console, it's going to be output on the console,
>> which might take quite some time. And you don't want to take
>> quite some time in the interrupt handler.
>>- printk might not even work in the interrupt context in some
>> scenarios.
>>
>> Removing that handler, you can register the same interrupt handler on
>> all the interrupts.
>>
>
> Oops. Yes, I will fix that!
>
>>> +static int axp20x_ac_power_probe(struct platform_device *pdev)
>>> +{
>>> +struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>>> +struct power_supply_config psy_cfg = {};
>>> +struct axp20x_ac_power *power;
>>> +static const char * const irq_names[] = { "ACIN_PLUGIN",
>>> +"ACIN_REMOVAL", "ACIN_OVER_V" };
>>> +irqreturn_t (*irq_funcs[])(int, void *) = { axp20x_irq_ac_plugin,
>>> +axp20x_irq_ac_removal, axp20x_irq_ac_over_v };
>>> +int i, irq, r, input;
>>> +
>>> +if (!of_device_is_available(pdev->dev.of_node))
>>> +return -ENODEV;
>>
>> That's useless. If the device is not available, you're not going to be
>> probed in the first place.
>
> Ok.
>
>>
>>> +power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
>>> +if (!power)
>>> +return -ENOMEM;
>>> +
>>> +power->regmap = axp20x->regmap;
>>> +
>>> +r = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &input);
>>> +if (r < 0)
>>> +return r;
>>> +
>>> +if (!!(input & AXP20X_PWR_STATUS_AC_VBUS_SHORT)) {
>>> +dev_err(&pdev->dev, "AC is connected to VBUS. Use
>>> axp20x_usb_power-supply driver instead!");
>>> +return -ENODEV;
>>> +}
>>
>> Can't that change over time? Can't we support both drivers at the same time?
>
> Both drivers are supported at the same time. I did even try with both
> AC and USB connected and both return meaningful values, at least for
> voltage. The AXP20x can drawn power from both sources at the same time.
>
> The check above fires in a specific scenario where ACIN and VBUS are
> physically connected on the PCB. The Datasheet states for that
> particular register:
>
> "Indicates whether ACIN/VBUS short circuits on PCB or not"
>
> So, assuming the chip detects the condition correctly, this does not
> change over that. But you can switch from ACIN to VBUS and vice-versa
> just fine if they are not short-circuited. If they are connected
> together, I'd prefer the DTS to only enable the usb driver. The check
> above is a last resort there.
>
> Then again, with the quality of those data sheets, one never knows.
If they are short circuited, does that mean the PMIC will only
draw power from one source? If both are still used, then the
current and voltage readings still make sense. Then it makes
sense to have both drivers enabled, no?
Or would something bad happen?
ChenYu
>>
>>> +/* Enable ac voltage and current measurement */
>>> +r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
>>> +AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT,
>>> +
[linux-sunxi] Re: [PATCH 1/4] power: Add an axp20x-ac-power driver
Hi Maxime,
thanks for taking the time to review this.
On 04/04/2016 11:38 PM, Maxime Ripard wrote:
> Hi,
>>
>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> index a57d6e9..9351c0e 100644
>> --- a/drivers/mfd/axp20x.c
>> +++ b/drivers/mfd/axp20x.c
>> @@ -178,6 +178,12 @@ static struct resource axp288_power_button_resources[]
>> = {
>> },
>> };
>>
>> +static struct resource axp20x_ac_power_supply_resources[] = {
>> +DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
>> +DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
>> +DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_OVER_V, "ACIN_OVER_V"),
>> +};
>> +
>> static struct resource axp288_fuel_gauge_resources[] = {
>> {
>> .start = AXP288_IRQ_QWBTU,
>> @@ -440,6 +446,11 @@ static struct mfd_cell axp20x_cells[] = {
>> .of_compatible = "x-powers,axp202-usb-power-supply",
>> .num_resources = ARRAY_SIZE(axp20x_usb_power_supply_resources),
>> .resources = axp20x_usb_power_supply_resources,
>> +}, {
>> +.name = "axp20x-ac-power-supply",
>> +.of_compatible = "x-powers,axp202-ac-power-supply",
>> +.num_resources = ARRAY_SIZE(axp20x_ac_power_supply_resources),
>> +.resources = axp20x_ac_power_supply_resources,
>> },
>> };
>>
>
> These changes should be in a separate patch.
Will do!
>
>> +
>> +static irqreturn_t axp20x_irq_ac_removal(int irq, void *devid)
>> +{
>> +struct axp20x_ac_power *power = devid;
>> +
>> +dev_info(&power->supply->dev, "IRQ#%d AC disconnected\n", irq);
>> +power_supply_changed(power->supply);
>> +
>> +return IRQ_HANDLED;
>> +}
>
> Logging in the interrupt handler is usually a bad idea, for several
> reasons:
>- If you have a console, it's going to be output on the console,
> which might take quite some time. And you don't want to take
> quite some time in the interrupt handler.
>- printk might not even work in the interrupt context in some
> scenarios.
>
> Removing that handler, you can register the same interrupt handler on
> all the interrupts.
>
Oops. Yes, I will fix that!
>> +static int axp20x_ac_power_probe(struct platform_device *pdev)
>> +{
>> +struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>> +struct power_supply_config psy_cfg = {};
>> +struct axp20x_ac_power *power;
>> +static const char * const irq_names[] = { "ACIN_PLUGIN",
>> +"ACIN_REMOVAL", "ACIN_OVER_V" };
>> +irqreturn_t (*irq_funcs[])(int, void *) = { axp20x_irq_ac_plugin,
>> +axp20x_irq_ac_removal, axp20x_irq_ac_over_v };
>> +int i, irq, r, input;
>> +
>> +if (!of_device_is_available(pdev->dev.of_node))
>> +return -ENODEV;
>
> That's useless. If the device is not available, you're not going to be
> probed in the first place.
Ok.
>
>> +power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
>> +if (!power)
>> +return -ENOMEM;
>> +
>> +power->regmap = axp20x->regmap;
>> +
>> +r = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &input);
>> +if (r < 0)
>> +return r;
>> +
>> +if (!!(input & AXP20X_PWR_STATUS_AC_VBUS_SHORT)) {
>> +dev_err(&pdev->dev, "AC is connected to VBUS. Use
>> axp20x_usb_power-supply driver instead!");
>> +return -ENODEV;
>> +}
>
> Can't that change over time? Can't we support both drivers at the same time?
Both drivers are supported at the same time. I did even try with both
AC and USB connected and both return meaningful values, at least for
voltage. The AXP20x can drawn power from both sources at the same time.
The check above fires in a specific scenario where ACIN and VBUS are
physically connected on the PCB. The Datasheet states for that
particular register:
"Indicates whether ACIN/VBUS short circuits on PCB or not"
So, assuming the chip detects the condition correctly, this does not
change over that. But you can switch from ACIN to VBUS and vice-versa
just fine if they are not short-circuited. If they are connected
together, I'd prefer the DTS to only enable the usb driver. The check
above is a last resort there.
Then again, with the quality of those data sheets, one never knows.
>
>> +/* Enable ac voltage and current measurement */
>> +r = regmap_update_bits(power->regmap, AXP20X_ADC_EN1,
>> +AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT,
>> +AXP20X_ADC_EN1_ACIN_CURR | AXP20X_ADC_EN1_ACIN_VOLT);
>> +if (r)
>> +return r;
>> +
>> +psy_cfg.of_node = pdev->dev.of_node;
>> +psy_cfg.drv_data = power;
>> +
>> +power->supply = devm_power_supply_register(&pdev->dev,
>> +&axp20x_ac_power_desc, &psy_cfg);
>> +if (IS_ERR(power->supply))
>> +return PTR_ERR(power->supply);
>> +
>> +/* Request irqs after registering, as irqs
[linux-sunxi] Re: [PATCH 1/4] power: Add an axp20x-ac-power driver
Hi,
On Sun, Apr 03, 2016 at 03:15:04PM +0200, Michael Haas wrote:
> This adds a driver for the ac power_supply bits of the axp20x
> PMICs.
>
> This submission is taken directly from Bruno Prémonts 2015 RFC [0].
> The original RFC contains drivers for AC, battery and backup
> battery. This commit only adds the AC driver for now.
>
> My only change to Bruno's axp20x_ac_power.c is the additional
> check on a possible short-circuit between ACIN and VBUS. In this
> case, axp20x-ac-power-supply refuses to attach itself to the device
> and axp20x-usb-power-supply must be used.
>
> [0] http://permalink.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/17980
>
> Signed-off-by: Michael Haas
> ---
> drivers/mfd/axp20x.c| 11 +++
> drivers/power/Makefile | 2 +-
> drivers/power/axp20x_ac_power.c | 212
>
> 3 files changed, 224 insertions(+), 1 deletion(-)
> create mode 100644 drivers/power/axp20x_ac_power.c
>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index a57d6e9..9351c0e 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -178,6 +178,12 @@ static struct resource axp288_power_button_resources[] =
> {
> },
> };
>
> +static struct resource axp20x_ac_power_supply_resources[] = {
> + DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_PLUGIN, "ACIN_PLUGIN"),
> + DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_REMOVAL, "ACIN_REMOVAL"),
> + DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_ACIN_OVER_V, "ACIN_OVER_V"),
> +};
> +
> static struct resource axp288_fuel_gauge_resources[] = {
> {
> .start = AXP288_IRQ_QWBTU,
> @@ -440,6 +446,11 @@ static struct mfd_cell axp20x_cells[] = {
> .of_compatible = "x-powers,axp202-usb-power-supply",
> .num_resources = ARRAY_SIZE(axp20x_usb_power_supply_resources),
> .resources = axp20x_usb_power_supply_resources,
> + }, {
> + .name = "axp20x-ac-power-supply",
> + .of_compatible = "x-powers,axp202-ac-power-supply",
> + .num_resources = ARRAY_SIZE(axp20x_ac_power_supply_resources),
> + .resources = axp20x_ac_power_supply_resources,
> },
> };
>
These changes should be in a separate patch.
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index e46b75d..3a785cc 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -9,7 +9,7 @@ obj-$(CONFIG_GENERIC_ADC_BATTERY) += generic-adc-battery.o
>
> obj-$(CONFIG_PDA_POWER) += pda_power.o
> obj-$(CONFIG_APM_POWER) += apm_power.o
> -obj-$(CONFIG_AXP20X_POWER) += axp20x_usb_power.o
> +obj-$(CONFIG_AXP20X_POWER) += axp20x_usb_power.o axp20x_ac_power.o
> obj-$(CONFIG_MAX8925_POWER) += max8925_power.o
> obj-$(CONFIG_WM831X_BACKUP) += wm831x_backup.o
> obj-$(CONFIG_WM831X_POWER) += wm831x_power.o
> diff --git a/drivers/power/axp20x_ac_power.c b/drivers/power/axp20x_ac_power.c
> new file mode 100644
> index 000..c5bcbeb
> --- /dev/null
> +++ b/drivers/power/axp20x_ac_power.c
> @@ -0,0 +1,212 @@
> +/*
> + * AXP20x PMIC AC power driver
> + *
> + * Copyright 2014-2015 Bruno Prémont
> + *
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define DRVNAME "axp20x-ac-power"
> +/* Fields of AXP20X_PWR_INPUT_STATUS */
> +#define AXP20X_PWR_STATUS_AC_PRESENT BIT(7)
> +#define AXP20X_PWR_STATUS_AC_AVAILABLE BIT(6)
> +#define AXP20X_PWR_STATUS_AC_VBUS_SHORT BIT(1)
> +#define AXP20X_PWR_STATUS_AC_VBUS_SELBIT(0)
> +
> +/* Fields of AXP20X_ADC_EN1 */
> +#define AXP20X_ADC_EN1_ACIN_VOLT BIT(5)
> +#define AXP20X_ADC_EN1_ACIN_CURR BIT(4)
That should probably be defined in the global header, next to the
registers.
> +struct axp20x_ac_power {
> + struct regmap *regmap;
> + struct power_supply *supply;
> +};
> +
> +static int axp20x_ac_power_get_property(struct power_supply *psy,
> + enum power_supply_property psp, union power_supply_propval *val)
The alignment is supposed to be on the opening parentheses
> +{
> + struct axp20x_ac_power *power = power_supply_get_drvdata(psy);
> + unsigned int input;
> + int r;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + r = axp20x_read_variable_width(power->regmap,
> +AXP20X_ACIN_V_ADC_H, 12);
> + if (r < 0)
> + return r;
> +
> + val->intval = r * 1700; /* 1 step = 1.7 mV */
> + return 0;
> + case POWER_SUPPLY_PROP_CURRENT_NOW:
> + r = axp20x_read_variable_width(power->regmap,
>
[linux-sunxi] Re: [PATCH] clk: sunxi: Add CSI (camera's Sensors Interface) module clock driver for sun[457]i
Hi Stephen On Fri, Apr 01, 2016 at 06:14:36PM -0700, Stephen Boyd wrote: > On 03/19, Rob Herring wrote: > > On Thu, Mar 17, 2016 at 07:43:42PM +1100, yassinjaf...@gmail.com wrote: > > > From: Yassin Jaffer > > > > > > This patch adds a composite clock type consisting of > > > a clock gate, mux, configurable dividers, and a reset control. > > > > > > Signed-off-by: Yassin Jaffer > > > --- > > > Documentation/devicetree/bindings/clock/sunxi.txt | 1 + > > > > I wish someone would just add all the sunxi clocks in one pass instead > > of one by one. > > > > Acked-by: Rob Herring > > Me too! I understand, but it's probably not going to happen. The clock tree is massive, and obviously you have to multiply that by the number of SoCs we have. And to be honest, I don't really see the point of merging clock drivers that have never been tested, and might be massively broken, especially now that we have this ABI requirement for the DT, and we can't adjust the binding when we actually start using that clock. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout. signature.asc Description: Digital signature
[linux-sunxi] Re: [PATCH v4 2/3] dmaengine: sun6i: Set default maxburst size and bus width
On Sat, Apr 02, 2016 at 11:27:57AM +0200, Jean-Francois Moine wrote: > Some DMA clients, as audio, don't set the maxburst size and bus width > on the memory side when starting DMA transfers. > This patch prevents such transfers to be aborted by providing system > default values to the lacking ones. > > Signed-off-by: Jean-Francois Moine Acked-by: Maxime Ripard Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout. signature.asc Description: Digital signature
[linux-sunxi] Re: [PATCH v4 1/3] dmaengine: sun6i: Simplify lli setting
On Sat, Apr 02, 2016 at 11:24:45AM +0200, Jean-Francois Moine wrote: > Checking the DMA config before setting the lli list avoids to do tests > inside the setting loop. > > Signed-off-by: Jean-Francois Moine Acked-by: Maxime Ripard Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout. signature.asc Description: Digital signature
[linux-sunxi] Re: [PATCH v5 36/46] input: misc: max77693: switch to the atomic API
On Thu, Mar 31, 2016 at 08:57:18PM +0200, Boris Brezillon wrote:
> On Thu, 31 Mar 2016 10:48:01 -0700
> Dmitry Torokhov wrote:
>
> > Hi Boris,
> >
> > On Wed, Mar 30, 2016 at 10:03:59PM +0200, Boris Brezillon wrote:
> > > pwm_config/enable/disable() have been deprecated and should be replaced
> > > by pwm_apply_state().
> > >
> > > Signed-off-by: Boris Brezillon
> > > ---
> > > drivers/input/misc/max77693-haptic.c | 23 +--
> > > 1 file changed, 17 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/input/misc/max77693-haptic.c
> > > b/drivers/input/misc/max77693-haptic.c
> > > index cf6aac0..aef7dc4 100644
> > > --- a/drivers/input/misc/max77693-haptic.c
> > > +++ b/drivers/input/misc/max77693-haptic.c
> > > @@ -70,13 +70,16 @@ struct max77693_haptic {
> > >
> > > static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic)
> > > {
> > > + struct pwm_state pstate;
> > > struct pwm_args pargs = { };
> > > - int delta;
> > > int error;
> > >
> > > pwm_get_args(haptic->pwm_dev, &pargs);
> > > - delta = (pargs.period + haptic->pwm_duty) / 2;
> > > - error = pwm_config(haptic->pwm_dev, delta, pargs.period);
> > > + pwm_get_state(haptic->pwm_dev, &pstate);
> > > +
> > > + pstate.period = pargs.period;
> > > + pstate.duty_cycle = (pargs.period + haptic->pwm_duty) / 2;
> > > + error = pwm_apply_state(haptic->pwm_dev, &pstate);
> >
> > This does not make sense with regard to the atomic API. If you look in
> > max77693_haptic_play_work(), right after calling
> > max77693_haptic_set_duty_cycle() we either try to enable or disable the
> > pwm. When switching to this new API we should combine both actions.
>
> True. I'll address that, unless Thierry is fine keeping the non-atomic
> API, in which case I'll just drop patches 32 to 46.
I'm fine with keeping the pwm_enable(), pwm_disable() and pwm_config()
APIs, but they should only be used as shortcuts. Where possible the new
atomic API should be used to combine multiple operations into one.
Thierry
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: PGP signature
[linux-sunxi] Re: [PATCH v5 34/46] clk: pwm: switch to the atomic API
On Thu, Mar 31, 2016 at 08:57:35AM +0200, Boris Brezillon wrote:
> Hi Stephen,
>
> On Wed, 30 Mar 2016 15:01:49 -0700
> Stephen Boyd wrote:
>
> > On 03/30, Boris Brezillon wrote:
> > > diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
> > > index ebcd738..49ec5b1 100644
> > > --- a/drivers/clk/clk-pwm.c
> > > +++ b/drivers/clk/clk-pwm.c
> > > @@ -28,15 +28,29 @@ static inline struct clk_pwm *to_clk_pwm(struct
> > > clk_hw *hw)
> > > static int clk_pwm_prepare(struct clk_hw *hw)
> > > {
> > > struct clk_pwm *clk_pwm = to_clk_pwm(hw);
> > > + struct pwm_state pstate;
> > >
> > > - return pwm_enable(clk_pwm->pwm);
> > > + pwm_get_state(clk_pwm->pwm, &pstate);
> > > + if (pstate.enabled)
> > > + return 0;
> > > +
> > > + pstate.enabled = true;
> > > +
> > > + return pwm_apply_state(clk_pwm->pwm, &pstate);
> >
> > This doesn't seem atomic anymore if we're checking the state and
> > then not calling apply_state if it's already enabled. But I
> > assume this doesn't matter because we "own" the pwm here?
>
> Yep. Actually it's not atomic in term of concurrency (maybe the
> 'atomic' word is not appropriate here). Atomicity is here referring to
> the fact that we're now providing all the PWM parameters in the same
> request instead of splitting it in pwm_config() + pwm_enable/disable()
> calls.
It's usually not possible to do really atomic updates with PWM hardware.
The idea is merely that we should be able to submit one request and the
framework (and drivers) will be responsible for making sure it is
applied as a whole or not at all. With the legacy API it is possible for
users to set the duty cycle and period, but then fail to enable/disable
the PWM.
pwm_apply_state() reporting success should indicate that the hardware
state is now what software wanted it to be. That kind of implies that
the application is serialized.
This doesn't imply that hardware state won't change between a call to
pwm_get_state() and pwm_apply_state(), though technically this is what
will usually happen because PWM devices are exclusively used by a single
user. Users are responsible for synchronizing accesses within their own
code.
> Concurrent accesses still have to be controlled by the PWM user (which
> is already the case for this driver, thanks to the locking
> infrastructure in the CCF).
>
> > Otherwise I would think this would be unconditional apply state
> > and duplicates would be ignored in the pwm framework.
> >
>
> Yep, I'll remove the if (pstate.enabled) branch.
Yes, it should be the PWM framework's job to check for changes in state
and discard no-ops.
Thierry
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: PGP signature
[linux-sunxi] Re: [PATCH v5 32/46] pwm: deprecate pwm_config(), pwm_enable() and pwm_disable()
On Thu, Mar 31, 2016 at 08:54:54PM +0200, Boris Brezillon wrote: > Hi Dmitry, > > On Thu, 31 Mar 2016 10:38:58 -0700 > Dmitry Torokhov wrote: > > > Hi Boris, > > > > On Wed, Mar 30, 2016 at 10:03:55PM +0200, Boris Brezillon wrote: > > > Prefix those function as deprecated to encourage all existing users to > > > switch to pwm_apply_state(). > > > > Why not keep at least some of them as wrappers where we do not need to > > chnage several parameters at once? It is much easier to have a driver > > do: > > > > error = pwm_enable(pwm); > > if (error) > > ... > > > > rather than declaring the state variable, fectch it, adjust and then > > apply. > > True. Actually deprecating the non-atomic API was not my primary goal. > Thierry would you mind if we keep both APIs around? I'm fine with keeping these around, though purely as shortcuts. If users need to modify two parameters at once (e.g. duty cycle and enable) then they should be converted to use the atomic API, otherwise there'd be little point in introduce it. Thierry -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout. signature.asc Description: PGP signature
[linux-sunxi] Re: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate
On Thu, Mar 31, 2016 at 09:07:09AM +0200, Boris Brezillon wrote:
> Hi Guenter,
>
> On Wed, 30 Mar 2016 15:52:44 -0700
> Guenter Roeck wrote:
>
> > On Wed, Mar 30, 2016 at 10:03:31PM +0200, Boris Brezillon wrote:
> > > The PWM framework has clarified the concept of reference PWM config
> > > (the platform dependent config retrieved from the DT or the PWM
> > > lookup table) and real PWM state.
> > >
> > > Use pwm_get_args() when the PWM user wants to retrieve this reference
> > > config and not the current state.
> > >
> > > This is part of the rework allowing the PWM framework to support
> > > hardware readout and expose real PWM state even when the PWM has
> > > just been requested (before the user calls pwm_config/enable/disable()).
> > >
> > > Signed-off-by: Boris Brezillon
> > > ---
> > > drivers/hwmon/pwm-fan.c | 19 +--
> > > 1 file changed, 13 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > > index 3e23003..82c5656 100644
> > > --- a/drivers/hwmon/pwm-fan.c
> > > +++ b/drivers/hwmon/pwm-fan.c
> > > @@ -40,15 +40,18 @@ struct pwm_fan_ctx {
> > >
> > > static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> > > {
> > > + struct pwm_args pargs = { };
> >
> > Hi Boris,
> >
> > I guess I am missing some context; sorry for that. Unfortunately,
> > I did not easily find an explanation, so please bear with me.
> >
> > Two questions: Why do we need a local copy of struct pwm_args instead
> > of a pointer to it ? If it can change while being used, isn't it
> > inconsistent anyway ?
>
> It cannot change after pwm_get() is called. For the reason behind
> prototype: I just followed the Thierry's proposal, but I'm perfectly
> fine returning a const struct pwm_args pointer intead of passing
> pwm_args as a parameter.
>
> Thierry, what's your opinion?
I do prefer the current variant because it is more consistent with the
new atomic API, even if not strictly necessary because of the immutable
data.
> > Also, assuming the local copy is necessary, why initialize pargs ?
> > After all, pwm_get_args() just overwrites it.
>
> It's a leftover from a previous version where pwm_get_args was
> implemented this way:
>
> static inline void pwm_get_args(pwm, args)
> {
> if (pwm)
> *args = pwm->args
> }
>
> and this implementation was generating a lot of 'uninitialized
> variable' warnings.
>
> I just decided to drop the 'if (pwm)' test, because, IMO, this
> should be checked way before calling pwm_get_args() is called.
Sounds fine to me.
Thierry
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: PGP signature
Re: [linux-sunxi] A10 bring up and DRAM configuration procedure
On Sun, Apr 03, 2016 at 07:11:21PM +0300, Siarhei Siamashka wrote: > Hi Piotr, Hi Siarhei, thanks for comprehensive reply. > > Other question is how to proceed from this point ? I have datasheet for > > DDR3, > > but number of possible parameters to tweak seems to be overwhelming. Also as > > sunxi wiki claim timing from datasheet have to be converted to number of > > cycles > > - I'm not sure how to approach that. Is there any guide where to start ? > > Well, the standard JEDEC timings are already tabulated in > > http://git.denx.de/?p=u-boot.git;a=blob;f=board/sunxi/dram_timings_sun4i.h > > Some DRAM chips may have better timings than mandated by the standard, > but this is more like performance tuning. Either way, these timings are > a "digital" thing, they specify how many cycles to wait between sending > different commands to the DRAM chip. And they are rather unlikely to be > related to your reliability problems. > > It is much more likely that you have "analog" problems and the signal > gets distorted too badly on the way from the DRAM controller in the > SoC to the DRAM chips. From what I heard, the DDR3 tracks routing is a > major PITA, one needs to get both the tracks length and the tracks > impedance matched within certain tolerance levels. As you are dealing > with a custom hardware design, then maybe the DDR3 routing is a little > bit screwed up? You're right. I think this is the reason, because of that first thing I tried when I saw corrupted serial output was decreasing DRAM_CLK. > > The DRAM controller configuration registers allow to configure the > delays and drive/termination impedance to correct the PCB imperfection > to some extent. In practice, all the "professionally" designed Allwinner > devices work with the same generic Allwinner blessed DRAM settings, > because the PCB routing is apparently usually done and tested to work > fine with these default settings. Also reducing the DRAM clock speed > usually improves reliability. It was believed that any A10/A13/A20 > device should work stable if we clock the DRAM down to 360MHz. > > On the hardware side, you can maybe check the ZQ and RZQ resistors > (one connected to the SoC and also one extra resistor for each DRAM > chip). They should at least exist and be preferably 240 ohm. Some board > manufacturers tune the nominal value of these resistors, apparently > as a way to fine tune the reliability. That's the configuration knob > that is easily available to board manufacturers (they don't dare to > touch the DRAM settings in U-boot and probably don't know how to do > this). I have schematics and confirmed that there is 240 ohm (%1 tolerance) resistor although I cannot be sure about its quality. So there are resistors on ZQ from DRAM die an SZQ (AA7) from SoC side. If I will get to wall I will probably check if those have good enough quality and maybe replace it. Is there any guide what can be used or I should just try slightly higher/lower and see if situation change ? > > One more thing is the VDD-DLL voltage. It is served from DCDC3 (AXP209 > PMIC) and is usually set to 1.250V by default. Increasing this voltage > usually improves the DRAM reliability and allows it to be clocked > higher. You can try to increase this voltage to 1.300V to check > if this helps. Don't go beyond 1.325V on Allwinner A10 though. > > > sunxi wiki describe calibration, when it is possible to run system, but what > > can be done in situation when U-Boot cannot reach that state ? > > The whole idea of this calibration process is to find good ZQ and TPR3 > settings. The settings are assumed to be good if the board is able to > pass the lima-memtester reliability test at a higher DRAM clock speed > without failing. We exploit the fact that lima-memster is by far more > sensitive to DRAM misconfiguration than the usual software. So the > borderline unreliable DRAM settings may not exhibit any visible bad > effects (the system is able to boot, the usual linux software works > fine too), but easily detected by lima-memtester. Because the system > is sufficiently usable at such borderline unstable DRAM settings, we > can run automation scripts to do a bruteforce search, which is probing > different TPR3 values without human supervision. That's what is > explained at: > > https://linux-sunxi.org/A10_DRAM_Controller_Calibration > > But in your case, you can try different ZQ and TPR3 settings manually > in the U-Boot defconfig and instead of running lima-memtester (which > first needs a bootable userland) just monitor how far the system is > able to boot as an alternative selection criteria for picking better > configuration values. If you manage to boot the system up to a > semi-working userland, then you can switch to using lima-memtester > for the final tuning and validation. I applied above suggestions those were very helpful. In addition, for further reference, to tweak ZQ for A10 configuration requires enabling ODT. So I applied thos
[linux-sunxi] Re: [PATCH 03/12] mtd: nand: omap2: rely on generic DT parsing done in nand_scan_ident()
On 01/04/16 15:54, Boris Brezillon wrote:
> The core now takes care of parsing generic DT properties in
> nand_scan_ident() when nand_set_flash_node() has been called.
> Rely on this initialization instead of calling of_get_nand_xxx()
> manually.
>
> Signed-off-by: Boris Brezillon
Acked-by: Roger Quadros
cheers,
-roger
> ---
> drivers/memory/omap-gpmc.c | 7 ---
> drivers/mtd/nand/omap2.c | 9 ++---
> 2 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 21825dd..85aa85e 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -25,7 +25,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -1876,12 +1875,6 @@ static int gpmc_probe_nand_child(struct
> platform_device *pdev,
> break;
> }
>
> - gpmc_nand_data->flash_bbt = of_get_nand_on_flash_bbt(child);
> -
> - val = of_get_nand_bus_width(child);
> - if (val == 16)
> - gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
> -
> gpmc_read_timings_dt(child, &gpmc_t);
> gpmc_nand_init(gpmc_nand_data, &gpmc_t);
>
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 0749ca1..8921283 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1704,9 +1704,7 @@ static int omap_nand_probe(struct platform_device *pdev)
> }
>
> if (pdata->flash_bbt)
> - nand_chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> - else
> - nand_chip->options |= NAND_SKIP_BBTSCAN;
> + nand_chip->bbt_options |= NAND_BBT_USE_FLASH;
>
> /* scan NAND device connected to chip controller */
> nand_chip->options |= pdata->devsize & NAND_BUSWIDTH_16;
> @@ -1716,6 +1714,11 @@ static int omap_nand_probe(struct platform_device
> *pdev)
> goto return_error;
> }
>
> + if (nand_chip->bbt_options & NAND_BBT_USE_FLASH)
> + nand_chip->bbt_options |= NAND_BBT_NO_OOB;
> + else
> + nand_chip->options |= NAND_SKIP_BBTSCAN;
> +
> /* re-populate low-level callbacks based on xfer modes */
> switch (pdata->xfer_type) {
> case NAND_OMAP_PREFETCH_POLLED:
>
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Re: [linux-sunxi] Re: Allwinner dev board code names
Jon Smirl wrote: > Pine 64 has an android/lichee Allwinner code drop from 1/13/16. > http://wiki.pine64.org/index.php/Pine_A64_Software_Release > > Is these anything interesting in it? > I don't think the Android image the Pine64 folks provide is based on this. They might get support from Allwinner but I don't know. Looks likes there is only H3 support in their 3.4 kernel, not the 3.10 one. > True. And this is the state of 'getting Android sources for H3': https://github.com/orangepi-xunlong/orangepi_android ;) -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
