svn commit: r315466 - in head/sys/dev: mmc sdhci

Fri Mar 17 23:45:07 UTC 2017

On Sat, Mar 18, 2017 at 12:39:47AM +0100, Marius Strobl wrote:
> On Fri, Mar 17, 2017 at 05:09:26PM -0600, Ian Lepore wrote:
> > On Fri, 2017-03-17 at 22:57 +0000, Marius Strobl wrote:
> > > Author: marius
> > > Date: Fri Mar 17 22:57:37 2017
> > > New Revision: 315466
> > > URL: https://svnweb.freebsd.org/changeset/base/315466
> > > 
> > > Log:
> > >   Again, fixes regarding style(4), to comments, includes and unused
> > >   parameters.
> > > 
> > > Modified:
> > >   head/sys/dev/mmc/bridge.h
> > >   head/sys/dev/mmc/mmc.c
> > >   head/sys/dev/mmc/mmcbr_if.m
> > >   head/sys/dev/sdhci/sdhci.c
> > >   head/sys/dev/sdhci/sdhci_acpi.c
> > >   head/sys/dev/sdhci/sdhci_if.m
> > >   head/sys/dev/sdhci/sdhci_pci.c
> > > 
> > > Modified: head/sys/dev/mmc/bridge.h
> > > =====================================================================
> > > =========
> > > --- head/sys/dev/mmc/bridge.h	Fri Mar 17 22:02:02 2017	
> > > (r315465)
> > > +++ head/sys/dev/mmc/bridge.h	Fri Mar 17 22:57:37 2017	
> > > (r315466)
> > > @@ -111,7 +111,7 @@ enum mmc_bus_timing {
> > >  
> > >  struct mmc_ios {
> > >  	uint32_t	clock;	/* Speed of the clock in Hz to
> > > move data */
> > > -	enum mmc_vdd	vdd;	/* Voltage to apply to the
> > > power pins/ */
> > > +	enum mmc_vdd	vdd;	/* Voltage to apply to the
> > > power pins */
> > >  	enum mmc_bus_mode bus_mode;
> > >  	enum mmc_chip_select chip_select;
> > >  	enum mmc_bus_width bus_width;
> > > 
> > > Modified: head/sys/dev/mmc/mmc.c
> > > =====================================================================
> > > =========
> > > --- head/sys/dev/mmc/mmc.c	Fri Mar 17 22:02:02 2017	(r3
> > > 15465)
> > > +++ head/sys/dev/mmc/mmc.c	Fri Mar 17 22:57:37 2017	(r3
> > > 15466)
> > > @@ -792,6 +792,7 @@ mmc_get_bits(uint32_t *bits, int bit_len
> > >  	const int i = (bit_len / 32) - (start / 32) - 1;
> > >  	const int shift = start & 31;
> > >  	uint32_t retval = bits[i] >> shift;
> > > +
> > >  	if (size + shift > 32)
> > >  		retval |= bits[i - 1] << (32 - shift);
> > >  	return (retval & ((1llu << size) - 1));
> > > @@ -1464,7 +1465,7 @@ mmc_rescan_cards(struct mmc_softc *sc)
> > >  		return;
> > >  	for (i = 0; i < devcount; i++) {
> > >  		ivar = device_get_ivars(devlist[i]);
> > > -		if (mmc_select_card(sc, ivar->rca)) {
> > > +		if (mmc_select_card(sc, ivar->rca) != MMC_ERR_NONE)
> > > {
> > >  			if (bootverbose || mmc_debug)
> > >  				device_printf(sc->dev,
> > >  				    "Card at relative address %d
> > > lost.\n",
> > > 
> > > Modified: head/sys/dev/mmc/mmcbr_if.m
> > > =====================================================================
> > > =========
> > > --- head/sys/dev/mmc/mmcbr_if.m	Fri Mar 17 22:02:02 2017	
> > > (r315465)
> > > +++ head/sys/dev/mmc/mmcbr_if.m	Fri Mar 17 22:57:37 2017	
> > > (r315466)
> > > @@ -65,8 +65,9 @@
> > >  INTERFACE mmcbr;
> > >  
> > >  #
> > > -# Called by the mmcbus to setup the IO pins correctly, the voltage
> > > to use
> > > -# for the card, the type of selects, power modes and bus width.
> > > +# Called by the mmcbus to set up the IO pins correctly, the
> > > common/core
> > > +# supply voltage (VDD/VCC) to use for the device, the clock
> > > frequency, the
> > > +# type of SPI chip select, power mode and bus width.
> > >  #
> > >  METHOD int update_ios {
> > >  	device_t	brdev;
> > > @@ -76,8 +77,8 @@ METHOD int update_ios {
> > >  #
> > >  # Called by the mmcbus or its children to schedule a mmc
> > > request.  These
> > >  # requests are queued.  Time passes.  The bridge then gets
> > > notification
> > > -# of the status of request, who then notifies the requesting device
> > > via
> > > -# the xfer_done mmcbus method.
> > > +# of the status of the request, who then notifies the requesting
> > > device
> > > +# by calling the completion function supplied as part of the
> > > request.
> > >  #
> > >  METHOD int request {
> > >  	device_t	brdev;
> > > 
> > > Modified: head/sys/dev/sdhci/sdhci.c
> > > =====================================================================
> > > =========
> > > --- head/sys/dev/sdhci/sdhci.c	Fri Mar 17 22:02:02 2017	
> > > (r315465)
> > > +++ head/sys/dev/sdhci/sdhci.c	Fri Mar 17 22:57:37 2017	
> > > (r315466)
> > > @@ -309,9 +309,8 @@ sdhci_set_clock(struct sdhci_slot *slot,
> > >  		}
> > >  		/* Divider 1:1 is 0x00, 2:1 is 0x01, 256:1 is 0x80
> > > ... */
> > >  		div >>= 1;
> > > -	}
> > > -	else {
> > > -		/* Version 3.0 divisors are multiples of two up to
> > > 1023*2 */
> > > +	} else {
> > > +		/* Version 3.0 divisors are multiples of two up to
> > > 1023 * 2 */
> > >  		if (clock >= clk_base)
> > >  			div = 0;
> > >  		else {
> > > @@ -1349,7 +1348,7 @@ sdhci_data_irq(struct sdhci_slot *slot, 
> > >  	if (intmask & SDHCI_INT_DMA_END) {
> > >  		data = slot->curcmd->data;
> > >  
> > > -		/* Unload DMA buffer... */
> > > +		/* Unload DMA buffer ... */
> > 
> > All this churn in the driver for meaningless style fixes and __unused
> > markup and so on is bad enough, especially considering that you know
> > people are doing out-of-tree work on this and have to deal with all the
> >  merge conflicts this is causing, but changes to comments that actually
> > change correct grammar and punctuation to incorrect, such as this, just
> > serves to turn annoying into infuriating.
> > 
> 
> Hrm, could you please tell me were I changed correct grammar into
> incorrect one? In the examples you quoted above I can't spot such
> a case but introducing incorrect grammar certainly wasn't the intent.
> 
> As for the out-of-tree work, I still haven't been told what's the branch
> that actually is used for MMCCAM; the branch imp@ mentioned last time
> was deleted shortly after. As I said before, my best guess, therefore,
> is mmccam-refactoring-rebased in kibab's tree, but that one is even still
> missing functional changes to sdhci(4) you did. Nevertheless, looking

Err, sorry, the claim that kibab's mmccam-refactoring-rebased is
missing some of your changes is wrong; diff(1)'ed against the wrong
thing. Still, the rest holds.

> at that branch, as for sdhci(4) mostly new sdhci_cam_*() functions
> have been added but hardly any existing code was touched, so there
> should be only a few conflicts, if any at all. And as was determined
> before, mmc(4) and mmcsd(4) aren't touched at all by the MMCCAM work.
> That said, I'm more than willing to help with resolving merge conflicts
> I'm causing, if someone tells me the right tree and branch ...

Marius