| Message ID | 20250318-mdb-max7360-support-v5-4-fb20baf97da0@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for MAX7360 | expand |
On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote: > From: Kamel Bouhara <kamel.bouhara@bootlin.com> > > Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to > 8 independent PWM outputs. ... > +#include <linux/bits.h> > +#include <linux/dev_printk.h> > +#include <linux/err.h> > +#include <linux/math64.h> > +#include <linux/mfd/max7360.h> > +#include <linux/minmax.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > +#include <linux/time.h> > +#include <linux/types.h> ... > +static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct regmap *regmap; > + struct device *dev; > + > + regmap = pwmchip_get_drvdata(chip); > + dev = regmap_get_device(regmap); Huh?! > +} ... > +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const struct pwm_waveform *wf, > + void *_wfhw) I would expect other way around, i.e. naming with leading underscore(s) to be private / local. Ditto for all similar cases. ... > +static int max7360_pwm_write_waveform(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const void *_wfhw) > +{ > + const struct max7360_pwm_waveform *wfhw = _wfhw; > + struct regmap *regmap; > + unsigned int val; > + int ret; > + > + regmap = pwmchip_get_drvdata(chip); > + val = (wfhw->enabled) ? BIT(pwm->hwpwm) : 0; Redundant parentheses. > + ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCTRL, BIT(pwm->hwpwm), val); > + if (ret) > + return ret; > + > + if (wfhw->duty_steps) > + return regmap_write(regmap, MAX7360_REG_PWM(pwm->hwpwm), wfhw->duty_steps); > + > + return 0; > +} ... > +static int max7360_pwm_read_waveform(struct pwm_chip *chip, > + struct pwm_device *pwm, > + void *_wfhw) > +{ > + struct max7360_pwm_waveform *wfhw = _wfhw; > + struct regmap *regmap; > + unsigned int val; > + int ret; > + > + regmap = pwmchip_get_drvdata(chip); > + > + ret = regmap_read(regmap, MAX7360_REG_GPIOCTRL, &val); > + if (ret) > + return ret; > + > + if (val & BIT(pwm->hwpwm)) { > + wfhw->enabled = true; Also can be (but up to you) wfhw->enabled = val & BIT(pwm->hwpwm); if (wfhw->enabled) { And also see below. Perhaps it is not a good suggestion after all. > + ret = regmap_read(regmap, MAX7360_REG_PWM(pwm->hwpwm), &val); > + wfhw->duty_steps = val; Set to a garbage in case of error, why? > + } else { > + wfhw->enabled = false; > + wfhw->duty_steps = 0; > + } > + > + return ret; > +} ... > +static int max7360_pwm_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct pwm_chip *chip; > + struct regmap *regmap; > + int ret; > + > + if (!dev->parent) > + return dev_err_probe(dev, -ENODEV, "no parent device\n"); Why? Code most likely will fail on the regmap retrieval. Just do that first. > + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); This is quite worrying. The devm_ to parent makes a lot of assumptions that may not be realised. If you really need this, it has to have a very good comment explaining why and object lifetimes. > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + chip->ops = &max7360_pwm_ops; > + > + regmap = dev_get_regmap(dev->parent, NULL); > + if (!regmap) > + return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n"); > + > + pwmchip_set_drvdata(chip, regmap); > + > + ret = devm_pwmchip_add(dev, chip); > + if (ret) > + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); > + > + return 0; > +}
Hi, kernel test robot noticed the following build warnings: [auto build test WARNING on a64dcfb451e254085a7daee5fe51bf22959d52d3] url: https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-mfd-gpio-Add-MAX7360/20250319-003750 base: a64dcfb451e254085a7daee5fe51bf22959d52d3 patch link: https://lore.kernel.org/r/20250318-mdb-max7360-support-v5-4-fb20baf97da0%40bootlin.com patch subject: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20250319/202503192044.ICDBVYzc-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 8.5.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250319/202503192044.ICDBVYzc-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202503192044.ICDBVYzc-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/pwm/pwm-max7360.c: In function 'max7360_pwm_request': >> drivers/pwm/pwm-max7360.c:41:17: warning: variable 'dev' set but not used [-Wunused-but-set-variable] struct device *dev; ^~~ drivers/pwm/pwm-max7360.c: In function 'max7360_pwm_free': drivers/pwm/pwm-max7360.c:58:17: warning: variable 'dev' set but not used [-Wunused-but-set-variable] struct device *dev; ^~~ vim +/dev +41 drivers/pwm/pwm-max7360.c 37 38 static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) 39 { 40 struct regmap *regmap; > 41 struct device *dev; 42 int ret; 43 44 regmap = pwmchip_get_drvdata(chip); 45 dev = regmap_get_device(regmap); 46 47 ret = regmap_write_bits(regmap, MAX7360_REG_PWMCFG(pwm->hwpwm), 48 MAX7360_PORT_CFG_COMMON_PWM, 0); 49 if (ret) 50 return ret; 51 52 return regmap_write_bits(regmap, MAX7360_REG_PORTS, BIT(pwm->hwpwm), BIT(pwm->hwpwm)); 53 } 54
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on a64dcfb451e254085a7daee5fe51bf22959d52d3]
url: https://github.com/intel-lab-lkp/linux/commits/Mathieu-Dubois-Briand/dt-bindings-mfd-gpio-Add-MAX7360/20250319-003750
base: a64dcfb451e254085a7daee5fe51bf22959d52d3
patch link: https://lore.kernel.org/r/20250318-mdb-max7360-support-v5-4-fb20baf97da0%40bootlin.com
patch subject: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support
config: nios2-kismet-CONFIG_PINCTRL_MAX7360-CONFIG_PWM_MAX7360-0-0 (https://download.01.org/0day-ci/archive/20250320/202503201022.7smCPVZj-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20250320/202503201022.7smCPVZj-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503201022.7smCPVZj-lkp@intel.com/
kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for PINCTRL_MAX7360 when selected by PWM_MAX7360
WARNING: unmet direct dependencies detected for PINCTRL_MAX7360
Depends on [n]: PINCTRL [=n] && MFD_MAX7360 [=y]
Selected by [y]:
- PWM_MAX7360 [=y] && PWM [=y] && MFD_MAX7360 [=y]
On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote: > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote: > > +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + const struct pwm_waveform *wf, > > + void *_wfhw) > > I would expect other way around, i.e. naming with leading underscore(s) to be > private / local. Ditto for all similar cases. I guess that one of the other waveform drivers is the source of that. I chose to name the void pointer with the underscore because I consider that the strange one that has the void* type for technical reasons. That's obviously subjective, but I'm happy with that choice. > > +static int max7360_pwm_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct pwm_chip *chip; > > + struct regmap *regmap; > > + int ret; > > + > > + if (!dev->parent) > > + return dev_err_probe(dev, -ENODEV, "no parent device\n"); > > Why? Code most likely will fail on the regmap retrieval. Just do that first. > > > + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may > not be realised. If you really need this, it has to have a very good comment > explaining why and object lifetimes. Pretty sure this is broken. This results for example in the device link being created on the parent. So if the pwm devices goes away a consumer might not notice (at least in the usual way). I guess this was done to ensure that #pwm-cells is parsed from the right dt node? If so, that needs a different adaption. That will probably involve calling device_set_of_node_from_dev(). Best regards Uwe
On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote: > On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote: > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote: ... > > > + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); > > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may > > not be realised. If you really need this, it has to have a very good comment > > explaining why and object lifetimes. > > Pretty sure this is broken. This results for example in the device link > being created on the parent. So if the pwm devices goes away a consumer > might not notice (at least in the usual way). I guess this was done to > ensure that #pwm-cells is parsed from the right dt node? If so, that > needs a different adaption. That will probably involve calling > device_set_of_node_from_dev(). It's an MFD based driver, and MFD core cares about propagating fwnode by default. I believe it should just work if we drop that '->parent' part.
On Wed Mar 19, 2025 at 12:18 PM CET, Andy Shevchenko wrote: > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote: > > From: Kamel Bouhara <kamel.bouhara@bootlin.com> > > > > Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to > > 8 independent PWM outputs. > > ... > > > > +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + const struct pwm_waveform *wf, > > + void *_wfhw) > > I would expect other way around, i.e. naming with leading underscore(s) to be > private / local. Ditto for all similar cases. I get the point, but the 2 existing drivers based on pwm_ops structure name it that way: drivers/pwm/pwm-axi-pwmgen.c and drivers/pwm/pwm-stm32.c. Also, the parameter is mostly unusable as-is, as it is a void*, so I believe it also makes sense to have no underscore for the correctly casted one, that we will be using in the function body (wfhw). > > ... > > > +static int max7360_pwm_read_waveform(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + void *_wfhw) > > +{ > > + struct max7360_pwm_waveform *wfhw = _wfhw; > > + struct regmap *regmap; > > + unsigned int val; > > + int ret; > > + > > + regmap = pwmchip_get_drvdata(chip); > > + > > + ret = regmap_read(regmap, MAX7360_REG_GPIOCTRL, &val); > > + if (ret) > > + return ret; > > + > > + if (val & BIT(pwm->hwpwm)) { > > + wfhw->enabled = true; > > Also can be (but up to you) > > wfhw->enabled = val & BIT(pwm->hwpwm); > if (wfhw->enabled) { > > And also see below. Perhaps it is not a good suggestion after all. > > > + ret = regmap_read(regmap, MAX7360_REG_PWM(pwm->hwpwm), &val); > > + wfhw->duty_steps = val; > > Set to a garbage in case of error, why? > Ok, I'm fixing the whole block of code. > > + } else { > > + wfhw->enabled = false; > > + wfhw->duty_steps = 0; > > + } > > + > > + return ret; > > +} > > ... > > > +static int max7360_pwm_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct pwm_chip *chip; > > + struct regmap *regmap; > > + int ret; > > + > > + if (!dev->parent) > > + return dev_err_probe(dev, -ENODEV, "no parent device\n"); > > Why? Code most likely will fail on the regmap retrieval. Just do that first. > > > + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may > not be realised. If you really need this, it has to have a very good comment > explaining why and object lifetimes. > Thanks, I'm fixing this in this driver and similar code in keypad, rotary and pinctrl. More details in the child mail. Thanks for your review! Mathieu
On Thu Mar 20, 2025 at 11:48 AM CET, Andy Shevchenko wrote: > On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote: > > On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote: > > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote: > > ... > > > > > + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); > > > > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may > > > not be realised. If you really need this, it has to have a very good comment > > > explaining why and object lifetimes. > > > > Pretty sure this is broken. This results for example in the device link > > being created on the parent. So if the pwm devices goes away a consumer > > might not notice (at least in the usual way). I guess this was done to > > ensure that #pwm-cells is parsed from the right dt node? If so, that > > needs a different adaption. That will probably involve calling > > device_set_of_node_from_dev(). > > It's an MFD based driver, and MFD core cares about propagating fwnode by > default. I believe it should just work if we drop that '->parent' part. Are you sure about that? On my side it does not work if I just drop the '->parent', this is why I ended whit this (bad) pattern. Now it does work if I do call device_set_of_node_from_dev() manually, so it's definitely better. But I believe the MFD core is not propagating OF data, and I did not find where it would do that in the code. Yet it does something like this for ACPI in mfd_acpi_add_device(). Or maybe we do something bad in our MFD driver?
On Tue, Mar 25, 2025 at 03:29:18PM +0100, Mathieu Dubois-Briand wrote: > On Wed Mar 19, 2025 at 12:18 PM CET, Andy Shevchenko wrote: > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote: ... > > > +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + const struct pwm_waveform *wf, > > > + void *_wfhw) > > > > I would expect other way around, i.e. naming with leading underscore(s) to be > > private / local. Ditto for all similar cases. > > I get the point, but the 2 existing drivers based on pwm_ops structure > name it that way: drivers/pwm/pwm-axi-pwmgen.c and > drivers/pwm/pwm-stm32.c. > > Also, the parameter is mostly unusable as-is, as it is a void*, so I > believe it also makes sense to have no underscore for the correctly > casted one, that we will be using in the function body (wfhw). It's all up to PWM maintainers, but I find this style a bit weird, sorry. I only saw this in the macros, where it's kinda okay. In functions it's something that needs an additional thinking and understanding the semantics of the underscore. ... > > > +static int max7360_pwm_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + struct pwm_chip *chip; > > > + struct regmap *regmap; > > > + int ret; > > > + > > > + if (!dev->parent) > > > + return dev_err_probe(dev, -ENODEV, "no parent device\n"); > > > > Why? Code most likely will fail on the regmap retrieval. Just do that first. > > > > > + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); > > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may > > not be realised. If you really need this, it has to have a very good comment > > explaining why and object lifetimes. > > Thanks, I'm fixing this in this driver and similar code in keypad, > rotary and pinctrl. More details in the child mail. Sure, thanks!
On Tue, Mar 25, 2025 at 03:37:29PM +0100, Mathieu Dubois-Briand wrote: > On Thu Mar 20, 2025 at 11:48 AM CET, Andy Shevchenko wrote: > > On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote: > > > On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote: > > > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote: ... > > > > > + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); > > > > > > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may > > > > not be realised. If you really need this, it has to have a very good comment > > > > explaining why and object lifetimes. > > > > > > Pretty sure this is broken. This results for example in the device link > > > being created on the parent. So if the pwm devices goes away a consumer > > > might not notice (at least in the usual way). I guess this was done to > > > ensure that #pwm-cells is parsed from the right dt node? If so, that > > > needs a different adaption. That will probably involve calling > > > device_set_of_node_from_dev(). > > > > It's an MFD based driver, and MFD core cares about propagating fwnode by > > default. I believe it should just work if we drop that '->parent' part. > > Are you sure about that? Yes and no. If your DT looks like (pseudo code as I don't know DTS syntax by heart): device: { parent-property = value; child0: ... child1: ... } the parent-property value is automatically accessible via fwnode API, but I don't know what will happen to the cases when each of the children has its own compatible string. This might be your case, but again, I'm not an expert in DT. > On my side it does not work if I just drop the '->parent', this is why I > ended whit this (bad) pattern. > Now it does work if I do call device_set_of_node_from_dev() manually, AFAICT, this is wrong API to be called in the children. Are you talking about parent code? > so it's definitely better. But I believe the MFD core is not propagating > OF data, and I did not find where it would do that in the code. Yet it > does something like this for ACPI in mfd_acpi_add_device(). Or maybe we > do something bad in our MFD driver? ...or MFD needs something to have... Dunno.
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 0915c1e7df16..930cc7c61b82 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -745,4 +745,15 @@ config PWM_XILINX To compile this driver as a module, choose M here: the module will be called pwm-xilinx. +config PWM_MAX7360 + tristate "MAX7360 PWMs" + depends on MFD_MAX7360 + select PINCTRL_MAX7360 + help + PWM driver for Maxim Integrated MAX7360 multifunction device, with + support for up to 8 PWM outputs. + + To compile this driver as a module, choose M here: the module + will be called pwm-max7360. + endif diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 9081e0c0e9e0..ae8908ffc892 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -36,6 +36,7 @@ obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o obj-$(CONFIG_PWM_LPSS_PLATFORM) += pwm-lpss-platform.o +obj-$(CONFIG_PWM_MAX7360) += pwm-max7360.o obj-$(CONFIG_PWM_MESON) += pwm-meson.o obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o obj-$(CONFIG_PWM_MICROCHIP_CORE) += pwm-microchip-core.o diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c new file mode 100644 index 000000000000..059b39df791d --- /dev/null +++ b/drivers/pwm/pwm-max7360.c @@ -0,0 +1,194 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright 2025 Bootlin + * + * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com> + * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> + * + * Limitations: + * - Only supports normal polarity. + * - The period is fixed to 2 ms. + * - Only the duty cycle can be changed, new values are applied at the beginning + * of the next cycle. + * - When disabled, the output is put in Hi-Z. + */ +#include <linux/bits.h> +#include <linux/dev_printk.h> +#include <linux/err.h> +#include <linux/math64.h> +#include <linux/mfd/max7360.h> +#include <linux/minmax.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/regmap.h> +#include <linux/time.h> +#include <linux/types.h> + +#define MAX7360_NUM_PWMS 8 +#define MAX7360_PWM_MAX_RES 255 +#define MAX7360_PWM_PERIOD_NS (2 * NSEC_PER_MSEC) + +struct max7360_pwm_waveform { + u8 duty_steps; + bool enabled; +}; + +static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct regmap *regmap; + struct device *dev; + int ret; + + regmap = pwmchip_get_drvdata(chip); + dev = regmap_get_device(regmap); + + ret = regmap_write_bits(regmap, MAX7360_REG_PWMCFG(pwm->hwpwm), + MAX7360_PORT_CFG_COMMON_PWM, 0); + if (ret) + return ret; + + return regmap_write_bits(regmap, MAX7360_REG_PORTS, BIT(pwm->hwpwm), BIT(pwm->hwpwm)); +} + +static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct regmap *regmap; + struct device *dev; + + regmap = pwmchip_get_drvdata(chip); + dev = regmap_get_device(regmap); +} + +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip, + struct pwm_device *pwm, + const struct pwm_waveform *wf, + void *_wfhw) +{ + struct max7360_pwm_waveform *wfhw = _wfhw; + u64 duty_steps; + + /* + * Ignore user provided values for period_length_ns and duty_offset_ns: + * we only support fixed period of MAX7360_PWM_PERIOD_NS and offset of 0. + */ + duty_steps = mul_u64_u64_div_u64(wf->duty_length_ns, MAX7360_PWM_MAX_RES, + MAX7360_PWM_PERIOD_NS); + + wfhw->duty_steps = min(MAX7360_PWM_MAX_RES, duty_steps); + wfhw->enabled = (wf->duty_length_ns != 0); + + return 0; +} + +static int max7360_pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm, + const void *_wfhw, struct pwm_waveform *wf) +{ + const struct max7360_pwm_waveform *wfhw = _wfhw; + + wf->period_length_ns = wfhw->enabled ? MAX7360_PWM_PERIOD_NS : 0; + wf->duty_offset_ns = 0; + wf->duty_length_ns = DIV64_U64_ROUND_UP(wfhw->duty_steps * MAX7360_PWM_PERIOD_NS, + MAX7360_PWM_MAX_RES); + + return 0; +} + +static int max7360_pwm_write_waveform(struct pwm_chip *chip, + struct pwm_device *pwm, + const void *_wfhw) +{ + const struct max7360_pwm_waveform *wfhw = _wfhw; + struct regmap *regmap; + unsigned int val; + int ret; + + regmap = pwmchip_get_drvdata(chip); + val = (wfhw->enabled) ? BIT(pwm->hwpwm) : 0; + ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCTRL, BIT(pwm->hwpwm), val); + if (ret) + return ret; + + if (wfhw->duty_steps) + return regmap_write(regmap, MAX7360_REG_PWM(pwm->hwpwm), wfhw->duty_steps); + + return 0; +} + +static int max7360_pwm_read_waveform(struct pwm_chip *chip, + struct pwm_device *pwm, + void *_wfhw) +{ + struct max7360_pwm_waveform *wfhw = _wfhw; + struct regmap *regmap; + unsigned int val; + int ret; + + regmap = pwmchip_get_drvdata(chip); + + ret = regmap_read(regmap, MAX7360_REG_GPIOCTRL, &val); + if (ret) + return ret; + + if (val & BIT(pwm->hwpwm)) { + wfhw->enabled = true; + ret = regmap_read(regmap, MAX7360_REG_PWM(pwm->hwpwm), &val); + wfhw->duty_steps = val; + } else { + wfhw->enabled = false; + wfhw->duty_steps = 0; + } + + return ret; +} + +static const struct pwm_ops max7360_pwm_ops = { + .request = max7360_pwm_request, + .free = max7360_pwm_free, + .round_waveform_tohw = max7360_pwm_round_waveform_tohw, + .round_waveform_fromhw = max7360_pwm_round_waveform_fromhw, + .read_waveform = max7360_pwm_read_waveform, + .write_waveform = max7360_pwm_write_waveform, +}; + +static int max7360_pwm_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct pwm_chip *chip; + struct regmap *regmap; + int ret; + + if (!dev->parent) + return dev_err_probe(dev, -ENODEV, "no parent device\n"); + + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); + if (IS_ERR(chip)) + return PTR_ERR(chip); + chip->ops = &max7360_pwm_ops; + + regmap = dev_get_regmap(dev->parent, NULL); + if (!regmap) + return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n"); + + pwmchip_set_drvdata(chip, regmap); + + ret = devm_pwmchip_add(dev, chip); + if (ret) + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); + + return 0; +} + +static struct platform_driver max7360_pwm_driver = { + .driver = { + .name = "max7360-pwm", + }, + .probe = max7360_pwm_probe, +}; +module_platform_driver(max7360_pwm_driver); + +MODULE_DESCRIPTION("MAX7360 PWM driver"); +MODULE_AUTHOR("Kamel BOUHARA <kamel.bouhara@bootlin.com>"); +MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>"); +MODULE_LICENSE("GPL");