lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 20 Mar 2017 19:08:22 +0100 From: Hans de Goede <hdegoede@...hat.com> To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, "Rafael J . Wysocki" <rjw@...ysocki.net>, Len Brown <lenb@...nel.org>, Wolfram Sang <wsa@...-dreams.de>, Lee Jones <lee.jones@...aro.org>, Sebastian Reichel <sre@...nel.org>, MyungJoo Ham <myungjoo.ham@...sung.com>, Chanwoo Choi <cw00.choi@...sung.com> Cc: linux-acpi@...r.kernel.org, Takashi Iwai <tiwai@...e.de>, linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, Felipe Balbi <felipe.balbi@...ux.intel.com> Subject: Re: [PATCH 03/15] extcon: cht-wc: Add Intel Cherry Trail Whiskey Cove PMIC extcon driver Hi, On 17-03-17 18:18, Andy Shevchenko wrote: > On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote: >> Add a driver for charger detection / control on the Intel Cherrytrail >> Whiskey Cove PMIC. > > +Cc: Felipe for some question(s) below. > >> drivers/extcon/extcon-cht-wc.c | 356 > > I would use same pattern across drivers, i.e. "chtwc" (same for the rest > of the drivers in this series). I already answered this in another part of the thread, but for the archives sake let me copy and paste my answer: "One thing I disagree with is the cht_wc > chtwc rename you are proposing for one Cherry Trail and Whiskey Cove are 2 different words (4 if you look at spelling but 2 if you look at pronunciation, so IMHO cht_wc is more readable other then that I see the suggested rename as a lot of extra churn without any tangible benefits." > >> +#define CHT_WC_PWRSRC_IRQ 0x6e03 >> +#define CHT_WC_PWRSRC_IRQ_MASK 0x6e0f >> +#define CHT_WC_PWRSRC_STS 0x6e1e >> +#define CHT_WC_PWRSRC_VBUS BIT(0) >> +#define CHT_WC_PWRSRC_DC BIT(1) >> +#define CHT_WC_PWRSRC_BAT BIT(2) >> +#define CHT_WC_PWRSRC_ID_GND BIT(3) >> +#define CHT_WC_PWRSRC_ID_FLOAT BIT(4) > > Not obvious for which register those bit definitions are. They are for all 3 CHT_WC_PWRSRC_* registers, this is the more or less usual irq setup for some i2c devices where there is a status register, an irq register where the hardware sets bits to 1 (and we write 1 to clear) when the corresponding status bits changes and a mask register to select which irq register bits actually will raise the interrupt line. > Also, keep them ordered by offset. Will fix. > >> + >> +#define CHT_WC_PHYCTRL 0x5e07 >> + > >> +#define CHT_WC_CHGRCTRL0 0x5e16 > > Dup! Good catch, will fix. > >> + >> +#define CHT_WC_CHGRCTRL0 0x5e16 > >> +static int cht_wc_extcon_get_charger(struct cht_wc_extcon_data *ext) >> +{ >> + int ret, usbsrc, status, retries = 5; >> + >> + do { >> + ret = regmap_read(ext->regmap, CHT_WC_USBSRC, >> &usbsrc); >> + if (ret) { >> + dev_err(ext->dev, "Error reading usbsrc: >> %d\n", ret); >> + return ret; >> + } >> + status = usbsrc & CHT_WC_USBSRC_STS_MASK; >> + if (status == CHT_WC_USBSRC_STS_SUCCESS || >> + status == CHT_WC_USBSRC_STS_FAIL) >> + break; >> + > >> + msleep(200); > > Comment why and why so long? Fixed for v2 (actually switched to using jiffies + time_before for a more accurate timeout). >> + } while (retries--); > >> +static void cht_wc_extcon_det_event(struct cht_wc_extcon_data *ext) > > det -> detect ? Renamed to more accurate cht_wc_extcon_pwrsrc_event > +static irqreturn_t cht_wc_extcon_isr(int irq, void *data) >> +{ >> + struct cht_wc_extcon_data *ext = data; >> + int ret, irqs; >> + >> + ret = regmap_read(ext->regmap, CHT_WC_PWRSRC_IRQ, &irqs); >> + if (ret) >> + dev_err(ext->dev, "Error reading irqs: %d\n", ret); > > Shouldn't we return IRQ_NONE here? I was wondering the same myself here, there is no good answer here, this should simply never fail ... which does make returning IRQ_NONE a good idea, I was worried that would lead to a "nobody cared, disabling irq", but as said this should never happen, so when it does, disabling the irq is probably for the best. Will fix. > >> + >> + cht_wc_extcon_det_event(ext); >> + >> + ret = regmap_write(ext->regmap, CHT_WC_PWRSRC_IRQ, irqs); >> + if (ret) >> + dev_err(ext->dev, "Error writing irqs: %d\n", ret); >> + >> + return IRQ_HANDLED; >> +} >> + > >> +/* usb_id sysfs attribute for debug / testing purposes */ > > Hmm... I would use debugfs for debug, otherwise it looks like it should > be framework (extcon) wide. Unfortunately these kinda sysfs files are somewhat normal when dealing with USB-OTG. For example my board does not have the id-pin hooked up to the connector, so to test host mode I need to echo "gnd" to the sysfs attr. But also if I actually want to use host-mode (or anyone else with the same or a similar board). This definitely is not something which belongs in the extcon-core. > Perhaps Felipe can advise something here. > >> +static int cht_wc_extcon_probe(struct platform_device *pdev) >> +{ > >> + struct cht_wc_extcon_data *ext; >> + struct intel_soc_pmic *pmic = dev_get_drvdata(pdev- >>> dev.parent); > > Exchange them (assignment first). Fixed. > >> + int irq, ret; >> + > > >> + ret = devm_request_threaded_irq(ext->dev, irq, NULL, >> cht_wc_extcon_isr, >> + IRQF_ONESHOT, pdev->name, >> ext); >> + if (ret) { >> + dev_err(ext->dev, "Failed to request interrupt\n"); >> + return ret; >> + } >> + >> + /* Unmask irqs */ >> + ret = regmap_write(ext->regmap, CHT_WC_PWRSRC_IRQ_MASK, >> + (int)~(CHT_WC_PWRSRC_VBUS | > > Hmm... Do you need explicit casting here? Yes because the BIT(x) macros are unsigned longs and the ~ sets the MSB so then the compiler complaints about truncating the variable without the cast. > >> CHT_WC_PWRSRC_ID_GND | >> + CHT_WC_PWRSRC_ID_FLOAT)); >> + if (ret) { >> + dev_err(ext->dev, "Error writing irq-mask: %d\n", >> ret); >> + return ret; >> + } > Regards, Hans
Powered by blists - more mailing lists