Add keypad data node in omap5-evm.
Based on I2C support patch for omap5, which has been
already posted as a different series.
Cc: Benoit Cousson <redacted>
Cc: Felipe Balbi <redacted>
Cc: Santosh Shilimkar <redacted>
Tested on omap5430 sdp with 3.5 custom kernel.
Signed-off-by: Sourav Poddar <redacted>
---
arch/arm/boot/dts/omap5-evm.dts | 95 +++++++++++++++++++++++++++++++++++++++
1 files changed, 95 insertions(+), 0 deletions(-)
@@ -0,0 +1,308 @@+/*+*SMSC_ECE1099Keypaddriver+*+*Copyright(C)2012TexasInstrumentsIncorporated-http://www.ti.com/+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicenseversion2as+*publishedbytheFreeSoftwareFoundation.+*/++#include<linux/i2c.h>+#include<linux/kernel.h>+#include<linux/module.h>+#include<linux/init.h>+#include<linux/interrupt.h>+#include<linux/input.h>+#include<linux/gpio.h>+#include<linux/slab.h>+#include<linux/jiffies.h>+#include<linux/input/matrix_keypad.h>+#include<linux/delay.h>+#include<linux/mfd/core.h>+#include<linux/mfd/smsc.h>+#include<linux/of_gpio.h>+#include<linux/of.h>++#define KEYPRESS_TIME 200++structsmsc_keypad{+structsmsc*smsc;+structmatrix_keymap_data*keymap_data;+unsignedintlast_key_state[16];+unsignedintlast_col;+unsignedintlast_key_ms[16];+unsignedshort*keymap;+structi2c_client*client;+structinput_dev*input;+introws,cols;+introw_shift;+boolno_autorepeat;+unsignedirq;+structdevice*dev;+};++staticvoidsmsc_kp_scan(structsmsc_keypad*kp)+{+structinput_dev*input=kp->input;+inti,j;+introw,col;+inttemp,code;+unsignedintnew_state[16];+unsignedintbits_changed;+intthis_ms;++smsc_write(kp->dev,SMSC_KP_INT_MASK,0x00);+smsc_write(kp->dev,SMSC_KP_INT_STAT,0xFF);++/* Scan for row and column */+for(i=0;i<kp->cols;i++){+smsc_write(kp->dev,SMSC_KP_OUT,SMSC_KSO_EVAL+i);+/* Read Row Status */+smsc_read(kp->dev,SMSC_KP_IN,&temp);+if(temp==0xFF)+continue;++col=i;+for(j=0;j<kp->rows;j++){+if((temp&0x01)!=0x00){+temp=temp>>1;+continue;+}++row=j;+new_state[col]=(1<<row);+bits_changed=kp->last_key_state[col]^new_state[col];+this_ms=jiffies_to_msecs(jiffies);+if(bits_changed!=0||(!bits_changed&&+((this_ms-kp->last_key_ms[col])>=KEYPRESS_TIME))){+code=MATRIX_SCAN_CODE(row,col,kp->row_shift);+input_event(input,EV_MSC,MSC_SCAN,code);+input_report_key(input,kp->keymap[code],1);+input_report_key(input,kp->keymap[code],0);+kp->last_key_state[col]=new_state[col];+if(kp->last_col!=col)+kp->last_key_state[kp->last_col]=0;+kp->last_key_ms[col]=this_ms;+}+temp=temp>>1;+}+}+input_sync(input);++smsc_write(kp->dev,SMSC_KP_INT_MASK,0xFF);++/* Set up Low Power Mode (Wake-up) (0xFB) */+smsc_write(kp->dev,SMSC_WKUP_CTRL,SMSC_KP_SET_LOW_PWR);++/*Enable Keypad Scan (generate interrupt on key press) (0x40)*/+smsc_write(kp->dev,SMSC_KP_OUT,SMSC_KSO_ALL_LOW);+}++staticirqreturn_tdo_kp_irq(intirq,void*_kp)+{+structsmsc_keypad*kp=_kp;+intint_status;++smsc_read(kp->dev,SMSC_KP_INT_STAT,&int_status);+if(int_status)+smsc_kp_scan(kp);++returnIRQ_HANDLED;+}++#ifdef CONFIG_OF+staticint__devinitsmsc_keypad_parse_dt(structdevice*dev,+structsmsc_keypad*kp)+{+structdevice_node*np=dev->of_node;++if(!np){+dev_err(dev,"missing DT data");+return-EINVAL;+}++of_property_read_u32(np,"keypad,num-rows",&kp->rows);+of_property_read_u32(np,"keypad,num-columns",&kp->cols);+if(!kp->rows||!kp->cols){+dev_err(dev,"number of keypad rows/columns not specified\n");+return-EINVAL;+}++if(of_get_property(np,"linux,input-no-autorepeat",NULL))+kp->no_autorepeat=true;++return0;+}+#else+staticinlineintsmsc_keypad_parse_dt(structdevice*dev,+structsmsc_keypad*kp)+{+return-ENOSYS;+}+#endif++staticint__devinit+smsc_probe(structplatform_device*pdev)+{+structdevice*dev=&pdev->dev;+structsmsc*smsc=dev_get_drvdata(pdev->dev.parent);+structinput_dev*input;+structsmsc_keypad*kp;+intret=0,error;+intcol,i,max_keys,row_shift;+intirq;+intaddr_start,addr;++kp=devm_kzalloc(dev,sizeof(*kp),GFP_KERNEL);++input=input_allocate_device();+if(!kp||!input){+error=-ENOMEM;+gotoerr1;+}++error=smsc_keypad_parse_dt(&pdev->dev,kp);+if(error)+returnerror;++/* Get the debug Device */+kp->input=input;+kp->smsc=smsc;+kp->irq=platform_get_irq(pdev,0);+kp->dev=dev;++for(col=0;col<16;col++){+kp->last_key_state[col]=0;+kp->last_key_ms[col]=0;+}++/* setup input device */+__set_bit(EV_KEY,input->evbit);++/* Enable auto repeat feature of Linux input subsystem */+if(!(kp->no_autorepeat))+__set_bit(EV_REP,input->evbit);++input_set_capability(input,EV_MSC,MSC_SCAN);+input->name="SMSC Keypad";+input->phys="smsc_keypad/input0";+input->dev.parent=&pdev->dev;+input->id.bustype=BUS_HOST;+input->id.vendor=0x0001;+input->id.product=0x0001;+input->id.version=0x0003;++error=input_register_device(input);+if(error){+dev_err(kp->dev,+"Unable to register twl4030 keypad device\n");+gotoerr1;+}++/* Mask all GPIO interrupts (0x37-0x3B) */+for(addr=0x37;addr<0x3B;addr++)+smsc_write(dev,addr,0);++/* Set all outputs high (0x05-0x09) */+for(addr=0x05;addr<0x09;addr++)+smsc_write(dev,addr,0xff);++/* Clear all GPIO interrupts (0x32-0x36) */+for(addr=0x32;addr<0x36;addr++)+smsc_write(dev,addr,0xff);++addr_start=0x12;+for(i=0;i<=kp->rows;i++){+addr=0x12+i;+smsc_write(dev,addr,SMSC_KP_KSI);+}++addr_start=0x1A;+for(i=0;i<=kp->cols;i++){+addr=0x1A+i;+smsc_write(dev,addr,SMSC_KP_KSO);+}++addr=SMSC_KP_INT_STAT;+smsc_write(dev,addr,SMSC_KP_SET_HIGH);++addr=SMSC_WKUP_CTRL;+smsc_write(dev,addr,SMSC_KP_SET_LOW_PWR);++addr=SMSC_KP_OUT;+smsc_write(dev,addr,SMSC_KSO_ALL_LOW);++row_shift=get_count_order(kp->cols);+max_keys=kp->rows<<row_shift;++kp->row_shift=row_shift;+kp->keymap=kzalloc(max_keys*sizeof(kp->keymap[0]),+GFP_KERNEL);+if(!kp->keymap){+dev_err(&pdev->dev,"Not enough memory for keymap\n");+error=-ENOMEM;+}++matrix_keypad_build_keymap(NULL,NULL,kp->rows,+kp->cols,kp->keymap,input);++/*+*ThisISRwillalwaysexecuteinkernelthreadcontextbecauseof+*theneedtoaccesstheSMSCovertheI2Cbus.+*/+ret=devm_request_threaded_irq(dev,kp->irq,NULL,do_kp_irq,+IRQF_TRIGGER_FALLING|IRQF_ONESHOT,pdev->name,kp);+if(ret){+dev_dbg(&pdev->dev,"request_irq failed for irq no=%d\n",+irq);+gotoerr2;+}++/* Enable smsc keypad interrupts */+ret=smsc_write(dev,SMSC_KP_INT_MASK,0xff);+if(ret<0)+gotoerr2;++return0;++err2:+input_unregister_device(input);+free_irq(kp->irq,NULL);+err1:+input_free_device(input);+returnret;+}++staticintsmsc_remove(structplatform_device*pdev)+{+structsmsc_keypad*kp=platform_get_drvdata(pdev);+free_irq(kp->irq,kp);+input_unregister_device(kp->input);++return0;+}++#ifdef CONFIG_OF+staticconststructof_device_idsmsc_keypad_dt_match[]={+{.compatible="smsc,keypad"},+{},+};+MODULE_DEVICE_TABLE(of,smsc_keypad_dt_match);+#endif++staticstructplatform_driversmsc_driver={+.driver={+.name="smsc-keypad",+.of_match_table=of_match_ptr(smsc_keypad_dt_match),+.owner=THIS_MODULE,+},+.probe=smsc_probe,+.remove=smsc_remove,+};++module_platform_driver(smsc_driver);++MODULE_AUTHOR("G Kondaiah Manjunath <manjugk@ti.com>");+MODULE_DESCRIPTION("SMSC ECE1099 Keypad driver");+MODULE_LICENSE("GPL v2");
@@ -0,0 +1,56 @@+What is smsc-ece1099?+----------------------++The ECE1099 is a 40-Pin 3.3V Keyboard Scan Expansion+or GPIO Expansion device. The device supports a keyboard+scan matrix of 23x8. The device is connected to a Master+via the SMSC BC-Link interface or via the SMBus.+Keypad scan Input(KSI) and Keypad Scan Output(KSO) signals+are multiplexed with GPIOs.++Interrupt generation+--------------------++Interrupts can be generated by an edge detection on a GPIO+pin or an edge detection on one of the bus interface pins.+Interrupts can also be detected on the keyboard scan interface.+The bus interrupt pin (BC_INT# or SMBUS_INT#) is asserted if+any bit in one of the Interrupt Status registers is 1 and+the corresponding Interrupt Mask bit is also 1.++In order for software to determine which device is the source+of an interrupt, it should first read the Group Interrupt Status Register+to determine which Status register group is a source for the interrupt.+Software should read both the Status register and the associated Mask register,+then AND the two values together. Bits that are 1 in the result of the AND+are active interrupts. Software clears an interrupt by writing a 1 to the+corresponding bit in the Status register.++Communication Protocol+----------------------++- SMbus slave Interface+ The host processor communicates with the ECE1099 device+ through a series of read/write registers via the SMBus+ interface. SMBus is a serial communication protocol between+ a computer host and its peripheral devices. The SMBus data+ rate is 10KHz minimum to 400 KHz maximum++- Slave Bus Interface+ The ECE1099 device SMBus implementation is a subset of the+ SMBus interface to the host. The device is a slave-only SMBus device.+ The implementation in the device is a subset of SMBus since it+ only supports four protocols.++ The Write Byte, Read Byte, Send Byte, and Receive Byte protocols are the+ only valid SMBus protocols for the device.++- BC-LinkTM Interface+ The BC-Link is a proprietary bus that allows communication+ between a Master device and a Companion device. The Master+ device uses this serial bus to read and write registers+ located on the Companion device. The bus comprises three signals,+ BC_CLK, BC_DAT and BC_INT#. The Master device always provides the+ clock, BC_CLK, and the Companion device is the source for an+ independent asynchronous interrupt signal, BC_INT#. The ECE1099+ supports BC-Link speeds up to 24MHz.
From: Felipe Balbi <hidden> Date: 2012-08-21 10:51:45
On Tue, Aug 21, 2012 at 04:15:39PM +0530, Sourav Poddar wrote:
Add keypad data node in omap5-evm.
Based on I2C support patch for omap5, which has been
already posted as a different series.
Cc: Benoit Cousson <redacted>
Cc: Felipe Balbi <redacted>
Cc: Santosh Shilimkar <redacted>
Tested on omap5430 sdp with 3.5 custom kernel.
Signed-off-by: Sourav Poddar <redacted>
after fixing my only comment below, you can add my:
Acked-by: Felipe Balbi <redacted>
this will leak the irq (please move to devm_request_threaded_irq).
It's also leaking irq_descs and irq_domain, you need to free those
resources by calling irq_domain_remove and irq_free_descs.
--
balbi
Hi,
On Tue, Aug 21, 2012 at 4:17 PM, Felipe Balbi [off-list ref] wrote:
On Tue, Aug 21, 2012 at 04:15:39PM +0530, Sourav Poddar wrote:
quoted
Add keypad data node in omap5-evm.
Based on I2C support patch for omap5, which has been
already posted as a different series.
Cc: Benoit Cousson <redacted>
Cc: Felipe Balbi <redacted>
Cc: Santosh Shilimkar <redacted>
Tested on omap5430 sdp with 3.5 custom kernel.
Signed-off-by: Sourav Poddar <redacted>
after fixing my only comment below, you can add my:
Acked-by: Felipe Balbi <redacted>
@@ -0,0 +1,373 @@+/*+*GPIOChipdriverforsmsc+*SMSCI/OExpanderandQWERTYKeypadController+*+*Copyright2012TexasInstrumentsInc.+*+*LicensedundertheGPL-2orlater.+*/++#include<linux/module.h>+#include<linux/kernel.h>+#include<linux/slab.h>+#include<linux/init.h>+#include<linux/i2c.h>+#include<linux/gpio.h>+#include<linux/interrupt.h>+#include<linux/irqdomain.h>+#include<linux/irq.h>+#include<linux/mfd/smsc.h>+#include<linux/err.h>++structsmsc_gpio{+structdevice*dev;+structsmsc*smsc;+structgpio_chipgpio_chip;+structmutexlock;/* protect cached dir, dat_out */+/* protect serialized access to the interrupt controller bus */+structmutexirq_lock;+unsignedgpio_start;+inttype;+intflags;+intirq;+intirq_base;+unsignedintgpio_base;+unsignedintdat_out[5];+unsignedintdir[5];+unsignedintint_lvl[5];+unsignedintint_en[5];+unsignedintirq_mask[5];+unsignedintirq_stat[5];+};++staticintsmsc_gpio_get_value(structgpio_chip*chip,unsignedoff)+{+structsmsc_gpio*sg=+container_of(chip,structsmsc_gpio,gpio_chip);+unsignedintget;+return!!(smsc_read(sg->dev,+(SMSC_GPIO_DATA_IN_START+SMSC_BANK(off))&SMSC_BIT(off),+&get));+}++staticvoidsmsc_gpio_set_value(structgpio_chip*chip,+unsignedoff,intval)+{+unsignedbank,bit;+structsmsc_gpio*sg=+container_of(chip,structsmsc_gpio,gpio_chip);++bank=SMSC_BANK(off);+bit=SMSC_BIT(off);++mutex_lock(&sg->lock);+if(val)+sg->dat_out[bank]|=bit;+else+sg->dat_out[bank]&=~bit;++smsc_write(sg->dev,SMSC_GPIO_DATA_OUT_START+bank,+sg->dat_out[bank]);+mutex_unlock(&sg->lock);+}++staticintsmsc_gpio_direction_input(structgpio_chip*chip,unsignedoff)+{+unsignedintreg;+structsmsc_gpio*sg=+container_of(chip,structsmsc_gpio,gpio_chip);+intreg_dir;++mutex_lock(&sg->lock);+reg_dir=SMSC_CFG_START+off;+smsc_read(sg->dev,reg_dir,®);+reg|=SMSC_GPIO_INPUT_LOW;+mutex_unlock(&sg->lock);++returnsmsc_write(sg->dev,reg_dir,reg);+}++staticintsmsc_gpio_direction_output(structgpio_chip*chip,+unsignedoff,intval)+{+unsignedintreg;+structsmsc_gpio*sg=+container_of(chip,structsmsc_gpio,gpio_chip);+intreg_dir;++mutex_lock(&sg->lock);+reg_dir=SMSC_CFG_START+off;+smsc_read(sg->dev,reg_dir,®);+reg|=SMSC_GPIO_OUTPUT_PP;+mutex_unlock(&sg->lock);++returnsmsc_write(sg->dev,reg_dir,reg);+}++staticintsmsc_gpio_to_irq(structgpio_chip*chip,unsignedoff)+{+structsmsc_gpio*sg=+container_of(chip,structsmsc_gpio,gpio_chip);+returnsg->irq_base+off;+}++staticvoidsmsc_irq_bus_lock(structirq_data*d)+{+structsmsc_gpio*sg=irq_data_get_irq_chip_data(d);++mutex_lock(&sg->irq_lock);+}++staticvoidsmsc_irq_bus_sync_unlock(structirq_data*d)+{+structsmsc_gpio*sg=irq_data_get_irq_chip_data(d);+inti;++for(i=0;i<SMSC_BANK(SMSC_MAXGPIO);i++)+if(sg->int_en[i]^sg->irq_mask[i]){+sg->int_en[i]=sg->irq_mask[i];+smsc_write(sg->dev,SMSC_GPIO_INT_MASK_START+i,+sg->int_en[i]);+}++mutex_unlock(&sg->irq_lock);+}++staticvoidsmsc_irq_mask(structirq_data*d)+{+structsmsc_gpio*sg=irq_data_get_irq_chip_data(d);+unsignedgpio=d->irq-sg->irq_base;++sg->irq_mask[SMSC_BANK(gpio)]&=~SMSC_BIT(gpio);+}++staticvoidsmsc_irq_unmask(structirq_data*d)+{+structsmsc_gpio*sg=irq_data_get_irq_chip_data(d);+unsignedgpio=d->irq-sg->irq_base;++sg->irq_mask[SMSC_BANK(gpio)]|=SMSC_BIT(gpio);+}++staticintsmsc_irq_set_type(structirq_data*d,unsignedinttype)+{+structsmsc_gpio*sg=irq_data_get_irq_chip_data(d);+uint16_tgpio=d->irq-sg->irq_base;+unsignedbank,bit;++if((type&IRQ_TYPE_EDGE_BOTH)){+dev_err(sg->dev,"irq %d: unsupported type %d\n",+d->irq,type);+return-EINVAL;+}++bank=SMSC_BANK(gpio);+bit=SMSC_BIT(gpio);++if(type&IRQ_TYPE_LEVEL_HIGH)+sg->int_lvl[bank]|=bit;+elseif(type&IRQ_TYPE_LEVEL_LOW)+sg->int_lvl[bank]&=~bit;+else+return-EINVAL;
this looks wrong. You could have a user who wants to trigger on both
HIGH and LOW levels, no ?
Yes, I think there can be a scenario where gpio_keys are attached
to this driver and signals a "key press" at low and "key release" at
high. ?
Will figure out a way to add support to check for case where
both High and low levels are used.
this will leak the irq (please move to devm_request_threaded_irq).
It's also leaking irq_descs and irq_domain, you need to free those
resources by calling irq_domain_remove and irq_free_descs.
this looks wrong. You could have a user who wants to trigger on both
HIGH and LOW levels, no ?
Yes, I think there can be a scenario where gpio_keys are attached
to this driver and signals a "key press" at low and "key release" at
high. ?
Will figure out a way to add support to check for case where
both High and low levels are used.
could probably be done on a separate patch, maybe... Just now I saw that
HIGH and LOW levels use the same bit.
--
balbi
this looks wrong. You could have a user who wants to trigger on both
HIGH and LOW levels, no ?
Yes, I think there can be a scenario where gpio_keys are attached
to this driver and signals a "key press" at low and "key release" at
high. ?
Will figure out a way to add support to check for case where
both High and low levels are used.
could probably be done on a separate patch, maybe... Just now I saw that
HIGH and LOW levels use the same bit.
If I am understanding correctly, if they both uses the same bit we cannot
use both for a particular user. ?
this looks wrong. You could have a user who wants to trigger on both
HIGH and LOW levels, no ?
Yes, I think there can be a scenario where gpio_keys are attached
to this driver and signals a "key press" at low and "key release" at
high. ?
Will figure out a way to add support to check for case where
both High and low levels are used.
could probably be done on a separate patch, maybe... Just now I saw that
HIGH and LOW levels use the same bit.
If I am understanding correctly, if they both uses the same bit we cannot
use both for a particular user. ?
we can, it's just a bit more complex. If a user request both LOW and
HIGH, then you start with HIGH, once it triggers, before calling the
nested IRQ handler, you need to change it LOW. When low triggers, before
calling the nested IRQ handler, you need to change it to HIGH again. And
so on. I'm just not sure if that's valid on linux IRQ subsystem.
Anyone ?
--
balbi
Indentation is weird here. For the cache we should have at least
.max_register defined and given the functionality there must surely be
some volatile registers (I'm surprised this works at all as it is, the
cache should break things).
I'd make these log messages dev_info() or something.
dev_info() ? It'lll just make boot noisier for no good reason. Which
user wants to see this during boot up ? That's a debugging feature for
develop IMHO.
--
balbi
I'd make these log messages dev_info() or something.
dev_info() ? It'lll just make boot noisier for no good reason. Which
user wants to see this during boot up ? That's a debugging feature for
develop IMHO.
Most of the registers appeared to be chip revision information which is
most definitely useful to tell people about, though possibly with neater
formatting ("why is this batch of boards failing... oh, right"). If
they're fixed device IDs then the driver should instead be verifying
that the registers contain the expected values and bombing out if they
don't. Either way dev_dbg() isn't too helpful.
I'd make these log messages dev_info() or something.
quoted
dev_info() ? It'lll just make boot noisier for no good reason. Which
user wants to see this during boot up ? That's a debugging feature for
develop IMHO.
Most of the registers appeared to be chip revision information which is
most definitely useful to tell people about, though possibly with neater
formatting ("why is this batch of boards failing... oh, right"). If
they're fixed device IDs then the driver should instead be verifying
that the registers contain the expected values and bombing out if they
don't. Either way dev_dbg() isn't too helpful.
I still beg to differ. Even if it fails, dmesg will still contain the
message (provided you have it enabled). I really don't think we want
this to print to console on every boot.
If you're still testing your new batch of boards, you're not just a
simple user and you will have debugging enabled anyway. dev_info() will
be visible to anyone who's got a console running. Not sure how useful
that would be to my neighbor.
--
balbi
From: Mark Brown <hidden> Date: 2012-08-21 13:49:40
On Tue, Aug 21, 2012 at 04:27:44PM +0300, Felipe Balbi wrote:
I still beg to differ. Even if it fails, dmesg will still contain the
message (provided you have it enabled). I really don't think we want
this to print to console on every boot.
Only if it's enabled which is the trick...
If you're still testing your new batch of boards, you're not just a
simple user and you will have debugging enabled anyway. dev_info() will
be visible to anyone who's got a console running. Not sure how useful
that would be to my neighbor.
Also think about hobbyists and so on, and ideally at some point the
people using distros. We shouldn't be requiring kernel rebuilds for
this sort of diagnostic information. I guess sysfs is another option
but frankly the overhead on boot just doesn't seem meaningful in the
context of the overall kernel boot style - I'd really expect people who
are bothered by this sort of output would be raising the minimum log
level appearing on the console.
From: Felipe Balbi <hidden> Date: 2012-08-21 13:56:44
On Tue, Aug 21, 2012 at 02:49:37PM +0100, Mark Brown wrote:
On Tue, Aug 21, 2012 at 04:27:44PM +0300, Felipe Balbi wrote:
quoted
I still beg to differ. Even if it fails, dmesg will still contain the
message (provided you have it enabled). I really don't think we want
this to print to console on every boot.
Only if it's enabled which is the trick...
quoted
If you're still testing your new batch of boards, you're not just a
simple user and you will have debugging enabled anyway. dev_info() will
be visible to anyone who's got a console running. Not sure how useful
that would be to my neighbor.
Also think about hobbyists and so on, and ideally at some point the
people using distros. We shouldn't be requiring kernel rebuilds for
this sort of diagnostic information. I guess sysfs is another option
but we don't. We have dynamic printk for that, right ?
but frankly the overhead on boot just doesn't seem meaningful in the
context of the overall kernel boot style - I'd really expect people who
are bothered by this sort of output would be raising the minimum log
level appearing on the console.
Well, if you consider a single driver then surely it doesn't make a
difference. But when you add many drivers, each with its own dev_info()
output, it can delay bootup rather significantly, actually.
Fair enough, we have "quiet", but I'm not sure that's enough argument to
allow any simple driver to start poluting dmesg with whatever random
messages.
my 2 cents
--
balbi
From: Mark Brown <hidden> Date: 2012-08-21 14:08:20
On Tue, Aug 21, 2012 at 04:52:41PM +0300, Felipe Balbi wrote:
Fair enough, we have "quiet", but I'm not sure that's enough argument to
allow any simple driver to start poluting dmesg with whatever random
messages.
I think if the driver is just logging to say "I'm running" that's noise
and I do push back on that routinely myself; if the driver is providing
information it's discovered from the running system then that seems much
more useful and we should have a sensible way of getting that out in a
place where users are likely to find.
From: Felipe Balbi <hidden> Date: 2012-08-21 14:13:26
Hi,
On Tue, Aug 21, 2012 at 03:08:03PM +0100, Mark Brown wrote:
On Tue, Aug 21, 2012 at 04:52:41PM +0300, Felipe Balbi wrote:
quoted
Fair enough, we have "quiet", but I'm not sure that's enough argument to
allow any simple driver to start poluting dmesg with whatever random
messages.
I think if the driver is just logging to say "I'm running" that's noise
and I do push back on that routinely myself; if the driver is providing
information it's discovered from the running system then that seems much
more useful and we should have a sensible way of getting that out in a
place where users are likely to find.
fair enough. But look at the messages which that driver is printing:
+ regmap_read(smsc->regmap, SMSC_DEV_ID, &ret);
+ dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
+
+ regmap_read(smsc->regmap, SMSC_DEV_REV, &ret);
+ dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
+
+ regmap_read(smsc->regmap, SMSC_VEN_ID_L, &ret);
+ dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
+
+ regmap_read(smsc->regmap, SMSC_VEN_ID_H, &ret);
+ dev_dbg(&i2c->dev, "SMSC Device ID: %d\n", ret);
You can't possibly understand what that'll print. First of all, VEN_ID_H
and VEN_ID_L should be ORed together. Second, the user will see the same
message four times in a row, with different values, but see that driver
claims that all four values refer to the device id. What this should do,
is at least combine all four messages into a single one of the format:
dev_(dbg|info)(&i2c->dev, "SMSCxxx devid: %02x rev: %02x venid: %02x\n",
devid, rev, (venid_h << 8) | venid_l);
or something similar.
--
balbi
From: Mark Brown <hidden> Date: 2012-08-21 14:20:14
On Tue, Aug 21, 2012 at 05:09:24PM +0300, Felipe Balbi wrote:
You can't possibly understand what that'll print. First of all, VEN_ID_H
and VEN_ID_L should be ORed together. Second, the user will see the same
message four times in a row, with different values, but see that driver
claims that all four values refer to the device id. What this should do,
is at least combine all four messages into a single one of the format:
Yes, I agree that the formatting here should also be improved - as I
said in one of my earlier mails if any of these things are fixed things
that should never change the driver should instead verify the value
rather than log it.
From: Mark Brown <hidden> Date: 2012-08-21 14:50:16
On Tue, Aug 21, 2012 at 03:22:18PM +0300, Felipe Balbi wrote:
On Tue, Aug 21, 2012 at 05:50:28PM +0530, Poddar, Sourav wrote:
quoted
If I am understanding correctly, if they both uses the same bit we cannot
use both for a particular user. ?
we can, it's just a bit more complex. If a user request both LOW and
HIGH, then you start with HIGH, once it triggers, before calling the
nested IRQ handler, you need to change it LOW. When low triggers, before
calling the nested IRQ handler, you need to change it to HIGH again. And
so on. I'm just not sure if that's valid on linux IRQ subsystem.
The given example was for keypress - usually a system would use edge
triggered interrupts in combination with reading the GPIO state rather
than level triggered interrupts.