| Message ID | 20220623115631.22209-1-peterwu.pub@gmail.com |
|---|---|
| Headers | show |
| Series | Add Mediatek MT6370 PMIC support | expand |
On Thu, Jun 23, 2022 at 07:56:31PM +0800, ChiaEn Wu wrote: > From: ChiaEn Wu <chiaen_wu@richtek.com> > > Add Mediatek MT6370 Backlight support. > > Signed-off-by: ChiaEn Wu <chiaen_wu@richtek.com> > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > index a003e02..7cd823d 100644 > <snip> > +static int mt6370_init_backlight_properties(struct mt6370_priv *priv, > + struct backlight_properties *props) > +{ > + struct device *dev = priv->dev; > + u8 prop_val; > + u32 brightness, ovp_uV, ocp_uA; > + unsigned int mask, val; > + int ret; > + > + /* Vendor optional properties */ > + val = 0; > + if (device_property_read_bool(dev, "mediatek,bled-pwm-enable")) > + val |= MT6370_BL_PWM_EN_MASK; > + > + if (device_property_read_bool(dev, "mediatek,bled-pwm-hys-enable")) > + val |= MT6370_BL_PWM_HYS_EN_MASK; > + > + ret = device_property_read_u8(dev, > + "mediatek,bled-pwm-hys-input-th-steps", > + &prop_val); > + if (!ret) { > + prop_val = clamp_val(prop_val, > + MT6370_BL_PWM_HYS_TH_MIN_STEP, > + MT6370_BL_PWM_HYS_TH_MAX_STEP); > + /* > + * prop_val = 1 --> 1 steps --> 0x00 > + * prop_val = 2 ~ 4 --> 4 steps --> 0x01 > + * prop_val = 5 ~ 16 --> 16 steps --> 0x10 > + * prop_val = 17 ~ 64 --> 64 steps --> 0x11 ^^^^^ These numbers are binary, not hex, right? If so, the comments should be 0b00 to 0b03 . > + */ > + prop_val = (ilog2(roundup_pow_of_two(prop_val)) + 1) >> 1; > + val |= prop_val << (ffs(MT6370_BL_PWM_HYS_SEL_MASK) - 1); > + } > + > + ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_PWM, > + val, val); > + if (ret) > + return ret; Overall, I like this approach! Easy to read and understand. > <snip> > +static int mt6370_bl_probe(struct platform_device *pdev) > +{ > + struct mt6370_priv *priv; > + struct backlight_properties props = { > + .type = BACKLIGHT_RAW, > + .scale = BACKLIGHT_SCALE_LINEAR, Sorry, I missed this before but the KConfig comment says that the backlight can support both linear and exponential curves. Is there a good reason to default to linear? Daniel. >
On Thu, 23 Jun 2022 19:56:17 +0800, ChiaEn Wu wrote: > From: ChiaEn Wu <chiaen_wu@richtek.com> > > This patch series add Mediatek MT6370 PMIC support. The MT6370 is a > highly-integrated smart power management IC, which includes a single > cell Li-Ion/Li-Polymer switching battery charger, a USB > Type-C & Power Delivery (PD) controller, dual Flash LED current sources, > a RGB LED driver, a backlight WLED driver, a display bias driver and a > general LDO for portable devices. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next Thanks! [09/14] regulator: mt6370: Add mt6370 DisplayBias and VibLDO support commit: 8171c93bac1bf9e98269b2efb19ef4e6c4e55ed7 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
On Thu, Jun 23, 2022 at 1:59 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > From: ChiYuan Huang <cy_huang@richtek.com> > > Add Mediatek MT6370 MFD support. ... > +config MFD_MT6370 > + tristate "Mediatek MT6370 SubPMIC" > + select MFD_CORE > + select REGMAP_I2C > + select REGMAP_IRQ > + depends on I2C > + help > + Say Y here to enable MT6370 SubPMIC functional support. > + It consists of a single cell battery charger with ADC monitoring, RGB > + LEDs, dual channel flashlight, WLED backlight driver, display bias > + voltage supply, one general purpose LDO, and the USB Type-C & PD > + controller complies with the latest USB Type-C and PD standards. What will be the module name in case it's chosen to be built as a module? ... > obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC) += intel_soc_pmic_bxtwc.o > obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC) += intel_soc_pmic_chtwc.o > obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o > obj-$(CONFIG_MFD_MT6360) += mt6360-core.o > +obj-$(CONFIG_MFD_MT6370) += mt6370.o > mt6397-objs := mt6397-core.o mt6397-irq.o mt6358-irq.o > obj-$(CONFIG_MFD_MT6397) += mt6397.o > obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD) += intel_soc_pmic_mrfld.o This whole bunch of drivers is in the wrong place in Makefile. https://lore.kernel.org/all/20220616182524.7956-2-andriy.shevchenko@linux.intel.com/ ... > +#define MT6370_REG_MAXADDR 0x1FF Wondering if (BIT(10) - 1) gives a better hint on how hardware limits this (so it will be clear it's 10-bit address). ... > +static int mt6370_check_vendor_info(struct mt6370_info *info) > +{ > + unsigned int devinfo; > + int ret; > + > + ret = regmap_read(info->regmap, MT6370_REG_DEV_INFO, &devinfo); > + if (ret) > + return ret; > + > + switch (FIELD_GET(MT6370_VENID_MASK, devinfo)) { > + case MT6370_VENID_RT5081: > + case MT6370_VENID_RT5081A: > + case MT6370_VENID_MT6370: > + case MT6370_VENID_MT6371: > + case MT6370_VENID_MT6372P: > + case MT6370_VENID_MT6372CP: return 0; > + break; > + default: > + dev_err(info->dev, "Unknown Vendor ID 0x%02x\n", devinfo); > + return -ENODEV; > + } > + > + return 0; ...and drop these two lines? > +} ... > + bank_idx = *(u8 *)reg_buf; > + bank_addr = *(u8 *)(reg_buf + 1); Why not const u8 *u8_buf = reg_buf; bank_idx = u8_buf[0]; bank_addr = u8_buf[1]; ? ... > + if (ret < 0) > + return ret; > + else if (ret != val_size) Redundant 'else'. > + return -EIO; ... > + bank_idx = *(u8 *)data; > + bank_addr = *(u8 *)(data + 1); As per above.
On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > From: ChiYuan Huang <cy_huang@richtek.com> > > Add chip level mt6370 tcpci driver. ... > +config TYPEC_TCPCI_MT6370 > + tristate "Mediatek MT6370 Type-C driver" > + depends on MFD_MT6370 > + help > + Mediatek MT6370 is a multi-functional IC that includes > + USB Type-C. It works with Type-C Port Controller Manager > + to provide USB PD and USB Type-C functionalities. What will be the module name? ... > +#include <linux/bits.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> No user of this header is found in this file. > +#include <linux/platform_device.h> > +#include <linux/pm_wakeup.h> > +#include <linux/pm_wakeirq.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/usb/tcpm.h> > +#include "tcpci.h" ... > + if (did == MT6370_TCPC_DID_A) { > + ret = regmap_write(data->regmap, TCPC_FAULT_CTRL, 0x80); > + if (ret) > + return ret; return regmap_write(...); > + } > + > + return 0; ... > + if (ret && !source) > + ret = regulator_disable(priv->vbus); > + else if (!ret && source) > + ret = regulator_enable(priv->vbus); > + else > + ret = 0; > + > + return ret; Can it be if (ret && ...) return regulator_disable(...); if (!ret && ...) return regulator_enable(...); return 0; ? ... > + if (!priv->tcpci_data.regmap) { > + dev_err(&pdev->dev, "Failed to init regmap\n"); > + return -ENODEV; > + } return dev_err_probe(...); ? ... > + if (ret) { > + dev_err(&pdev->dev, "Failed to check vendor info (%d)\n", ret); > + return ret; > + } Ditto. ... > + priv->irq = platform_get_irq(pdev, 0); > + if (priv->irq < 0) { > + dev_err(&pdev->dev, "Failed to get TCPC irq (%d)\n", priv->irq); The message like this is printed in case of error inside platform_get_irq(), no need to duplicate. > + return priv->irq; > + } ... > + priv->tcpci = tcpci_register_port(&pdev->dev, &priv->tcpci_data); > + if (IS_ERR(priv->tcpci)) { > + dev_err(&pdev->dev, "Failed to register tcpci port\n"); > + return PTR_ERR(priv->tcpci); return dev_err_probe(); ? > + } ... > + if (ret) { > + dev_err(&pdev->dev, "Failed to allocate irq (%d)\n", ret); > + tcpci_unregister_port(priv->tcpci); > + return ret; Ditto. > + }
On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > From: ChiYuan Huang <cy_huang@richtek.com> > > Add mt6370 DisplayBias and VibLDO support. ... > +#include <linux/bits.h> > +#include <linux/gpio/consumer.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> Any users of this? (See below) > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/regulator/driver.h> > +#include <linux/regulator/machine.h> ... > +#define MT6370_LDO_MINUV 1600000 > +#define MT6370_LDO_STPUV 200000 > +#define MT6370_LDO_N_VOLT 13 > +#define MT6370_DBVBOOST_MINUV 4000000 > +#define MT6370_DBVBOOST_STPUV 50000 > +#define MT6370_DBVBOOST_N_VOLT 45 > +#define MT6370_DBVOUT_MINUV 4000000 > +#define MT6370_DBVOUT_STPUV 50000 > +#define MT6370_DBVOUT_N_VOLT 41 If UV is a unit suffix, make it _UV. ... > + .of_match = of_match_ptr("dsvbst"), Would it even be called / used if CONFIG_OF=n? ... > + .regulators_node = of_match_ptr("regulators"), Ditto. ... > + for (i = 0; i < ARRAY_SIZE(mt6370_irqs); i++) { > + irq = platform_get_irq_byname(pdev, mt6370_irqs[i].name); > + > + rdev = priv->rdev[mt6370_irqs[i].rid]; > + > + ret = devm_request_threaded_irq(priv->dev, irq, NULL, > + mt6370_irqs[i].handler, 0, > + mt6370_irqs[i].name, rdev); > + if (ret) { > + dev_err(priv->dev, > + "Failed to register (%d) interrupt\n", i); > + return ret; return dev_err_probe(...); ? > + } > + } ... > + for (i = 0; i < MT6370_MAX_IDX; i++) { > + rdev = devm_regulator_register(priv->dev, > + mt6370_regulator_descs + i, > + &cfg); > + if (IS_ERR(rdev)) { > + dev_err(priv->dev, > + "Failed to register (%d) regulator\n", i); > + return PTR_ERR(rdev); return dev_err_probe(...); ? > + } > + > + priv->rdev[i] = rdev; > + } ... > + if (!priv->regmap) { > + dev_err(&pdev->dev, "Failed to init regmap\n"); > + return -ENODEV; > + } return dev_err_probe(...);
On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > From: ChiaEn Wu <chiaen_wu@richtek.com> > > Add Mediatek MT6370 ADC support. ... > +config MEDIATEK_MT6370_ADC > + tristate "Mediatek MT6370 ADC driver" > + depends on MFD_MT6370 > + help > + Say yes here to enable Mediatek MT6370 ADC support. > + > + This ADC driver provides 9 channels for system monitoring (charger > + current, voltage, and temperature). What will be the module name? ... > +#include <dt-bindings/iio/adc/mediatek,mt6370_adc.h> Usually this goes after linux/* asm/* as it's not so generic. > +#include <linux/bits.h> > +#include <linux/bitfield.h> > +#include <linux/iio/iio.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> I believe the order should be otherwise, this is first followed by module.h. > +#include <linux/mutex.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> ... > +#define ADC_CONV_POLLING_TIME 1000 If it's time, add a unit suffix, if it's a counter, make it clear. ... > + msleep(ADC_CONV_TIME_US / 1000); Why define microseconds if milliseconds are in use? ... > + ret = regmap_read_poll_timeout(priv->regmap, > + MT6370_REG_CHG_ADC, reg_val, > + !(reg_val & MT6370_ADC_START_MASK), > + ADC_CONV_POLLING_TIME, > + ADC_CONV_TIME_US * 3); > + if (ret) { > + if (ret == -ETIMEDOUT) > + dev_err(priv->dev, "Failed to wait ADC conversion\n"); wait for > + else > + dev_err(priv->dev, > + "Failed to read ADC register (%d)\n", ret); Do you really need to differentiate the errors here? I believe the latter one covers all cases. > + goto adc_unlock; > + } ... > +#define MT6370_ADC_CHAN(_idx, _type, _addr, _extra_info) { \ > + .type = _type, \ > + .channel = MT6370_CHAN_##_idx, \ > + .address = _addr, \ > + .scan_index = MT6370_CHAN_##_idx, \ > + .indexed = 1, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + _extra_info \ Leave a comma after the last member as well. > +} ... > + regmap = dev_get_regmap(pdev->dev.parent, NULL); > + if (!regmap) { > + dev_err(&pdev->dev, "Failed to get regmap\n"); > + return -ENODEV; return dev_err_probe(...); > + } ... > + ret = regmap_write(priv->regmap, MT6370_REG_CHG_ADC, 0); > + if (ret) { > + dev_err(&pdev->dev, "Failed to reset ADC\n"); > + return ret; > + } Ditto.
On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > From: ChiaEn Wu <chiaen_wu@richtek.com> > > Add Mediatek MT6370 charger driver. ... > +config CHARGER_MT6370 > + tristate "Mediatek MT6370 Charger Driver" > + depends on MFD_MT6370 > + depends on REGULATOR > + select LINEAR_RANGES > + help > + Say Y here to enable MT6370 Charger Part. > + The device supports High-Accuracy Voltage/Current Regulation, > + Average Input Current Regulation, Battery Temperature Sensing, > + Over-Temperature Protection, DPDM Detection for BC1.2. Module name? ... > +#include <dt-bindings/iio/adc/mediatek,mt6370_adc.h> This usually goes after linux/* > +#include <linux/atomic.h> > +#include <linux/bitfield.h> > +#include <linux/bits.h> > +#include <linux/gpio/consumer.h> > +#include <linux/iio/consumer.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/power_supply.h> > +#include <linux/regmap.h> > +#include <linux/regulator/driver.h> > +#include <linux/workqueue.h> ... > +#define MT6370_MIVR_IBUS_TH 100000 /* 100 mA */ Instead of comment, add proper units. ... > + MT6370_USB_STAT_DCP, > + MT6370_USB_STAT_CDP, > + MT6370_USB_STAT_MAX, No comma for a terminator line. ... > +static inline u32 mt6370_chg_val_to_reg(const struct mt6370_chg_range *range, > + u32 val) > +static inline u32 mt6370_chg_reg_to_val(const struct mt6370_chg_range *range, > + u8 reg) I'm wondering if you can use the https://elixir.bootlin.com/linux/v5.19-rc3/source/include/linux/linear_range.h APIs. ... > + int ret = 0; This seems a redundant assignment, see below. > + rcfg->ena_gpiod = fwnode_gpiod_get_index(of_fwnode_handle(of), > + "enable", 0, For index == 0 don't use _index API. > + GPIOD_OUT_LOW | > + GPIOD_FLAGS_BIT_NONEXCLUSIVE, > + rdesc->name); > + if (IS_ERR(rcfg->ena_gpiod)) { > + dev_err(priv->dev, "Failed to requeset OTG EN Pin\n"); request > + rcfg->ena_gpiod = NULL; So, use _optional and return any errors you got. > + } else { > + val = MT6370_OPA_MODE_MASK | MT6370_OTG_PIN_EN_MASK; > + ret = regmap_update_bits(priv->regmap, MT6370_REG_CHG_CTRL1, > + val, val); > + if (ret) > + dev_err(priv->dev, "Failed to set otg bits\n"); > + } ... > + irq_num = platform_get_irq_byname(pdev, irq_name); > + Unwanted blank line. > + if (irq_num < 0) { > + dev_err(priv->dev, "Failed to get platform resource\n"); Isn't it printed by the call? > + } else { > + if (en) > + enable_irq(irq_num); > + else > + disable_irq_nosync(irq_num); > + } ... > +toggle_cfo_exit: The useless label. > + return ret; > +} ... > + ret = mt6370_chg_get_online(priv, val); > + if (!val->intval) { No error check? > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + return 0; > + } ... > +static int mt6370_chg_set_online(struct mt6370_priv *priv, > + const union power_supply_propval *val) > +{ > + int attach; > + u32 pwr_rdy = !!val->intval; > + > + mutex_lock(&priv->attach_lock); > + attach = atomic_read(&priv->attach); > + if (pwr_rdy == !!attach) { > + dev_err(priv->dev, "pwr_rdy is same(%d)\n", pwr_rdy); > + mutex_unlock(&priv->attach_lock); > + return 0; > + } > + > + atomic_set(&priv->attach, pwr_rdy); > + mutex_unlock(&priv->attach_lock); > + > + if (!queue_work(priv->wq, &priv->bc12_work)) > + dev_err(priv->dev, "bc12 work has already queued\n"); > + > + return 0; > + Unwanted blank line. > +} > +static int mt6370_chg_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct mt6370_priv *priv = power_supply_get_drvdata(psy); > + int ret = 0; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + ret = mt6370_chg_get_online(priv, val); > + break; > + case POWER_SUPPLY_PROP_STATUS: > + ret = mt6370_chg_get_status(priv, val); > + break; > + case POWER_SUPPLY_PROP_CHARGE_TYPE: > + ret = mt6370_chg_get_charge_type(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: > + ret = mt6370_chg_get_ichg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX: > + ret = mt6370_chg_get_max_ichg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > + ret = mt6370_chg_get_cv(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX: > + ret = mt6370_chg_get_max_cv(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: > + ret = mt6370_chg_get_aicr(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT: > + ret = mt6370_chg_get_mivr(priv, val); > + break; > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > + ret = mt6370_chg_get_iprechg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > + ret = mt6370_chg_get_ieoc(priv, val); > + break; > + case POWER_SUPPLY_PROP_TYPE: > + val->intval = priv->psy_desc->type; > + break; > + case POWER_SUPPLY_PROP_USB_TYPE: > + val->intval = priv->psy_usb_type; > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; In all cases, return directly. > +} ... > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + ret = mt6370_chg_set_online(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: > + ret = mt6370_chg_set_ichg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > + ret = mt6370_chg_set_cv(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: > + ret = mt6370_chg_set_aicr(priv, val); > + break; > + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT: > + ret = mt6370_chg_set_mivr(priv, val); > + break; > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > + ret = mt6370_chg_set_iprechg(priv, val); > + break; > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > + ret = mt6370_chg_set_ieoc(priv, val); > + break; > + default: > + ret = -EINVAL; > + } > + return ret; As per above. ... > + for (i = 0; i < F_MAX; i++) { > + priv->rmap_fields[i] = devm_regmap_field_alloc(priv->dev, > + priv->regmap, > + fds[i].field); > + if (IS_ERR(priv->rmap_fields[i])) { > + dev_err(priv->dev, > + "Failed to allocate regmap field [%s]\n", > + fds[i].name); > + return PTR_ERR(priv->rmap_fields[i]); return dev_err_probe(); > + } > + } ... > + mutex_init(&priv->attach_lock); > + atomic_set(&priv->attach, 0); Why not atomic_init() ? But yeah, usage of it and other locking mechanisms in this driver are questionable. ... > + /* ICHG/IEOC Workaroud, ICHG can not be set less than 900mA */ Workaround ... > + return IS_ERR(priv->rdev) ? PTR_ERR(priv->rdev) : 0; PTR_ERR_OR_ZERO() ... > + .of_node = priv->dev->of_node, dev_of_node() ? > + }; > + > + priv->psy_desc = &mt6370_chg_psy_desc; > + priv->psy_desc->name = dev_name(priv->dev); > + priv->psy = devm_power_supply_register(priv->dev, priv->psy_desc, &cfg); > + > + return IS_ERR(priv->psy) ? PTR_ERR(priv->psy) : 0; PTR_ERR_OR_ZERO() > +} ... > +static irqreturn_t mt6370_attach_i_handler(int irq, void *data) > +{ > + struct mt6370_priv *priv = data; > + u32 otg_en; > + int ret; > + > + /* Check in otg mode or not */ > + ret = mt6370_chg_field_get(priv, F_BOOST_STAT, &otg_en); > + if (ret < 0) { > + dev_err(priv->dev, "failed to get otg state\n"); > + return IRQ_HANDLED; Handled error? > + } > + > + if (otg_en) > + return IRQ_HANDLED; > + mutex_lock(&priv->attach_lock); > + atomic_set(&priv->attach, MT6370_ATTACH_STAT_ATTACH_BC12_DONE); > + mutex_unlock(&priv->attach_lock); Mutex around atomic?! It's interesting... > + if (!queue_work(priv->wq, &priv->bc12_work)) > + dev_err(priv->dev, "bc12 work has already queued\n"); > + > + return IRQ_HANDLED; > +} ... > + for (i = 0; i < ARRAY_SIZE(mt6370_chg_irqs); i++) { > + ret = platform_get_irq_byname(to_platform_device(priv->dev), > + mt6370_chg_irqs[i].name); > + if (ret < 0) { > + dev_err(priv->dev, "Failed to get irq %s\n", > + mt6370_chg_irqs[i].name); Isn't the same printed by the above call? > + return ret; > + } > + > + ret = devm_request_threaded_irq(priv->dev, ret, NULL, > + mt6370_chg_irqs[i].handler, > + IRQF_TRIGGER_FALLING, > + dev_name(priv->dev), > + priv); > + > + if (ret < 0) { > + dev_err(priv->dev, "Failed to request irq %s\n", > + mt6370_chg_irqs[i].name); > + return ret; return dev_err_probe(); > + } > + } ... > +static int mt6370_chg_probe(struct platform_device *pdev) > +{ Use return dev_err_probe(...); pattern. > +probe_out: > + destroy_workqueue(priv->wq); > + mutex_destroy(&priv->attach_lock); I don't see clearly the initialization of these in the ->probe(). Besides that, does destroy_workque() synchronize the actual queue(s)? Mixing devm_ and non-devm_ may lead to a wrong release order that's why it is better to see allocating and destroying resources in one function (they may be wrapped, but should be both of them, seems like you have done it only for the first parts). > + return ret; > +} ... > +static int mt6370_chg_remove(struct platform_device *pdev) > +{ > + struct mt6370_priv *priv = platform_get_drvdata(pdev); > + > + if (priv) { Can you describe when this condition can be false? > + mt6370_chg_enable_irq(priv, "mivr", false); > + cancel_delayed_work_sync(&priv->mivr_dwork); > + destroy_workqueue(priv->wq); > + mutex_destroy(&priv->attach_lock); > + } > + > + return 0; > +} ... > +static struct platform_driver mt6370_chg_driver = { > + .probe = mt6370_chg_probe, > + .remove = mt6370_chg_remove, > + .driver = { > + .name = "mt6370-charger", > + .of_match_table = of_match_ptr(mt6370_chg_of_match), No good use of of_match_ptr(), please drop it. > + }, > +};
On 6/23/22 04:56, ChiaEn Wu wrote: > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index a49979f..a8c58c3 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -244,6 +244,17 @@ config LEDS_MT6323 > This option enables support for on-chip LED drivers found on > Mediatek MT6323 PMIC. > > +config LEDS_MT6370_RGB > + tristate "LED Support for Mediatek MT6370 PMIC" > + depends on LEDS_CLASS > + depends on MFD_MT6370 > + select LINEAR_RANGE > + help > + Say Y here to enable support for MT6370_RGB LED device. > + In MT6370, there are four channel current-sink LED drivers that > + support hardware pattern for const current, PWM, and breath mode. Spell out "constant" (if that is what "const" means here). ? > + Isink4 channel can also be used as a CHG_VIN power good indicator.
Hi ChiaEn! Thanks for your patch! On Thu, Jun 23, 2022 at 1:58 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > From: ChiYuan Huang <cy_huang@richtek.com> > > Add Mediatek MT6370 current sink type LED Indicator driver. > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> (...) > drivers/leds/Kconfig | 11 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-mt6370.c | 989 +++++++++++++++++++++++++++++++++++++++++++++ There is a drivers/leds/flash subdirectory these days, put the driver in that directory instead. Yours, Linus Walleij
On Fri, Jun 24, 2022 at 8:23 AM Linus Walleij <linus.walleij@linaro.org> wrote: > Thanks for your patch! > > On Thu, Jun 23, 2022 at 1:58 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > > From: ChiYuan Huang <cy_huang@richtek.com> > > > > Add Mediatek MT6370 current sink type LED Indicator driver. > > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > (...) > > drivers/leds/Kconfig | 11 + > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-mt6370.c | 989 +++++++++++++++++++++++++++++++++++++++++++++ > > There is a drivers/leds/flash subdirectory these days, put the driver > in that directory instead. Sorry I'm commenting on the wrong patch. I meant this one. Move that into drivers/leds/flash drivers/leds/flash/leds-mt6370-flash.c | 657 ++++++++++++ Yours, Linus Walleij
Hi Linus, Thank you for the comment. Linus Walleij <linus.walleij@linaro.org> 於 2022年6月24日 週五 下午2:25寫道： > > On Fri, Jun 24, 2022 at 8:23 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > Thanks for your patch! > > > > On Thu, Jun 23, 2022 at 1:58 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > > > > From: ChiYuan Huang <cy_huang@richtek.com> > > > > > > Add Mediatek MT6370 current sink type LED Indicator driver. > > > > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > > (...) > > > drivers/leds/Kconfig | 11 + > > > drivers/leds/Makefile | 1 + > > > drivers/leds/leds-mt6370.c | 989 +++++++++++++++++++++++++++++++++++++++++++++ > > > > There is a drivers/leds/flash subdirectory these days, put the driver > > in that directory instead. > > Sorry I'm commenting on the wrong patch. > > I meant this one. Move that into drivers/leds/flash > drivers/leds/flash/leds-mt6370-flash.c | 657 ++++++++++++ In next version, I'll use "leds: flash: ......" instead of "leds: flashlight: ......" in subject. May I confirm that the driver has already in the drivers/leds/flash, so I don’t have to move it in next version? Sincerely, Alice Chen
Hi Daniel, Thanks for your comments! Daniel Thompson <daniel.thompson@linaro.org> 於 2022年6月23日 週四 晚上9:43寫道： > > On Thu, Jun 23, 2022 at 07:56:31PM +0800, ChiaEn Wu wrote: > > From: ChiaEn Wu <chiaen_wu@richtek.com> > > > > Add Mediatek MT6370 Backlight support. > > > > Signed-off-by: ChiaEn Wu <chiaen_wu@richtek.com> > > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > > index a003e02..7cd823d 100644 > > <snip> > > +static int mt6370_init_backlight_properties(struct mt6370_priv *priv, > > + struct backlight_properties *props) > > +{ > > + struct device *dev = priv->dev; > > + u8 prop_val; > > + u32 brightness, ovp_uV, ocp_uA; > > + unsigned int mask, val; > > + int ret; > > + > > + /* Vendor optional properties */ > > + val = 0; > > + if (device_property_read_bool(dev, "mediatek,bled-pwm-enable")) > > + val |= MT6370_BL_PWM_EN_MASK; > > + > > + if (device_property_read_bool(dev, "mediatek,bled-pwm-hys-enable")) > > + val |= MT6370_BL_PWM_HYS_EN_MASK; > > + > > + ret = device_property_read_u8(dev, > > + "mediatek,bled-pwm-hys-input-th-steps", > > + &prop_val); > > + if (!ret) { > > + prop_val = clamp_val(prop_val, > > + MT6370_BL_PWM_HYS_TH_MIN_STEP, > > + MT6370_BL_PWM_HYS_TH_MAX_STEP); > > + /* > > + * prop_val = 1 --> 1 steps --> 0x00 > > + * prop_val = 2 ~ 4 --> 4 steps --> 0x01 > > + * prop_val = 5 ~ 16 --> 16 steps --> 0x10 > > + * prop_val = 17 ~ 64 --> 64 steps --> 0x11 > > ^^^^^ > These numbers are binary, not hex, right? If so, the comments > should be 0b00 to 0b03 . Ohh! Yes! These numbers are binary! I so apologize for making this mistake... I will revise the comments in the next patch! Thank you so much! > > > > + */ > > + prop_val = (ilog2(roundup_pow_of_two(prop_val)) + 1) >> 1; > > + val |= prop_val << (ffs(MT6370_BL_PWM_HYS_SEL_MASK) - 1); > > + } > > + > > + ret = regmap_update_bits(priv->regmap, MT6370_REG_BL_PWM, > > + val, val); > > + if (ret) > > + return ret; > > Overall, I like this approach! Easy to read and understand. > > > > <snip> > > +static int mt6370_bl_probe(struct platform_device *pdev) > > +{ > > + struct mt6370_priv *priv; > > + struct backlight_properties props = { > > + .type = BACKLIGHT_RAW, > > + .scale = BACKLIGHT_SCALE_LINEAR, > > Sorry, I missed this before but the KConfig comment says that the > backlight can support both linear and exponential curves. > > Is there a good reason to default to linear? Well... The customers who used this PMIC have very few or even no use exponential curve, so I set the default to linear. If you think this is inappropriate, I will add a DT property to control this feature in the next patch! By the way, I found some mistakes in my probe() function... I didn't use "return" when I use dev_err_probe()... I will refine it in the next patch! > > > Daniel. > > Best regards, ChiaEn Wu
Hi Andy, Thanks for your helpful comments! We have some questions below. Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月24日 週五 凌晨2:01寫道： > > On Thu, Jun 23, 2022 at 1:59 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > > > From: ChiYuan Huang <cy_huang@richtek.com> > > > > Add Mediatek MT6370 MFD support. > > ... > > > +config MFD_MT6370 > > + tristate "Mediatek MT6370 SubPMIC" > > + select MFD_CORE > > + select REGMAP_I2C > > + select REGMAP_IRQ > > + depends on I2C > > + help > > + Say Y here to enable MT6370 SubPMIC functional support. > > + It consists of a single cell battery charger with ADC monitoring, RGB > > + LEDs, dual channel flashlight, WLED backlight driver, display bias > > + voltage supply, one general purpose LDO, and the USB Type-C & PD > > + controller complies with the latest USB Type-C and PD standards. > > What will be the module name in case it's chosen to be built as a module? OK, we will add related text in the next patch! Thanks! > > ... > > > obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC) += intel_soc_pmic_bxtwc.o > > obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC) += intel_soc_pmic_chtwc.o > > obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o > > obj-$(CONFIG_MFD_MT6360) += mt6360-core.o > > +obj-$(CONFIG_MFD_MT6370) += mt6370.o > > mt6397-objs := mt6397-core.o mt6397-irq.o mt6358-irq.o > > obj-$(CONFIG_MFD_MT6397) += mt6397.o > > obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD) += intel_soc_pmic_mrfld.o > > This whole bunch of drivers is in the wrong place in Makefile. > > https://lore.kernel.org/all/20220616182524.7956-2-andriy.shevchenko@linux.intel.com/ > hmm... So shall we need to cherry-pick your this patch first, then modify the Makefile before the next submission?? > ... > > > +#define MT6370_REG_MAXADDR 0x1FF > > Wondering if (BIT(10) - 1) gives a better hint on how hardware limits > this (so it will be clear it's 10-bit address). well... This "0x1FF" is just a virtual mapping value to map the max address of the PMU bank(0x1XX). So, I feel its means is different from using (BIT(10) - 1) here. > > ... > > > +static int mt6370_check_vendor_info(struct mt6370_info *info) > > +{ > > + unsigned int devinfo; > > + int ret; > > + > > + ret = regmap_read(info->regmap, MT6370_REG_DEV_INFO, &devinfo); > > + if (ret) > > + return ret; > > + > > + switch (FIELD_GET(MT6370_VENID_MASK, devinfo)) { > > + case MT6370_VENID_RT5081: > > + case MT6370_VENID_RT5081A: > > + case MT6370_VENID_MT6370: > > + case MT6370_VENID_MT6371: > > + case MT6370_VENID_MT6372P: > > + case MT6370_VENID_MT6372CP: > > return 0; > > > + break; > > + default: > > + dev_err(info->dev, "Unknown Vendor ID 0x%02x\n", devinfo); > > + return -ENODEV; > > + } > > + > > + return 0; > > ...and drop these two lines? OK! We will refine it in the next patch! > > > +} > > ... > > > + bank_idx = *(u8 *)reg_buf; > > + bank_addr = *(u8 *)(reg_buf + 1); > > Why not > > const u8 *u8_buf = reg_buf; > > bank_idx = u8_buf[0]; > bank_addr = u8_buf[1]; > > ? We will refine it in the next patch! Thanks! > > ... > > > + if (ret < 0) > > + return ret; > > + else if (ret != val_size) > > Redundant 'else'. I'm not quite sure what you mean, so I made the following changes first. ------------------------------------ if (ret < 0) return ret; if (ret != val_size) return -EIO; ------------------------------------ I don't know if it meets your expectations?? > > > + return -EIO; > > ... > > > + bank_idx = *(u8 *)data; > > + bank_addr = *(u8 *)(data + 1); > > As per above. > > -- > With Best Regards, > Andy Shevchenko Best regards, ChiaEn Wu
Hi Andy, Thanks for your helpful comments! Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月24日 週五 凌晨2:19寫道： > > On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > > > From: ChiYuan Huang <cy_huang@richtek.com> > > > > Add mt6370 DisplayBias and VibLDO support. > > ... > > > +#include <linux/bits.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > > +#include <linux/of.h> > > Any users of this? (See below) > > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/regulator/driver.h> > > +#include <linux/regulator/machine.h> > > ... > > > +#define MT6370_LDO_MINUV 1600000 > > +#define MT6370_LDO_STPUV 200000 > > +#define MT6370_LDO_N_VOLT 13 > > +#define MT6370_DBVBOOST_MINUV 4000000 > > +#define MT6370_DBVBOOST_STPUV 50000 > > +#define MT6370_DBVBOOST_N_VOLT 45 > > +#define MT6370_DBVOUT_MINUV 4000000 > > +#define MT6370_DBVOUT_STPUV 50000 > > +#define MT6370_DBVOUT_N_VOLT 41 > > If UV is a unit suffix, make it _UV. > > ... > > > + .of_match = of_match_ptr("dsvbst"), > > Would it even be called / used if CONFIG_OF=n? We got a notification from Mark telling us that this patch has been applied to git. ( https://lore.kernel.org/linux-arm-kernel/165599931844.321775.8085559092337130067.b4-ty@kernel.org/ ) So, should we need to make any other changes in the next submission? > > ... > > > + .regulators_node = of_match_ptr("regulators"), > > Ditto. > > ... > > > + for (i = 0; i < ARRAY_SIZE(mt6370_irqs); i++) { > > + irq = platform_get_irq_byname(pdev, mt6370_irqs[i].name); > > + > > + rdev = priv->rdev[mt6370_irqs[i].rid]; > > + > > + ret = devm_request_threaded_irq(priv->dev, irq, NULL, > > + mt6370_irqs[i].handler, 0, > > + mt6370_irqs[i].name, rdev); > > + if (ret) { > > > + dev_err(priv->dev, > > + "Failed to register (%d) interrupt\n", i); > > + return ret; > > return dev_err_probe(...); ? > > > + } > > + } > > ... > > > + for (i = 0; i < MT6370_MAX_IDX; i++) { > > + rdev = devm_regulator_register(priv->dev, > > + mt6370_regulator_descs + i, > > + &cfg); > > + if (IS_ERR(rdev)) { > > > + dev_err(priv->dev, > > + "Failed to register (%d) regulator\n", i); > > + return PTR_ERR(rdev); > > return dev_err_probe(...); ? > > > + } > > + > > + priv->rdev[i] = rdev; > > + } > > ... > > > + if (!priv->regmap) { > > + dev_err(&pdev->dev, "Failed to init regmap\n"); > > + return -ENODEV; > > + } > > return dev_err_probe(...); > > -- > With Best Regards, > Andy Shevchenko Best regards, ChiaEn Wu
On Thu, Jun 23, 2022 at 07:56:25PM +0800, ChiaEn Wu wrote: > --- /dev/null > +++ b/drivers/usb/typec/tcpm/tcpci_mt6370.c > @@ -0,0 +1,212 @@ > +// SPDX-License-Identifier: GPL-2.0+ Are you sure you mean "+" here? I have to ask, sorry. And no copyright line? Your company is ok with that, nice! :) thanks, greg k-h
On Fri, Jun 24, 2022 at 9:20 AM szuni chen <szunichen@gmail.com> wrote: > > I meant this one. Move that into drivers/leds/flash > > drivers/leds/flash/leds-mt6370-flash.c | 657 ++++++++++++ > > In next version, I'll use "leds: flash: ......" instead of "leds: > flashlight: ......" in subject. > May I confirm that the driver has already in the drivers/leds/flash, > so I don’t have to move it in next version? Yeah you're right, I am just writing wrong comments today, it is already correct. Sorry! Yours, Linus Walleij
On Fri, Jun 24, 2022 at 12:19 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月24日 週五 凌晨2:01寫道： > > On Thu, Jun 23, 2022 at 1:59 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: ... > > > obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC) += intel_soc_pmic_bxtwc.o > > > obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC) += intel_soc_pmic_chtwc.o > > > obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o > > > obj-$(CONFIG_MFD_MT6360) += mt6360-core.o > > > +obj-$(CONFIG_MFD_MT6370) += mt6370.o > > > mt6397-objs := mt6397-core.o mt6397-irq.o mt6358-irq.o > > > obj-$(CONFIG_MFD_MT6397) += mt6397.o > > > obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD) += intel_soc_pmic_mrfld.o > > > > This whole bunch of drivers is in the wrong place in Makefile. > > > > https://lore.kernel.org/all/20220616182524.7956-2-andriy.shevchenko@linux.intel.com/ > > hmm... So shall we need to cherry-pick your this patch first, > then modify the Makefile before the next submission?? I don't know what Lee's preferences are, but at least I have these options in mind: 1) wait until Lee applies my series; 2) take that single patch to your tree as a precursor. In the second case you will need to send the series with that patch as well. ... > > > +#define MT6370_REG_MAXADDR 0x1FF > > > > Wondering if (BIT(10) - 1) gives a better hint on how hardware limits > > this (so it will be clear it's 10-bit address). > > well... This "0x1FF" is just a virtual mapping value to map the max > address of the PMU bank(0x1XX). > So, I feel its means is different from using (BIT(10) - 1) here. Perhaps a comment then? ... > > > + if (ret < 0) > > > + return ret; > > > + else if (ret != val_size) > > > > Redundant 'else'. > > I'm not quite sure what you mean, so I made the following changes first. > ------------------------------------ > if (ret < 0) > return ret; > if (ret != val_size) > return -EIO; > ------------------------------------ > I don't know if it meets your expectations?? Yes. > > > + return -EIO;
On Fri, Jun 24, 2022 at 12:33 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月24日 週五 凌晨2:19寫道： > > On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: ... > We got a notification from Mark telling us that this patch has been > applied to git. > ( https://lore.kernel.org/linux-arm-kernel/165599931844.321775.8085559092337130067.b4-ty@kernel.org/ > ) > So, should we need to make any other changes in the next submission? You may do the followup(s) to address all or some of the comments depending on the case (e.g. good to clean up code with dev_err_probe() use).
Hi Andy, Sorry for the late reply, I have some questions to ask you below. Thanks! Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月24日 週五 凌晨2:56寫道： > > On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote: > > > > From: ChiaEn Wu <chiaen_wu@richtek.com> > > > > Add Mediatek MT6370 charger driver. > > ... > > > +config CHARGER_MT6370 > > + tristate "Mediatek MT6370 Charger Driver" > > + depends on MFD_MT6370 > > + depends on REGULATOR > > + select LINEAR_RANGES > > + help > > + Say Y here to enable MT6370 Charger Part. > > + The device supports High-Accuracy Voltage/Current Regulation, > > + Average Input Current Regulation, Battery Temperature Sensing, > > + Over-Temperature Protection, DPDM Detection for BC1.2. > > Module name? > > ... > > > +#include <dt-bindings/iio/adc/mediatek,mt6370_adc.h> > > This usually goes after linux/* > > > +#include <linux/atomic.h> > > +#include <linux/bitfield.h> > > +#include <linux/bits.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/iio/consumer.h> > > +#include <linux/init.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > > +#include <linux/of.h> > > > > +#include <linux/platform_device.h> > > +#include <linux/power_supply.h> > > +#include <linux/regmap.h> > > +#include <linux/regulator/driver.h> > > +#include <linux/workqueue.h> > > ... > > > +#define MT6370_MIVR_IBUS_TH 100000 /* 100 mA */ > > Instead of comment, add proper units. > > ... > > > + MT6370_USB_STAT_DCP, > > + MT6370_USB_STAT_CDP, > > + MT6370_USB_STAT_MAX, > > No comma for a terminator line. > > ... > > > +static inline u32 mt6370_chg_val_to_reg(const struct mt6370_chg_range *range, > > + u32 val) > > +static inline u32 mt6370_chg_reg_to_val(const struct mt6370_chg_range *range, > > + u8 reg) > > I'm wondering if you can use the > https://elixir.bootlin.com/linux/v5.19-rc3/source/include/linux/linear_range.h > APIs. Thanks for your helpful comments! I will refine it in the next patch! > > ... > > > + int ret = 0; > > This seems a redundant assignment, see below. > > > + rcfg->ena_gpiod = fwnode_gpiod_get_index(of_fwnode_handle(of), > > + "enable", 0, > > For index == 0 don't use _index API. > > > + GPIOD_OUT_LOW | > > + GPIOD_FLAGS_BIT_NONEXCLUSIVE, > > + rdesc->name); > > + if (IS_ERR(rcfg->ena_gpiod)) { > > + dev_err(priv->dev, "Failed to requeset OTG EN Pin\n"); > > request > > > + rcfg->ena_gpiod = NULL; > > So, use _optional and return any errors you got. These days, I tried to use various APIs in <gpio/consumer.h>, and also try to use _optional APIs. But my OTG regulator node is a child node of the charger node, like below. ---------------------------------------------------------------------------- // copy-paste from our mfd dt-binding example charger { compatible = "mediatek,mt6370-charger"; interrupts = <48>, <68>, <6>; interrupt-names = "attach_i", "uvp_d_evt", "mivr"; io-channels = <&mt6370_adc MT6370_CHAN_IBUS>; mt6370_otg_vbus: usb-otg-vbus-regulator { regulator-name = "mt6370-usb-otg-vbus"; regulator-min-microvolt = <4350000>; regulator-max-microvolt = <5800000>; regulator-min-microamp = <500000>; regulator-max-microamp = <3000000>; }; }; ---------------------------------------------------------------------------- Hence, if I use _optional APIs, it will always get NULL. And, If I use 'gpiod_get_from_of_node' here, this API will only parse the 'enable' property, not 'enable-gpio' or 'enable-gpios', we need to add the '-gpio' suffix before we use this API. Only 'fwnode_gpiod_get_index' can match this case. Although fwnode parsing is not preferred, 'of_parse_cb' already can guarantee the callback will only be used by the regulator of_node parsing. > > > + } else { > > + val = MT6370_OPA_MODE_MASK | MT6370_OTG_PIN_EN_MASK; > > + ret = regmap_update_bits(priv->regmap, MT6370_REG_CHG_CTRL1, > > + val, val); > > + if (ret) > > + dev_err(priv->dev, "Failed to set otg bits\n"); > > + } > > ... > > > + irq_num = platform_get_irq_byname(pdev, irq_name); > > > + > > Unwanted blank line. > > > + if (irq_num < 0) { > > > + dev_err(priv->dev, "Failed to get platform resource\n"); > > Isn't it printed by the call? > > > + } else { > > + if (en) > > + enable_irq(irq_num); > > + else > > + disable_irq_nosync(irq_num); > > + } > > ... > > > +toggle_cfo_exit: > > The useless label. > > > + return ret; > > +} > > ... > > > + ret = mt6370_chg_get_online(priv, val); > > + if (!val->intval) { > > No error check? I replace "mt6370_chg_get_online()" with "power_supply_get_property()" and add some error check. Could it meet your expectations?? > > > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > > + return 0; > > + } > > ... > > > +static int mt6370_chg_set_online(struct mt6370_priv *priv, > > + const union power_supply_propval *val) > > +{ > > + int attach; > > + u32 pwr_rdy = !!val->intval; > > + > > + mutex_lock(&priv->attach_lock); > > + attach = atomic_read(&priv->attach); > > + if (pwr_rdy == !!attach) { > > + dev_err(priv->dev, "pwr_rdy is same(%d)\n", pwr_rdy); > > + mutex_unlock(&priv->attach_lock); > > + return 0; > > + } > > + > > + atomic_set(&priv->attach, pwr_rdy); > > + mutex_unlock(&priv->attach_lock); > > + > > + if (!queue_work(priv->wq, &priv->bc12_work)) > > + dev_err(priv->dev, "bc12 work has already queued\n"); > > + > > + return 0; > > > + > > Unwanted blank line. > > > +} > > > +static int mt6370_chg_get_property(struct power_supply *psy, > > + enum power_supply_property psp, > > + union power_supply_propval *val) > > +{ > > + struct mt6370_priv *priv = power_supply_get_drvdata(psy); > > + int ret = 0; > > + > > + switch (psp) { > > + case POWER_SUPPLY_PROP_ONLINE: > > + ret = mt6370_chg_get_online(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_STATUS: > > + ret = mt6370_chg_get_status(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_CHARGE_TYPE: > > + ret = mt6370_chg_get_charge_type(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: > > + ret = mt6370_chg_get_ichg(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX: > > + ret = mt6370_chg_get_max_ichg(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > > + ret = mt6370_chg_get_cv(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX: > > + ret = mt6370_chg_get_max_cv(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: > > + ret = mt6370_chg_get_aicr(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT: > > + ret = mt6370_chg_get_mivr(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > > + ret = mt6370_chg_get_iprechg(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > > + ret = mt6370_chg_get_ieoc(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_TYPE: > > + val->intval = priv->psy_desc->type; > > + break; > > + case POWER_SUPPLY_PROP_USB_TYPE: > > + val->intval = priv->psy_usb_type; > > + break; > > + default: > > + ret = -EINVAL; > > + break; > > + } > > + > > + return ret; > > In all cases, return directly. > > > +} > > ... > > > + switch (psp) { > > + case POWER_SUPPLY_PROP_ONLINE: > > + ret = mt6370_chg_set_online(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: > > + ret = mt6370_chg_set_ichg(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > > + ret = mt6370_chg_set_cv(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT: > > + ret = mt6370_chg_set_aicr(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT: > > + ret = mt6370_chg_set_mivr(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_PRECHARGE_CURRENT: > > + ret = mt6370_chg_set_iprechg(priv, val); > > + break; > > + case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT: > > + ret = mt6370_chg_set_ieoc(priv, val); > > + break; > > + default: > > + ret = -EINVAL; > > + } > > + return ret; > > As per above. > > ... > > > + for (i = 0; i < F_MAX; i++) { > > + priv->rmap_fields[i] = devm_regmap_field_alloc(priv->dev, > > + priv->regmap, > > + fds[i].field); > > + if (IS_ERR(priv->rmap_fields[i])) { > > + dev_err(priv->dev, > > + "Failed to allocate regmap field [%s]\n", > > + fds[i].name); > > + return PTR_ERR(priv->rmap_fields[i]); > > return dev_err_probe(); > > > + } > > + } > > ... > > > + mutex_init(&priv->attach_lock); > > + atomic_set(&priv->attach, 0); > > Why not atomic_init() ? > But yeah, usage of it and other locking mechanisms in this driver are > questionable. I will refine it in the next patch! > > ... > > > + /* ICHG/IEOC Workaroud, ICHG can not be set less than 900mA */ > > Workaround > > ... > > > + return IS_ERR(priv->rdev) ? PTR_ERR(priv->rdev) : 0; > > PTR_ERR_OR_ZERO() > > ... > > > + .of_node = priv->dev->of_node, > > dev_of_node() ? > > > + }; > > + > > + priv->psy_desc = &mt6370_chg_psy_desc; > > + priv->psy_desc->name = dev_name(priv->dev); > > + priv->psy = devm_power_supply_register(priv->dev, priv->psy_desc, &cfg); > > + > > + return IS_ERR(priv->psy) ? PTR_ERR(priv->psy) : 0; > > PTR_ERR_OR_ZERO() > > > +} > > ... > > > +static irqreturn_t mt6370_attach_i_handler(int irq, void *data) > > +{ > > + struct mt6370_priv *priv = data; > > + u32 otg_en; > > + int ret; > > + > > + /* Check in otg mode or not */ > > + ret = mt6370_chg_field_get(priv, F_BOOST_STAT, &otg_en); > > + if (ret < 0) { > > + dev_err(priv->dev, "failed to get otg state\n"); > > + return IRQ_HANDLED; > > Handled error? > > > + } > > + > > + if (otg_en) > > + return IRQ_HANDLED; > > > + mutex_lock(&priv->attach_lock); > > + atomic_set(&priv->attach, MT6370_ATTACH_STAT_ATTACH_BC12_DONE); > > + mutex_unlock(&priv->attach_lock); > > Mutex around atomic?! It's interesting... I will revise it in the next patch. > > > + if (!queue_work(priv->wq, &priv->bc12_work)) > > + dev_err(priv->dev, "bc12 work has already queued\n"); > > + > > + return IRQ_HANDLED; > > +} > > ... > > > + for (i = 0; i < ARRAY_SIZE(mt6370_chg_irqs); i++) { > > + ret = platform_get_irq_byname(to_platform_device(priv->dev), > > + mt6370_chg_irqs[i].name); > > + if (ret < 0) { > > + dev_err(priv->dev, "Failed to get irq %s\n", > > + mt6370_chg_irqs[i].name); > > Isn't the same printed by the above call? well... yes they are similar, I will remove one of them in the next patch. > > > + return ret; > > + } > > + > > + ret = devm_request_threaded_irq(priv->dev, ret, NULL, > > + mt6370_chg_irqs[i].handler, > > + IRQF_TRIGGER_FALLING, > > + dev_name(priv->dev), > > + priv); > > + > > + if (ret < 0) { > > + dev_err(priv->dev, "Failed to request irq %s\n", > > + mt6370_chg_irqs[i].name); > > + return ret; > > return dev_err_probe(); > > > + } > > + } > > ... > > > +static int mt6370_chg_probe(struct platform_device *pdev) > > +{ > > > Use return dev_err_probe(...); pattern. > > > +probe_out: > > + destroy_workqueue(priv->wq); > > + mutex_destroy(&priv->attach_lock); > > I don't see clearly the initialization of these in the ->probe(). > Besides that, does destroy_workque() synchronize the actual queue(s)? > > Mixing devm_ and non-devm_ may lead to a wrong release order that's > why it is better to see allocating and destroying resources in one > function (they may be wrapped, but should be both of them, seems like > you have done it only for the first parts). OK, I will try to revise these in the next patch! > > > + return ret; > > +} > > ... > > > +static int mt6370_chg_remove(struct platform_device *pdev) > > +{ > > + struct mt6370_priv *priv = platform_get_drvdata(pdev); > > + > > + if (priv) { > > Can you describe when this condition can be false? well... I will remove it in the next patch, sorry for making this stupid mistake... > > > + mt6370_chg_enable_irq(priv, "mivr", false); > > + cancel_delayed_work_sync(&priv->mivr_dwork); > > + destroy_workqueue(priv->wq); > > + mutex_destroy(&priv->attach_lock); > > + } > > + > > + return 0; > > +} > > ... > > > +static struct platform_driver mt6370_chg_driver = { > > + .probe = mt6370_chg_probe, > > + .remove = mt6370_chg_remove, > > + .driver = { > > + .name = "mt6370-charger", > > + .of_match_table = of_match_ptr(mt6370_chg_of_match), > > No good use of of_match_ptr(), please drop it. > > > + }, > > +}; > > -- > With Best Regards, > Andy Shevchenko Thanks for your review! Best regards, ChiaEn Wu
On Thu, 23 Jun 2022, ChiaEn Wu wrote: > From: ChiYuan Huang <cy_huang@richtek.com> > > Add Mediatek MT6370 MFD support. No such thing as "MFD support". And you're not getting away with submitting a 370 line patch with a 5 word change log either. :) Please at least tell us what the device is and what it's used for. > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > --- > > v3 > - Refine Kconfig help text > - Refine error message of unknown vendor ID in > mt6370_check_vendor_info() > - Refine return value handling of mt6370_regmap_read() > - Refine all probe error by using dev_err_probe() > - Refine "bank_idx" and "bank_addr" in mt6370_regmap_read() and > mt6370_regmap_write() > - Add "#define VENID*" and drop the comments in > mt6370_check_vendor_info() > - Drop "MFD" in MODULE_DESCRIPTION() > --- > drivers/mfd/Kconfig | 13 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/mt6370.c | 358 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 372 insertions(+) > create mode 100644 drivers/mfd/mt6370.c > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 3b59456..4c900c4 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -937,6 +937,19 @@ config MFD_MT6360 > PMIC part includes 2-channel BUCKs and 2-channel LDOs > LDO part includes 4-channel LDOs > > +config MFD_MT6370 > + tristate "Mediatek MT6370 SubPMIC" > + select MFD_CORE > + select REGMAP_I2C > + select REGMAP_IRQ > + depends on I2C > + help > + Say Y here to enable MT6370 SubPMIC functional support. > + It consists of a single cell battery charger with ADC monitoring, RGB > + LEDs, dual channel flashlight, WLED backlight driver, display bias > + voltage supply, one general purpose LDO, and the USB Type-C & PD > + controller complies with the latest USB Type-C and PD standards. > + > config MFD_MT6397 > tristate "MediaTek MT6397 PMIC Support" > select MFD_CORE > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 858cacf..62b2712 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -242,6 +242,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC) += intel_soc_pmic_bxtwc.o > obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC) += intel_soc_pmic_chtwc.o > obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o > obj-$(CONFIG_MFD_MT6360) += mt6360-core.o > +obj-$(CONFIG_MFD_MT6370) += mt6370.o > mt6397-objs := mt6397-core.o mt6397-irq.o mt6358-irq.o > obj-$(CONFIG_MFD_MT6397) += mt6397.o > obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD) += intel_soc_pmic_mrfld.o > diff --git a/drivers/mfd/mt6370.c b/drivers/mfd/mt6370.c > new file mode 100644 > index 0000000..49f02b1 > --- /dev/null > +++ b/drivers/mfd/mt6370.c > @@ -0,0 +1,358 @@ > +// SPDX-License-Identifier: GPL-2.0 No Copyright? > +#include <linux/bits.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/mfd/core.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > + > +enum { > + MT6370_USBC_I2C = 0, > + MT6370_PMU_I2C, > + MT6370_MAX_I2C > +}; > + > +#define MT6370_REG_DEV_INFO 0x100 > +#define MT6370_REG_CHG_IRQ1 0x1C0 > +#define MT6370_REG_CHG_MASK1 0x1E0 > + > +#define MT6370_VENID_MASK GENMASK(7, 4) > + > +#define MT6370_NUM_IRQREGS 16 > +#define MT6370_USBC_I2CADDR 0x4E > +#define MT6370_REG_ADDRLEN 2 > +#define MT6370_REG_MAXADDR 0x1FF > + > +#define MT6370_VENID_RT5081 0x8 > +#define MT6370_VENID_RT5081A 0xA > +#define MT6370_VENID_MT6370 0xE > +#define MT6370_VENID_MT6371 0xF > +#define MT6370_VENID_MT6372P 0x9 > +#define MT6370_VENID_MT6372CP 0xB > + > +/* IRQ definitions */ > +#define MT6370_IRQ_DIRCHGON 0 > +#define MT6370_IRQ_CHG_TREG 4 > +#define MT6370_IRQ_CHG_AICR 5 > +#define MT6370_IRQ_CHG_MIVR 6 > +#define MT6370_IRQ_PWR_RDY 7 > +#define MT6370_IRQ_FL_CHG_VINOVP 11 > +#define MT6370_IRQ_CHG_VSYSUV 12 > +#define MT6370_IRQ_CHG_VSYSOV 13 > +#define MT6370_IRQ_CHG_VBATOV 14 > +#define MT6370_IRQ_CHG_VINOVPCHG 15 > +#define MT6370_IRQ_TS_BAT_COLD 20 > +#define MT6370_IRQ_TS_BAT_COOL 21 > +#define MT6370_IRQ_TS_BAT_WARM 22 > +#define MT6370_IRQ_TS_BAT_HOT 23 > +#define MT6370_IRQ_TS_STATC 24 > +#define MT6370_IRQ_CHG_FAULT 25 > +#define MT6370_IRQ_CHG_STATC 26 > +#define MT6370_IRQ_CHG_TMR 27 > +#define MT6370_IRQ_CHG_BATABS 28 > +#define MT6370_IRQ_CHG_ADPBAD 29 > +#define MT6370_IRQ_CHG_RVP 30 > +#define MT6370_IRQ_TSHUTDOWN 31 > +#define MT6370_IRQ_CHG_IINMEAS 32 > +#define MT6370_IRQ_CHG_ICCMEAS 33 > +#define MT6370_IRQ_CHGDET_DONE 34 > +#define MT6370_IRQ_WDTMR 35 > +#define MT6370_IRQ_SSFINISH 36 > +#define MT6370_IRQ_CHG_RECHG 37 > +#define MT6370_IRQ_CHG_TERM 38 > +#define MT6370_IRQ_CHG_IEOC 39 > +#define MT6370_IRQ_ADC_DONE 40 > +#define MT6370_IRQ_PUMPX_DONE 41 > +#define MT6370_IRQ_BST_BATUV 45 > +#define MT6370_IRQ_BST_MIDOV 46 > +#define MT6370_IRQ_BST_OLP 47 > +#define MT6370_IRQ_ATTACH 48 > +#define MT6370_IRQ_DETACH 49 > +#define MT6370_IRQ_HVDCP_STPDONE 51 > +#define MT6370_IRQ_HVDCP_VBUSDET_DONE 52 > +#define MT6370_IRQ_HVDCP_DET 53 > +#define MT6370_IRQ_CHGDET 54 > +#define MT6370_IRQ_DCDT 55 > +#define MT6370_IRQ_DIRCHG_VGOK 59 > +#define MT6370_IRQ_DIRCHG_WDTMR 60 > +#define MT6370_IRQ_DIRCHG_UC 61 > +#define MT6370_IRQ_DIRCHG_OC 62 > +#define MT6370_IRQ_DIRCHG_OV 63 > +#define MT6370_IRQ_OVPCTRL_SWON 67 > +#define MT6370_IRQ_OVPCTRL_UVP_D 68 > +#define MT6370_IRQ_OVPCTRL_UVP 69 > +#define MT6370_IRQ_OVPCTRL_OVP_D 70 > +#define MT6370_IRQ_OVPCTRL_OVP 71 > +#define MT6370_IRQ_FLED_STRBPIN 72 > +#define MT6370_IRQ_FLED_TORPIN 73 > +#define MT6370_IRQ_FLED_TX 74 > +#define MT6370_IRQ_FLED_LVF 75 > +#define MT6370_IRQ_FLED2_SHORT 78 > +#define MT6370_IRQ_FLED1_SHORT 79 > +#define MT6370_IRQ_FLED2_STRB 80 > +#define MT6370_IRQ_FLED1_STRB 81 > +#define mT6370_IRQ_FLED2_STRB_TO 82 > +#define MT6370_IRQ_FLED1_STRB_TO 83 > +#define MT6370_IRQ_FLED2_TOR 84 > +#define MT6370_IRQ_FLED1_TOR 85 > +#define MT6370_IRQ_OTP 93 > +#define MT6370_IRQ_VDDA_OVP 94 > +#define MT6370_IRQ_VDDA_UV 95 > +#define MT6370_IRQ_LDO_OC 103 > +#define MT6370_IRQ_BLED_OCP 118 > +#define MT6370_IRQ_BLED_OVP 119 > +#define MT6370_IRQ_DSV_VNEG_OCP 123 > +#define MT6370_IRQ_DSV_VPOS_OCP 124 > +#define MT6370_IRQ_DSV_BST_OCP 125 > +#define MT6370_IRQ_DSV_VNEG_SCP 126 > +#define MT6370_IRQ_DSV_VPOS_SCP 127 Can you pop these into a header file please? > +struct mt6370_info { > + struct i2c_client *i2c[MT6370_MAX_I2C]; > + struct device *dev; You don't need both 'i2c' and 'dev'. You can derive one from the other. > + struct regmap *regmap; > + struct regmap_irq_chip_data *irq_data; > +}; This can do into the header file too. > +static const struct regmap_irq mt6370_irqs[] = { > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHGON, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_TREG, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_AICR, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_MIVR, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_PWR_RDY, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FL_CHG_VINOVP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VSYSUV, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VSYSOV, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VBATOV, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VINOVPCHG, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_COLD, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_COOL, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_WARM, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_HOT, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_STATC, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_FAULT, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_STATC, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_TMR, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_BATABS, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_ADPBAD, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_RVP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_TSHUTDOWN, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_IINMEAS, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_ICCMEAS, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHGDET_DONE, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_WDTMR, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_SSFINISH, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_RECHG, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_TERM, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_IEOC, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_ADC_DONE, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_PUMPX_DONE, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_BST_BATUV, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_BST_MIDOV, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_BST_OLP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_ATTACH, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DETACH, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_HVDCP_STPDONE, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_HVDCP_VBUSDET_DONE, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_HVDCP_DET, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHGDET, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DCDT, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_VGOK, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_WDTMR, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_UC, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_OC, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_OV, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_SWON, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_UVP_D, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_UVP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_OVP_D, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_OVP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_STRBPIN, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_TORPIN, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_TX, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_LVF, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED2_SHORT, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_SHORT, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED2_STRB, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_STRB, 8), > + REGMAP_IRQ_REG_LINE(mT6370_IRQ_FLED2_STRB_TO, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_STRB_TO, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED2_TOR, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_TOR, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_OTP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_VDDA_OVP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_VDDA_UV, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_LDO_OC, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_BLED_OCP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_BLED_OVP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VNEG_OCP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VPOS_OCP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_BST_OCP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VNEG_SCP, 8), > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VPOS_SCP, 8) > +}; > + > +static const struct regmap_irq_chip mt6370_irq_chip = { > + .name = "mt6370-irqs", > + .status_base = MT6370_REG_CHG_IRQ1, > + .mask_base = MT6370_REG_CHG_MASK1, > + .num_regs = MT6370_NUM_IRQREGS, > + .irqs = mt6370_irqs, > + .num_irqs = ARRAY_SIZE(mt6370_irqs), > +}; > + > +static const struct resource mt6370_regulator_irqs[] = { > + DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VPOS_SCP, "db_vpos_scp"), > + DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VNEG_SCP, "db_vneg_scp"), > + DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_BST_OCP, "db_vbst_ocp"), > + DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VPOS_OCP, "db_vpos_ocp"), > + DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VNEG_OCP, "db_vneg_ocp"), > + DEFINE_RES_IRQ_NAMED(MT6370_IRQ_LDO_OC, "ldo_oc") > +}; > + > +static const struct mfd_cell mt6370_devices[] = { > + MFD_CELL_OF("adc", NULL, NULL, 0, 0, "mediatek,mt6370-adc"), > + MFD_CELL_OF("charger", NULL, NULL, 0, 0, "mediatek,mt6370-charger"), > + MFD_CELL_OF("backlight", NULL, NULL, 0, 0, "mediatek,mt6370-backlight"), > + MFD_CELL_OF("flashlight", NULL, NULL, 0, 0, "mediatek,mt6370-flashlight"), > + MFD_CELL_OF("indicator", NULL, NULL, 0, 0, "mediatek,mt6370-indicator"), > + MFD_CELL_OF("tcpc", NULL, NULL, 0, 0, "mediatek,mt6370-tcpc"), > + MFD_CELL_RES("regulator", mt6370_regulator_irqs) The first parameters here should be prepended with something, perhaps "mt6370_"? > +}; > + > +static int mt6370_check_vendor_info(struct mt6370_info *info) > +{ > + unsigned int devinfo; > + int ret; > + > + ret = regmap_read(info->regmap, MT6370_REG_DEV_INFO, &devinfo); > + if (ret) > + return ret; > + > + switch (FIELD_GET(MT6370_VENID_MASK, devinfo)) { > + case MT6370_VENID_RT5081: > + case MT6370_VENID_RT5081A: > + case MT6370_VENID_MT6370: > + case MT6370_VENID_MT6371: > + case MT6370_VENID_MT6372P: > + case MT6370_VENID_MT6372CP: > + break; > + default: > + dev_err(info->dev, "Unknown Vendor ID 0x%02x\n", devinfo); > + return -ENODEV; > + } > + > + return 0; > +} > + > +static int mt6370_regmap_read(void *context, const void *reg_buf, > + size_t reg_size, void *val_buf, size_t val_size) > +{ > + struct mt6370_info *info = context; > + u8 bank_idx, bank_addr; > + int ret; > + > + bank_idx = *(u8 *)reg_buf; > + bank_addr = *(u8 *)(reg_buf + 1); > + > + ret = i2c_smbus_read_i2c_block_data(info->i2c[bank_idx], bank_addr, > + val_size, val_buf); > + if (ret < 0) > + return ret; > + else if (ret != val_size) > + return -EIO; > + > + return 0; > +} > + > +static int mt6370_regmap_write(void *context, const void *data, size_t count) > +{ > + struct mt6370_info *info = context; > + u8 bank_idx, bank_addr; > + int len = count - MT6370_REG_ADDRLEN; > + > + bank_idx = *(u8 *)data; > + bank_addr = *(u8 *)(data + 1); > + > + return i2c_smbus_write_i2c_block_data(info->i2c[bank_idx], bank_addr, > + len, data + MT6370_REG_ADDRLEN); > +} > + > +static const struct regmap_bus mt6370_regmap_bus = { > + .read = mt6370_regmap_read, > + .write = mt6370_regmap_write, > +}; > + > +static const struct regmap_config mt6370_regmap_config = { > + .reg_bits = 16, > + .val_bits = 8, > + .reg_format_endian = REGMAP_ENDIAN_BIG, > + .max_register = MT6370_REG_MAXADDR, > +}; > + > +static int mt6370_probe(struct i2c_client *i2c) > +{ > + struct mt6370_info *info; > + struct i2c_client *usbc_i2c; > + int ret; > + > + info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + > + info->dev = &i2c->dev; > + > + usbc_i2c = devm_i2c_new_dummy_device(&i2c->dev, i2c->adapter, > + MT6370_USBC_I2CADDR); > + if (IS_ERR(usbc_i2c)) > + return dev_err_probe(&i2c->dev, PTR_ERR(usbc_i2c), > + "Failed to register USBC I2C client\n"); > + > + /* Assign I2C client for PMU and TypeC */ > + info->i2c[MT6370_PMU_I2C] = i2c; > + info->i2c[MT6370_USBC_I2C] = usbc_i2c; > + > + info->regmap = devm_regmap_init(&i2c->dev, &mt6370_regmap_bus, info, > + &mt6370_regmap_config); Apart from in mt6370_check_vendor_info() where is this actually used? > + if (IS_ERR(info->regmap)) > + return dev_err_probe(&i2c->dev, PTR_ERR(info->regmap), > + "Failed to register regmap\n"); > + > + ret = mt6370_check_vendor_info(info); > + if (ret) > + return dev_err_probe(&i2c->dev, ret, > + "Failed to check vendor info\n"); > + > + ret = devm_regmap_add_irq_chip(&i2c->dev, info->regmap, i2c->irq, > + IRQF_ONESHOT, -1, &mt6370_irq_chip, > + &info->irq_data); > + if (ret) > + return dev_err_probe(&i2c->dev, ret, > + "Failed to add irq chip\n"); > + > + return devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, > + mt6370_devices, ARRAY_SIZE(mt6370_devices), > + NULL, 0, > + regmap_irq_get_domain(info->irq_data)); > +} > + > +static const struct of_device_id mt6370_match_table[] = { > + { .compatible = "mediatek,mt6370", }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, mt6370_match_table); > + > +static struct i2c_driver mt6370_driver = { > + .driver = { > + .name = "mt6370", > + .of_match_table = mt6370_match_table, > + }, > + .probe_new = mt6370_probe, > +}; > +module_i2c_driver(mt6370_driver); > + > +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>"); > +MODULE_DESCRIPTION("MT6370 I2C Driver"); This is not an I2C driver. > +MODULE_LICENSE("GPL v2");
Hi Lee, Thanks for your reply. Lee Jones <lee.jones@linaro.org> 於 2022年7月12日 週二 晚上11:29寫道： > > On Thu, 23 Jun 2022, ChiaEn Wu wrote: > > > From: ChiYuan Huang <cy_huang@richtek.com> > > > > Add Mediatek MT6370 MFD support. > > No such thing as "MFD support". > > And you're not getting away with submitting a 370 line patch with a 5 > word change log either. :) > > Please at least tell us what the device is and what it's used for. I sincerely apologize. We will add more descriptions of the MT6370 feature in the v5 patch. > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > > --- > > > > v3 > > - Refine Kconfig help text > > - Refine error message of unknown vendor ID in > > mt6370_check_vendor_info() > > - Refine return value handling of mt6370_regmap_read() > > - Refine all probe error by using dev_err_probe() > > - Refine "bank_idx" and "bank_addr" in mt6370_regmap_read() and > > mt6370_regmap_write() > > - Add "#define VENID*" and drop the comments in > > mt6370_check_vendor_info() > > - Drop "MFD" in MODULE_DESCRIPTION() > > --- > > drivers/mfd/Kconfig | 13 ++ > > drivers/mfd/Makefile | 1 + > > drivers/mfd/mt6370.c | 358 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 372 insertions(+) > > create mode 100644 drivers/mfd/mt6370.c > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index 3b59456..4c900c4 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -937,6 +937,19 @@ config MFD_MT6360 > > PMIC part includes 2-channel BUCKs and 2-channel LDOs > > LDO part includes 4-channel LDOs > > > > +config MFD_MT6370 > > + tristate "Mediatek MT6370 SubPMIC" > > + select MFD_CORE > > + select REGMAP_I2C > > + select REGMAP_IRQ > > + depends on I2C > > + help > > + Say Y here to enable MT6370 SubPMIC functional support. > > + It consists of a single cell battery charger with ADC monitoring, RGB > > + LEDs, dual channel flashlight, WLED backlight driver, display bias > > + voltage supply, one general purpose LDO, and the USB Type-C & PD > > + controller complies with the latest USB Type-C and PD standards. > > + > > config MFD_MT6397 > > tristate "MediaTek MT6397 PMIC Support" > > select MFD_CORE > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index 858cacf..62b2712 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -242,6 +242,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC) += intel_soc_pmic_bxtwc.o > > obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC) += intel_soc_pmic_chtwc.o > > obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o > > obj-$(CONFIG_MFD_MT6360) += mt6360-core.o > > +obj-$(CONFIG_MFD_MT6370) += mt6370.o > > mt6397-objs := mt6397-core.o mt6397-irq.o mt6358-irq.o > > obj-$(CONFIG_MFD_MT6397) += mt6397.o > > obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD) += intel_soc_pmic_mrfld.o > > diff --git a/drivers/mfd/mt6370.c b/drivers/mfd/mt6370.c > > new file mode 100644 > > index 0000000..49f02b1 > > --- /dev/null > > +++ b/drivers/mfd/mt6370.c > > @@ -0,0 +1,358 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > No Copyright? We have already added Copyright in the v4 patch. > > > +#include <linux/bits.h> > > +#include <linux/i2c.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel.h> > > +#include <linux/mfd/core.h> > > +#include <linux/module.h> > > +#include <linux/regmap.h> > > + > > +enum { > > + MT6370_USBC_I2C = 0, > > + MT6370_PMU_I2C, > > + MT6370_MAX_I2C > > +}; > > + > > +#define MT6370_REG_DEV_INFO 0x100 > > +#define MT6370_REG_CHG_IRQ1 0x1C0 > > +#define MT6370_REG_CHG_MASK1 0x1E0 > > + > > +#define MT6370_VENID_MASK GENMASK(7, 4) > > + > > +#define MT6370_NUM_IRQREGS 16 > > +#define MT6370_USBC_I2CADDR 0x4E > > +#define MT6370_REG_ADDRLEN 2 > > +#define MT6370_REG_MAXADDR 0x1FF > > + > > +#define MT6370_VENID_RT5081 0x8 > > +#define MT6370_VENID_RT5081A 0xA > > +#define MT6370_VENID_MT6370 0xE > > +#define MT6370_VENID_MT6371 0xF > > +#define MT6370_VENID_MT6372P 0x9 > > +#define MT6370_VENID_MT6372CP 0xB > > + > > +/* IRQ definitions */ > > +#define MT6370_IRQ_DIRCHGON 0 > > +#define MT6370_IRQ_CHG_TREG 4 > > +#define MT6370_IRQ_CHG_AICR 5 > > +#define MT6370_IRQ_CHG_MIVR 6 > > +#define MT6370_IRQ_PWR_RDY 7 > > +#define MT6370_IRQ_FL_CHG_VINOVP 11 > > +#define MT6370_IRQ_CHG_VSYSUV 12 > > +#define MT6370_IRQ_CHG_VSYSOV 13 > > +#define MT6370_IRQ_CHG_VBATOV 14 > > +#define MT6370_IRQ_CHG_VINOVPCHG 15 > > +#define MT6370_IRQ_TS_BAT_COLD 20 > > +#define MT6370_IRQ_TS_BAT_COOL 21 > > +#define MT6370_IRQ_TS_BAT_WARM 22 > > +#define MT6370_IRQ_TS_BAT_HOT 23 > > +#define MT6370_IRQ_TS_STATC 24 > > +#define MT6370_IRQ_CHG_FAULT 25 > > +#define MT6370_IRQ_CHG_STATC 26 > > +#define MT6370_IRQ_CHG_TMR 27 > > +#define MT6370_IRQ_CHG_BATABS 28 > > +#define MT6370_IRQ_CHG_ADPBAD 29 > > +#define MT6370_IRQ_CHG_RVP 30 > > +#define MT6370_IRQ_TSHUTDOWN 31 > > +#define MT6370_IRQ_CHG_IINMEAS 32 > > +#define MT6370_IRQ_CHG_ICCMEAS 33 > > +#define MT6370_IRQ_CHGDET_DONE 34 > > +#define MT6370_IRQ_WDTMR 35 > > +#define MT6370_IRQ_SSFINISH 36 > > +#define MT6370_IRQ_CHG_RECHG 37 > > +#define MT6370_IRQ_CHG_TERM 38 > > +#define MT6370_IRQ_CHG_IEOC 39 > > +#define MT6370_IRQ_ADC_DONE 40 > > +#define MT6370_IRQ_PUMPX_DONE 41 > > +#define MT6370_IRQ_BST_BATUV 45 > > +#define MT6370_IRQ_BST_MIDOV 46 > > +#define MT6370_IRQ_BST_OLP 47 > > +#define MT6370_IRQ_ATTACH 48 > > +#define MT6370_IRQ_DETACH 49 > > +#define MT6370_IRQ_HVDCP_STPDONE 51 > > +#define MT6370_IRQ_HVDCP_VBUSDET_DONE 52 > > +#define MT6370_IRQ_HVDCP_DET 53 > > +#define MT6370_IRQ_CHGDET 54 > > +#define MT6370_IRQ_DCDT 55 > > +#define MT6370_IRQ_DIRCHG_VGOK 59 > > +#define MT6370_IRQ_DIRCHG_WDTMR 60 > > +#define MT6370_IRQ_DIRCHG_UC 61 > > +#define MT6370_IRQ_DIRCHG_OC 62 > > +#define MT6370_IRQ_DIRCHG_OV 63 > > +#define MT6370_IRQ_OVPCTRL_SWON 67 > > +#define MT6370_IRQ_OVPCTRL_UVP_D 68 > > +#define MT6370_IRQ_OVPCTRL_UVP 69 > > +#define MT6370_IRQ_OVPCTRL_OVP_D 70 > > +#define MT6370_IRQ_OVPCTRL_OVP 71 > > +#define MT6370_IRQ_FLED_STRBPIN 72 > > +#define MT6370_IRQ_FLED_TORPIN 73 > > +#define MT6370_IRQ_FLED_TX 74 > > +#define MT6370_IRQ_FLED_LVF 75 > > +#define MT6370_IRQ_FLED2_SHORT 78 > > +#define MT6370_IRQ_FLED1_SHORT 79 > > +#define MT6370_IRQ_FLED2_STRB 80 > > +#define MT6370_IRQ_FLED1_STRB 81 > > +#define mT6370_IRQ_FLED2_STRB_TO 82 > > +#define MT6370_IRQ_FLED1_STRB_TO 83 > > +#define MT6370_IRQ_FLED2_TOR 84 > > +#define MT6370_IRQ_FLED1_TOR 85 > > +#define MT6370_IRQ_OTP 93 > > +#define MT6370_IRQ_VDDA_OVP 94 > > +#define MT6370_IRQ_VDDA_UV 95 > > +#define MT6370_IRQ_LDO_OC 103 > > +#define MT6370_IRQ_BLED_OCP 118 > > +#define MT6370_IRQ_BLED_OVP 119 > > +#define MT6370_IRQ_DSV_VNEG_OCP 123 > > +#define MT6370_IRQ_DSV_VPOS_OCP 124 > > +#define MT6370_IRQ_DSV_BST_OCP 125 > > +#define MT6370_IRQ_DSV_VNEG_SCP 126 > > +#define MT6370_IRQ_DSV_VPOS_SCP 127 > > Can you pop these into a header file please? We have already popped them into "mt6370.h" in the v4 patch. > > > +struct mt6370_info { > > + struct i2c_client *i2c[MT6370_MAX_I2C]; > > + struct device *dev; > > You don't need both 'i2c' and 'dev'. > > You can derive one from the other. > > > + struct regmap *regmap; > > + struct regmap_irq_chip_data *irq_data; > > +}; > > This can do into the header file too. > > > +static const struct regmap_irq mt6370_irqs[] = { > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHGON, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_TREG, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_AICR, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_MIVR, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_PWR_RDY, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FL_CHG_VINOVP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VSYSUV, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VSYSOV, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VBATOV, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_VINOVPCHG, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_COLD, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_COOL, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_WARM, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_BAT_HOT, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_TS_STATC, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_FAULT, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_STATC, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_TMR, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_BATABS, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_ADPBAD, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_RVP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_TSHUTDOWN, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_IINMEAS, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_ICCMEAS, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHGDET_DONE, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_WDTMR, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_SSFINISH, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_RECHG, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_TERM, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHG_IEOC, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_ADC_DONE, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_PUMPX_DONE, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_BST_BATUV, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_BST_MIDOV, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_BST_OLP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_ATTACH, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DETACH, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_HVDCP_STPDONE, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_HVDCP_VBUSDET_DONE, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_HVDCP_DET, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_CHGDET, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DCDT, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_VGOK, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_WDTMR, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_UC, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_OC, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DIRCHG_OV, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_SWON, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_UVP_D, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_UVP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_OVP_D, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_OVPCTRL_OVP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_STRBPIN, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_TORPIN, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_TX, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED_LVF, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED2_SHORT, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_SHORT, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED2_STRB, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_STRB, 8), > > + REGMAP_IRQ_REG_LINE(mT6370_IRQ_FLED2_STRB_TO, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_STRB_TO, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED2_TOR, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_FLED1_TOR, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_OTP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_VDDA_OVP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_VDDA_UV, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_LDO_OC, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_BLED_OCP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_BLED_OVP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VNEG_OCP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VPOS_OCP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_BST_OCP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VNEG_SCP, 8), > > + REGMAP_IRQ_REG_LINE(MT6370_IRQ_DSV_VPOS_SCP, 8) > > +}; > > + > > +static const struct regmap_irq_chip mt6370_irq_chip = { > > + .name = "mt6370-irqs", > > + .status_base = MT6370_REG_CHG_IRQ1, > > + .mask_base = MT6370_REG_CHG_MASK1, > > + .num_regs = MT6370_NUM_IRQREGS, > > + .irqs = mt6370_irqs, > > + .num_irqs = ARRAY_SIZE(mt6370_irqs), > > +}; > > + > > +static const struct resource mt6370_regulator_irqs[] = { > > + DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VPOS_SCP, "db_vpos_scp"), > > + DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VNEG_SCP, "db_vneg_scp"), > > + DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_BST_OCP, "db_vbst_ocp"), > > + DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VPOS_OCP, "db_vpos_ocp"), > > + DEFINE_RES_IRQ_NAMED(MT6370_IRQ_DSV_VNEG_OCP, "db_vneg_ocp"), > > + DEFINE_RES_IRQ_NAMED(MT6370_IRQ_LDO_OC, "ldo_oc") > > +}; > > + > > +static const struct mfd_cell mt6370_devices[] = { > > + MFD_CELL_OF("adc", NULL, NULL, 0, 0, "mediatek,mt6370-adc"), > > + MFD_CELL_OF("charger", NULL, NULL, 0, 0, "mediatek,mt6370-charger"), > > + MFD_CELL_OF("backlight", NULL, NULL, 0, 0, "mediatek,mt6370-backlight"), > > + MFD_CELL_OF("flashlight", NULL, NULL, 0, 0, "mediatek,mt6370-flashlight"), > > + MFD_CELL_OF("indicator", NULL, NULL, 0, 0, "mediatek,mt6370-indicator"), > > + MFD_CELL_OF("tcpc", NULL, NULL, 0, 0, "mediatek,mt6370-tcpc"), > > + MFD_CELL_RES("regulator", mt6370_regulator_irqs) > > The first parameters here should be prepended with something, perhaps > "mt6370_"? OK, we will add the prefix in the next patch. > > > +}; > > + > > +static int mt6370_check_vendor_info(struct mt6370_info *info) > > +{ > > + unsigned int devinfo; > > + int ret; > > + > > + ret = regmap_read(info->regmap, MT6370_REG_DEV_INFO, &devinfo); > > + if (ret) > > + return ret; > > + > > + switch (FIELD_GET(MT6370_VENID_MASK, devinfo)) { > > + case MT6370_VENID_RT5081: > > + case MT6370_VENID_RT5081A: > > + case MT6370_VENID_MT6370: > > + case MT6370_VENID_MT6371: > > + case MT6370_VENID_MT6372P: > > + case MT6370_VENID_MT6372CP: > > + break; > > + default: > > + dev_err(info->dev, "Unknown Vendor ID 0x%02x\n", devinfo); > > + return -ENODEV; > > + } > > + > > + return 0; > > +} > > + > > +static int mt6370_regmap_read(void *context, const void *reg_buf, > > + size_t reg_size, void *val_buf, size_t val_size) > > +{ > > + struct mt6370_info *info = context; > > + u8 bank_idx, bank_addr; > > + int ret; > > + > > + bank_idx = *(u8 *)reg_buf; > > + bank_addr = *(u8 *)(reg_buf + 1); > > + > > + ret = i2c_smbus_read_i2c_block_data(info->i2c[bank_idx], bank_addr, > > + val_size, val_buf); > > + if (ret < 0) > > + return ret; > > + else if (ret != val_size) > > + return -EIO; > > + > > + return 0; > > +} > > + > > +static int mt6370_regmap_write(void *context, const void *data, size_t count) > > +{ > > + struct mt6370_info *info = context; > > + u8 bank_idx, bank_addr; > > + int len = count - MT6370_REG_ADDRLEN; > > + > > + bank_idx = *(u8 *)data; > > + bank_addr = *(u8 *)(data + 1); > > + > > + return i2c_smbus_write_i2c_block_data(info->i2c[bank_idx], bank_addr, > > + len, data + MT6370_REG_ADDRLEN); > > +} > > + > > +static const struct regmap_bus mt6370_regmap_bus = { > > + .read = mt6370_regmap_read, > > + .write = mt6370_regmap_write, > > +}; > > + > > +static const struct regmap_config mt6370_regmap_config = { > > + .reg_bits = 16, > > + .val_bits = 8, > > + .reg_format_endian = REGMAP_ENDIAN_BIG, > > + .max_register = MT6370_REG_MAXADDR, > > +}; > > + > > +static int mt6370_probe(struct i2c_client *i2c) > > +{ > > + struct mt6370_info *info; > > + struct i2c_client *usbc_i2c; > > + int ret; > > + > > + info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL); > > + if (!info) > > + return -ENOMEM; > > + > > + info->dev = &i2c->dev; > > + > > + usbc_i2c = devm_i2c_new_dummy_device(&i2c->dev, i2c->adapter, > > + MT6370_USBC_I2CADDR); > > + if (IS_ERR(usbc_i2c)) > > + return dev_err_probe(&i2c->dev, PTR_ERR(usbc_i2c), > > + "Failed to register USBC I2C client\n"); > > + > > + /* Assign I2C client for PMU and TypeC */ > > + info->i2c[MT6370_PMU_I2C] = i2c; > > + info->i2c[MT6370_USBC_I2C] = usbc_i2c; > > + > > + info->regmap = devm_regmap_init(&i2c->dev, &mt6370_regmap_bus, info, > > + &mt6370_regmap_config); > > Apart from in mt6370_check_vendor_info() where is this actually used? Well... from my understanding, we use this MFD driver to make other drivers of MT6370 (e.g. charger, ADC, led...) use the same regmap settings. Thus, this regmap is not only used in mt6370_check_vendor_info(). > > > + if (IS_ERR(info->regmap)) > > + return dev_err_probe(&i2c->dev, PTR_ERR(info->regmap), > > + "Failed to register regmap\n"); > > + > > + ret = mt6370_check_vendor_info(info); > > + if (ret) > > + return dev_err_probe(&i2c->dev, ret, > > + "Failed to check vendor info\n"); > > + > > + ret = devm_regmap_add_irq_chip(&i2c->dev, info->regmap, i2c->irq, > > + IRQF_ONESHOT, -1, &mt6370_irq_chip, > > + &info->irq_data); > > + if (ret) > > + return dev_err_probe(&i2c->dev, ret, > > + "Failed to add irq chip\n"); > > + > > + return devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, > > + mt6370_devices, ARRAY_SIZE(mt6370_devices), > > + NULL, 0, > > + regmap_irq_get_domain(info->irq_data)); > > +} > > + > > +static const struct of_device_id mt6370_match_table[] = { > > + { .compatible = "mediatek,mt6370", }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, mt6370_match_table); > > + > > +static struct i2c_driver mt6370_driver = { > > + .driver = { > > + .name = "mt6370", > > + .of_match_table = mt6370_match_table, > > + }, > > + .probe_new = mt6370_probe, > > +}; > > +module_i2c_driver(mt6370_driver); > > + > > +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>"); > > +MODULE_DESCRIPTION("MT6370 I2C Driver"); > > This is not an I2C driver. > > > +MODULE_LICENSE("GPL v2"); > > -- > Lee Jones [李琼斯] > Principal Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog Best regards, ChiaEn Wu
On Wed, 13 Jul 2022, ChiaEn Wu wrote: > Hi Lee, > > Thanks for your reply. > > Lee Jones <lee.jones@linaro.org> 於 2022年7月12日 週二 晚上11:29寫道： > > > > On Thu, 23 Jun 2022, ChiaEn Wu wrote: > > > > > From: ChiYuan Huang <cy_huang@richtek.com> > > > > > > Add Mediatek MT6370 MFD support. > > > > No such thing as "MFD support". > > > > And you're not getting away with submitting a 370 line patch with a 5 > > word change log either. :) > > > > Please at least tell us what the device is and what it's used for. > > I sincerely apologize. > We will add more descriptions of the MT6370 feature in the v5 patch. > > > > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > > > --- > > > > > > v3 > > > - Refine Kconfig help text > > > - Refine error message of unknown vendor ID in > > > mt6370_check_vendor_info() > > > - Refine return value handling of mt6370_regmap_read() > > > - Refine all probe error by using dev_err_probe() > > > - Refine "bank_idx" and "bank_addr" in mt6370_regmap_read() and > > > mt6370_regmap_write() > > > - Add "#define VENID*" and drop the comments in > > > mt6370_check_vendor_info() > > > - Drop "MFD" in MODULE_DESCRIPTION() > > > --- > > > drivers/mfd/Kconfig | 13 ++ > > > drivers/mfd/Makefile | 1 + > > > drivers/mfd/mt6370.c | 358 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 372 insertions(+) > > > create mode 100644 drivers/mfd/mt6370.c [...] > > > +static int mt6370_probe(struct i2c_client *i2c) > > > +{ > > > + struct mt6370_info *info; > > > + struct i2c_client *usbc_i2c; > > > + int ret; > > > + > > > + info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL); > > > + if (!info) > > > + return -ENOMEM; > > > + > > > + info->dev = &i2c->dev; > > > + > > > + usbc_i2c = devm_i2c_new_dummy_device(&i2c->dev, i2c->adapter, > > > + MT6370_USBC_I2CADDR); > > > + if (IS_ERR(usbc_i2c)) > > > + return dev_err_probe(&i2c->dev, PTR_ERR(usbc_i2c), > > > + "Failed to register USBC I2C client\n"); > > > + > > > + /* Assign I2C client for PMU and TypeC */ > > > + info->i2c[MT6370_PMU_I2C] = i2c; > > > + info->i2c[MT6370_USBC_I2C] = usbc_i2c; > > > + > > > + info->regmap = devm_regmap_init(&i2c->dev, &mt6370_regmap_bus, info, > > > + &mt6370_regmap_config); > > > > Apart from in mt6370_check_vendor_info() where is this actually used? > > Well... from my understanding, we use this MFD driver to make other > drivers of MT6370 (e.g. charger, ADC, led...) use the same regmap > settings. > Thus, this regmap is not only used in mt6370_check_vendor_info(). Well for that to happen you need to store the data somewhere for the child devices to fetch from. I don't see that happening in this patch? What did I miss?
Hi Lee, Lee Jones <lee.jones@linaro.org> 於 2022年7月13日 週三 下午4:04寫道： > > On Wed, 13 Jul 2022, ChiaEn Wu wrote: > > > Hi Lee, > > > > Thanks for your reply. > > > > Lee Jones <lee.jones@linaro.org> 於 2022年7月12日 週二 晚上11:29寫道： > > > > > > On Thu, 23 Jun 2022, ChiaEn Wu wrote: > > > > > > > From: ChiYuan Huang <cy_huang@richtek.com> > > > > > > > > Add Mediatek MT6370 MFD support. > > > > > > No such thing as "MFD support". > > > > > > And you're not getting away with submitting a 370 line patch with a 5 > > > word change log either. :) > > > > > > Please at least tell us what the device is and what it's used for. > > > > I sincerely apologize. > > We will add more descriptions of the MT6370 feature in the v5 patch. > > > > > > > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com> > > > > --- > > > > > > > > v3 > > > > - Refine Kconfig help text > > > > - Refine error message of unknown vendor ID in > > > > mt6370_check_vendor_info() > > > > - Refine return value handling of mt6370_regmap_read() > > > > - Refine all probe error by using dev_err_probe() > > > > - Refine "bank_idx" and "bank_addr" in mt6370_regmap_read() and > > > > mt6370_regmap_write() > > > > - Add "#define VENID*" and drop the comments in > > > > mt6370_check_vendor_info() > > > > - Drop "MFD" in MODULE_DESCRIPTION() > > > > --- > > > > drivers/mfd/Kconfig | 13 ++ > > > > drivers/mfd/Makefile | 1 + > > > > drivers/mfd/mt6370.c | 358 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 372 insertions(+) > > > > create mode 100644 drivers/mfd/mt6370.c > > [...] > > > > > +static const struct mfd_cell mt6370_devices[] = { > > > > + MFD_CELL_OF("adc", NULL, NULL, 0, 0, "mediatek,mt6370-adc"), > > > > + MFD_CELL_OF("charger", NULL, NULL, 0, 0, "mediatek,mt6370-charger"), > > > > + MFD_CELL_OF("backlight", NULL, NULL, 0, 0, "mediatek,mt6370-backlight"), > > > > + MFD_CELL_OF("flashlight", NULL, NULL, 0, 0, "mediatek,mt6370-flashlight"), > > > > + MFD_CELL_OF("indicator", NULL, NULL, 0, 0, "mediatek,mt6370-indicator"), > > > > + MFD_CELL_OF("tcpc", NULL, NULL, 0, 0, "mediatek,mt6370-tcpc"), > > > > + MFD_CELL_RES("regulator", mt6370_regulator_irqs) > > > > > > The first parameters here should be prepended with something, perhaps > > > "mt6370_"? > > > OK, we will add the prefix in the next patch. Sorry, I forgot to ask a question in the last mail. I wonder if using "mt6370-xxx" (dash) is better than using "mt6370_" (underline) ?? Thanks. > [...] > > > > > +static int mt6370_probe(struct i2c_client *i2c) > > > > +{ > > > > + struct mt6370_info *info; > > > > + struct i2c_client *usbc_i2c; > > > > + int ret; > > > > + > > > > + info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL); > > > > + if (!info) > > > > + return -ENOMEM; > > > > + > > > > + info->dev = &i2c->dev; > > > > + > > > > + usbc_i2c = devm_i2c_new_dummy_device(&i2c->dev, i2c->adapter, > > > > + MT6370_USBC_I2CADDR); > > > > + if (IS_ERR(usbc_i2c)) > > > > + return dev_err_probe(&i2c->dev, PTR_ERR(usbc_i2c), > > > > + "Failed to register USBC I2C client\n"); > > > > + > > > > + /* Assign I2C client for PMU and TypeC */ > > > > + info->i2c[MT6370_PMU_I2C] = i2c; > > > > + info->i2c[MT6370_USBC_I2C] = usbc_i2c; > > > > + > > > > + info->regmap = devm_regmap_init(&i2c->dev, &mt6370_regmap_bus, info, > > > > + &mt6370_regmap_config); > > > > > > Apart from in mt6370_check_vendor_info() where is this actually used? > > > > Well... from my understanding, we use this MFD driver to make other > > drivers of MT6370 (e.g. charger, ADC, led...) use the same regmap > > settings. > > Thus, this regmap is not only used in mt6370_check_vendor_info(). > > Well for that to happen you need to store the data somewhere for the > child devices to fetch from. I don't see that happening in this > patch? What did I miss? hmmm... I got your point... I will let regmap be a local var in probe() in the next patch. Thank you so much! > > -- > Lee Jones [李琼斯] > Principal Technical Lead - Developer Services > Linaro.org │ Open source software for Arm SoCs > Follow Linaro: Facebook | Twitter | Blog Best regards, ChiaEn Wu
From: ChiaEn Wu <chiaen_wu@richtek.com> This patch series add Mediatek MT6370 PMIC support. The MT6370 is a highly-integrated smart power management IC, which includes a single cell Li-Ion/Li-Polymer switching battery charger, a USB Type-C & Power Delivery (PD) controller, dual Flash LED current sources, a RGB LED driver, a backlight WLED driver, a display bias driver and a general LDO for portable devices. Among with this we took some changes and refined the device tree files to comply with DT specifications. Thank you, ChiaEn Wu --- Changes in v3: - Remove ADC ABI file, which is added in v2 Patch 7 - In patch 02/14: - Add items and remove maxItems of io-channels - Add io-channel-names and describe each item - Add "unevaluatedProperties: false" in "usb-otg-vbus-regulator" - Rename "enable-gpio" to "enable-gpios" in "usb-otg-vbus-regulator" - In patch 03/14: - Use leds-class-multicolor.yaml instead of common.yaml. - Split multi-led and led node. - Add subdevice "led" in "multi-led". - In patch 04/14: - Remove the description of enum. - In patch 05/14: - Rename "mediatek,bled-pwm-hys-input-threshold-steps" to "mediatek,bled-pwm-hys-input-th-steps" - Refine "bled-pwm-hys-input-th-steps", "bled-ovp-microvolt", "bled-ocp-microamp" enum values - In patch 06/14: - Use " in entire patchset - Refine ADC description - Rename "enable-gpio" to "enable-gpios" in "regualtor" - Change "/schemas/" to "../" in every reference of all MT6370 modules - In patch 07/14: - Refine Kconfig help text - Refine error message of unknown vendor ID in mt6370_check_vendor_info() - Refine return value handling of mt6370_regmap_read() - Refine all probe error by using dev_err_probe() - Refine "bank_idx" and "bank_addr" in mt6370_regmap_read() and mt6370_regmap_write() - Add "#define VENID*" and drop the comments in mt6370_check_vendor_info() - Drop "MFD" in MODULE_DESCRIPTION() - In patch 09/14: - Refine Kconfig help text - In patch 10/14: - Refine Kconfig help text - Refine all channel value in read_scale() a. current: uA --> mA b. voltage: uV --> mV c. temperature: degrees Celsius --> milli degrees Celsius - Add "default:" condition of switch statement in read_scale() and read_raw() - Add error message for reading ADC register failed - Add the comment for adc_lock - Add <linux/mod_devicetable.h> header file for struct of_device_id - Replace "adc" text with "ADC" in all of the error messages - In patch 12/14: - Refine the grammer of the Kconfig. - Change reg mode to the const current mode. - In patch 14/14: - Refine bool properties parsing (pwm-enable, ovp-shutdown, ocp-shutdown) in DT parsing function - Refine u32 and u8 properties parsing (pwm-hys-input-th-steps, ovp-microvolt, ocp-microamp), from using register value to using actual value - Refine error string of "channle-use" parsing failed - Refine Kconfig help text Changes in v2: - In patch 01/15: - Add "unevaluatedProperties: false". - Delete "DT bindings". - Refine the description to fit in 80 columns. - Skip the connector description. - In patch 02/15: - Refine items description of interrupt-name - Rename "usb-otg-vbus" to "usb-otg-vbus-regulator" - Add constraint properties for ADC - In patch 03/15: - Skip not useful description of "^(multi-)?led@[0-3]$" and reg. - Due to the dependency, remove the mention of mfd document directory. - Delete Soft-start property. In design aspect, we think soft-restart should always be enabled, our new chip has deleted the related setting register , also, we don’t allow user adjust this parameter in this chip. - Refine the commit message. - In patch 04/15: - Skip not useful description of "^led@[0-1]$" and reg. - Add apace after '#'. - Refine the commit message. - In patch 05/15: - Remove "binding documentation" in subject title - Refine description of mt6370 backlight binding document - Refine properties name(bled-pwm-hys-input-bit, bled-ovp-microvolt, bled-ocp-microamp) and their description - In patch 06/15: - Refine ADC and Regulator descriptions - Refine include header usage in example - Refine node name to generic node name("pmic@34") - Refine led example indentation - Refine license of mediatek,mt6370_adc.h - Rename the dts example from IRQ define to number. - Remove mediatek,mt6370.h - In patch 07/15: - Add ABI documentation for mt6370 non-standard ADC sysfs interfaces. - In patch 08/15: - Add all IRQ define into mt6370.c. - Refine include header usage - In patch 09/15: - No changes. - In patch 10/15: - Use 'gpiod_get_from_of_node' to replace 'fwnode_gpiod_get_index'. - In patch 11/15: - Refine Kconfig mt6370 help text - Refine mask&shift to FIELD_PREP() - Refine mutex lock name ("lock" -> "adc_lock") - Refine mt6370_adc_read_scale() - Refine mt6370_adc_read_offset() - Refine mt6370_channel_labels[] by using enum to index chan spec - Refine MT6370_ADC_CHAN() - Refine indio_dev->name - Remove useless include header files - In patch 12/15: - Refine mt6370_chg_otg_rdesc.of_match ("mt6370,otg-vbus" -> "usb-otg-vbus-regulator") to match DT binding - In patch 13/15: - Refine Kconfig description. - Remove include "linux/of.h" and use "linux/mod_devicetable.h". - Place a comma for the last element of the const unsigned int array. - Add a comment line for the mutex 'lock'. - In probe function, use 'dev_err_probe' in some judgement to reduce the LOC. - Refine include header usage. BIT/GENMASK -> linux/bits.h FIELD_GET -> linux/bitfield.h - In patch 14/15: - Add blank line. - Replace container_of() with to_mt6370_led() . - Refine description of ramping. - Refine the mt6370_init_common_properties function. - Refine the probe return. - In patch 15/15: - Refine MT6370 help text in Kconfig - Refine DT Parse function - Remove useless enum - Add comment for 6372 backward compatible in bl_update_status() and check_vendor_info() - Using dev_err_probe(); insteads dev_err()&return; in the probe() Alice Chen (2): dt-bindings: leds: Add Mediatek MT6370 flashlight leds: flashlight: mt6370: Add Mediatek MT6370 flashlight support ChiYuan Huang (8): dt-bindings: usb: Add Mediatek MT6370 TCPC dt-bindings: leds: mt6370: Add Mediatek mt6370 current sink type LED indicator dt-bindings: backlight: Add Mediatek MT6370 backlight dt-bindings: mfd: Add Mediatek MT6370 mfd: mt6370: Add Mediatek MT6370 support usb: typec: tcpci_mt6370: Add Mediatek MT6370 tcpci driver regulator: mt6370: Add mt6370 DisplayBias and VibLDO support leds: mt6370: Add Mediatek MT6370 current sink type LED Indicator support ChiaEn Wu (4): dt-bindings: power: supply: Add Mediatek MT6370 Charger iio: adc: mt6370: Add Mediatek MT6370 support power: supply: mt6370: Add Mediatek MT6370 charger driver video: backlight: mt6370: Add Mediatek MT6370 support .../leds/backlight/mediatek,mt6370-backlight.yaml | 92 ++ .../bindings/leds/mediatek,mt6370-flashlight.yaml | 41 + .../bindings/leds/mediatek,mt6370-indicator.yaml | 77 ++ .../devicetree/bindings/mfd/mediatek,mt6370.yaml | 280 +++++ .../power/supply/mediatek,mt6370-charger.yaml | 87 ++ .../bindings/usb/mediatek,mt6370-tcpc.yaml | 36 + drivers/iio/adc/Kconfig | 9 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/mt6370-adc.c | 275 +++++ drivers/leds/Kconfig | 11 + drivers/leds/Makefile | 1 + drivers/leds/flash/Kconfig | 9 + drivers/leds/flash/Makefile | 1 + drivers/leds/flash/leds-mt6370-flash.c | 657 ++++++++++++ drivers/leds/leds-mt6370.c | 989 +++++++++++++++++ drivers/mfd/Kconfig | 13 + drivers/mfd/Makefile | 1 + drivers/mfd/mt6370.c | 358 +++++++ drivers/power/supply/Kconfig | 11 + drivers/power/supply/Makefile | 1 + drivers/power/supply/mt6370-charger.c | 1132 ++++++++++++++++++++ drivers/regulator/Kconfig | 8 + drivers/regulator/Makefile | 1 + drivers/regulator/mt6370-regulator.c | 388 +++++++ drivers/usb/typec/tcpm/Kconfig | 8 + drivers/usb/typec/tcpm/Makefile | 1 + drivers/usb/typec/tcpm/tcpci_mt6370.c | 212 ++++ drivers/video/backlight/Kconfig | 9 + drivers/video/backlight/Makefile | 1 + drivers/video/backlight/mt6370-backlight.c | 346 ++++++ include/dt-bindings/iio/adc/mediatek,mt6370_adc.h | 18 + 31 files changed, 5074 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/backlight/mediatek,mt6370-backlight.yaml create mode 100644 Documentation/devicetree/bindings/leds/mediatek,mt6370-flashlight.yaml create mode 100644 Documentation/devicetree/bindings/leds/mediatek,mt6370-indicator.yaml create mode 100644 Documentation/devicetree/bindings/mfd/mediatek,mt6370.yaml create mode 100644 Documentation/devicetree/bindings/power/supply/mediatek,mt6370-charger.yaml create mode 100644 Documentation/devicetree/bindings/usb/mediatek,mt6370-tcpc.yaml create mode 100644 drivers/iio/adc/mt6370-adc.c create mode 100644 drivers/leds/flash/leds-mt6370-flash.c create mode 100644 drivers/leds/leds-mt6370.c create mode 100644 drivers/mfd/mt6370.c create mode 100644 drivers/power/supply/mt6370-charger.c create mode 100644 drivers/regulator/mt6370-regulator.c create mode 100644 drivers/usb/typec/tcpm/tcpci_mt6370.c create mode 100644 drivers/video/backlight/mt6370-backlight.c create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6370_adc.h