On Mon, Jul 18, 2016 at 07:04:57AM +0000, Jun Li wrote: > Hi Peter, > > > -----Original Message----- > > From: Peter Chen [mailto:hzpeterchen@xxxxxxxxx] > > Sent: Friday, July 15, 2016 5:21 PM > > To: Jun Li <jun.li@xxxxxxx> > > Cc: Peter Chen <peter.chen@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx > > Subject: Re: [PATCH 2/2] usb: chipidea: usbmisc: set over current polarity > > for imx6 and imx7 > > > > On Fri, Jul 15, 2016 at 07:38:23AM +0000, Jun Li wrote: > > > Hi, > > > > -----Original Message----- > > > > From: Peter Chen [mailto:hzpeterchen@xxxxxxxxx] > > > > Sent: Friday, July 15, 2016 3:02 PM > > > > To: Jun Li <jun.li@xxxxxxx> > > > > Cc: Peter Chen <peter.chen@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx > > > > Subject: Re: [PATCH 2/2] usb: chipidea: usbmisc: set over current > > > > polarity for imx6 and imx7 > > > > > > > > On Tue, Jul 12, 2016 at 03:24:49PM +0800, Li Jun wrote: > > > > > As all usb power supply use low active for over current flag on > > > > > imx6 > > > > > imx7 boards, and the default register setting(0) is for high > > > > > active, this patch is to correct it. > > > > > > > > > > > > > We may can't ensure all USB power switch chips work like that, I > > > > suggest you making this as default. > > > > > > > > I will change the commit log like below if you are ok. > > > > > > > > As most of all usb power switch chips use active-low for over > > > > current flag, but the default register setting(0) is for active-high > > > > at imx6/imx7, this patch changes default value as active-low. > > > > > > Looks better, I am okay with it except a tiny comment :%s/As most of > > > all usb power/As most of usb power > > > > > > Li Jun > > > > > > Since we can't break current default behaviour, but with your patch, the > > imx6sx sdb board creates over current event. > > > > I think you may need to introduce a flag for OC polarity, and use it for > > exist platforms if necessary. It can narrow down affect only on single > > platform. > > > > For new platforms, you can change SoC values by default. > > To keep those platforms without specify either disable-over-current > or over-current-polarity out of problem, we need set the disable_oc > to be "1", or oc_polarity to be "1"(then register value to be "0" > as before) for them. > > if (!of_property_read_u32(np, "over-current-polarity", &ret)) > data->oc_polarity = ret ? 1 : 0; > else > data->oc_polarity = 1; > If there is no property "over-current-polarity", you should keep register value unchanging. You know we should not break the platforms which may still use old dts. So, we could set the value according to DT property, but can't change default value unless for new SoC platform. Peter > or > > if (!of_property_read_u32(np, "over-current-polarity", &ret)) > data->oc_polarity = ret ? 1 : 0; > else > data->disable_oc = 1; > > which way do you prefer? > > Li Jun > > > > -- > > > > Best Regards, > > Peter Chen -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
