| 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: Fri, 17 Mar 2017 20:22:12 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Hans de Goede <hdegoede@...hat.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
Subject: Re: [PATCH 15/15] i2c-cht-wc: Add Intel Cherry Trail Whiskey Cove
SMBUS controller driver
On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote:
> The Intel Cherry Trail Whiskey Cove PMIC has a builtin SMBUS
> controller
> for talking to an external PMIC. Add a driver for this.
Looking to all this mess we have with PMICs, perhaps some file under
Documentation to explain all those dependencies with nice ASCII flow
charts would be created.
> drivers/i2c/busses/i2c-cht-wc.c | 338
File name: i2c-chtwc.c
> +#include <linux/power/cht_wc_fuel_gauge.h>
Perhaps chtwc_fuel_gauge or intel_chtwc_fuel_gauge.h (see previous patch
for comment regarding naming).
> +#include <linux/slab.h>
> +
> +#define CHT_WC_I2C_CTRL 0x5e24
> +#define CHT_WC_I2C_CTRL_WR BIT(0)
> +#define CHT_WC_I2C_CTRL_RD BIT(1)
> +#define CHT_WC_I2C_CLIENT_ADDR 0x5e25
> +#define CHT_WC_I2C_REG_OFFSET 0x5e26
> +#define CHT_WC_I2C_WRDATA 0x5e27
> +#define CHT_WC_I2C_RDDATA 0x5e28
> +
> +#define CHT_WC_EXTCHGRIRQ 0x6e0a
> +#define CHT_WC_EXTCHGRIRQ_CLIENT_IRQ BIT(0)
> +#define CHT_WC_EXTCHGRIRQ_WRITE_IRQ BIT(1)
> +#define CHT_WC_EXTCHGRIRQ_READ_IRQ BIT(2)
> +#define CHT_WC_EXTCHGRIRQ_NACK_IRQ BIT(3)
>
> +#define CHT_WC_EXTCHGRIRQ_ADAP_IRQS ((u8)(BIT(1) | BIT(2) |
> BIT(3)))
_IRQ_MASK ?
GENMASK() ?
> +#define CHT_WC_EXTCHGRIRQ_MSK 0x6e17
> +struct cht_wc_i2c_adap {
> + struct i2c_adapter adapter;
> + wait_queue_head_t wait;
> + struct irq_chip irqchip;
> + struct mutex irqchip_lock;
> + struct regmap *regmap;
> + struct irq_domain *irq_domain;
> + struct i2c_client *client;
> + int client_irq;
> + u8 irq_mask;
> + u8 old_irq_mask;
> + bool nack;
> + bool done;
> +};
> +
> +static irqreturn_t cht_wc_i2c_adap_thread_handler(int id, void *data)
> +{
> + struct cht_wc_i2c_adap *adap = data;
> + int ret, reg;
> +
> + /* Read irqs */
IRQs
> + ret = regmap_read(adap->regmap, CHT_WC_EXTCHGRIRQ, ®);
> + if (ret) {
> + dev_err(&adap->adapter.dev, "Error reading extchgrirq
> reg\n");
> + return IRQ_NONE;
> + }
> +
> + reg &= ~adap->irq_mask;
> +
> + /*
> + * Immediately ack irqs, so that if new irqs arrives while
> we're
> + * handling the previous ones our irq will re-trigger when
> we're done.
> + */
> + ret = regmap_write(adap->regmap, CHT_WC_EXTCHGRIRQ, reg);
> + if (ret)
> + dev_err(&adap->adapter.dev, "Error writing extchgrirq
> reg\n");
> +
> + /*
> + * Do NOT use handle_nested_irq here, the client irq handler
> will
> + * likely want to do i2c transfers and the i2c controller
> uses this
> + * interrupt handler as well, so running the client irq
> handler from
> + * this thread will cause things to lock up.
> + */
> + if (reg & CHT_WC_EXTCHGRIRQ_CLIENT_IRQ) {
> + /*
> + * generic_handle_irq expects local irqs to be
> disabled
> + * as normally it is called from interrupt context.
> + */
> + local_irq_disable();
> + generic_handle_irq(adap->client_irq);
> + local_irq_enable();
> + }
> +
> + if (reg & CHT_WC_EXTCHGRIRQ_ADAP_IRQS) {
> + if (reg & CHT_WC_EXTCHGRIRQ_NACK_IRQ)
> + adap->nack = true;
adap->nack = !!(reg & ...);
> + adap->done = true;
> + wake_up(&adap->wait);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +static u32 cht_wc_i2c_adap_master_func(struct i2c_adapter *adap)
> +{
> + /* This i2c adapter only supports smbus byte transfers */
smbus -> SMBUS
> + return I2C_FUNC_SMBUS_BYTE_DATA;
> +}
> +
> +static int cht_wc_i2c_adap_smbus_xfer(struct i2c_adapter *_adap, u16
> addr,
> + unsigned short flags, char
> read_write,
> + u8 command, int size,
> + union i2c_smbus_data *data)
> +{
> + struct cht_wc_i2c_adap *adap = i2c_get_adapdata(_adap);
> + int ret, reg;
> +
> +
> + /* 3 second timeout, during cable plug the PMIC responds
> quite slow */
> + ret = wait_event_timeout(adap->wait, adap->done, HZ * 3);
3 * HZ
> + if (ret == 0)
> + return -ETIMEDOUT;
> + if (adap->nack)
> + return -EIO;
> +
> + if (read_write == I2C_SMBUS_READ) {
> + ret = regmap_read(adap->regmap, CHT_WC_I2C_RDDATA,
> ®);
> + if (ret)
> + return ret;
> +
> + data->byte = reg;
> + }
> +
> + return 0;
> +}
> +/**** irqchip for the client connected to the extchgr i2c adapter
> ****/
Useless ?
> +static void cht_wc_i2c_irq_lock(struct irq_data *data)
> +{
> + struct cht_wc_i2c_adap *adap =
> irq_data_get_irq_chip_data(data);
> +
> + mutex_lock(&adap->irqchip_lock);
> +}
> +
> +static void cht_wc_i2c_irq_sync_unlock(struct irq_data *data)
> +{
> + struct cht_wc_i2c_adap *adap =
> irq_data_get_irq_chip_data(data);
> + int ret;
> +
> + if (adap->irq_mask != adap->old_irq_mask) {
> + ret = regmap_write(adap->regmap,
> CHT_WC_EXTCHGRIRQ_MSK,
> + adap->irq_mask);
> + if (ret == 0)
> + adap->old_irq_mask = adap->irq_mask;
> + else
> + dev_err(&adap->adapter.dev, "Error writing
> extchgrirq_msk\n");
extchgrirq_msk -> EXTCHGRIRQ_MSK ?
> + }
> +
> + mutex_unlock(&adap->irqchip_lock);
> +}
> +static const struct bq24190_platform_data bq24190_pdata = {
> + .no_register_reset = true,
> + .extcon_name = "cht_wcove_pwrsrc",
> + .get_ext_bat_property = cht_wc_fg_get_property,
> +};
> +
> +static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
> +{
> + struct intel_soc_pmic *pmic = dev_get_drvdata(pdev-
> >dev.parent);
> + struct cht_wc_i2c_adap *adap;
> + struct i2c_board_info board_info = {
> + .type = "bq24190",
> + .addr = 0x6b,
> + .platform_data = (void *)&bq24190_pdata,
> + };
> + int ret, irq;
> +
> + /* Clear and activate i2c-adapter interrupts, disable client
> irq */
irq -> IRQ
> +
> + /* Alloc and register client irq */
Ditto.
> + adap->irq_domain = irq_domain_add_linear(pdev->dev.of_node,
> 1,
> + &irq_domain_simple_o
> ps, NULL);
Can you use irq_domain_add_simple()?
And why do we need separate domain for one IRQ line?
> + if (!adap->irq_domain)
> + return -ENOMEM;
> +
> + adap->client_irq = irq_create_mapping(adap->irq_domain, 0);
> + if (!adap->client_irq) {
> + ret = -ENOMEM;
> + goto remove_irq_domain;
> + }
> +
> + irq_set_chip_data(adap->client_irq, adap);
> + irq_set_chip_and_handler(adap->client_irq, &adap->irqchip,
> + handle_simple_irq);
> +
> + board_info.irq = adap->client_irq;
> + adap->client = i2c_new_device(&adap->adapter, &board_info);
> + if (!adap->client) {
> + ret = -ENOMEM;
> + goto del_adapter;
> + }
I would split this to some other module with board info.
Does it make sense?
By the way, doesn't ACPI have the charger IC node?
--
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy
Powered by blists - more mailing lists