* Re: [PATCH v25 01/16] dt: bindings: Add multicolor class dt bindings documention
From: Pavel Machek @ 2020-06-05 12:11 UTC (permalink / raw)
To: Rob Herring
Cc: Dan Murphy, Jacek Anaszewski, devicetree, Linux LED Subsystem,
linux-kernel@vger.kernel.org
In-Reply-To: <CAL_JsqJ1XOYXyqj_VO2cFigVT=k5NTX3BO6RsDqQ-+pDBNJsrw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]
> > > There's no risk because you are supposed to apply both patches. I
> > > don't apply binding patches that are a part of a series like this.
> >
> > Yes, this is always guaranteed to happen, because "git bisect"
> > understand patch series. Oh, wait.
>
> What!? If the binding patch with the header comes first, how would
> bisect build the driver change without the header?
The driver is already in tree, and includes array of strings. When you
change the define, you need to update the array, too, because you
don't want to have invalid value in there.
> > Patches are supposed to be correct on their own. If your repository
> > filtering can not handle that, you need to fix that...
>
> I'm just asking you to follow the process that *everyone* else is
> following and works. It's not really about the repository filtering.
> That doesn't care. A binding ABI is defined by the schema and any
> defines it has. That is the logical unit that stands on its own.
It does not work in this case.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH v4 07/11] hwmon: add support for the sl28cpld hardware monitoring controller
From: Andy Shevchenko @ 2020-06-05 12:06 UTC (permalink / raw)
To: Michael Walle
Cc: open list:GPIO SUBSYSTEM, devicetree, Linux Kernel Mailing List,
linux-hwmon, linux-pwm, linux-watchdog, linux-arm Mailing List,
Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
Andy Shevchenko
In-Reply-To: <20200604211039.12689-8-michael@walle.cc>
On Fri, Jun 5, 2020 at 12:14 AM Michael Walle <michael@walle.cc> wrote:
>
> Add support for the hardware monitoring controller of the sl28cpld board
> management controller. This driver is part of a multi-function device.
...
> +#include <linux/of_device.h>
mod_devicetable.h ?
...
> + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
> + "sl28cpld_hwmon", hwmon,
> + &sl28cpld_hwmon_chip_info, NULL);
> + if (IS_ERR(hwmon_dev)) {
> + dev_err(&pdev->dev, "failed to register as hwmon device");
> + return PTR_ERR(hwmon_dev);
> + }
> +
> + return 0;
PTR_ERR_OR_ZERO() ?
...
> +static const struct of_device_id sl28cpld_hwmon_of_match[] = {
> + { .compatible = "kontron,sl28cpld-fan" },
> + {},
No comma.
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 06/11] gpio: add support for the sl28cpld GPIO controller
From: Andy Shevchenko @ 2020-06-05 12:00 UTC (permalink / raw)
To: Michael Walle
Cc: open list:GPIO SUBSYSTEM, devicetree, Linux Kernel Mailing List,
linux-hwmon, linux-pwm, linux-watchdog, linux-arm Mailing List,
Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
Andy Shevchenko
In-Reply-To: <20200604211039.12689-7-michael@walle.cc>
On Fri, Jun 5, 2020 at 12:14 AM Michael Walle <michael@walle.cc> wrote:
> Add support for the GPIO controller of the sl28 board management
> controller. This driver is part of a multi-function device.
>
> A controller has 8 lines. There are three different flavors:
> full-featured GPIO with interrupt support, input-only and output-only.
...
> +#include <linux/of_device.h>
I think also not needed.
But wait...
> + return devm_regmap_add_irq_chip_np(dev, dev_of_node(dev), regmap,
It seems regmap needs to be converted to use fwnode.
> + irq, IRQF_SHARED | IRQF_ONESHOT, 0,
> + irq_chip, &gpio->irq_data);
...
> + if (!pdev->dev.parent)
> + return -ENODEV;
Are we expecting to get shot into foot? I mean why we need this check?
> + dev_id = platform_get_device_id(pdev);
> + if (dev_id)
> + type = dev_id->driver_data;
Oh, no. In new code we don't need this. We have facilities to provide
platform data in a form of fwnode.
> + else
> + type = (uintptr_t)of_device_get_match_data(&pdev->dev);
So does this. device_get_match_data().
> + if (!type)
> + return -ENODEV;
...
> + if (irq_support &&
Why do you need this flag? Can't simple IRQ number be sufficient?
> + device_property_read_bool(&pdev->dev, "interrupt-controller")) {
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + ret = sl28cpld_gpio_irq_init(&pdev->dev, gpio, regmap,
> + base, irq);
> + if (ret)
> + return ret;
> +
> + config.irq_domain = regmap_irq_get_domain(gpio->irq_data);
> + }
...
> +static const struct of_device_id sl28cpld_gpio_of_match[] = {
> + { .compatible = "kontron,sl28cpld-gpio",
> + .data = (void *)SL28CPLD_GPIO },
> + { .compatible = "kontron,sl28cpld-gpi",
> + .data = (void *)SL28CPLD_GPI },
> + { .compatible = "kontron,sl28cpld-gpo",
> + .data = (void *)SL28CPLD_GPO },
All above can be twice less LOCs.
> + {},
No comma.
> +};
...
> + .name = KBUILD_MODNAME,
This actually not good idea in long term. File name can change and break an ABI.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 02/11] mfd: Add support for Kontron sl28cpld management controller
From: Michael Walle @ 2020-06-05 11:51 UTC (permalink / raw)
To: Andy Shevchenko
Cc: open list:GPIO SUBSYSTEM, devicetree, Linux Kernel Mailing List,
linux-hwmon, linux-pwm, linux-watchdog, linux-arm Mailing List,
Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
Andy Shevchenko
In-Reply-To: <CAHp75Vf00w_UUvXULVd=OgSVM+p_pmNMJRPVnf8GNZW10c_j5w@mail.gmail.com>
Am 2020-06-05 12:48, schrieb Andy Shevchenko:
> On Fri, Jun 5, 2020 at 1:09 PM Michael Walle <michael@walle.cc> wrote:
>> Am 2020-06-05 10:01, schrieb Andy Shevchenko:
>> > On Fri, Jun 5, 2020 at 12:16 AM Michael Walle <michael@walle.cc> wrote:
>
> ...
>
>> >> + bool "Kontron sl28 core driver"
>> >> + depends on I2C=y
>> >
>> > Why not module?
>>
>> There are users of the interupt lines provided by the interrupt
>> controller.
>> For example, the gpio-button driver. If this is compiled into the
>> kernel
>> (which it is by default in the arm64 defconfig), probing will fail
>> because
>> the interrupt is not found. Is there a better way for that? I guess
>> the
>> same
>> is true for the GPIO driver.
>
> And GPIO nicely handles this via deferred probe mechanism. Why it
> can't be used here?
> So, we really need to have a strong argument to limit module nowadays
> to be only builtin.
Was that a question for me? TBH thats how other interrupt drivers doing
it for now. And it would be the users who need to be fixed, right? Or
even the platform code? Because it will complain with
[ 2.962990] irq: no irq domain found for interrupt-controller@1c !
[ 2.975762] gpio-keys buttons0: Found button without gpio or irq
[ 2.981872] gpio-keys: probe of buttons0 failed with error -22
>> >> + depends on OF
>> >
>> > I didn't find an evidence this is needed.
>
>> >> +#include <linux/of_platform.h>
>> >
>> > No evidence of user of this.
>> > I think you meant mod_devicetable.h.
>>
>> devm_of_platform_populate(), so I need CONFIG_OF, too right?
>
> Ah, this explains header, thanks!
> But it doesn't explain depends OF.
>
> So, perhaps,
>
> depends OF || COMPILE_TEST will be more informative, i.e.
> tells "okay, this driver can be compiled w/o OF, but won't be
> functional".
ok
--
-michael
^ permalink raw reply
* RE: [PATCH v4 1/6] PCI: dwc: Add msi_host_isr() callback
From: Gustavo Pimentel @ 2020-06-05 11:44 UTC (permalink / raw)
To: Kunihiko Hayashi, Bjorn Helgaas, Lorenzo Pieralisi, Jingoo Han,
Rob Herring, Masahiro Yamada, Marc Zyngier
Cc: linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Masami Hiramatsu, Jassi Brar
In-Reply-To: <1591350276-15816-2-git-send-email-hayashi.kunihiko@socionext.com>
On Fri, Jun 5, 2020 at 10:44:31, Kunihiko Hayashi
<hayashi.kunihiko@socionext.com> wrote:
> This adds msi_host_isr() callback function support to describe
> SoC-dependent service triggered by MSI.
>
> For example, when AER interrupt is triggered by MSI, the callback function
> reads SoC-dependent registers and detects that the interrupt is from AER,
> and invoke AER interrupts related to MSI.
>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
> drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
> drivers/pci/controller/dwc/pcie-designware.h | 1 +
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 0a4a5aa..026edb1 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -83,6 +83,9 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
> u32 status, num_ctrls;
> irqreturn_t ret = IRQ_NONE;
>
> + if (pp->ops->msi_host_isr)
> + pp->ops->msi_host_isr(pp);
> +
> num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
>
> for (i = 0; i < num_ctrls; i++) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 656e00f..e741967 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -170,6 +170,7 @@ struct dw_pcie_host_ops {
> void (*scan_bus)(struct pcie_port *pp);
> void (*set_num_vectors)(struct pcie_port *pp);
> int (*msi_host_init)(struct pcie_port *pp);
> + void (*msi_host_isr)(struct pcie_port *pp);
> };
>
> struct pcie_port {
> --
> 2.7.4
Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
^ permalink raw reply
* Re: [PATCH V3 1/2] mmc: sdhci-msm: Add interconnect bandwidth scaling support
From: Sibi Sankar @ 2020-06-05 11:40 UTC (permalink / raw)
To: Pradeep P V K
Cc: bjorn.andersson, adrian.hunter, robh+dt, ulf.hansson, vbadigan,
sboyd, georgi.djakov, mka, linux-mmc, linux-kernel, linux-arm-msm,
devicetree, linux-mmc-owner, rnayak, matthias,
linux-arm-msm-owner
In-Reply-To: <1591349427-27004-2-git-send-email-ppvk@codeaurora.org>
Hey Pradeep,
Thanks for the patch.
On 2020-06-05 15:00, Pradeep P V K wrote:
> Interconnect bandwidth scaling support is now added as a
> part of OPP [1]. So, make sure interconnect driver is ready
can you please replace driver with paths
instead?
> before handling interconnect scaling.
>
> This change is based on
> [1] [Patch v8] Introduce OPP bandwidth bindings
> (https://lkml.org/lkml/2020/5/12/493)
>
> [2] [Patch v3] mmc: sdhci-msm: Fix error handling
> for dev_pm_opp_of_add_table()
> (https://lkml.org/lkml/2020/5/5/491)
sry didn't notice ^^ earlier
you might want to place these
comments and dependencies similar
to the following patch.
https://patchwork.kernel.org/patch/11573903/
>
> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>
> ---
> drivers/mmc/host/sdhci-msm.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c
> b/drivers/mmc/host/sdhci-msm.c
> index b277dd7..a945e84 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -14,6 +14,7 @@
> #include <linux/slab.h>
> #include <linux/iopoll.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/interconnect.h>
>
> #include "sdhci-pltfm.h"
> #include "cqhci.h"
> @@ -2070,6 +2071,13 @@ static int sdhci_msm_probe(struct
> platform_device *pdev)
> }
> msm_host->bulk_clks[0].clk = clk;
>
> + /* Make sure that ICC driver is ready for interconnect bandwdith
typo /s/bandwdith/bandwidth
> + * scaling before registering the device for OPP.
> + */
/* Check for optional interconnect paths */
Maybe using ^^ would suffice since
that's what we are actually doing
Reviewed-by: Sibi Sankar <sibis@codeaurora.org>
> + ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL);
> + if (ret)
> + goto bus_clk_disable;
> +
> msm_host->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core");
> if (IS_ERR(msm_host->opp_table)) {
> ret = PTR_ERR(msm_host->opp_table);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH v4 05/11] pwm: add support for sl28cpld PWM controller
From: Michael Walle @ 2020-06-05 11:39 UTC (permalink / raw)
To: Lee Jones
Cc: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
linux-watchdog, linux-arm-kernel, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Jean Delvare, Guenter Roeck,
Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Mark Brown, Greg Kroah-Hartman, Andy Shevchenko
In-Reply-To: <20200605084915.GE3714@dell>
Am 2020-06-05 10:49, schrieb Lee Jones:
> On Thu, 04 Jun 2020, Michael Walle wrote:
>
>> Add support for the PWM controller of the sl28cpld board management
>> controller. This is part of a multi-function device driver.
>>
>> The controller has one PWM channel and can just generate four distinct
>> frequencies.
>>
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> drivers/pwm/Kconfig | 10 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-sl28cpld.c | 201
>> +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 212 insertions(+)
>> create mode 100644 drivers/pwm/pwm-sl28cpld.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index cb8d739067d2..a39371c11ff6 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -437,6 +437,16 @@ config PWM_SIFIVE
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-sifive.
>>
>> +config PWM_SL28CPLD
>> + tristate "Kontron sl28 PWM support"
>> + depends on MFD_SL28CPLD
>> + help
>> + Generic PWM framework driver for board management controller
>> + found on the Kontron sl28 CPLD.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called pwm-sl28cpld.
>> +
>> config PWM_SPEAR
>> tristate "STMicroelectronics SPEAr PWM support"
>> depends on PLAT_SPEAR || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index a59c710e98c7..c479623724e8 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -41,6 +41,7 @@ obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
>> obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
>> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
>> obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
>> +obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o
>> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
>> obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o
>> obj-$(CONFIG_PWM_STI) += pwm-sti.o
>> diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
>> new file mode 100644
>> index 000000000000..d82303f509f5
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-sl28cpld.c
>> @@ -0,0 +1,201 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * sl28cpld PWM driver.
>> + *
>> + * Copyright 2019 Kontron Europe GmbH
>
> This is out of date.
ok
>
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/regmap.h>
>> +
>> +/*
>> + * PWM timer block registers.
>> + */
>> +#define PWM_CTRL 0x00
>> +#define PWM_ENABLE BIT(7)
>> +#define PWM_MODE_250HZ 0
>> +#define PWM_MODE_500HZ 1
>> +#define PWM_MODE_1KHZ 2
>> +#define PWM_MODE_2KHZ 3
>> +#define PWM_MODE_MASK GENMASK(1, 0)
>> +#define PWM_CYCLE 0x01
>> +#define PWM_CYCLE_MAX 0x7f
>> +
>> +struct sl28cpld_pwm {
>> + struct pwm_chip pwm_chip;
>> + struct regmap *regmap;
>> + u32 offset;
>> +};
>> +
>> +struct sl28cpld_pwm_periods {
>> + u8 ctrl;
>> + unsigned long duty_cycle;
>> +};
>> +
>> +struct sl28cpld_pwm_config {
>> + unsigned long period_ns;
>> + u8 max_duty_cycle;
>> +};
>
> Also, instead of hand rolling your own structure here, I think it
> would be prudent to re-use something that already exists. Seeing as
> this will be used to describe possible state, perhaps 'struct
> pwm_state' would be suitable - leaving polarity and enabled
> unpopulated of course.
>
> Ah wait (sorry, thinking allowed and on-the-fly here), what is
> max_duty_cycle here? I assume this does not have the same
> meaning/value-type as the one in 'struct pwm_state'. What does
> max_duty_cycle represent in your use-case?
Its the max value of the PWM_CYCLE register, with one exception
of the 250Hz mode. There it would be 0x7f; but it is used as a scaling
factor too. Thus I added the "fixup" below.
>> +static struct sl28cpld_pwm_config sl28cpld_pwm_config[] = {
>> + [PWM_MODE_250HZ] = { .period_ns = 4000000, .max_duty_cycle = 0x80 },
>> + [PWM_MODE_500HZ] = { .period_ns = 2000000, .max_duty_cycle = 0x40 },
>> + [PWM_MODE_1KHZ] = { .period_ns = 1000000, .max_duty_cycle = 0x20 },
>> + [PWM_MODE_2KHZ] = { .period_ns = 500000, .max_duty_cycle = 0x10 },
>> +};
>
> Tiny nit: If you lined these up from the '{'s it would be easier to
> see/compare the period_ns values at first glance, rather than having
> to count the ' 's and '0's.
yep.
>
>> +static inline struct sl28cpld_pwm *to_sl28cpld_pwm(struct pwm_chip
>> *chip)
>> +{
>> + return container_of(chip, struct sl28cpld_pwm, pwm_chip);
>> +}
>
> Why not save yourself the trouble and just:
>
> struct sl28cpld_pwm *pwm = dev_get_drvdata(chip->dev);
looks better, yes.
>
>> +static void sl28cpld_pwm_get_state(struct pwm_chip *chip,
>> + struct pwm_device *pwm,
>> + struct pwm_state *state)
>> +{
>> + struct sl28cpld_pwm *spc = to_sl28cpld_pwm(chip);
>> + static struct sl28cpld_pwm_config *config;
>> + unsigned int reg;
>> + unsigned long cycle;
>
> Why is this 'long' here and 'long long' in *_apply()?
cycle has a max value of "u8_max * <defined ulong from config above>",
where below it might be "ulong * ulong". But for consinstency, I could
make it unsigned long long here, too.
>> + unsigned int mode;
>> +
>> + regmap_read(spc->regmap, spc->offset + PWM_CTRL, ®);
>> +
>> + state->enabled = reg & PWM_ENABLE;
>> +
>> + mode = FIELD_GET(PWM_MODE_MASK, reg);
>> + config = &sl28cpld_pwm_config[mode];
>> + state->period = config->period_ns;
>> +
>> + regmap_read(spc->regmap, spc->offset + PWM_CYCLE, ®);
>> + cycle = reg * config->period_ns;
>> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(cycle,
>> + config->max_duty_cycle);
>> +}
>> +
>> +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct
>> pwm_device *pwm,
>> + const struct pwm_state *state)
>> +{
>> + struct sl28cpld_pwm *spc = to_sl28cpld_pwm(chip);
>> + struct sl28cpld_pwm_config *config;
>> + unsigned long long cycle;
>> + int ret;
>> + int mode;
>> + u8 ctrl;
>> +
>> + /* update config, first search best matching period */
>
> Please use correct grammar (less full stops) in comments.
ok
>
>> + for (mode = 0; mode < ARRAY_SIZE(sl28cpld_pwm_config); mode++) {
>> + config = &sl28cpld_pwm_config[mode];
>> + if (state->period == config->period_ns)
>> + break;
>> + }
>> +
>> + if (mode == ARRAY_SIZE(sl28cpld_pwm_config))
>> + return -EINVAL;
>> +
>> + ctrl = FIELD_PREP(PWM_MODE_MASK, mode);
>> + if (state->enabled)
>> + ctrl |= PWM_ENABLE;
>> +
>> + cycle = state->duty_cycle * config->max_duty_cycle;
>> + do_div(cycle, state->period);
>
> Forgive my ignorance (I'm new here!), but what are these 2 lines
> doing? Here we are multiplying the current duty_cycle with the
> maximum value, then dividing by the period.
>
> So in the case of PWM_MODE_1KHZ with a 50% duty cycle, you'd have:
>
> (500000 * 0x20[16]) / 1000000 = [0x10]16
>
> Thus, the above gives as a proportional representation of the maximum
> valid value for placement into the cycle control register(s), right?
correct.
> Either way (whether I'm correct or not), I think it would be nice to
> mention this in a comment. Maybe even clarify with a simple example.
yes, I'll also look into the helper Andy mentioned. Thus it might be
even self explanatory.
>> + /*
>> + * The hardware doesn't allow to set max_duty_cycle if the
>> + * 250Hz mode is enabled. But since this is "all-high" output
>> + * just use the 500Hz mode with the duty cycle to max value.
>> + */
>> + if (cycle == config->max_duty_cycle) {
>> + ctrl &= ~PWM_MODE_MASK;
>> + ctrl |= FIELD_PREP(PWM_MODE_MASK, PWM_MODE_500HZ);
>> + cycle = PWM_CYCLE_MAX;
>> + }
>
> This is being executed even when 250Hz mode is not enabled.
>
> Is that by design?
Yes because the mode doesn't matter if you have a duty cycle of 100%.
You'd be free to choose any mode except 250Hz.
> If so, it doesn't match the comment.
Mh? Ok its a bit confusing and it might imply that this is only done
for the 250Hz case. But it doensn't mention it is _only_ used for this
mode.
/*
* The hardware doesn't allow to set max_duty_cycle if the
* 250Hz mode is enabled, thus we have to trap that here.
* But because a 100% duty cycle is equal on all modes, i.e.
* it is just a "all-high" output, we trap any case with a
* 100% duty cycle and use the 500Hz mode.
*/
>> + ret = regmap_write(spc->regmap, spc->offset + PWM_CTRL, ctrl);
>> + if (ret)
>> + return ret;
>> +
>> + return regmap_write(spc->regmap, spc->offset + PWM_CYCLE,
>> (u8)cycle);
>> +}
>> +
>> +static const struct pwm_ops sl28cpld_pwm_ops = {
>> + .apply = sl28cpld_pwm_apply,
>> + .get_state = sl28cpld_pwm_get_state,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int sl28cpld_pwm_probe(struct platform_device *pdev)
>> +{
>> + struct sl28cpld_pwm *pwm;
>
> This is super confusing. Here you call it 'pwm', but when you bring
> the data to the fore for consumption, you call it something different
> ('spc') for some reason.
yeah it is :(
> Is there logic behind this?
And no it is not. sorry for that.
>> + struct pwm_chip *chip;
>> + int ret;
>> +
>> + if (!pdev->dev.parent)
>> + return -ENODEV;
>> +
>> + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
>> + if (!pwm)
>> + return -ENOMEM;
>> +
>> + pwm->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> + if (!pwm->regmap)
>> + return -ENODEV;
>> +
>> + ret = device_property_read_u32(&pdev->dev, "reg", &pwm->offset);
>
> Really? Can you use the 'reg' property in this way?
Well formerly it was IORESOURCE_REG, which gives you a register offset,
see commit 72dcb1197228b ("resources: Add register address resource
type").
There is also the of_get_address(), but I doubt that would be correct
here,
because it does bus mapping etc.
So I looked at how other MFD drivers does it, most of the MFD have the
advantage of having fixed register offsets and then just use hardcoded
offsets. But there are some drivers which pull their offset out of the
reg property from the device tree itself.
$ grep -r "read_u32.*\"reg\"" drivers/
$ grep -r "read_u32.*\"reg\".*base" drivers/
Does anyone have a better idea?
> Side question:
> Do any of your child address spaces actually overlap/intersect?
nope. they are distinct.
>
>> + if (ret)
>> + return -EINVAL;
>> +
>> + /* initialize struct pwm_chip */
>
> Proper grammar please.
ok
>
>> + chip = &pwm->pwm_chip;
>> + chip->dev = &pdev->dev;
>> + chip->ops = &sl28cpld_pwm_ops;
>> + chip->base = -1;
>> + chip->npwm = 1;
>> +
>> + ret = pwmchip_add(&pwm->pwm_chip);
>> + if (ret < 0)
>
> Is '> 0' even valid?
>
> Suggest "!ret" here, as you have done above.
Yes, same comment as Andy had on the other patches.
>> + return ret;
>> +
>> + platform_set_drvdata(pdev, pwm);
>> +
>> + return 0;
>> +}
>> +
>> +static int sl28cpld_pwm_remove(struct platform_device *pdev)
>> +{
>> + struct sl28cpld_pwm *pwm = platform_get_drvdata(pdev);
>> +
>> + return pwmchip_remove(&pwm->pwm_chip);
>> +}
>> +
>> +static const struct of_device_id sl28cpld_pwm_of_match[] = {
>> + { .compatible = "kontron,sl28cpld-pwm" },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, sl28cpld_pwm_of_match);
>> +
>> +static const struct platform_device_id sl28cpld_pwm_id_table[] = {
>> + {"sl28cpld-pwm"},
>
> Spaces either side of the "'s please.
ok
>
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(platform, sl28cpld_pwm_id_table);
>
> What are you using this for?
They are from the time when these drivers were mfd_cells. But I wanted
to keep them here, if in the future there is another mfd driver which
uses these drivers.
>> +static struct platform_driver sl28cpld_pwm_driver = {
>> + .probe = sl28cpld_pwm_probe,
>> + .remove = sl28cpld_pwm_remove,
>> + .id_table = sl28cpld_pwm_id_table,
>> + .driver = {
>> + .name = KBUILD_MODNAME,
>
> Please just use the quoted name in full.
Mhh, is there any rule for this? Sometimes KBUILD_MODNAME is used
and sometimes an hardcoded name. I thought KBUILD_MODNAME is nice
because it is filled automatically. And the platform probe use
the .id_table anyway.
>> + .of_match_table = sl28cpld_pwm_of_match,
>> + },
>> +};
>> +module_platform_driver(sl28cpld_pwm_driver);
>> +
>> +MODULE_DESCRIPTION("sl28cpld PWM Driver");
>
> "SL28CPLD" ?
Actually no, I want to call that "sl28cpld". The first part
is "sl28" (not SL28) and sl28CPLD looks pretty weird. I tried
to be consistent on Kconfig/dt-bindings/drivers about this
naming.
>> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
>> +MODULE_LICENSE("GPL");
--
-michael
^ permalink raw reply
* Re: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
From: Lukas Wunner @ 2020-06-05 11:35 UTC (permalink / raw)
To: Florian Fainelli
Cc: Mark Brown, linux-kernel, Rob Herring, Nicolas Saenz Julienne,
Ray Jui, Scott Branden,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
open list:SPI SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Martin Sperl
In-Reply-To: <8c8d6007-e4c6-8484-9d40-3b679842be27@gmail.com>
On Thu, Jun 04, 2020 at 01:24:54PM -0700, Florian Fainelli wrote:
> So we do need to know for the first time we install the interrupt
> handler whether we will be in a shared situation or not, I cannot think
> of any solution other than counting the number of available DT nodes
> with the "brcm,bcm2835-spi" compatible string.
In principle it would be possible to iterate over the entire DT using
for_each_of_allnodes() and call of_irq_parse_one() on each device_node
if it's enabled and not the one we're probing. Then check if any of that
device_node's IRQs is identical to that of the device_node we're probing.
That would give you a generic method to test for sharedness of an
interrupt.
However the solution you've found is simpler and cheaper than such a
brute-force search, hence seems perfectly valid to me.
> I appreciate that
> Lukas has spent some tremendous amount of time working on this
> controller driver and he has a sensitivity for performance.
Thanks! Indeed I think spi-bcm2835.c is by now among the best performing
and most featureful SPI drivers in the kernel. I've recently had a
discussion on netdev with someone testing an SPI-attached Ethernet
chip on iMX6Q and on STM32MP1 and couldn't get it to work.
With a BCM2837 no problem at all:
https://lore.kernel.org/netdev/ac0f7227-a4ae-b6cd-36ec-3bcb02b1adbe@denx.de/
Lukas
^ permalink raw reply
* Re: [PATCH v2] spi: bcm2835: Enable shared interrupt support
From: Robin Murphy @ 2020-06-05 11:34 UTC (permalink / raw)
To: Florian Fainelli, linux-kernel
Cc: moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Scott Branden, lukas, Ray Jui, Rob Herring,
open list:SPI SUBSYSTEM, Mark Brown,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Martin Sperl, Nicolas Saenz Julienne
In-Reply-To: <20200604212819.715-1-f.fainelli@gmail.com>
On 2020-06-04 22:28, Florian Fainelli wrote:
> The 4 SPI controller instances added in BCM2711 and BCM7211 SoCs (SPI3,
> SPI4, SPI5 and SPI6) share the same interrupt line with SPI0.
>
> For the BCM2835 case which is deemed performance critical, we would like
> to continue using an interrupt handler which does not have the extra
> comparison on BCM2835_SPI_CS_INTR.
FWIW, if I'm reading the patch correctly, then with sensible codegen
that "overhead" should amount to a bit test on a live register plus a
not-taken conditional branch - according to the 1176 TRM that should add
up to a whopping 2 cycles. If that's really significant then I'd have to
wonder whether you want to be at the mercy of the whole generic IRQ
stack at all, and should perhaps consider using FIQ instead.
> To support that requirement the common interrupt handling code between
> the shared and non-shared interrupt paths is split into a
> bcm2835_spi_interrupt_common() and both bcm2835_spi_interrupt() as well
> as bcm2835_spi_shared_interrupt() make use of it.
>
> During probe, we determine if there is at least another instance of this
> SPI controller, and if there is, then we install a shared interrupt
> handler.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v2:
>
> - identify other available SPI nodes to determine if we need to set-up
> interrupt sharing. This needs to happen for the very first instance
> since we cannot know for the first instance whether interrupt sharing
> is needed or not.
>
> drivers/spi/spi-bcm2835.c | 61 ++++++++++++++++++++++++++++++++-------
> 1 file changed, 50 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> index 237bd306c268..0288b5b3de1e 100644
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c
> @@ -361,11 +361,10 @@ static void bcm2835_spi_reset_hw(struct spi_controller *ctlr)
> bcm2835_wr(bs, BCM2835_SPI_DLEN, 0);
> }
>
> -static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
> +static inline irqreturn_t bcm2835_spi_interrupt_common(struct spi_controller *ctlr,
> + u32 cs)
> {
> - struct spi_controller *ctlr = dev_id;
> struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
> - u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
>
> /*
> * An interrupt is signaled either if DONE is set (TX FIFO empty)
> @@ -394,6 +393,27 @@ static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
> +{
> + struct spi_controller *ctlr = dev_id;
> + struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
> + u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
> +
> + return bcm2835_spi_interrupt_common(ctlr, cs);
> +}
> +
> +static irqreturn_t bcm2835_spi_shared_interrupt(int irq, void *dev_id)
> +{
> + struct spi_controller *ctlr = dev_id;
> + struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
> + u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
> +
> + if (!(cs & BCM2835_SPI_CS_INTR))
> + return IRQ_NONE;
> +
> + return bcm2835_spi_interrupt_common(ctlr, cs);
> +}
> +
> static int bcm2835_spi_transfer_one_irq(struct spi_controller *ctlr,
> struct spi_device *spi,
> struct spi_transfer *tfr,
> @@ -1287,12 +1307,37 @@ static int bcm2835_spi_setup(struct spi_device *spi)
> return 0;
> }
>
> +static const struct of_device_id bcm2835_spi_match[] = {
> + { .compatible = "brcm,bcm2835-spi", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, bcm2835_spi_match);
> +
> static int bcm2835_spi_probe(struct platform_device *pdev)
> {
> + irq_handler_t bcm2835_spi_isr_func = bcm2835_spi_interrupt;
> struct spi_controller *ctlr;
> + unsigned long flags = 0;
> + struct device_node *dn;
> struct bcm2835_spi *bs;
> int err;
>
> + /* On BCM2711 there can be multiple SPI controllers enabled sharing the
> + * same interrupt line, but we also want to minimize the overhead if
> + * there is no need to support interrupt sharing. If we find at least
> + * another available instane (not counting the one we are probed from),
> + * then we assume that interrupt sharing is necessary.
> + */
> + for_each_compatible_node(dn, NULL, bcm2835_spi_match[0].compatible) {
> + err = of_device_is_available(dn) && dn != pdev->dev.of_node;
> + of_node_put(dn);
This is in the wrong place - it should only be where you terminate the
loop early and thus bypass the "of_node_put(from)" call in the iterator
itself.
> + if (err) {
> + flags = IRQF_SHARED;
Is there really any harm to setting IRQF_SHARED even when the interrupt
isn't shared in hardware? Sure, it means you lose a degree of API-level
validation and some other driver might also be able to simultaneously
claim it on bcm283x, but in that case the DT would be wrong and
*something* isn't going to work correctly anyway, so does this one
driver really need to care about trying to be the DT police?
Robin.
> + bcm2835_spi_isr_func = bcm2835_spi_shared_interrupt;
> + break;
> + }
> + }
> +
> ctlr = spi_alloc_master(&pdev->dev, ALIGN(sizeof(*bs),
> dma_get_cache_alignment()));
> if (!ctlr)
> @@ -1344,8 +1389,8 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
> bcm2835_wr(bs, BCM2835_SPI_CS,
> BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX);
>
> - err = devm_request_irq(&pdev->dev, bs->irq, bcm2835_spi_interrupt, 0,
> - dev_name(&pdev->dev), ctlr);
> + err = devm_request_irq(&pdev->dev, bs->irq, bcm2835_spi_isr_func,
> + flags, dev_name(&pdev->dev), ctlr);
> if (err) {
> dev_err(&pdev->dev, "could not request IRQ: %d\n", err);
> goto out_dma_release;
> @@ -1400,12 +1445,6 @@ static void bcm2835_spi_shutdown(struct platform_device *pdev)
> dev_err(&pdev->dev, "failed to shutdown\n");
> }
>
> -static const struct of_device_id bcm2835_spi_match[] = {
> - { .compatible = "brcm,bcm2835-spi", },
> - {}
> -};
> -MODULE_DEVICE_TABLE(of, bcm2835_spi_match);
> -
> static struct platform_driver bcm2835_spi_driver = {
> .driver = {
> .name = DRV_NAME,
>
^ permalink raw reply
* Re: [PATCH v2] spi: bcm2835: Enable shared interrupt support
From: Mark Brown @ 2020-06-05 11:14 UTC (permalink / raw)
To: linux-kernel, Florian Fainelli
Cc: Ray Jui, open list:SPI SUBSYSTEM, lukas, Rob Herring,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
Martin Sperl, Scott Branden, Nicolas Saenz Julienne,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
In-Reply-To: <20200604212819.715-1-f.fainelli@gmail.com>
On Thu, 4 Jun 2020 14:28:19 -0700, Florian Fainelli wrote:
> The 4 SPI controller instances added in BCM2711 and BCM7211 SoCs (SPI3,
> SPI4, SPI5 and SPI6) share the same interrupt line with SPI0.
>
> For the BCM2835 case which is deemed performance critical, we would like
> to continue using an interrupt handler which does not have the extra
> comparison on BCM2835_SPI_CS_INTR.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
Thanks!
[1/1] spi: bcm2835: Enable shared interrupt support
commit: ecfbd3cf3b8bb73ac6a80ddf430b5912fd4402a6
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
^ permalink raw reply
* Re: [PATCH v2] spi: bcm2835: Enable shared interrupt support
From: Nicolas Saenz Julienne @ 2020-06-05 10:58 UTC (permalink / raw)
To: Florian Fainelli, linux-kernel
Cc: moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Scott Branden, lukas, Ray Jui, Rob Herring,
open list:SPI SUBSYSTEM, Mark Brown,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Martin Sperl
In-Reply-To: <f728f55fe6266718b5041b6f3b1864a673991129.camel@suse.de>
[-- Attachment #1: Type: text/plain, Size: 597 bytes --]
On Fri, 2020-06-05 at 10:46 +0200, Nicolas Saenz Julienne wrote:
> Hi Florian,
> Thanks for taking over this!
>
> On Thu, 2020-06-04 at 14:28 -0700, Florian Fainelli wrote:
> > The 4 SPI controller instances added in BCM2711 and BCM7211 SoCs (SPI3,
> > SPI4, SPI5 and SPI6) share the same interrupt line with SPI0.
>
> I think this isn't 100% correct. SPI0 has its own interrupt, but SPI[3-6]
> share
> the same interrupt.
I'm wrong here, I missed this in bcm2711.dtsi:
&spi {
interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
};
Sorry for the noise.
Regards,
Nicolas
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [V7, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
From: Dongchun Zhu @ 2020-06-05 10:54 UTC (permalink / raw)
To: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg,
bingbu.cao
Cc: srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu
In-Reply-To: <20200605105412.18813-1-dongchun.zhu@mediatek.com>
Add a V4L2 sub-device driver for DW9768 voice coil motor, providing
control to set the desired focus via IIC serial interface.
Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
---
MAINTAINERS | 1 +
drivers/media/i2c/Kconfig | 13 ++
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/dw9768.c | 566 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 581 insertions(+)
create mode 100644 drivers/media/i2c/dw9768.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8d72c41..c92dc99 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5157,6 +5157,7 @@ L: linux-media@vger.kernel.org
S: Maintained
T: git git://linuxtv.org/media_tree.git
F: Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
+F: drivers/media/i2c/dw9768.c
DONGWOON DW9807 LENS VOICE COIL DRIVER
M: Sakari Ailus <sakari.ailus@linux.intel.com>
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 125d596..afdf994 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1040,6 +1040,19 @@ config VIDEO_DW9714
capability. This is designed for linear control of
voice coil motors, controlled via I2C serial interface.
+config VIDEO_DW9768
+ tristate "DW9768 lens voice coil support"
+ depends on I2C && VIDEO_V4L2
+ depends on PM
+ select MEDIA_CONTROLLER
+ select VIDEO_V4L2_SUBDEV_API
+ select V4L2_FWNODE
+ help
+ This is a driver for the DW9768 camera lens voice coil.
+ DW9768 is a 10 bit DAC with 100mA output current sink
+ capability. This is designed for linear control of
+ voice coil motors, controlled via I2C serial interface.
+
config VIDEO_DW9807_VCM
tristate "DW9807 lens voice coil support"
depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 77bf7d0..4057476 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
obj-$(CONFIG_VIDEO_AD5820) += ad5820.o
obj-$(CONFIG_VIDEO_AK7375) += ak7375.o
obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
+obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o
obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o
diff --git a/drivers/media/i2c/dw9768.c b/drivers/media/i2c/dw9768.c
new file mode 100644
index 0000000..f34a8ed
--- /dev/null
+++ b/drivers/media/i2c/dw9768.c
@@ -0,0 +1,566 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 MediaTek Inc.
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+#define DW9768_NAME "dw9768"
+#define DW9768_MAX_FOCUS_POS (1024 - 1)
+/*
+ * This sets the minimum granularity for the focus positions.
+ * A value of 1 gives maximum accuracy for a desired focus position
+ */
+#define DW9768_FOCUS_STEPS 1
+
+/*
+ * Ring control and Power control register
+ * Bit[1] RING_EN
+ * 0: Direct mode
+ * 1: AAC mode (ringing control mode)
+ * Bit[0] PD
+ * 0: Normal operation mode
+ * 1: Power down mode
+ * DW9768 requires waiting time of Topr after PD reset takes place.
+ */
+#define DW9768_RING_PD_CONTROL_REG 0x02
+#define DW9768_PD_MODE_OFF 0x00
+#define DW9768_PD_MODE_EN BIT(0)
+#define DW9768_AAC_MODE_EN BIT(1)
+
+/*
+ * DW9768 separates two registers to control the VCM position.
+ * One for MSB value, another is LSB value.
+ * DAC_MSB: D[9:8] (ADD: 0x03)
+ * DAC_LSB: D[7:0] (ADD: 0x04)
+ * D[9:0] DAC data input: positive output current = D[9:0] / 1023 * 100[mA]
+ */
+#define DW9768_MSB_ADDR 0x03
+#define DW9768_LSB_ADDR 0x04
+#define DW9768_STATUS_ADDR 0x05
+
+/*
+ * AAC mode control & prescale register
+ * Bit[7:5] Namely AC[2:0], decide the VCM mode and operation time.
+ * 001 AAC2 0.48 x Tvib
+ * 010 AAC3 0.70 x Tvib
+ * 011 AAC4 0.75 x Tvib
+ * 101 AAC8 1.13 x Tvib
+ * Bit[2:0] Namely PRESC[2:0], set the internal clock dividing rate as follow.
+ * 000 2
+ * 001 1
+ * 010 1/2
+ * 011 1/4
+ * 100 8
+ * 101 4
+ */
+#define DW9768_AAC_PRESC_REG 0x06
+#define DW9768_AAC_MODE_SEL_MASK GENMASK(7, 5)
+#define DW9768_CLOCK_PRE_SCALE_SEL_MASK GENMASK(2, 0)
+
+/*
+ * VCM period of vibration register
+ * Bit[5:0] Defined as VCM rising periodic time (Tvib) together with PRESC[2:0]
+ * Tvib = (6.3ms + AACT[5:0] * 0.1ms) * Dividing Rate
+ * Dividing Rate is the internal clock dividing rate that is defined at
+ * PRESCALE register (ADD: 0x06)
+ */
+#define DW9768_AAC_TIME_REG 0x07
+
+/*
+ * DW9768 requires waiting time (delay time) of t_OPR after power-up,
+ * or in the case of PD reset taking place.
+ */
+#define DW9768_T_OPR_US 1000
+#define DW9768_Tvib_MS_BASE10 (64 - 1)
+#define DW9768_AAC_MODE_DEFAULT 2
+#define DW9768_AAC_TIME_DEFAULT 0x20
+#define DW9768_CLOCK_PRE_SCALE_DEFAULT 1
+
+/*
+ * This acts as the minimum granularity of lens movement.
+ * Keep this value power of 2, so the control steps can be
+ * uniformly adjusted for gradual lens movement, with desired
+ * number of control steps.
+ */
+#define DW9768_MOVE_STEPS 16
+
+static const char * const dw9768_supply_names[] = {
+ "vin", /* Digital I/O power */
+ "vdd", /* Digital core power */
+};
+
+/* dw9768 device structure */
+struct dw9768 {
+ struct regulator_bulk_data supplies[ARRAY_SIZE(dw9768_supply_names)];
+ struct v4l2_ctrl_handler ctrls;
+ struct v4l2_ctrl *focus;
+ struct v4l2_subdev sd;
+
+ u32 aac_mode;
+ u32 aac_timing;
+ u32 clock_presc;
+};
+
+static inline struct dw9768 *sd_to_dw9768(struct v4l2_subdev *subdev)
+{
+ return container_of(subdev, struct dw9768, sd);
+}
+
+struct regval_list {
+ u8 reg_num;
+ u8 value;
+};
+
+struct dw9768_aac_mode_ot_multi {
+ u32 aac_mode_enum;
+ u32 ot_multi_base100;
+};
+
+struct dw9768_clk_presc_dividing_rate {
+ u32 clk_presc_enum;
+ u32 dividing_rate_base100;
+};
+
+static const struct dw9768_aac_mode_ot_multi aac_mode_ot_multi[] = {
+ {1, 48},
+ {2, 70},
+ {3, 75},
+ {5, 113},
+};
+
+static const struct dw9768_clk_presc_dividing_rate presc_dividing_rate[] = {
+ {0, 200},
+ {1, 100},
+ {2, 50},
+ {3, 25},
+ {4, 800},
+ {5, 400},
+};
+
+static u32 dw9768_find_ot_multi(u32 aac_mode_param)
+{
+ u32 cur_ot_multi_base100 = 70;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(aac_mode_ot_multi); i++) {
+ if (aac_mode_ot_multi[i].aac_mode_enum == aac_mode_param) {
+ cur_ot_multi_base100 =
+ aac_mode_ot_multi[i].ot_multi_base100;
+ }
+ }
+
+ return cur_ot_multi_base100;
+}
+
+static u32 dw9768_find_dividing_rate(u32 presc_param)
+{
+ u32 cur_clk_dividing_rate_base100 = 100;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(presc_dividing_rate); i++) {
+ if (presc_dividing_rate[i].clk_presc_enum == presc_param) {
+ cur_clk_dividing_rate_base100 =
+ presc_dividing_rate[i].dividing_rate_base100;
+ }
+ }
+
+ return cur_clk_dividing_rate_base100;
+}
+
+/*
+ * DW9768_AAC_PRESC_REG & DW9768_AAC_TIME_REG determine VCM operation time.
+ * For current VCM mode: AAC3, Operation Time would be 0.70 x Tvib.
+ * Tvib = (6.3ms + AACT[5:0] * 0.1MS) * Dividing Rate.
+ * Below is calculation of the operation delay for each step.
+ */
+static inline u32 dw9768_cal_move_delay(u32 aac_mode_param, u32 presc_param,
+ u32 aac_timing_param)
+{
+ u32 Tvib_us;
+ u32 ot_multi_base100;
+ u32 clk_dividing_rate_base100;
+
+ ot_multi_base100 = dw9768_find_ot_multi(aac_mode_param);
+
+ clk_dividing_rate_base100 = dw9768_find_dividing_rate(presc_param);
+
+ Tvib_us = (DW9768_Tvib_MS_BASE10 + aac_timing_param) *
+ clk_dividing_rate_base100;
+
+ return Tvib_us * ot_multi_base100;
+}
+
+static int dw9768_mod_reg(struct dw9768 *dw9768, u8 reg, u8 mask, u8 val)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+ int ret;
+
+ ret = i2c_smbus_read_byte_data(client, reg);
+ if (ret < 0)
+ return ret;
+
+ val = ((unsigned char)ret & ~mask) | (val & mask);
+
+ return i2c_smbus_write_byte_data(client, reg, val);
+}
+
+static int dw9768_set_dac(struct dw9768 *dw9768, u16 val)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+
+ /* Write VCM position to registers */
+ return i2c_smbus_write_word_swapped(client, DW9768_MSB_ADDR, val);
+}
+
+static int dw9768_init(struct dw9768 *dw9768)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+ u32 move_delay_us;
+ int ret, val;
+
+ /* Reset DW9768_RING_PD_CONTROL_REG to default status 0x00 */
+ ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
+ DW9768_PD_MODE_OFF);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * DW9769 requires waiting delay time of t_OPR
+ * after PD reset takes place.
+ */
+ usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+
+ /* Set DW9768_RING_PD_CONTROL_REG to DW9768_AAC_MODE_EN(0x01) */
+ ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
+ DW9768_AAC_MODE_EN);
+ if (ret < 0)
+ return ret;
+
+ /* Set AAC mode */
+ ret = dw9768_mod_reg(dw9768, DW9768_AAC_PRESC_REG,
+ DW9768_AAC_MODE_SEL_MASK,
+ dw9768->aac_mode << 5);
+ if (ret < 0)
+ return ret;
+
+ /* Set clock presc */
+ if (dw9768->clock_presc != DW9768_CLOCK_PRE_SCALE_DEFAULT) {
+ ret = dw9768_mod_reg(dw9768, DW9768_AAC_PRESC_REG,
+ DW9768_CLOCK_PRE_SCALE_SEL_MASK,
+ dw9768->clock_presc);
+ if (ret < 0)
+ return ret;
+ }
+
+ /* Set AAC Timing */
+ if (dw9768->aac_timing != DW9768_AAC_TIME_DEFAULT) {
+ ret = i2c_smbus_write_byte_data(client, DW9768_AAC_TIME_REG,
+ dw9768->aac_timing);
+ if (ret < 0)
+ return ret;
+ }
+
+ move_delay_us = dw9768_cal_move_delay(dw9768->aac_mode,
+ dw9768->clock_presc,
+ dw9768->aac_timing) / 100;
+
+ for (val = dw9768->focus->val % DW9768_MOVE_STEPS;
+ val <= dw9768->focus->val;
+ val += DW9768_MOVE_STEPS) {
+ ret = dw9768_set_dac(dw9768, val);
+ if (ret) {
+ dev_err(&client->dev, "%s I2C failure: %d",
+ __func__, ret);
+ return ret;
+ }
+ usleep_range(move_delay_us, move_delay_us + 1000);
+ }
+
+ return 0;
+}
+
+static int dw9768_release(struct dw9768 *dw9768)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
+ u32 move_delay_us = dw9768_cal_move_delay(dw9768->aac_mode,
+ dw9768->clock_presc,
+ dw9768->aac_timing) / 100;
+ int ret, val;
+
+ val = round_down(dw9768->focus->val, DW9768_MOVE_STEPS);
+ for ( ; val >= 0; val -= DW9768_MOVE_STEPS) {
+ ret = dw9768_set_dac(dw9768, val);
+ if (ret) {
+ dev_err(&client->dev, "I2C write fail: %d", ret);
+ return ret;
+ }
+ usleep_range(move_delay_us, move_delay_us + 1000);
+ }
+
+ ret = i2c_smbus_write_byte_data(client, DW9768_RING_PD_CONTROL_REG,
+ DW9768_PD_MODE_EN);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * DW9769 requires waiting delay time of t_OPR
+ * after PD reset takes place.
+ */
+ usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+
+ return 0;
+}
+
+static int dw9768_runtime_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct dw9768 *dw9768 = sd_to_dw9768(sd);
+
+ dw9768_release(dw9768);
+ regulator_bulk_disable(ARRAY_SIZE(dw9768_supply_names),
+ dw9768->supplies);
+
+ return 0;
+}
+
+static int dw9768_runtime_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct dw9768 *dw9768 = sd_to_dw9768(sd);
+ int ret;
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(dw9768_supply_names),
+ dw9768->supplies);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable regulators\n");
+ return ret;
+ }
+
+ /*
+ * The datasheet refers to t_OPR that needs to be waited before sending
+ * I2C commands after power-up.
+ */
+ usleep_range(DW9768_T_OPR_US, DW9768_T_OPR_US + 100);
+
+ ret = dw9768_init(dw9768);
+ if (ret < 0)
+ goto disable_regulator;
+
+ return 0;
+
+disable_regulator:
+ regulator_bulk_disable(ARRAY_SIZE(dw9768_supply_names),
+ dw9768->supplies);
+
+ return ret;
+}
+
+static int dw9768_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct dw9768 *dw9768 = container_of(ctrl->handler,
+ struct dw9768, ctrls);
+
+ if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
+ return dw9768_set_dac(dw9768, ctrl->val);
+
+ return 0;
+}
+
+static const struct v4l2_ctrl_ops dw9768_ctrl_ops = {
+ .s_ctrl = dw9768_set_ctrl,
+};
+
+static int dw9768_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ int ret;
+
+ ret = pm_runtime_get_sync(sd->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(sd->dev);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int dw9768_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ pm_runtime_put(sd->dev);
+
+ return 0;
+}
+
+static const struct v4l2_subdev_internal_ops dw9768_int_ops = {
+ .open = dw9768_open,
+ .close = dw9768_close,
+};
+
+static const struct v4l2_subdev_ops dw9768_ops = { };
+
+static int dw9768_init_controls(struct dw9768 *dw9768)
+{
+ struct v4l2_ctrl_handler *hdl = &dw9768->ctrls;
+ const struct v4l2_ctrl_ops *ops = &dw9768_ctrl_ops;
+
+ v4l2_ctrl_handler_init(hdl, 1);
+
+ dw9768->focus = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE, 0,
+ DW9768_MAX_FOCUS_POS,
+ DW9768_FOCUS_STEPS, 0);
+
+ if (hdl->error)
+ return hdl->error;
+
+ dw9768->sd.ctrl_handler = hdl;
+
+ return 0;
+}
+
+static int dw9768_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct dw9768 *dw9768;
+ u32 aac_mode_select;
+ u32 aac_timing_select;
+ u32 clock_presc_select;
+ unsigned int i;
+ int ret;
+
+ dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
+ if (!dw9768)
+ return -ENOMEM;
+
+ /* Initialize subdev */
+ v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
+
+ dw9768->aac_mode = DW9768_AAC_MODE_DEFAULT;
+ dw9768->aac_timing = DW9768_AAC_TIME_DEFAULT;
+ dw9768->clock_presc = DW9768_CLOCK_PRE_SCALE_DEFAULT;
+
+ /* Optional indication of AAC mode select */
+ ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-mode",
+ &aac_mode_select);
+
+ if (!ret)
+ dw9768->aac_mode = aac_mode_select;
+
+ /* Optional indication of clock pre-scale select */
+ ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,clock-presc",
+ &clock_presc_select);
+
+ if (!ret)
+ dw9768->clock_presc = clock_presc_select;
+
+ /* Optional indication of AAC Timing */
+ ret = fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-timing",
+ &aac_timing_select);
+
+ if (!ret)
+ dw9768->aac_timing = aac_timing_select;
+
+ for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
+ dw9768->supplies[i].supply = dw9768_supply_names[i];
+
+ ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dw9768_supply_names),
+ dw9768->supplies);
+ if (ret) {
+ dev_err(dev, "failed to get regulators\n");
+ return ret;
+ }
+
+ /* Initialize controls */
+ ret = dw9768_init_controls(dw9768);
+ if (ret)
+ goto err_free_handler;
+
+ /* Initialize subdev */
+ dw9768->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ dw9768->sd.internal_ops = &dw9768_int_ops;
+
+ ret = media_entity_pads_init(&dw9768->sd.entity, 0, NULL);
+ if (ret < 0)
+ goto err_free_handler;
+
+ dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
+
+ pm_runtime_enable(dev);
+ if (!pm_runtime_enabled(dev)) {
+ ret = dw9768_runtime_resume(dev);
+ if (ret < 0) {
+ dev_err(dev, "failed to power on: %d\n", ret);
+ goto err_clean_entity;
+ }
+ }
+
+ ret = v4l2_async_register_subdev(&dw9768->sd);
+ if (ret < 0) {
+ dev_err(dev, "failed to register V4L2 subdev: %d", ret);
+ goto error_async_register;
+ }
+
+ return 0;
+
+error_async_register:
+ if (!pm_runtime_enabled(dev))
+ dw9768_runtime_suspend(dev);
+err_clean_entity:
+ media_entity_cleanup(&dw9768->sd.entity);
+err_free_handler:
+ v4l2_ctrl_handler_free(&dw9768->ctrls);
+
+ return ret;
+}
+
+static int dw9768_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct dw9768 *dw9768 = sd_to_dw9768(sd);
+
+ v4l2_async_unregister_subdev(&dw9768->sd);
+ v4l2_ctrl_handler_free(&dw9768->ctrls);
+ media_entity_cleanup(&dw9768->sd.entity);
+ pm_runtime_disable(&client->dev);
+ if (!pm_runtime_status_suspended(&client->dev))
+ dw9768_runtime_suspend(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
+
+ return 0;
+}
+
+static const struct of_device_id dw9768_of_table[] = {
+ { .compatible = "dongwoon,dw9768" },
+ { .compatible = "giantec,gt9769" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, dw9768_of_table);
+
+static const struct dev_pm_ops dw9768_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(dw9768_runtime_suspend, dw9768_runtime_resume, NULL)
+};
+
+static struct i2c_driver dw9768_i2c_driver = {
+ .driver = {
+ .name = DW9768_NAME,
+ .pm = &dw9768_pm_ops,
+ .of_match_table = dw9768_of_table,
+ },
+ .probe_new = dw9768_probe,
+ .remove = dw9768_remove,
+};
+module_i2c_driver(dw9768_i2c_driver);
+
+MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>");
+MODULE_DESCRIPTION("DW9768 VCM driver");
+MODULE_LICENSE("GPL v2");
--
2.9.2
^ permalink raw reply related
* [V7, 1/2] media: dt-bindings: media: i2c: Document DW9768 bindings
From: Dongchun Zhu @ 2020-06-05 10:54 UTC (permalink / raw)
To: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg,
bingbu.cao
Cc: srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu
In-Reply-To: <20200605105412.18813-1-dongchun.zhu@mediatek.com>
Add DeviceTree binding documentation for Dongwoon Anatech DW9768 voice
coil actuator.
Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
---
.../bindings/media/i2c/dongwoon,dw9768.yaml | 100 +++++++++++++++++++++
MAINTAINERS | 7 ++
2 files changed, 107 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
diff --git a/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
new file mode 100644
index 0000000..cb96e95
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
@@ -0,0 +1,100 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (c) 2020 MediaTek Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/dongwoon,dw9768.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Dongwoon Anatech DW9768 Voice Coil Motor (VCM) Lens Device Tree Bindings
+
+maintainers:
+ - Dongchun Zhu <dongchun.zhu@mediatek.com>
+
+description: |-
+ The Dongwoon DW9768 is a single 10-bit digital-to-analog (DAC) converter
+ with 100 mA output current sink capability. VCM current is controlled with
+ a linear mode driver. The DAC is controlled via a 2-wire (I2C-compatible)
+ serial interface that operates at clock rates up to 1MHz. This chip
+ integrates Advanced Actuator Control (AAC) technology and is intended for
+ driving voice coil lenses in camera modules.
+
+properties:
+ compatible:
+ enum:
+ - dongwoon,dw9768 # for DW9768 VCM
+ - giantec,gt9769 # for GT9769 VCM
+
+ reg:
+ maxItems: 1
+
+ vin-supply:
+ description:
+ Definition of the regulator used as Digital I/O voltage supply.
+
+ vdd-supply:
+ description:
+ Definition of the regulator used as Digital core voltage supply.
+
+ dongwoon,aac-mode:
+ description:
+ Indication of AAC mode select.
+ allOf:
+ - $ref: "/schemas/types.yaml#/definitions/uint32"
+ - enum:
+ - 1 # AAC2 mode(operation time# 0.48 x Tvib)
+ - 2 # AAC3 mode(operation time# 0.70 x Tvib)
+ - 3 # AAC4 mode(operation time# 0.75 x Tvib)
+ - 5 # AAC8 mode(operation time# 1.13 x Tvib)
+ default: 2
+
+ dongwoon,aac-timing:
+ description:
+ Number of AAC Timing count that controlled by one 6-bit period of
+ vibration register AACT[5:0], the unit of which is 100 us.
+ allOf:
+ - $ref: "/schemas/types.yaml#/definitions/uint32"
+ - default: 0x20
+ minimum: 0x00
+ maximum: 0x3f
+
+ dongwoon,clock-presc:
+ description:
+ Indication of VCM internal clock dividing rate select, as one multiple
+ factor to calculate VCM ring periodic time Tvib.
+ allOf:
+ - $ref: "/schemas/types.yaml#/definitions/uint32"
+ - enum:
+ - 0 # Dividing Rate - 2
+ - 1 # Dividing Rate - 1
+ - 2 # Dividing Rate - 1/2
+ - 3 # Dividing Rate - 1/4
+ - 4 # Dividing Rate - 8
+ - 5 # Dividing Rate - 4
+ default: 1
+
+required:
+ - compatible
+ - reg
+ - vin-supply
+ - vdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ dw9768: camera-lens@c {
+ compatible = "dongwoon,dw9768";
+ reg = <0x0c>;
+
+ vin-supply = <&mt6358_vcamio_reg>;
+ vdd-supply = <&mt6358_vcama2_reg>;
+ dongwoon,aac-timing = <0x39>;
+ };
+ };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index e64e5db..8d72c41 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5151,6 +5151,13 @@ T: git git://linuxtv.org/media_tree.git
F: Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.txt
F: drivers/media/i2c/dw9714.c
+DONGWOON DW9768 LENS VOICE COIL DRIVER
+M: Dongchun Zhu <dongchun.zhu@mediatek.com>
+L: linux-media@vger.kernel.org
+S: Maintained
+T: git git://linuxtv.org/media_tree.git
+F: Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
+
DONGWOON DW9807 LENS VOICE COIL DRIVER
M: Sakari Ailus <sakari.ailus@linux.intel.com>
L: linux-media@vger.kernel.org
--
2.9.2
^ permalink raw reply related
* [V7, 0/2] media: i2c: Add support for DW9768 VCM driver
From: Dongchun Zhu @ 2020-06-05 10:54 UTC (permalink / raw)
To: linus.walleij, bgolaszewski, mchehab, andriy.shevchenko, robh+dt,
mark.rutland, sakari.ailus, drinkcat, tfiga, matthias.bgg,
bingbu.cao
Cc: srv_heupstream, linux-mediatek, linux-arm-kernel, sj.huang,
linux-media, devicetree, louis.kuo, shengnan.wang, dongchun.zhu
Hello,
This series adds DT bindings and V4L2 sub-device driver for Dongwoon's DW9768,
which is a 10-bit DAC with 100mA output current sink capability.
The driver controls the position with 10-bit DAC data D[9:0] and seperates
two 8-bit registers to control the VCM position as belows.
DAC_MSB: D[9:8](ADDR: 0x03):
+---+---+---+---+---+---+---+---+
|---|---|---|---|---|---|D09|D08|
+---+---+---+---+---+---+---+---+
DAC_LSB: D[7:0](ADDR: 0x04):
+---+---+---+---+---+---+---+---+
|D07|D06|D05|D04|D03|D02|D01|D00|
+---+---+---+---+---+---+---+---+
This driver supports:
- set DW9768 to standby mode once suspend and turn it back to active if resume
- set the desired focus via V4L2_CID_FOCUS_ABSOLUTE ctrl
Previous versions of this patch-set can be found here:
v6: https://lore.kernel.org/linux-media/20200518132731.20855-1-dongchun.zhu@mediatek.com/
v5: https://lore.kernel.org/linux-media/20200502161727.30463-1-dongchun.zhu@mediatek.com/
v4: https://lore.kernel.org/linux-media/20200330123634.363-1-dongchun.zhu@mediatek.com/
v3: https://lore.kernel.org/linux-media/20200228155958.20657-1-dongchun.zhu@mediatek.com/
v2: https://lore.kernel.org/linux-media/20190905072142.14606-1-dongchun.zhu@mediatek.com/
v1: https://lore.kernel.org/linux-media/20190708100641.2702-1-dongchun.zhu@mediatek.com/
Mainly changes of v7 are addressing comments from Rob, Sakari, Tomasz.
Compared to v6:
- Refine DT bindings
- Use i2c_smbus_read_byte_data() directly to replace of dw9768_read_smbus()
- Calculate operation time based on the configured board-specific DT settings
- Abstract async register error handling case
- Fix other review comments in v6
Please review.
Thanks.
Dongchun Zhu (2):
media: dt-bindings: media: i2c: Document DW9768 bindings
media: i2c: dw9768: Add DW9768 VCM driver
.../bindings/media/i2c/dongwoon,dw9768.yaml | 100 ++++
MAINTAINERS | 8 +
drivers/media/i2c/Kconfig | 13 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/dw9768.c | 566 +++++++++++++++++++++
5 files changed, 688 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
create mode 100644 drivers/media/i2c/dw9768.c
--
2.9.2
^ permalink raw reply
* Re: [PATCH v2] spi: bcm2835: Enable shared interrupt support
From: Mark Brown @ 2020-06-05 10:52 UTC (permalink / raw)
To: Nicolas Saenz Julienne
Cc: Florian Fainelli, linux-kernel,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Scott Branden, lukas, Ray Jui, Rob Herring,
open list:SPI SUBSYSTEM,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Martin Sperl
In-Reply-To: <f728f55fe6266718b5041b6f3b1864a673991129.camel@suse.de>
[-- Attachment #1: Type: text/plain, Size: 453 bytes --]
On Fri, Jun 05, 2020 at 10:46:57AM +0200, Nicolas Saenz Julienne wrote:
> > -static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
> > +static inline irqreturn_t bcm2835_spi_interrupt_common(struct spi_controller
> > *ctlr,
> > + u32 cs)
> Keep in mind the new 100 character limit.
That's more about stopping people doing awful contortions to shut
checkpatch up than saying that it's a particularly good idea to lengthen
lines.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2] spi: bcm2835: Enable shared interrupt support
From: Lukas Wunner @ 2020-06-05 10:51 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-kernel, Mark Brown, Rob Herring, Nicolas Saenz Julienne,
Ray Jui, Scott Branden,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
open list:SPI SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Martin Sperl
In-Reply-To: <20200604212819.715-1-f.fainelli@gmail.com>
On Thu, Jun 04, 2020 at 02:28:19PM -0700, Florian Fainelli wrote:
> The 4 SPI controller instances added in BCM2711 and BCM7211 SoCs (SPI3,
> SPI4, SPI5 and SPI6) share the same interrupt line with SPI0.
>
> For the BCM2835 case which is deemed performance critical, we would like
> to continue using an interrupt handler which does not have the extra
> comparison on BCM2835_SPI_CS_INTR.
>
> To support that requirement the common interrupt handling code between
> the shared and non-shared interrupt paths is split into a
> bcm2835_spi_interrupt_common() and both bcm2835_spi_interrupt() as well
> as bcm2835_spi_shared_interrupt() make use of it.
>
> During probe, we determine if there is at least another instance of this
> SPI controller, and if there is, then we install a shared interrupt
> handler.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
^ permalink raw reply
* Re: [PATCH v4 04/11] watchdog: add support for sl28cpld watchdog
From: Andy Shevchenko @ 2020-06-05 10:50 UTC (permalink / raw)
To: Michael Walle
Cc: open list:GPIO SUBSYSTEM, devicetree, Linux Kernel Mailing List,
linux-hwmon, linux-pwm, linux-watchdog, linux-arm Mailing List,
Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
Andy Shevchenko
In-Reply-To: <8f042c2442852c29519c381833f3d289@walle.cc>
On Fri, Jun 5, 2020 at 1:24 PM Michael Walle <michael@walle.cc> wrote:
> Am 2020-06-05 10:14, schrieb Andy Shevchenko:
> > On Fri, Jun 5, 2020 at 12:14 AM Michael Walle <michael@walle.cc> wrote:
...
> >> +static bool nowayout = WATCHDOG_NOWAYOUT;
> >> +module_param(nowayout, bool, 0);
> >> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started
> >> (default="
> >> + __MODULE_STRING(WATCHDOG_NOWAYOUT)
> >> ")");
> >> +
> >> +static int timeout;
> >> +module_param(timeout, int, 0);
> >> +MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds");
> >
> > Guenter ACKed this, but I'm wondering why we still need module
> > parameters...
>
> How would a user change the nowayout or the timeout? For the latter
> there is
> a device tree entry, but thats not easy changable by the user.
Yes, it's more question to VIm and Guenter than to you.
...
> >> + if (status & WDT_CTRL_EN) {
> >> + sl28cpld_wdt_start(wdd);
> >
> >> + set_bit(WDOG_HW_RUNNING, &wdd->status);
> >
> > Do you need atomic op here? Why?
>
> I'd say consistency, all watchdog drivers do it like that. I just
> had a look at where this is used, but it looks like access from
> userspace is protected by a lock.
Okay then.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 02/11] mfd: Add support for Kontron sl28cpld management controller
From: Mark Brown @ 2020-06-05 10:50 UTC (permalink / raw)
To: Lee Jones
Cc: Michael Walle, linux-gpio, devicetree, linux-kernel, linux-hwmon,
linux-pwm, linux-watchdog, linux-arm-kernel, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Jean Delvare, Guenter Roeck,
Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Greg Kroah-Hartman, Andy Shevchenko
In-Reply-To: <20200605065709.GD3714@dell>
[-- Attachment #1: Type: text/plain, Size: 911 bytes --]
On Fri, Jun 05, 2020 at 07:57:09AM +0100, Lee Jones wrote:
> On Thu, 04 Jun 2020, Michael Walle wrote:
> > + sl28cpld->regmap = devm_regmap_init_i2c(i2c, &sl28cpld_regmap_config);
> > + if (IS_ERR(sl28cpld->regmap))
> > + return PTR_ERR(sl28cpld->regmap);
> This is now a shared memory allocator and not an MFD at all.
> I'm clamping down on these type of drivers!
> Please find a better way to accomplish this.
What is the concern with this? Looking at the patch I'm guessing the
concern would be that the driver isn't instantiating any MFD children
and instead requiring them to be put in the DT?
> Potentially using "simple-mfd" and "simple-regmap".
> The former already exists and does what you want. The latter doesn't
> yet exist, but could solve your and lots of other contributor's
> issues.
I have no idea what you are thinking of when you say "simple-regmap" so
it is difficult to comment.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v4 02/11] mfd: Add support for Kontron sl28cpld management controller
From: Andy Shevchenko @ 2020-06-05 10:48 UTC (permalink / raw)
To: Michael Walle
Cc: open list:GPIO SUBSYSTEM, devicetree, Linux Kernel Mailing List,
linux-hwmon, linux-pwm, linux-watchdog, linux-arm Mailing List,
Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
Andy Shevchenko
In-Reply-To: <8ed988b3e0bc48ea9219d0847c1b1b8e@walle.cc>
On Fri, Jun 5, 2020 at 1:09 PM Michael Walle <michael@walle.cc> wrote:
> Am 2020-06-05 10:01, schrieb Andy Shevchenko:
> > On Fri, Jun 5, 2020 at 12:16 AM Michael Walle <michael@walle.cc> wrote:
...
> >> + bool "Kontron sl28 core driver"
> >> + depends on I2C=y
> >
> > Why not module?
>
> There are users of the interupt lines provided by the interrupt
> controller.
> For example, the gpio-button driver. If this is compiled into the kernel
> (which it is by default in the arm64 defconfig), probing will fail
> because
> the interrupt is not found. Is there a better way for that? I guess the
> same
> is true for the GPIO driver.
And GPIO nicely handles this via deferred probe mechanism. Why it
can't be used here?
So, we really need to have a strong argument to limit module nowadays
to be only builtin.
...
> >> + depends on OF
> >
> > I didn't find an evidence this is needed.
> >> +#include <linux/of_platform.h>
> >
> > No evidence of user of this.
> > I think you meant mod_devicetable.h.
>
> devm_of_platform_populate(), so I need CONFIG_OF, too right?
Ah, this explains header, thanks!
But it doesn't explain depends OF.
So, perhaps,
depends OF || COMPILE_TEST will be more informative, i.e.
tells "okay, this driver can be compiled w/o OF, but won't be functional".
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
From: Mark Brown @ 2020-06-05 10:28 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-kernel, Rob Herring, Nicolas Saenz Julienne, Ray Jui,
Scott Branden,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
open list:SPI SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Martin Sperl, lukas
In-Reply-To: <21772111-fa1f-7a50-aa92-e44b09cff4eb@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]
On Thu, Jun 04, 2020 at 09:05:46AM -0700, Florian Fainelli wrote:
> On 6/4/2020 5:32 AM, Mark Brown wrote:
> > This feels hacky - it's essentially using the compatible string to set a
> > boolean flag which isn't really about the IP but rather the platform
> > integration. It might cause problems if we do end up having to quirk
> > this version of the IP for some other reason.
> I am not sure why it would be a problem, when you describe a piece of
> hardware with Device Tree, even with the IP block being strictly the
> same, its very integration into a new SoC (with details like shared
> interrupt lines) do warrant a different compatible string. Maybe this is
> more of a philosophical question.
The big concern here is trying to support things going forwards - if it
turns out that any quirks are required by this version of the IP then it
gets very confusing and hard to keep DTs stable if you've already
quirked something that clearly isn't the IP version with the compatible
string. Conversely if we start putting flags into the binding for every
feature that might be changed in a given IP that gets complex as we
can't ever learn new things about an existing IP version without
updating all the DTs which is also bad.
> Instead of counting the number of SPI devices we culd request the
> interrupt first with flags = IRQF_PROBE_SHARED, if this works, good we
> have a single SPI master enabled, if it returns -EBUSY, try again with
> flags = IRQF_SHARED and set-up the bcm2835_spi_sh_interrupt interrupt
> handler to manage the sharing.
Like you said in a followup patch that doesn't work as the first device
to probe will think the interrupt isn't shared. You'd need a callback
to change to shared mode from genirq which feels... inelegant.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v4 04/11] watchdog: add support for sl28cpld watchdog
From: Michael Walle @ 2020-06-05 10:24 UTC (permalink / raw)
To: Andy Shevchenko
Cc: open list:GPIO SUBSYSTEM, devicetree, Linux Kernel Mailing List,
linux-hwmon, linux-pwm, linux-watchdog, linux-arm Mailing List,
Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
Andy Shevchenko
In-Reply-To: <CAHp75VdeD6zDc--R4NPHsiqQerzfNGwUikLN+WHMiZZVsQ8QSA@mail.gmail.com>
Am 2020-06-05 10:14, schrieb Andy Shevchenko:
> On Fri, Jun 5, 2020 at 12:14 AM Michael Walle <michael@walle.cc> wrote:
>>
>> Add support for the watchdog of the sl28cpld board management
>> controller. This is part of a multi-function device driver.
>
> ...
>
>> +#include <linux/of_device.h>
>
> Didn't find a user of this.
I guess mod_devicetable.h then.
>
> ...
>
>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>> +module_param(nowayout, bool, 0);
>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started
>> (default="
>> + __MODULE_STRING(WATCHDOG_NOWAYOUT)
>> ")");
>> +
>> +static int timeout;
>> +module_param(timeout, int, 0);
>> +MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds");
>
> Guenter ACKed this, but I'm wondering why we still need module
> parameters...
How would a user change the nowayout or the timeout? For the latter
there is
a device tree entry, but thats not easy changable by the user.
>
> ...
>
>> + int ret;
>> +
>> + ret = regmap_read(wdt->regmap, wdt->offset + WDT_COUNT, &val);
>> +
>> + return (ret < 0) ? 0 : val;
>
> Besides extra parentheses and questionable ' < 0' part, the following
> would look better I think
>
> ret = ...
> if (ret)
> return 0;
>
> return val;
yes, you're right.
>
> ...
>
>> + int ret;
>> +
>> + ret = regmap_write(wdt->regmap, wdt->offset + WDT_TIMEOUT,
>> timeout);
>> + if (!ret)
>> + wdd->timeout = timeout;
>> +
>> + return ret;
>
> Similar story here:
>
> ret = ...
> if (ret)
> return ret;
>
> wdd->... = ...
> return 0;
>
> ...
ok
>
>> + ret = regmap_read(wdt->regmap, wdt->offset + WDT_CTRL,
>> &status);
>
>> + if (ret < 0)
>
> What ' < 0' means? Do we have some positive return values?
> Ditto for all your code.
probably not, I'll go over all return values and change them.
>> + return ret;
>
> ...
>
>> + if (status & WDT_CTRL_EN) {
>> + sl28cpld_wdt_start(wdd);
>
>> + set_bit(WDOG_HW_RUNNING, &wdd->status);
>
> Do you need atomic op here? Why?
I'd say consistency, all watchdog drivers do it like that. I just
had a look at where this is used, but it looks like access from
userspace is protected by a lock.
>
>> + }
>
> ...
>
>> +static const struct of_device_id sl28cpld_wdt_of_match[] = {
>> + { .compatible = "kontron,sl28cpld-wdt" },
>
>> + {},
>
> No comma.
yeah ;)
--
-michael
^ permalink raw reply
* Re: [PATCH v4 02/11] mfd: Add support for Kontron sl28cpld management controller
From: Michael Walle @ 2020-06-05 10:09 UTC (permalink / raw)
To: Andy Shevchenko
Cc: open list:GPIO SUBSYSTEM, devicetree, Linux Kernel Mailing List,
linux-hwmon, linux-pwm, linux-watchdog, linux-arm Mailing List,
Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
Andy Shevchenko
In-Reply-To: <CAHp75Vd-R3yqhq88-whY6vdDhESpzvFCsbi-ygSTjfXfUzOrtg@mail.gmail.com>
Hi Andy,
Am 2020-06-05 10:01, schrieb Andy Shevchenko:
> On Fri, Jun 5, 2020 at 12:16 AM Michael Walle <michael@walle.cc> wrote:
>>
>> Add the core support for the board management controller found on the
>> SMARC-sAL28 board. It consists of the following functions:
>> - watchdog
>> - GPIO controller
>> - PWM controller
>> - fan sensor
>> - interrupt controller
>>
>> At the moment, this controller is used on the Kontron SMARC-sAL28
>> board.
>>
>> Please note that the MFD driver is defined as bool in the Kconfig
>> because the next patch will add interrupt support.
>
> ...
>
>> +config MFD_SL28CPLD
>> + bool "Kontron sl28 core driver"
>> + depends on I2C=y
>
> Why not module?
There are users of the interupt lines provided by the interrupt
controller.
For example, the gpio-button driver. If this is compiled into the kernel
(which it is by default in the arm64 defconfig), probing will fail
because
the interrupt is not found. Is there a better way for that? I guess the
same
is true for the GPIO driver.
>
>> + depends on OF
>
> I didn't find an evidence this is needed.
see below.
>
> No Compile Test?
ok
>> + select REGMAP_I2C
>> + select MFD_CORE
>
> ...
>
>> +#include <linux/of_platform.h>
>
> No evidence of user of this.
> I think you meant mod_devicetable.h.
devm_of_platform_populate(), so I need CONFIG_OF, too right?
>> +static struct i2c_driver sl28cpld_driver = {
>> + .probe_new = sl28cpld_probe,
>> + .driver = {
>> + .name = "sl28cpld",
>> + .of_match_table = of_match_ptr(sl28cpld_of_match),
>
> Drop of_match_ptr(). It has a little sense in this context (depends
> OF).
> It will have a little sense even if you drop depends OF b/c you will
> introduce a compiler warning.
ok
>
>> + },
>> +};
--
-michael
^ permalink raw reply
* [PATCH v1 2/2] ARM: dts: aspeed: mihawk: Add 8 tmp401 thermal sensor
From: Ben Pai @ 2020-06-05 10:06 UTC (permalink / raw)
To: joel, devicetree, linux-arm-kernel, linux-aspeed, linux-kernel
Cc: claire_ku, Ben Pai
Signed-off-by: Ben Pai <Ben_Pai@wistron.com>
---
arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts | 40 +++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts b/arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
index 25ffe65fbdc0..5bf1f13dda3b 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
@@ -834,6 +834,11 @@
line-name = "smbus0";
};
};
+
+ tmp431@4c {
+ compatible = "ti,tmp401";
+ reg = <0x4c>;
+ };
};
bus9_mux232: i2c@1 {
@@ -854,6 +859,11 @@
line-name = "smbus1";
};
};
+
+ tmp431@4c {
+ compatible = "ti,tmp401";
+ reg = <0x4c>;
+ };
};
bus9_mux233: i2c@2 {
@@ -897,6 +907,11 @@
line-name = "smbus2";
};
};
+
+ tmp431@4c {
+ compatible = "ti,tmp401";
+ reg = <0x4c>;
+ };
};
bus9_mux236: i2c@1 {
@@ -917,6 +932,11 @@
line-name = "smbus3";
};
};
+
+ tmp431@4c {
+ compatible = "ti,tmp401";
+ reg = <0x4c>;
+ };
};
bus9_mux237: i2c@2 {
@@ -979,6 +999,11 @@
line-name = "smbus4";
};
};
+
+ tmp431@4c {
+ compatible = "ti,tmp401";
+ reg = <0x4c>;
+ };
};
bus10_mux240: i2c@1 {
@@ -999,6 +1024,11 @@
line-name = "smbus5";
};
};
+
+ tmp431@4c {
+ compatible = "ti,tmp401";
+ reg = <0x4c>;
+ };
};
bus10_mux241: i2c@2 {
@@ -1042,6 +1072,11 @@
line-name = "smbus6";
};
};
+
+ tmp431@4c {
+ compatible = "ti,tmp401";
+ reg = <0x4c>;
+ };
};
bus10_mux244: i2c@1 {
@@ -1062,6 +1097,11 @@
line-name = "smbus7";
};
};
+
+ tmp431@4c {
+ compatible = "ti,tmp401";
+ reg = <0x4c>;
+ };
};
bus10_mux245: i2c@2 {
--
2.17.1
---------------------------------------------------------------------------------------------------------------------------------------------------------------
This email contains confidential or legally privileged information and is for the sole use of its intended recipient.
Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited.
If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately.
---------------------------------------------------------------------------------------------------------------------------------------------------------------
^ permalink raw reply related
* [PATCH v1 1/2] ARM: dts: aspeed: mihawk: IO expander uses TCA9554 driver
From: Ben Pai @ 2020-06-05 10:06 UTC (permalink / raw)
To: joel, devicetree, linux-arm-kernel, linux-aspeed, linux-kernel
Cc: claire_ku, Ben Pai
Set smbus_en of IO expander to 1 in order to be able to read sensor.
Signed-off-by: Ben Pai <Ben_Pai@wistron.com>
---
arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts | 112 ++++++++++++++++++++
1 file changed, 112 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts b/arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
index eac6ed2bbc67..25ffe65fbdc0 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
@@ -820,12 +820,40 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;
+
+ tca9554@39 {
+ compatible = "ti,tca9554";
+ reg = <0x39>;
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ smbus0 {
+ gpio-hog;
+ gpios = <4 GPIO_ACTIVE_HIGH>;
+ output-high;
+ line-name = "smbus0";
+ };
+ };
};
bus9_mux232: i2c@1 {
#address-cells = <1>;
#size-cells = <0>;
reg = <1>;
+
+ tca9554@39 {
+ compatible = "ti,tca9554";
+ reg = <0x39>;
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ smbus1 {
+ gpio-hog;
+ gpios = <4 GPIO_ACTIVE_HIGH>;
+ output-high;
+ line-name = "smbus1";
+ };
+ };
};
bus9_mux233: i2c@2 {
@@ -855,12 +883,40 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;
+
+ tca9554@39 {
+ compatible = "ti,tca9554";
+ reg = <0x39>;
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ smbus2 {
+ gpio-hog;
+ gpios = <4 GPIO_ACTIVE_HIGH>;
+ output-high;
+ line-name = "smbus2";
+ };
+ };
};
bus9_mux236: i2c@1 {
#address-cells = <1>;
#size-cells = <0>;
reg = <1>;
+
+ tca9554@39 {
+ compatible = "ti,tca9554";
+ reg = <0x39>;
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ smbus3 {
+ gpio-hog;
+ gpios = <4 GPIO_ACTIVE_HIGH>;
+ output-high;
+ line-name = "smbus3";
+ };
+ };
};
bus9_mux237: i2c@2 {
@@ -909,12 +965,40 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;
+
+ tca9554@39 {
+ compatible = "ti,tca9554";
+ reg = <0x39>;
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ smbus4 {
+ gpio-hog;
+ gpios = <4 GPIO_ACTIVE_HIGH>;
+ output-high;
+ line-name = "smbus4";
+ };
+ };
};
bus10_mux240: i2c@1 {
#address-cells = <1>;
#size-cells = <0>;
reg = <1>;
+
+ tca9554@39 {
+ compatible = "ti,tca9554";
+ reg = <0x39>;
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ smbus5 {
+ gpio-hog;
+ gpios = <4 GPIO_ACTIVE_HIGH>;
+ output-high;
+ line-name = "smbus5";
+ };
+ };
};
bus10_mux241: i2c@2 {
@@ -944,12 +1028,40 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;
+
+ tca9554@39 {
+ compatible = "ti,tca9554";
+ reg = <0x39>;
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ smbus6 {
+ gpio-hog;
+ gpios = <4 GPIO_ACTIVE_HIGH>;
+ output-high;
+ line-name = "smbus6";
+ };
+ };
};
bus10_mux244: i2c@1 {
#address-cells = <1>;
#size-cells = <0>;
reg = <1>;
+
+ tca9554@39 {
+ compatible = "ti,tca9554";
+ reg = <0x39>;
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ smbus7 {
+ gpio-hog;
+ gpios = <4 GPIO_ACTIVE_HIGH>;
+ output-high;
+ line-name = "smbus7";
+ };
+ };
};
bus10_mux245: i2c@2 {
--
2.17.1
---------------------------------------------------------------------------------------------------------------------------------------------------------------
This email contains confidential or legally privileged information and is for the sole use of its intended recipient.
Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited.
If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately.
---------------------------------------------------------------------------------------------------------------------------------------------------------------
^ permalink raw reply related
* Re: [PATCH v4 02/11] mfd: Add support for Kontron sl28cpld management controller
From: Michael Walle @ 2020-06-05 9:51 UTC (permalink / raw)
To: Lee Jones
Cc: broonie, linux-gpio, devicetree, linux-kernel, linux-hwmon,
linux-pwm, linux-watchdog, linux-arm-kernel, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Jean Delvare, Guenter Roeck,
Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Greg Kroah-Hartman, Andy Shevchenko
In-Reply-To: <20200605065709.GD3714@dell>
Hi Lee,
thanks for the review.
Am 2020-06-05 08:57, schrieb Lee Jones:
> Mark, what do you think?
>
> On Thu, 04 Jun 2020, Michael Walle wrote:
>
>> Add the core support for the board management controller found on the
>> SMARC-sAL28 board. It consists of the following functions:
>> - watchdog
>> - GPIO controller
>> - PWM controller
>> - fan sensor
>> - interrupt controller
>>
>> At the moment, this controller is used on the Kontron SMARC-sAL28
>> board.
>>
>> Please note that the MFD driver is defined as bool in the Kconfig
>> because the next patch will add interrupt support.
>>
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> drivers/mfd/Kconfig | 19 ++++++++++
>> drivers/mfd/Makefile | 2 ++
>> drivers/mfd/sl28cpld.c | 79
>> ++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 100 insertions(+)
>> create mode 100644 drivers/mfd/sl28cpld.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 4f8b73d92df3..5c0cd514d197 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -2109,5 +2109,24 @@ config SGI_MFD_IOC3
>> If you have an SGI Origin, Octane, or a PCI IOC3 card,
>> then say Y. Otherwise say N.
>>
>> +config MFD_SL28CPLD
>> + bool "Kontron sl28 core driver"
>
> "Kontron SL28 Core Driver"
ok
>
>> + depends on I2C=y
>> + depends on OF
>> + select REGMAP_I2C
>> + select MFD_CORE
>> + help
>> + This option enables support for the board management controller
>> + found on the Kontron sl28 CPLD. You have to select individual
>
> I can't find any reference to the "Kontron sl28 CPLD" online.
>
> Is there a datasheet?
Unfortunately not.
>> + functions, such as watchdog, GPIO, etc, under the corresponding
>> menus
>> + in order to enable them.
>> +
>> + Currently supported boards are:
>> +
>> + Kontron SMARC-sAL28
>> +
>> + To compile this driver as a module, choose M here: the module will
>> be
>> + called sl28cpld.
>> +
>> endmenu
>> endif
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 9367a92f795a..be59fb40aa28 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -264,3 +264,5 @@ obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o
>> obj-$(CONFIG_MFD_STMFX) += stmfx.o
>>
>> obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o
>> +
>> +obj-$(CONFIG_MFD_SL28CPLD) += sl28cpld.o
>> diff --git a/drivers/mfd/sl28cpld.c b/drivers/mfd/sl28cpld.c
>> new file mode 100644
>> index 000000000000..a23194bb6efa
>> --- /dev/null
>> +++ b/drivers/mfd/sl28cpld.c
>> @@ -0,0 +1,79 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * MFD core for the sl28cpld.
>
> Ideally this would match the Kconfig subject line.
ok
>
>> + * Copyright 2019 Kontron Europe GmbH
>
> This is out of date.
ok
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/regmap.h>
>> +
>> +#define SL28CPLD_VERSION 0x03
>> +#define SL28CPLD_MIN_REQ_VERSION 14
>> +
>> +struct sl28cpld {
>> + struct device *dev;
>> + struct regmap *regmap;
>> +};
>
> Why do you need this structure?
see below
>> +static const struct regmap_config sl28cpld_regmap_config = {
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> + .reg_stride = 1,
>> +};
>> +
>> +static int sl28cpld_probe(struct i2c_client *i2c)
>> +{
>> + struct sl28cpld *sl28cpld;
>> + struct device *dev = &i2c->dev;
>> + unsigned int cpld_version;
>> + int ret;
>> +
>> + sl28cpld = devm_kzalloc(dev, sizeof(*sl28cpld), GFP_KERNEL);
>> + if (!sl28cpld)
>> + return -ENOMEM;
>> +
>> + sl28cpld->regmap = devm_regmap_init_i2c(i2c,
>> &sl28cpld_regmap_config);
>> + if (IS_ERR(sl28cpld->regmap))
>> + return PTR_ERR(sl28cpld->regmap);
>
> This is now a shared memory allocator and not an MFD at all.
>
> I'm clamping down on these type of drivers!
>
> Please find a better way to accomplish this.
>
> Potentially using "simple-mfd" and "simple-regmap".
>
> The former already exists and does what you want. The latter doesn't
> yet exist, but could solve your and lots of other contributor's
> issues.
Mh, the former doesn't provide a regmap, correct? Most MMIO drivers
won't
need it, but this is an I2C device. So yes, I could come up with a
"simple-regmap" proposal. I guess that should go into drivers/mfd/
because
it is more than just adding one line, i.e. you would have to configure
the
regmap and its different interfaces.
But how would you model that version check below for example? (Not that
I'm
very keen of it.)
>
> Heck maybe I'll implement it myself if this keeps happening.
>
>> + ret = regmap_read(sl28cpld->regmap, SL28CPLD_VERSION,
>> &cpld_version);
>> + if (ret)
>> + return ret;
>> +
>> + if (cpld_version < SL28CPLD_MIN_REQ_VERSION) {
>> + dev_err(dev, "unsupported CPLD version %d\n", cpld_version);
>> + return -ENODEV;
>> + }
>> +
>> + sl28cpld->dev = dev;
>> + i2c_set_clientdata(i2c, sl28cpld);
>
> If the struct definition is in here, how do you use it elsewhere?
I wanted to store the regmap somewhere, but because there are no users,
I'll
drop that.
>> + dev_info(dev, "successfully probed. CPLD version %d\n",
>> cpld_version);
>> +
>> + return devm_of_platform_populate(&i2c->dev);
>> +}
>> +
>> +static const struct of_device_id sl28cpld_of_match[] = {
>> + { .compatible = "kontron,sl28cpld-r1", },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, sl28cpld_of_match);
>> +
>> +static struct i2c_driver sl28cpld_driver = {
>> + .probe_new = sl28cpld_probe,
>> + .driver = {
>> + .name = "sl28cpld",
>> + .of_match_table = of_match_ptr(sl28cpld_of_match),
>> + },
>> +};
>> +module_i2c_driver(sl28cpld_driver);
>> +
>> +MODULE_DESCRIPTION("sl28cpld MFD Core Driver");
>> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
>> +MODULE_LICENSE("GPL");
--
-michael
^ permalink raw reply
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