On Thu, 2013-11-21 at 10:34 +0000, Lee Jones wrote:
> <snip>
>
> > +/**
> > + * After resuming from suspend it may happen that IRQ is signalled but
> > + * IRQ GPIO is not high. Also the interrupt registers won't have any data
> > + * (all of them equal to 0x00).
> > + *
> > + * In such case retry few times reading the interrupt registers.
> > + */
> > +#define IRQ_READ_REG_RETRY_CNT 5
>
> Where is this used?
>
> <snip>
>
> > +#define DECLARE_IRQ(idx, _group, _mask) \
> > + [(idx)] = { .group = (_group), .mask = (_mask) }
>
> I'm pretty sure the parentheses are supurfluous.
>
> > +static const inline struct max14577_irq_data *
> > irq_to_max14577_irq(struct max14577 *max14577, int irq)
>
> We should tab this back out.
>
> > +{
> > + struct irq_data *data = irq_get_irq_data(irq);
> > + return &max14577_irqs[data->hwirq];
>
> Shouldn't you be using irq_create_mapping() or irq_find_mapping() here?
>
> > +static void max14577_irq_mask(struct irq_data *data)
> > +{
> > + struct max14577 *max14577 = irq_get_chip_data(data->irq);
>
> irq_data_get_irq_chip_data(data); ?
>
> > + const struct max14577_irq_data *irq_data =
> > + irq_to_max14577_irq(max14577, data->irq);
> > +
> > + if (!irq_data)
> > + return;
> > +
> > + if (irq_data->group >= MAX14577_IRQ_REGS_NUM)
> > + return;
> > +
> > + max14577->irq_masks_cur[irq_data->group] &= ~irq_data->mask;
> > +}
>
> <snip>
>
> > +int max14577_irq_resume(struct max14577 *max14577)
> > +{
> > + int ret = 0;
> > +
> > + if (max14577->irq && max14577->irq_domain)
> > + ret = max14577_irq_thread(0, max14577);
> > +
> > + return ret >= 0 ? 0 : ret;
>
> Please open this out to something more easily comprehensible.
Thanks for pointing these out however after using regmap_irq_chip whole
file won't be needed anymore.
(...)
> > +static int max14577_i2c_probe(struct i2c_client *i2c,
> > + const struct i2c_device_id *id)
> > +{
> > + struct max14577 *max14577;
> > + struct max14577_platform_data *pdata = dev_get_platdata(&i2c->dev);
> > + u8 reg_data;
> > + int ret = 0;
> > +
> > + if (i2c->dev.of_node) {
>
> Can you pull this out. It looks neater as the top as:
> struct device_node *np = i2c->dev.of_node;
I am not sure if I understand you correctly. You would like to change
this to:
-------------------------
static int max14577_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
struct max14577 *max14577;
struct max14577_platform_data *pdata =
dev_get_platdata(&i2c->dev);
struct device_node *np = i2c->dev.of_node;
u8 reg_data;
int ret = 0;
if (np) {
pdata = devm_kzalloc(&i2c->dev,
sizeof(struct max14577_platform_data),
GFP_KERNEL);
-------------------------
The variable 'np' would be used only once.
>
> > + pdata = devm_kzalloc(&i2c->dev,
> > + sizeof(struct max14577_platform_data),
>
> I prefer:
> sizeof(*pdata),
OK. Line will be shorter. I will change this patter also in other
places.
>
> > + GFP_KERNEL);
> > + if (!pdata)
> > + return -ENOMEM;
> > + i2c->dev.platform_data = pdata;
> > + }
> > +
> > + if (IS_ERR_OR_NULL(pdata)) {
>
> It's not likely to be an ERR, just do:
> if (!pdata) {
OK.
> > + dev_err(&i2c->dev, "No platform data found: %ld\n",
> > + PTR_ERR(pdata));
> > + return -EINVAL;
> > + }
> > +
> > + max14577 = devm_kzalloc(&i2c->dev, sizeof(struct max14577), GFP_KERNEL);
> > + if (!max14577)
> > + return -ENOMEM;
> > +
> > + i2c_set_clientdata(i2c, max14577);
> > + max14577->pdata = pdata;
>
> How many different places do you want to store this?
>
> > + max14577->dev = &i2c->dev;
>
> You're storing dev here anyway.
>
> Remove the pdata property and get it from dev when/if you require it.
OK.
> > + max14577->i2c = i2c;
> > + max14577->irq = i2c->irq;
> > +
> > + max14577->regmap = devm_regmap_init_i2c(i2c, &max14577_regmap_config);
> > + if (IS_ERR(max14577->regmap)) {
> > + ret = PTR_ERR(max14577->regmap);
> > + dev_err(max14577->dev, "Failed to allocate register map: %d\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > + ret = max14577_read_reg(max14577->regmap, MAX14577_REG_DEVICEID,
> > + ®_data);
> > + if (ret) {
> > + dev_err(max14577->dev, "Device not found on this channel: %d\n",
> > + ret);
> > + return ret;
> > + }
> > + max14577->vendor_id = (reg_data & 0x7);
> > + max14577->device_id = ((reg_data & 0xF8) >> 0x3);
> > + dev_info(max14577->dev, "Device ID: 0x%x, vendor: 0x%x\n",
> > + max14577->device_id, max14577->vendor_id);
> > +
> > + ret = max14577_irq_init(max14577);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = mfd_add_devices(max14577->dev, -1, max14577_devs,
> > + ARRAY_SIZE(max14577_devs), NULL, 0, NULL);
>
> You should be passing the irqdomain as the final parameter here.
I replaced the IRQ handling with regmap_irq_chip so this would look like
this:
-------------------------
ret = mfd_add_devices(max14577->dev, -1, max14577_devs,
ARRAY_SIZE(max14577_devs), NULL, 0,
regmap_irq_get_domain(max14577->irq_data));
-------------------------
Am I correct?
> > + if (ret < 0)
> > + goto err_mfd;
> > +
> > + device_init_wakeup(max14577->dev, 1);
> > +
> > + return 0;
> > +
> > +err_mfd:
> > + max14577_irq_exit(max14577);
> > + return ret;
> > +}
> > +
> > +static int max14577_i2c_remove(struct i2c_client *i2c)
> > +{
> > + struct max14577 *max14577 = i2c_get_clientdata(i2c);
> > +
> > + mfd_remove_devices(max14577->dev);
> > + max14577_irq_exit(max14577);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id max14577_i2c_id[] = {
> > + { MAX14577_MFD_DEV_NAME, 0 },
>
> Can you use the proper name here, these types of defines are pretty ugly.
OK.
> <snip>
>
> > +#ifdef CONFIG_OF
> > +static struct of_device_id max14577_dt_match[] = {
> > + { .compatible = "maxim,max14577", },
> > + {},
> > +};
> > +#else
> > +#define max14577_dt_match NULL
> > +#endif
>
> You don't need to do this and use of_match_ptr().
>
> Remove the #ifdef.
Yes, Mark also pointed this.
>
> > +static SIMPLE_DEV_PM_OPS(max14577_pm, max14577_suspend, max14577_resume);
> > +
> > +static struct i2c_driver max14577_i2c_driver = {
> > + .driver = {
> > + .name = MAX14577_MFD_DEV_NAME,
>
> Can you use the proper name here, these types of defines are pretty ugly.
OK.
>
> > + .owner = THIS_MODULE,
> > + .pm = &max14577_pm,
> > + .of_match_table = of_match_ptr(max14577_dt_match),
> > + },
> > + .probe = max14577_i2c_probe,
> > + .remove = max14577_i2c_remove,
> > + .id_table = max14577_i2c_id,
> > +};
>
> >>>>>>>>>>>>>>>>>>>>>>
>
> > +static int __init max14577_i2c_init(void)
> > +{
> > + return i2c_add_driver(&max14577_i2c_driver);
> > +}
> > +subsys_initcall(max14577_i2c_init);
> > +
> > +static void __exit max14577_i2c_exit(void)
> > +{
> > + i2c_del_driver(&max14577_i2c_driver);
> > +}
> > +module_exit(max14577_i2c_exit);
>
> >>>>>>>>>>>>>>>>>>>>>>
>
> Remove all this and replace with:
> module_i2c_driver(max14577_i2c_driver);
The subsys_initcall is needed. Marek Szyprowski replied to this here:
http://thread.gmane.org/gmane.linux.drivers.devicetree/51903/focus=1594651
(...)
> > +struct max14577_regulator_platform_data {
> > + int id;
> > + struct regulator_init_data *initdata;
> > + struct device_node *of_node;
>
> Do you ever set this? What's the point of it is it's set in the device?
Do you mean the whole struct max14577_regulator_platform_data or only
some member of it (of_node?)?
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
